| Message ID | 1473240152-47112-1-git-send-email-meng.yi@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Meng, One more thing which I have my concern: On 2016-09-07 02:22, Meng Yi wrote: > Gamma correction is optional and can be used to adjust the color > output values to match the gamut of a particular TFT LCD panel > Errata: > Gamma_R, Gamma_G and Gamma_B registers are little-endian registers > while the rest of the address-space in 2D-ACE is big-endian. > Workaround: > Split the DCU regs into "regs", "palette", "gamma" and "cursor". > Create a second regmap for gamma memory space using little endian. > > Suggested-by: Stefan Agner <stefan@agner.ch> > Signed-off-by: Meng Yi <meng.yi@nxp.com> > --- > Changes in V3: > -created a second regmap for gamma > -updated the DCU DT binding > --- > .../devicetree/bindings/display/fsl,dcu.txt | 6 +++- > drivers/gpu/drm/fsl-dcu/Kconfig | 6 ++++ > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 38 ++++++++++++++++++++++ > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 30 ++++++++++++++++- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 8 +++++ > 5 files changed, 86 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt > b/Documentation/devicetree/bindings/display/fsl,dcu.txt > index 63ec2a6..1b1321a 100644 > --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt > +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt > @@ -20,7 +20,11 @@ Optional properties: > Examples: > dcu: dcu@2ce0000 { > compatible = "fsl,ls1021a-dcu"; > - reg = <0x0 0x2ce0000 0x0 0x10000>; > + reg = <0x0 0x2ce0000 0x0 0x2000>, > + <0x0 0x2ce2000 0x0 0x2000>, > + <0x0 0x2ce4000 0x0 0xc00>, > + <0x0 0x2ce4c00 0x0 0x400>; > + reg-names = "regs", "palette", "gamma", "cursor"; > clocks = <&platform_clk 0>, <&platform_clk 0>; > clock-names = "dcu", "pix"; > big-endian; > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig > index 14a72c4..f9c76b1 100644 > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > @@ -11,3 +11,9 @@ config DRM_FSL_DCU > help > Choose this option if you have an Freescale DCU chipset. > If M is selected the module will be called fsl-dcu-drm. > + > +config DRM_FSL_DCU_GAMMA > + bool "Gamma Correction Support for NXP/Freescale DCU" > + depends on DRM_FSL_DCU > + help > + Enable support for gamma correction. What is the reason for making this a configuration option? Are there implementation without support for the Gamma tables? -- Stefan > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > index 3371635..25ab0ff 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > @@ -22,6 +22,22 @@ > #include "fsl_dcu_drm_drv.h" > #include "fsl_dcu_drm_plane.h" > > +static void fsl_crtc_gamma_set(struct drm_crtc *crtc, struct > drm_color_lut *lut, > + uint32_t size) > +{ > + struct fsl_dcu_drm_device *fsl_dev = crtc->dev->dev_private; > + unsigned int i; > + > + for (i = 0; i < size; i++) { > + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_R + 4 * i, > + lut[i].red); > + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_G + 4 * i, > + lut[i].green); > + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_B + 4 * i, > + lut[i].blue); > + } > +} > + > static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc, > struct drm_crtc_state *old_crtc_state) > { > @@ -37,6 +53,11 @@ static void fsl_dcu_drm_crtc_atomic_flush(struct > drm_crtc *crtc, > drm_crtc_send_vblank_event(crtc, event); > spin_unlock_irq(&crtc->dev->event_lock); > } > + > + if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) > + fsl_crtc_gamma_set(crtc, (struct drm_color_lut *) > + crtc->state->gamma_lut->data, > + 256); > } > > static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) > @@ -46,6 +67,10 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) > > drm_crtc_vblank_off(crtc); > > + if (fsl_dev->enable_color_mgmt) > + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > + DCU_MODE_EN_GAMMA_MASK, 0); > + > regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > DCU_MODE_DCU_MODE_MASK, > DCU_MODE_DCU_MODE(DCU_MODE_OFF)); > @@ -58,6 +83,11 @@ static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > + if (fsl_dev->enable_color_mgmt) > + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > + DCU_MODE_EN_GAMMA_MASK, > + DCU_MODE_GAMMA_ENABLE); > + > regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > DCU_MODE_DCU_MODE_MASK, > DCU_MODE_DCU_MODE(DCU_MODE_NORMAL)); > @@ -135,6 +165,7 @@ static const struct drm_crtc_funcs > fsl_dcu_drm_crtc_funcs = { > .page_flip = drm_atomic_helper_page_flip, > .reset = drm_atomic_helper_crtc_reset, > .set_config = drm_atomic_helper_set_config, > + .gamma_set = drm_atomic_helper_legacy_gamma_set, > }; > > int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) > @@ -158,5 +189,12 @@ int fsl_dcu_drm_crtc_create(struct > fsl_dcu_drm_device *fsl_dev) > > drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs); > > +#ifdef CONFIG_DRM_FSL_DCU_GAMMA > + fsl_dev->enable_color_mgmt = true; > + > + drm_crtc_enable_color_mgmt(crtc, 0, false, 256); > + drm_mode_crtc_set_gamma_size(crtc, 256); > +#endif > + > return 0; > } > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > index 7882387..f70ea68 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > @@ -48,6 +48,15 @@ static const struct regmap_config fsl_dcu_regmap_config = { > .volatile_reg = fsl_dcu_drm_is_volatile_reg, > }; > > +static const struct regmap_config fsl_dcu_regmap_gamma_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > + > + .volatile_reg = fsl_dcu_drm_is_volatile_reg, > +}; > + > static int fsl_dcu_drm_irq_init(struct drm_device *dev) > { > struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > @@ -323,7 +332,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev) > struct drm_device *drm; > struct device *dev = &pdev->dev; > struct resource *res; > - void __iomem *base; > + void __iomem *base, *base_gamma; > struct drm_driver *driver = &fsl_dcu_drm_driver; > struct clk *pix_clk_in; > char pix_clk_name[32]; > @@ -365,6 +374,25 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev) > return PTR_ERR(fsl_dev->regmap); > } > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma"); > + if (!res) { > + dev_err(dev, "could not get gamma memory resource\n"); > + return -ENODEV; > + } > + > + base_gamma = devm_ioremap_resource(dev, res); > + if (IS_ERR(base_gamma)) { > + ret = PTR_ERR(base_gamma); > + return ret; > + } > + > + fsl_dev->regmap_gamma = devm_regmap_init_mmio(dev, base_gamma, > + &fsl_dcu_regmap_config); > + if (IS_ERR(fsl_dev->regmap_gamma)) { > + dev_err(dev, "regmap_gamma init failed\n"); > + return PTR_ERR(fsl_dev->regmap_gamma); > + } > + > fsl_dev->clk = devm_clk_get(dev, "dcu"); > if (IS_ERR(fsl_dev->clk)) { > dev_err(dev, "failed to get dcu clock\n"); > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > index 3b371fe7..95427f6 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > @@ -25,6 +25,8 @@ > #define DCU_MODE_NORMAL 1 > #define DCU_MODE_TEST 2 > #define DCU_MODE_COLORBAR 3 > +#define DCU_MODE_EN_GAMMA_MASK 0x04 > +#define DCU_MODE_GAMMA_ENABLE BIT(2) > > #define DCU_BGND 0x0014 > #define DCU_BGND_R(x) ((x) << 16) > @@ -165,6 +167,10 @@ > #define VF610_LAYER_REG_NUM 9 > #define LS1021A_LAYER_REG_NUM 10 > > +#define FSL_GAMMA_R 0x000 > +#define FSL_GAMMA_G 0x400 > +#define FSL_GAMMA_B 0x800 > + > struct clk; > struct device; > struct drm_device; > @@ -182,6 +188,7 @@ struct fsl_dcu_drm_device { > struct device *dev; > struct device_node *np; > struct regmap *regmap; > + struct regmap *regmap_gamma; > int irq; > struct clk *clk; > struct clk *pix_clk; > @@ -195,6 +202,7 @@ struct fsl_dcu_drm_device { > struct fsl_dcu_drm_connector connector; > const struct fsl_dcu_soc_data *soc; > struct drm_atomic_state *state; > + bool enable_color_mgmt; > }; > > void fsl_dcu_fbdev_init(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig > > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 > > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > > @@ -11,3 +11,9 @@ config DRM_FSL_DCU > > help > > Choose this option if you have an Freescale DCU chipset. > > If M is selected the module will be called fsl-dcu-drm. > > + > > +config DRM_FSL_DCU_GAMMA > > + bool "Gamma Correction Support for NXP/Freescale DCU" > > + depends on DRM_FSL_DCU > > + help > > + Enable support for gamma correction. > > What is the reason for making this a configuration option? Are there > implementation without support for the Gamma tables? > When gamma correction is enabled, the color won't display normally since The gamma tables are not filled with correct data. So I give a choice to not using The gamma correction when you don't want to use it. Should I remove this configuration? Best Regards, Meng
On 2016-09-13 01:49, Meng Yi wrote: >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU >> > help >> > Choose this option if you have an Freescale DCU chipset. >> > If M is selected the module will be called fsl-dcu-drm. >> > + >> > +config DRM_FSL_DCU_GAMMA >> > + bool "Gamma Correction Support for NXP/Freescale DCU" >> > + depends on DRM_FSL_DCU >> > + help >> > + Enable support for gamma correction. >> >> What is the reason for making this a configuration option? Are there >> implementation without support for the Gamma tables? >> > When gamma correction is enabled, the color won't display normally since > The gamma tables are not filled with correct data. So I give a choice > to not using > The gamma correction when you don't want to use it. > > Should I remove this configuration? Yeah making this a compile time configuration seems wrong to me. I guess we should fill the table with a reasonable default then. The omapdrm driver seems to do something similar. Best regards, Stefan
On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote: > On 2016-09-13 01:49, Meng Yi wrote: > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU > >> > help > >> > Choose this option if you have an Freescale DCU chipset. > >> > If M is selected the module will be called fsl-dcu-drm. > >> > + > >> > +config DRM_FSL_DCU_GAMMA > >> > + bool "Gamma Correction Support for NXP/Freescale DCU" > >> > + depends on DRM_FSL_DCU > >> > + help > >> > + Enable support for gamma correction. > >> > >> What is the reason for making this a configuration option? Are there > >> implementation without support for the Gamma tables? > >> > > When gamma correction is enabled, the color won't display normally since > > The gamma tables are not filled with correct data. So I give a choice > > to not using > > The gamma correction when you don't want to use it. > > > > Should I remove this configuration? > > Yeah making this a compile time configuration seems wrong to me. > > I guess we should fill the table with a reasonable default then. The > omapdrm driver seems to do something similar. Yup Kconfig is definitely not the correct way to fix a setup problem in your driver. Note that even when you enable gamma lut userspace is allowed to set a NULL table, and in that case you need to either disable the gamma lut, or fill it with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table will also be NULL (except when you have special setup code), and if that results in a black screen your driver is buggy. Papering over a driver bug with Kconfig is definitely not ok. -Daniel
> On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote: > > On 2016-09-13 01:49, Meng Yi wrote: > > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig > > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 > > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU > > >> > help > > >> > Choose this option if you have an Freescale DCU chipset. > > >> > If M is selected the module will be called fsl-dcu-drm. > > >> > + > > >> > +config DRM_FSL_DCU_GAMMA > > >> > + bool "Gamma Correction Support for NXP/Freescale DCU" > > >> > + depends on DRM_FSL_DCU > > >> > + help > > >> > + Enable support for gamma correction. > > >> > > >> What is the reason for making this a configuration option? Are > > >> there implementation without support for the Gamma tables? > > >> > > > When gamma correction is enabled, the color won't display normally > > > since The gamma tables are not filled with correct data. So I give a > > > choice to not using The gamma correction when you don't want to use > > > it. > > > > > > Should I remove this configuration? > > > > Yeah making this a compile time configuration seems wrong to me. > > > > I guess we should fill the table with a reasonable default then. The > > omapdrm driver seems to do something similar. > > Yup Kconfig is definitely not the correct way to fix a setup problem in your > driver. Note that even when you enable gamma lut userspace is allowed to set > a NULL table, and in that case you need to either disable the gamma lut, or fill it > with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table > will also be NULL (except when you have special setup code), and if that results > in a black screen your driver is buggy. Papering over a driver bug with Kconfig is > definitely not ok. How about initialize the tables with static array(such as gmma = 2.2, etc) /*gamma = 2.2*/ u32 gamma_default_lut[] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x02, 0x02, 0x02, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x04, 0x04, 0x04, 0x05, 0x05, 0x05, 0x06, 0x06, 0x06, 0x07, 0x07, 0x07, 0x08, 0x08, 0x08, 0x09, 0x09, 0x09, 0x0A, 0x0A, 0x0B, 0x0B, 0x0B, 0x0C, 0x0C, 0x0D, 0x0D, 0x0E, 0x0E, 0x0E, 0x0F, 0x0F, 0x10, 0x10, 0x11, 0x11, 0x12, 0x13, 0x13, 0x14, 0x14, 0x15, 0x15, 0x16, 0x17, 0x17, 0x18, 0x18, 0x19, 0x1A, 0x1A, 0x1B, 0x1C, 0x1C, 0x1D, 0x1E, 0x1E, 0x1F, 0x20, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25, 0x26, 0x27, 0x27, 0x28, 0x29, 0x2A, 0x2B, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F, 0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47, 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4F, 0x50, 0x51, 0x52, 0x53, 0x54, 0x56, 0x57, 0x58, 0x59, 0x5B, 0x5C, 0x5D, 0x5E, 0x60, 0x61, 0x62, 0x64, 0x65, 0x66, 0x68, 0x69, 0x6A, 0x6C, 0x6D, 0x6F, 0x70, 0x71, 0x73, 0x74, 0x76, 0x77, 0x79, 0x7A, 0x7C, 0x7D, 0x7F, 0x80, 0x82, 0x83, 0x85, 0x86, 0x88, 0x89, 0x8B, 0x8C, 0x8E, 0x90, 0x91, 0x93, 0x95, 0x96, 0x98, 0x9A, 0x9B, 0x9D, 0x9F, 0xA0, 0xA2, 0xA4, 0xA5, 0xA7, 0xA9, 0xAB, 0xAC, 0xAE, 0xB0, 0xB2, 0xB4, 0xB6, 0xB7, 0xB9, 0xBB, 0xBD, 0xBF, 0xC1, 0xC3, 0xC4, 0xC6, 0xC8, 0xCA, 0xCC, 0xCE, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC, 0xDE, 0xE0, 0xE2, 0xE4, 0xE6, 0xE8, 0xEB, 0xED, 0xEF, 0xF1, 0xF3, 0xF5, 0xF7, 0xFA, 0xFC, 0xFE, }; static void fsl_dcu_drm_init_gamma_tables(struct drm_crtc *crtc, uint32_t size) { unsigned int i; struct drm_color_lut lut[size]; for (i = 0; i < size; i++) { lut[i].red = gamma_default_lut[i]; lut[i].green = gamma_default_lut[i]; lut[i].blue = gamma_default_lut[i]; } fsl_crtc_gamma_set(crtc, lut, size); } And call "fsl_dcu_drm_init_gamma_tables" within "fsl_dcu_drm_crtc_create". But it seems not graceful putting a static array within the source code. Or generate that array within "fsl_dcu_drm_init_gamma_tables"? Best Regards, Meng > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Sep 26, 2016 at 06:04:09AM +0000, Meng Yi wrote: > > > > On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote: > > > On 2016-09-13 01:49, Meng Yi wrote: > > > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig > > > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 > > > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > > > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > > > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU > > > >> > help > > > >> > Choose this option if you have an Freescale DCU chipset. > > > >> > If M is selected the module will be called fsl-dcu-drm. > > > >> > + > > > >> > +config DRM_FSL_DCU_GAMMA > > > >> > + bool "Gamma Correction Support for NXP/Freescale DCU" > > > >> > + depends on DRM_FSL_DCU > > > >> > + help > > > >> > + Enable support for gamma correction. > > > >> > > > >> What is the reason for making this a configuration option? Are > > > >> there implementation without support for the Gamma tables? > > > >> > > > > When gamma correction is enabled, the color won't display normally > > > > since The gamma tables are not filled with correct data. So I give a > > > > choice to not using The gamma correction when you don't want to use > > > > it. > > > > > > > > Should I remove this configuration? > > > > > > Yeah making this a compile time configuration seems wrong to me. > > > > > > I guess we should fill the table with a reasonable default then. The > > > omapdrm driver seems to do something similar. > > > > Yup Kconfig is definitely not the correct way to fix a setup problem in your > > driver. Note that even when you enable gamma lut userspace is allowed to set > > a NULL table, and in that case you need to either disable the gamma lut, or fill it > > with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table > > will also be NULL (except when you have special setup code), and if that results > > in a black screen your driver is buggy. Papering over a driver bug with Kconfig is > > definitely not ok. > > How about initialize the tables with static array(such as gmma = 2.2, etc) Linear ramp as the default (which is dead easy to generate) should be good enough imo, but I guess you can go with something else too. -Daniel > > /*gamma = 2.2*/ > u32 gamma_default_lut[] = { > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, > 0x02, 0x02, 0x02, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x04, 0x04, 0x04, 0x05, 0x05, 0x05, > 0x06, 0x06, 0x06, 0x07, 0x07, 0x07, 0x08, 0x08, 0x08, 0x09, 0x09, 0x09, 0x0A, 0x0A, 0x0B, 0x0B, > 0x0B, 0x0C, 0x0C, 0x0D, 0x0D, 0x0E, 0x0E, 0x0E, 0x0F, 0x0F, 0x10, 0x10, 0x11, 0x11, 0x12, 0x13, > 0x13, 0x14, 0x14, 0x15, 0x15, 0x16, 0x17, 0x17, 0x18, 0x18, 0x19, 0x1A, 0x1A, 0x1B, 0x1C, 0x1C, > 0x1D, 0x1E, 0x1E, 0x1F, 0x20, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25, 0x26, 0x27, 0x27, 0x28, > 0x29, 0x2A, 0x2B, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32, 0x32, 0x33, 0x34, 0x35, 0x36, > 0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F, 0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47, > 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4F, 0x50, 0x51, 0x52, 0x53, 0x54, 0x56, 0x57, 0x58, 0x59, > 0x5B, 0x5C, 0x5D, 0x5E, 0x60, 0x61, 0x62, 0x64, 0x65, 0x66, 0x68, 0x69, 0x6A, 0x6C, 0x6D, 0x6F, > 0x70, 0x71, 0x73, 0x74, 0x76, 0x77, 0x79, 0x7A, 0x7C, 0x7D, 0x7F, 0x80, 0x82, 0x83, 0x85, 0x86, > 0x88, 0x89, 0x8B, 0x8C, 0x8E, 0x90, 0x91, 0x93, 0x95, 0x96, 0x98, 0x9A, 0x9B, 0x9D, 0x9F, 0xA0, > 0xA2, 0xA4, 0xA5, 0xA7, 0xA9, 0xAB, 0xAC, 0xAE, 0xB0, 0xB2, 0xB4, 0xB6, 0xB7, 0xB9, 0xBB, 0xBD, > 0xBF, 0xC1, 0xC3, 0xC4, 0xC6, 0xC8, 0xCA, 0xCC, 0xCE, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC, > 0xDE, 0xE0, 0xE2, 0xE4, 0xE6, 0xE8, 0xEB, 0xED, 0xEF, 0xF1, 0xF3, 0xF5, 0xF7, 0xFA, 0xFC, 0xFE, > }; > > static void fsl_dcu_drm_init_gamma_tables(struct drm_crtc *crtc, uint32_t size) > { > unsigned int i; > struct drm_color_lut lut[size]; > > for (i = 0; i < size; i++) { > lut[i].red = gamma_default_lut[i]; > lut[i].green = gamma_default_lut[i]; > lut[i].blue = gamma_default_lut[i]; > } > > fsl_crtc_gamma_set(crtc, lut, size); > } > > And call "fsl_dcu_drm_init_gamma_tables" within "fsl_dcu_drm_crtc_create". > > But it seems not graceful putting a static array within the source code. Or generate that array within "fsl_dcu_drm_init_gamma_tables"? > > Best Regards, > Meng > > > -Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Mon, Sep 26, 2016 at 09:59:25AM +0200, Daniel Vetter wrote: > On Mon, Sep 26, 2016 at 06:04:09AM +0000, Meng Yi wrote: > > > > > > > On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote: > > > > On 2016-09-13 01:49, Meng Yi wrote: > > > > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig > > > > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 > > > > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > > > > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > > > > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU > > > > >> > help > > > > >> > Choose this option if you have an Freescale DCU chipset. > > > > >> > If M is selected the module will be called fsl-dcu-drm. > > > > >> > + > > > > >> > +config DRM_FSL_DCU_GAMMA > > > > >> > + bool "Gamma Correction Support for NXP/Freescale DCU" > > > > >> > + depends on DRM_FSL_DCU > > > > >> > + help > > > > >> > + Enable support for gamma correction. > > > > >> > > > > >> What is the reason for making this a configuration option? Are > > > > >> there implementation without support for the Gamma tables? > > > > >> > > > > > When gamma correction is enabled, the color won't display normally > > > > > since The gamma tables are not filled with correct data. So I give a > > > > > choice to not using The gamma correction when you don't want to use > > > > > it. > > > > > > > > > > Should I remove this configuration? > > > > > > > > Yeah making this a compile time configuration seems wrong to me. > > > > > > > > I guess we should fill the table with a reasonable default then. The > > > > omapdrm driver seems to do something similar. > > > > > > Yup Kconfig is definitely not the correct way to fix a setup problem in your > > > driver. Note that even when you enable gamma lut userspace is allowed to set > > > a NULL table, and in that case you need to either disable the gamma lut, or fill it > > > with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table > > > will also be NULL (except when you have special setup code), and if that results > > > in a black screen your driver is buggy. Papering over a driver bug with Kconfig is > > > definitely not ok. > > > > How about initialize the tables with static array(such as gmma = 2.2, etc) > > Linear ramp as the default (which is dead easy to generate) should be good > enough imo, but I guess you can go with something else too. Linear would seem like a sane default choice to me as photos/videos etc. are typically stored with gamma included. And when the compositor/etc. is not gamma aware you might not want to apply the gamma twice for such things. > -Daniel > > > > > /*gamma = 2.2*/ > > u32 gamma_default_lut[] = { > > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, > > 0x02, 0x02, 0x02, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x04, 0x04, 0x04, 0x05, 0x05, 0x05, > > 0x06, 0x06, 0x06, 0x07, 0x07, 0x07, 0x08, 0x08, 0x08, 0x09, 0x09, 0x09, 0x0A, 0x0A, 0x0B, 0x0B, > > 0x0B, 0x0C, 0x0C, 0x0D, 0x0D, 0x0E, 0x0E, 0x0E, 0x0F, 0x0F, 0x10, 0x10, 0x11, 0x11, 0x12, 0x13, > > 0x13, 0x14, 0x14, 0x15, 0x15, 0x16, 0x17, 0x17, 0x18, 0x18, 0x19, 0x1A, 0x1A, 0x1B, 0x1C, 0x1C, > > 0x1D, 0x1E, 0x1E, 0x1F, 0x20, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25, 0x26, 0x27, 0x27, 0x28, > > 0x29, 0x2A, 0x2B, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32, 0x32, 0x33, 0x34, 0x35, 0x36, > > 0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F, 0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47, > > 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4F, 0x50, 0x51, 0x52, 0x53, 0x54, 0x56, 0x57, 0x58, 0x59, > > 0x5B, 0x5C, 0x5D, 0x5E, 0x60, 0x61, 0x62, 0x64, 0x65, 0x66, 0x68, 0x69, 0x6A, 0x6C, 0x6D, 0x6F, > > 0x70, 0x71, 0x73, 0x74, 0x76, 0x77, 0x79, 0x7A, 0x7C, 0x7D, 0x7F, 0x80, 0x82, 0x83, 0x85, 0x86, > > 0x88, 0x89, 0x8B, 0x8C, 0x8E, 0x90, 0x91, 0x93, 0x95, 0x96, 0x98, 0x9A, 0x9B, 0x9D, 0x9F, 0xA0, > > 0xA2, 0xA4, 0xA5, 0xA7, 0xA9, 0xAB, 0xAC, 0xAE, 0xB0, 0xB2, 0xB4, 0xB6, 0xB7, 0xB9, 0xBB, 0xBD, > > 0xBF, 0xC1, 0xC3, 0xC4, 0xC6, 0xC8, 0xCA, 0xCC, 0xCE, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC, > > 0xDE, 0xE0, 0xE2, 0xE4, 0xE6, 0xE8, 0xEB, 0xED, 0xEF, 0xF1, 0xF3, 0xF5, 0xF7, 0xFA, 0xFC, 0xFE, > > }; > > > > static void fsl_dcu_drm_init_gamma_tables(struct drm_crtc *crtc, uint32_t size) > > { > > unsigned int i; > > struct drm_color_lut lut[size]; > > > > for (i = 0; i < size; i++) { > > lut[i].red = gamma_default_lut[i]; > > lut[i].green = gamma_default_lut[i]; > > lut[i].blue = gamma_default_lut[i]; > > } > > > > fsl_crtc_gamma_set(crtc, lut, size); > > } > > > > And call "fsl_dcu_drm_init_gamma_tables" within "fsl_dcu_drm_crtc_create". > > > > But it seems not graceful putting a static array within the source code. Or generate that array within "fsl_dcu_drm_init_gamma_tables"? > > > > Best Regards, > > Meng > > > > > -Daniel > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2016-09-25 23:04, Meng Yi wrote: >> On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote: >> > On 2016-09-13 01:49, Meng Yi wrote: >> > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig >> > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 >> > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig >> > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig >> > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU >> > >> > help >> > >> > Choose this option if you have an Freescale DCU chipset. >> > >> > If M is selected the module will be called fsl-dcu-drm. >> > >> > + >> > >> > +config DRM_FSL_DCU_GAMMA >> > >> > + bool "Gamma Correction Support for NXP/Freescale DCU" >> > >> > + depends on DRM_FSL_DCU >> > >> > + help >> > >> > + Enable support for gamma correction. >> > >> >> > >> What is the reason for making this a configuration option? Are >> > >> there implementation without support for the Gamma tables? >> > >> >> > > When gamma correction is enabled, the color won't display normally >> > > since The gamma tables are not filled with correct data. So I give a >> > > choice to not using The gamma correction when you don't want to use >> > > it. >> > > >> > > Should I remove this configuration? >> > >> > Yeah making this a compile time configuration seems wrong to me. >> > >> > I guess we should fill the table with a reasonable default then. The >> > omapdrm driver seems to do something similar. >> >> Yup Kconfig is definitely not the correct way to fix a setup problem in your >> driver. Note that even when you enable gamma lut userspace is allowed to set >> a NULL table, and in that case you need to either disable the gamma lut, or fill it >> with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table >> will also be NULL (except when you have special setup code), and if that results >> in a black screen your driver is buggy. Papering over a driver bug with Kconfig is >> definitely not ok. > > How about initialize the tables with static array(such as gmma = 2.2, etc) The hardware allows to disable gamma, so I would rather prefer to only enable it when a valid table is set, and disable gamma at startup and when a NULL table is set. This also makes sure what we get the least interference with what we are doing now. -- Stefan
diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt b/Documentation/devicetree/bindings/display/fsl,dcu.txt index 63ec2a6..1b1321a 100644 --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt @@ -20,7 +20,11 @@ Optional properties: Examples: dcu: dcu@2ce0000 { compatible = "fsl,ls1021a-dcu"; - reg = <0x0 0x2ce0000 0x0 0x10000>; + reg = <0x0 0x2ce0000 0x0 0x2000>, + <0x0 0x2ce2000 0x0 0x2000>, + <0x0 0x2ce4000 0x0 0xc00>, + <0x0 0x2ce4c00 0x0 0x400>; + reg-names = "regs", "palette", "gamma", "cursor"; clocks = <&platform_clk 0>, <&platform_clk 0>; clock-names = "dcu", "pix"; big-endian; diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644 --- a/drivers/gpu/drm/fsl-dcu/Kconfig +++ b/drivers/gpu/drm/fsl-dcu/Kconfig @@ -11,3 +11,9 @@ config DRM_FSL_DCU help Choose this option if you have an Freescale DCU chipset. If M is selected the module will be called fsl-dcu-drm. + +config DRM_FSL_DCU_GAMMA + bool "Gamma Correction Support for NXP/Freescale DCU" + depends on DRM_FSL_DCU + help + Enable support for gamma correction. diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c index 3371635..25ab0ff 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c @@ -22,6 +22,22 @@ #include "fsl_dcu_drm_drv.h" #include "fsl_dcu_drm_plane.h" +static void fsl_crtc_gamma_set(struct drm_crtc *crtc, struct drm_color_lut *lut, + uint32_t size) +{ + struct fsl_dcu_drm_device *fsl_dev = crtc->dev->dev_private; + unsigned int i; + + for (i = 0; i < size; i++) { + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_R + 4 * i, + lut[i].red); + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_G + 4 * i, + lut[i].green); + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_B + 4 * i, + lut[i].blue); + } +} + static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { @@ -37,6 +53,11 @@ static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc, drm_crtc_send_vblank_event(crtc, event); spin_unlock_irq(&crtc->dev->event_lock); } + + if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) + fsl_crtc_gamma_set(crtc, (struct drm_color_lut *) + crtc->state->gamma_lut->data, + 256); } static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) @@ -46,6 +67,10 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) drm_crtc_vblank_off(crtc); + if (fsl_dev->enable_color_mgmt) + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, + DCU_MODE_EN_GAMMA_MASK, 0); + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, DCU_MODE_DCU_MODE_MASK, DCU_MODE_DCU_MODE(DCU_MODE_OFF)); @@ -58,6 +83,11 @@ static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; + if (fsl_dev->enable_color_mgmt) + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, + DCU_MODE_EN_GAMMA_MASK, + DCU_MODE_GAMMA_ENABLE); + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, DCU_MODE_DCU_MODE_MASK, DCU_MODE_DCU_MODE(DCU_MODE_NORMAL)); @@ -135,6 +165,7 @@ static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = { .page_flip = drm_atomic_helper_page_flip, .reset = drm_atomic_helper_crtc_reset, .set_config = drm_atomic_helper_set_config, + .gamma_set = drm_atomic_helper_legacy_gamma_set, }; int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) @@ -158,5 +189,12 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs); +#ifdef CONFIG_DRM_FSL_DCU_GAMMA + fsl_dev->enable_color_mgmt = true; + + drm_crtc_enable_color_mgmt(crtc, 0, false, 256); + drm_mode_crtc_set_gamma_size(crtc, 256); +#endif + return 0; } diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c index 7882387..f70ea68 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c @@ -48,6 +48,15 @@ static const struct regmap_config fsl_dcu_regmap_config = { .volatile_reg = fsl_dcu_drm_is_volatile_reg, }; +static const struct regmap_config fsl_dcu_regmap_gamma_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .val_format_endian = REGMAP_ENDIAN_LITTLE, + + .volatile_reg = fsl_dcu_drm_is_volatile_reg, +}; + static int fsl_dcu_drm_irq_init(struct drm_device *dev) { struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; @@ -323,7 +332,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev) struct drm_device *drm; struct device *dev = &pdev->dev; struct resource *res; - void __iomem *base; + void __iomem *base, *base_gamma; struct drm_driver *driver = &fsl_dcu_drm_driver; struct clk *pix_clk_in; char pix_clk_name[32]; @@ -365,6 +374,25 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev) return PTR_ERR(fsl_dev->regmap); } + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma"); + if (!res) { + dev_err(dev, "could not get gamma memory resource\n"); + return -ENODEV; + } + + base_gamma = devm_ioremap_resource(dev, res); + if (IS_ERR(base_gamma)) { + ret = PTR_ERR(base_gamma); + return ret; + } + + fsl_dev->regmap_gamma = devm_regmap_init_mmio(dev, base_gamma, + &fsl_dcu_regmap_config); + if (IS_ERR(fsl_dev->regmap_gamma)) { + dev_err(dev, "regmap_gamma init failed\n"); + return PTR_ERR(fsl_dev->regmap_gamma); + } + fsl_dev->clk = devm_clk_get(dev, "dcu"); if (IS_ERR(fsl_dev->clk)) { dev_err(dev, "failed to get dcu clock\n"); diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h index 3b371fe7..95427f6 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h @@ -25,6 +25,8 @@ #define DCU_MODE_NORMAL 1 #define DCU_MODE_TEST 2 #define DCU_MODE_COLORBAR 3 +#define DCU_MODE_EN_GAMMA_MASK 0x04 +#define DCU_MODE_GAMMA_ENABLE BIT(2) #define DCU_BGND 0x0014 #define DCU_BGND_R(x) ((x) << 16) @@ -165,6 +167,10 @@ #define VF610_LAYER_REG_NUM 9 #define LS1021A_LAYER_REG_NUM 10 +#define FSL_GAMMA_R 0x000 +#define FSL_GAMMA_G 0x400 +#define FSL_GAMMA_B 0x800 + struct clk; struct device; struct drm_device; @@ -182,6 +188,7 @@ struct fsl_dcu_drm_device { struct device *dev; struct device_node *np; struct regmap *regmap; + struct regmap *regmap_gamma; int irq; struct clk *clk; struct clk *pix_clk; @@ -195,6 +202,7 @@ struct fsl_dcu_drm_device { struct fsl_dcu_drm_connector connector; const struct fsl_dcu_soc_data *soc; struct drm_atomic_state *state; + bool enable_color_mgmt; }; void fsl_dcu_fbdev_init(struct drm_device *dev);
Gamma correction is optional and can be used to adjust the color output values to match the gamut of a particular TFT LCD panel Errata: Gamma_R, Gamma_G and Gamma_B registers are little-endian registers while the rest of the address-space in 2D-ACE is big-endian. Workaround: Split the DCU regs into "regs", "palette", "gamma" and "cursor". Create a second regmap for gamma memory space using little endian. Suggested-by: Stefan Agner <stefan@agner.ch> Signed-off-by: Meng Yi <meng.yi@nxp.com> --- Changes in V3: -created a second regmap for gamma -updated the DCU DT binding --- .../devicetree/bindings/display/fsl,dcu.txt | 6 +++- drivers/gpu/drm/fsl-dcu/Kconfig | 6 ++++ drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 38 ++++++++++++++++++++++ drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 30 ++++++++++++++++- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 8 +++++ 5 files changed, 86 insertions(+), 2 deletions(-)