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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux