[PATCH] drm/fsl-dcu: Add gamma set for crtc

[PATCH] drm/fsl-dcu: Add gamma set for crtc

[Meng Yi]

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.
+ */
+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

Re: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Daniel Vetter]

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

-- 
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

Re: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Stefan Agner]

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;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1439 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Meng Yi]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Stefan Agner]

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




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Stefan Agner]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Meng Yi]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 238 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Meng Yi]

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel