| Message ID | 1468572653-40332-1-git-send-email-meng.yi@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 15, 2016 at 04:50:53PM +0800, 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 > > Signed-off-by: Meng Yi <meng.yi@nxp.com> Please use the new atomic color management properties instead. There's a function to remap the old gamma table to these new properties for old userspace. -Daniel > --- > drivers/gpu/drm/fsl-dcu/Kconfig | 6 +++ > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 63 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 7 ++++ > 3 files changed, 76 insertions(+) > > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig > index b9c714d..ddfe3c4 100644 > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > @@ -16,3 +16,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..d8426b1 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > @@ -46,6 +46,11 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) > > drm_crtc_vblank_off(crtc); > > +#ifdef CONFIG_DRM_FSL_DCU_GAMMA > + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > + DCU_MODE_EN_GAMMA_MASK, > + DCU_MODE_GAMMA_DISABLE); > +#endif > regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > DCU_MODE_DCU_MODE_MASK, > DCU_MODE_DCU_MODE(DCU_MODE_OFF)); > @@ -58,6 +63,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; > > +#ifdef CONFIG_DRM_FSL_DCU_GAMMA > + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > + DCU_MODE_EN_GAMMA_MASK, > + DCU_MODE_GAMMA_ENABLE); > +#endif > regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > DCU_MODE_DCU_MODE_MASK, > DCU_MODE_DCU_MODE(DCU_MODE_NORMAL)); > @@ -128,6 +138,58 @@ static const struct drm_crtc_helper_funcs fsl_dcu_drm_crtc_helper_funcs = { > .mode_set_nofb = fsl_dcu_drm_crtc_mode_set_nofb, > }; > > +/* > + * 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. 2D-ACE Gamma_R, > + * Gamma_G and Gamma_B registers are 32 bit registers, where the first 24 > + * bits are reserved and last 8 bits denote the gamma value. Because of a > + * connection issue in the device, the first 8-bit [31:24] is connected and > + * the rest of the 24-bits[23:0] are reserved. > + * Workaround: Perform the byte_swapping for Gamma_[R/G/B]_registers. > + * For example: While writing 0000_00ABh to any of the gamma registers, byte > + * swap the data so it results in AB00_0000h. Write this value to the gamma > + * register. > + */ > +static u32 swap_bytes(u16 var) > +{ > + union res { > + char byte[4]; > + u32 val; > + }; > + union res in, out; > + unsigned int i = 0; > + > + in.val = var; > + for (i = 0; i < 4; i++) > + out.byte[i] = in.byte[3-i]; > + > + return out.val; > +} > + > +static int fsl_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, > + uint32_t size) > +{ > + struct rgb { > + u32 r[size], g[size], b[size]; > + }; > + > + struct drm_device *dev = crtc->dev; > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > + unsigned int i; > + struct rgb glut; > + > + for (i = 0; i < size; i++) { > + glut.r[i] = swap_bytes(r[i]); > + glut.g[i] = swap_bytes(g[i]); > + glut.b[i] = swap_bytes(b[i]); > + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i, glut.r[i]); > + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i, glut.g[i]); > + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i, glut.b[i]); > + } > + > + return 0; > +} > + > static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = { > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > @@ -135,6 +197,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 = fsl_crtc_gamma_set, > }; > > int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) > 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..d3bc540 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,9 @@ > #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_MODE_GAMMA_DISABLE 0 > > #define DCU_BGND 0x0014 > #define DCU_BGND_R(x) ((x) << 16) > @@ -165,6 +168,10 @@ > #define VF610_LAYER_REG_NUM 9 > #define LS1021A_LAYER_REG_NUM 10 > > +#define FSL_GAMMA_R 0x4000 > +#define FSL_GAMMA_G 0x4400 > +#define FSL_GAMMA_B 0x4800 > + > struct clk; > struct device; > struct drm_device; > -- > 2.1.0.27.g96db324 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Meng, hi Mark, [added Mark Brown to discuss the endian issue] On 2016-07-15 01:50, 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 > > Signed-off-by: Meng Yi <meng.yi@nxp.com> > --- > drivers/gpu/drm/fsl-dcu/Kconfig | 6 +++ > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 63 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 7 ++++ > 3 files changed, 76 insertions(+) > > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig > index b9c714d..ddfe3c4 100644 > --- a/drivers/gpu/drm/fsl-dcu/Kconfig > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig > @@ -16,3 +16,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..d8426b1 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > @@ -46,6 +46,11 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) > > drm_crtc_vblank_off(crtc); > > +#ifdef CONFIG_DRM_FSL_DCU_GAMMA > + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > + DCU_MODE_EN_GAMMA_MASK, > + DCU_MODE_GAMMA_DISABLE); > +#endif > regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > DCU_MODE_DCU_MODE_MASK, > DCU_MODE_DCU_MODE(DCU_MODE_OFF)); > @@ -58,6 +63,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; > > +#ifdef CONFIG_DRM_FSL_DCU_GAMMA > + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > + DCU_MODE_EN_GAMMA_MASK, > + DCU_MODE_GAMMA_ENABLE); > +#endif > regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, > DCU_MODE_DCU_MODE_MASK, > DCU_MODE_DCU_MODE(DCU_MODE_NORMAL)); > @@ -128,6 +138,58 @@ static const struct drm_crtc_helper_funcs > fsl_dcu_drm_crtc_helper_funcs = { > .mode_set_nofb = fsl_dcu_drm_crtc_mode_set_nofb, > }; > > +/* > + * 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. 2D-ACE Gamma_R, > + * Gamma_G and Gamma_B registers are 32 bit registers, where the first 24 > + * bits are reserved and last 8 bits denote the gamma value. Because of a > + * connection issue in the device, the first 8-bit [31:24] is connected and > + * the rest of the 24-bits[23:0] are reserved. > + * Workaround: Perform the byte_swapping for Gamma_[R/G/B]_registers. > + * For example: While writing 0000_00ABh to any of the gamma registers, byte > + * swap the data so it results in AB00_0000h. Write this value to the gamma > + * register. That really sounds like a big-endian register to me then... > + */ > +static u32 swap_bytes(u16 var) We certainly don't want a byte swapping function in the driver. I am sure Linux has appropriate functions for that already, however, I am not convinced that we need that at all. > +{ > + union res { > + char byte[4]; > + u32 val; > + }; > + union res in, out; > + unsigned int i = 0; > + > + in.val = var; > + for (i = 0; i < 4; i++) > + out.byte[i] = in.byte[3-i]; > + > + return out.val; > +} > + > +static int fsl_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, > + uint32_t size) > +{ > + struct rgb { > + u32 r[size], g[size], b[size]; > + }; > + > + struct drm_device *dev = crtc->dev; > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > + unsigned int i; > + struct rgb glut; > + > + for (i = 0; i < size; i++) { > + glut.r[i] = swap_bytes(r[i]); > + glut.g[i] = swap_bytes(g[i]); > + glut.b[i] = swap_bytes(b[i]); > + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i, glut.r[i]); > + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i, glut.g[i]); > + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i, glut.b[i]); I guess the problem is that regmap_write does byte swapping because ls1021a.dtsi defines the whole DCU register space to be big-endian. So you end up doing byte swapping twice. If the gamma area is really little-endian, then DCU on LS1021a seems to be quite a mess... :-( In this case, we probably should create a second regmap for the different areas specifically, e.g. change the device tree: reg = <0x0 0x2ce0000 0x0 0x2000 0x0 0x2ce2000 0x0 0x2000 0x0 0x2ce4000 0x0 0xc00 0x0 0x2ce4c00 0x0 0x400>; reg-names = "regs", "palette", "gamma", "cursor"; i /* Use Gamma is always little endian */ static const struct regmap_config fsl_dcu_regmap_gamma_config = { ... .val_format_endian = REGMAP_ENDIAN_LITTLE, ... }; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma"); base_gamma = devm_ioremap_resource(dev, res); fsl_dev->regmap_gamma = devm_regmap_init_mmio(...) regmap_write(fsl_dev->regmap_gamma, ...) @Mark, what do you think? Do we have a (better) solution for such cases? > + } > + > + return 0; > +} > + > static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = { > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > @@ -135,6 +197,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 = fsl_crtc_gamma_set, > }; > > int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) > 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..d3bc540 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,9 @@ > #define DCU_MODE_NORMAL 1 > #define DCU_MODE_TEST 2 > #define DCU_MODE_COLORBAR 3 > +#define DCU_MODE_EN_GAMMA_MASK 0x04 Nit: In cases where MASK is a single bit, you can use BIT(..)... > +#define DCU_MODE_GAMMA_ENABLE BIT(2) > +#define DCU_MODE_GAMMA_DISABLE 0 That sounds like a useless define to me. In the disable case, just use 0 in regmap_update_bits. The .._MASK shows which bit you clear. -- Stefan > > #define DCU_BGND 0x0014 > #define DCU_BGND_R(x) ((x) << 16) > @@ -165,6 +168,10 @@ > #define VF610_LAYER_REG_NUM 9 > #define LS1021A_LAYER_REG_NUM 10 > > +#define FSL_GAMMA_R 0x4000 > +#define FSL_GAMMA_G 0x4400 > +#define FSL_GAMMA_B 0x4800 > + > struct clk; > struct device; > struct drm_device;
On Fri, Sep 02, 2016 at 02:22:46PM -0700, Stefan Agner wrote: > Hi Meng, hi Mark, > > [added Mark Brown to discuss the endian issue] Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. > > + */ > > +static u32 swap_bytes(u16 var) > We certainly don't want a byte swapping function in the driver. I am > sure Linux has appropriate functions for that already, however, I am not > convinced that we need that at all. cpu_to_be16() and so on. > I guess the problem is that regmap_write does byte swapping because > ls1021a.dtsi defines the whole DCU register space to be big-endian. So > you end up doing byte swapping twice. > If the gamma area is really little-endian, then DCU on LS1021a seems to > be quite a mess... :-( Let's figure out what the hardware does first, espcially given that it's this chip where we seem to be seeing a lot of confusion about endianness issues. > @Mark, what do you think? Do we have a (better) solution for such cases? Is this area of the register map perhaps supposed to be accessed as a byte stream? Please don't add randomm characters that don't mean anything before people's names, it doesn't help make things legible especially when they're scanning through enormous quantities of text.
Hi Stefan, > > + */ > > +static u32 swap_bytes(u16 var) > > We certainly don't want a byte swapping function in the driver. I am sure Linux > has appropriate functions for that already, however, I am not convinced that > we need that at all. > Yeah, sure. Actually I had sent the V2 for this feature, which just using "<<24" to do this https://patchwork.kernel.org/patch/9252389/ ... > > + }; > > + > > + struct drm_device *dev = crtc->dev; > > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > + unsigned int i; > > + struct rgb glut; > > + > > + for (i = 0; i < size; i++) { > > + glut.r[i] = swap_bytes(r[i]); > > + glut.g[i] = swap_bytes(g[i]); > > + glut.b[i] = swap_bytes(b[i]); > > + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i, > glut.r[i]); > > + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i, > glut.g[i]); > > + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i, > glut.b[i]); > > I guess the problem is that regmap_write does byte swapping because > ls1021a.dtsi defines the whole DCU register space to be big-endian. So you end > up doing byte swapping twice. > > If the gamma area is really little-endian, then DCU on LS1021a seems to be > quite a mess... :-( > > In this case, we probably should create a second regmap for the different areas > specifically, e.g. change the device tree: > > reg = <0x0 0x2ce0000 0x0 0x2000 > 0x0 0x2ce2000 0x0 0x2000 > 0x0 0x2ce4000 0x0 0xc00 > 0x0 0x2ce4c00 0x0 0x400>; > > reg-names = "regs", "palette", "gamma", "cursor"; > i > /* Use Gamma is always little endian */ > static const struct regmap_config fsl_dcu_regmap_gamma_config = { ... > .val_format_endian = REGMAP_ENDIAN_LITTLE, ... > }; > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma"); > base_gamma = devm_ioremap_resource(dev, res); > > fsl_dev->regmap_gamma = devm_regmap_init_mmio(...) > > > regmap_write(fsl_dev->regmap_gamma, ...) > This is a errta for DCU, Gamma registers are supposed to be big endian, but actually it is little endian, do we Really need to create a second regmap? > > @Mark, what do you think? Do we have a (better) solution for such cases? > > > + } > > + > > + return 0; > > +} > > + > > static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = { > > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > > @@ -135,6 +197,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 = fsl_crtc_gamma_set, > > }; > > > > int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) 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..d3bc540 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,9 @@ > > #define DCU_MODE_NORMAL 1 > > #define DCU_MODE_TEST 2 > > #define DCU_MODE_COLORBAR 3 > > +#define DCU_MODE_EN_GAMMA_MASK 0x04 > > Nit: In cases where MASK is a single bit, you can use BIT(..)... > > > +#define DCU_MODE_GAMMA_ENABLE BIT(2) > > +#define DCU_MODE_GAMMA_DISABLE 0 > > That sounds like a useless define to me. In the disable case, just use 0 in > regmap_update_bits. The .._MASK shows which bit you clear. > Yeah, sure. Thanks. Best Regards, Meng
On 2016-09-04 19:28, Meng Yi wrote: > Hi Stefan, > >> > + */ >> > +static u32 swap_bytes(u16 var) >> >> We certainly don't want a byte swapping function in the driver. I am sure Linux >> has appropriate functions for that already, however, I am not convinced that >> we need that at all. >> > > Yeah, sure. Actually I had sent the V2 for this feature, which just > using "<<24" to do this > https://patchwork.kernel.org/patch/9252389/ > Oh sorry, missed that patch. However, the discussion below is still valid: > ... > >> > + }; >> > + >> > + struct drm_device *dev = crtc->dev; >> > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; >> > + unsigned int i; >> > + struct rgb glut; >> > + >> > + for (i = 0; i < size; i++) { >> > + glut.r[i] = swap_bytes(r[i]); >> > + glut.g[i] = swap_bytes(g[i]); >> > + glut.b[i] = swap_bytes(b[i]); >> > + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i, >> glut.r[i]); >> > + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i, >> glut.g[i]); >> > + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i, >> glut.b[i]); >> >> I guess the problem is that regmap_write does byte swapping because >> ls1021a.dtsi defines the whole DCU register space to be big-endian. So you end >> up doing byte swapping twice. >> >> If the gamma area is really little-endian, then DCU on LS1021a seems to be >> quite a mess... :-( >> >> In this case, we probably should create a second regmap for the different areas >> specifically, e.g. change the device tree: >> >> reg = <0x0 0x2ce0000 0x0 0x2000 >> 0x0 0x2ce2000 0x0 0x2000 >> 0x0 0x2ce4000 0x0 0xc00 >> 0x0 0x2ce4c00 0x0 0x400>; >> >> reg-names = "regs", "palette", "gamma", "cursor"; >> i >> /* Use Gamma is always little endian */ >> static const struct regmap_config fsl_dcu_regmap_gamma_config = { ... >> .val_format_endian = REGMAP_ENDIAN_LITTLE, ... >> }; >> >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma"); >> base_gamma = devm_ioremap_resource(dev, res); >> >> fsl_dev->regmap_gamma = devm_regmap_init_mmio(...) >> >> >> regmap_write(fsl_dev->regmap_gamma, ...) >> > > This is a errta for DCU, Gamma registers are supposed to be big > endian, but actually it is little endian, do we > Really need to create a second regmap? Do you have the exact wording of the errata? There are two problems to the current approach: First, the two byte swaps (yours and then the byte swap due to the big-endian configuration of regmap) seems ugly to me. Second, the gamma value is the lowest byte in Vybrid, and on Vybrid the regmap is configured little-endian. Hence your code won't work on Vybrid... Therefor I still think that using a second regmap would be nicer. -- Stefan
On 2016-09-03 03:49, Mark Brown wrote: > On Fri, Sep 02, 2016 at 02:22:46PM -0700, Stefan Agner wrote: >> I guess the problem is that regmap_write does byte swapping because >> ls1021a.dtsi defines the whole DCU register space to be big-endian. So >> you end up doing byte swapping twice. > >> If the gamma area is really little-endian, then DCU on LS1021a seems to >> be quite a mess... :-( > > Let's figure out what the hardware does first, espcially given that it's > this chip where we seem to be seeing a lot of confusion about endianness > issues. > According to Meng it is an errata for that whole area on that particular SoC (LS1021a)... Note that on Vybrid (the SoC I have on the table here) the whole DCU IP is little-endian, including the gamma area. >> @Mark, what do you think? Do we have a (better) solution for such cases? > > Is this area of the register map perhaps supposed to be accessed as a > byte stream? I don't think so, the Vybrid RM calls the area a table: The table is arranged as three separate memory blocks within the DCU4 memory map; one for each of the three color components. Each memory block has one entry for every possible 8-bit value and the entries are stored at 32-bit aligned addresses. This means that the upper 24 bits are not used while reading/writing the gamma memories. See Figure 16-22 for details of the memory arrangement. So, afaik, we deal with 3x 256 32-bit register which happen to be a different endianness on one SoC implementing the DCU IP... -- Stefan
Hi Stefan, > > > > This is a errta for DCU, Gamma registers are supposed to be big > > endian, but actually it is little endian, do we > > Really need to create a second regmap? > > Do you have the exact wording of the errata? Below is the description from hardware team. Affects: 2D-ACE Description: 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. 2D-ACE Gamma_R, Gamma_G and Gamma_B registers are 32 bit registers, where the first 24 bits are reserved and last 8 bits denote the gamma value. Because of a connection issue in the device, the first 8-bit [31:24] is connected and the rest of the 24-bits[23:0] are reserved. Impact: Gamma registers are not configurable. Workaround: Perform the byte_swapping for Gamma_[R/G/B]_registers. For example: While writing 0000_00ABh to any of the gamma registers, byte swap the data so it results in AB00_0000h. Write this value to the gamma register. > > There are two problems to the current approach: First, the two byte > swaps (yours and then the byte swap due to the big-endian configuration > of regmap) seems ugly to me. Second, the gamma value is the lowest byte > in Vybrid, and on Vybrid the regmap is configured little-endian. Hence > your code won't work on Vybrid... Therefor I still think that using a > second regmap would be nicer. > Oh sorry, I ignored there are two platforms. Using regmap according to DTS is nicer. Best Regards, Meng
On Mon, Sep 05, 2016 at 12:24:32AM -0700, Stefan Agner wrote: > So, afaik, we deal with 3x 256 32-bit register which happen to be a > different endianness on one SoC implementing the DCU IP... That does sound like a second regmap then.
> Subject: Re: [PATCH] drm/fsl-dcu: Add gamma set for crtc > > On Mon, Sep 05, 2016 at 12:24:32AM -0700, Stefan Agner wrote: > > > So, afaik, we deal with 3x 256 32-bit register which happen to be a > > different endianness on one SoC implementing the DCU IP... > > That does sound like a second regmap then. Here is the patch V3 for this https://patchwork.kernel.org/patch/9318711/ Will send V4 according to the comments. Best Regards, Meng
diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig index b9c714d..ddfe3c4 100644 --- a/drivers/gpu/drm/fsl-dcu/Kconfig +++ b/drivers/gpu/drm/fsl-dcu/Kconfig @@ -16,3 +16,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..d8426b1 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c @@ -46,6 +46,11 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc) drm_crtc_vblank_off(crtc); +#ifdef CONFIG_DRM_FSL_DCU_GAMMA + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, + DCU_MODE_EN_GAMMA_MASK, + DCU_MODE_GAMMA_DISABLE); +#endif regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, DCU_MODE_DCU_MODE_MASK, DCU_MODE_DCU_MODE(DCU_MODE_OFF)); @@ -58,6 +63,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; +#ifdef CONFIG_DRM_FSL_DCU_GAMMA + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, + DCU_MODE_EN_GAMMA_MASK, + DCU_MODE_GAMMA_ENABLE); +#endif regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE, DCU_MODE_DCU_MODE_MASK, DCU_MODE_DCU_MODE(DCU_MODE_NORMAL)); @@ -128,6 +138,58 @@ static const struct drm_crtc_helper_funcs fsl_dcu_drm_crtc_helper_funcs = { .mode_set_nofb = fsl_dcu_drm_crtc_mode_set_nofb, }; +/* + * 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. 2D-ACE Gamma_R, + * Gamma_G and Gamma_B registers are 32 bit registers, where the first 24 + * bits are reserved and last 8 bits denote the gamma value. Because of a + * connection issue in the device, the first 8-bit [31:24] is connected and + * the rest of the 24-bits[23:0] are reserved. + * Workaround: Perform the byte_swapping for Gamma_[R/G/B]_registers. + * For example: While writing 0000_00ABh to any of the gamma registers, byte + * swap the data so it results in AB00_0000h. Write this value to the gamma + * register. + */ +static u32 swap_bytes(u16 var) +{ + union res { + char byte[4]; + u32 val; + }; + union res in, out; + unsigned int i = 0; + + in.val = var; + for (i = 0; i < 4; i++) + out.byte[i] = in.byte[3-i]; + + return out.val; +} + +static int fsl_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, + uint32_t size) +{ + struct rgb { + u32 r[size], g[size], b[size]; + }; + + struct drm_device *dev = crtc->dev; + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; + unsigned int i; + struct rgb glut; + + for (i = 0; i < size; i++) { + glut.r[i] = swap_bytes(r[i]); + glut.g[i] = swap_bytes(g[i]); + glut.b[i] = swap_bytes(b[i]); + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i, glut.r[i]); + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i, glut.g[i]); + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i, glut.b[i]); + } + + return 0; +} + static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = { .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, @@ -135,6 +197,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 = fsl_crtc_gamma_set, }; int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) 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..d3bc540 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,9 @@ #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_MODE_GAMMA_DISABLE 0 #define DCU_BGND 0x0014 #define DCU_BGND_R(x) ((x) << 16) @@ -165,6 +168,10 @@ #define VF610_LAYER_REG_NUM 9 #define LS1021A_LAYER_REG_NUM 10 +#define FSL_GAMMA_R 0x4000 +#define FSL_GAMMA_G 0x4400 +#define FSL_GAMMA_B 0x4800 + struct clk; struct device; struct drm_device;
Gamma correction is optional and can be used to adjust the color output values to match the gamut of a particular TFT LCD panel Signed-off-by: Meng Yi <meng.yi@nxp.com> --- drivers/gpu/drm/fsl-dcu/Kconfig | 6 +++ drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 63 ++++++++++++++++++++++++++++++ drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 7 ++++ 3 files changed, 76 insertions(+)