Re: [PATCH v5] drm/fsl-dcu: Implement gamma_lut atomic crtc properties

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

 



Hi Meng,

On 2016-09-28 01:24, 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
> 
> Split the DCU regs into "regs", "palette", "gamma" and "cursor".
> Create a second regmap for gamma memory space using little endian.
> The registers after the first address space are not accessed yet,
> hence new device trees would even work with old kernels. Just new
> kernel need the new format so we can access the separate gamma
> reg space.
> 
> Suggested-by: Stefan Agner <stefan@xxxxxxxx>
> Signed-off-by: Meng Yi <meng.yi@xxxxxxx>

How did you actually test that? I have a hard time to get anything
useable with this code.

The display looks completely borked (colors are way off, and at random
either too dark or too bright).

I then also added Gamma 1.0 (and different values) to the Monitor
section of xorg.conf, but still not really usable.

I looked a bit more in depth into it, and some questions appeared, see
below:

> ---
> Changes since V1:
> -created a second regmap for gamma
> -updated the DCU DT binding
> -removed Kconfig for gamma and enable gamma when valid data filled.
> -extended and simplified comment lines.
> ---
>  .../devicetree/bindings/display/fsl,dcu.txt        | 12 +++++++-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c         | 33 ++++++++++++++++++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c          | 35 +++++++++++++++++++++-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h          |  7 +++++
>  4 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> index 63ec2a6..8140b5d 100644
> --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> @@ -6,6 +6,12 @@ Required properties:
>  	* "fsl,vf610-dcu".
>  
>  - reg:			Address and length of the register set for dcu.
> +			Must contain four address/length tuples:
> +			1. Register address space
> +			2. Palette/Tile address space
> +			3. Gamma address space
> +			4. Cursor address space
> +- reg-names:		Should be "regs", "palette", "gamma" and "cursor"
>  - clocks:		Handle to "dcu" and "pix" clock (in the order below)
>  			This can be the same clock (e.g. LS1021a)
>  			See ../clocks/clock-bindings.txt for details.
> @@ -20,7 +26,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/fsl_dcu_drm_crtc.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> index 3371635..6371e4d 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,31 @@
>  #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;
> +
> +	if (crtc->state->gamma_lut->data) {
> +		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);

I think you should not use the color values directly. They are also too
precise for DCU, DCU only support 8 bit.

You can use drm_color_lut_extract(.., 8) to get the 8-bit precision of
the LUT. See also:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html?highlight=gamma_lut#c.drm_color_lut_extract


> +		}
> +
> +		regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> +				   DCU_MODE_EN_GAMMA_MASK,
> +				   DCU_MODE_GAMMA_ENABLE);
> +	} else {
> +		regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> +				   DCU_MODE_EN_GAMMA_MASK, 0);
> +	}
> +}
> +
>  static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  					  struct drm_crtc_state *old_crtc_state)
>  {
> @@ -37,6 +62,10 @@ 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);

So this is called while the CRTC is enabled. Others do it there too, but
I think in our case we should not. The reference manual says:

"The gamma table can only be read or written when the 2D-ACE is not
enabled or during
the vertical blanking period."

This is the same for Vybrid and LS1021a.

I am not sure if we can time writes into the vertical blanking period,
are other drivers doing this?

So the only way I see that this would work is if we set it when the CRTC
is off. I guess mode_set_nofb or crtc enable callback would be
candidates.

Maybe somebody more familiar with the DRM subsystem can help us on the
right track?

>  }
>  
>  static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
> @@ -135,6 +164,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 +188,8 @@ int fsl_dcu_drm_crtc_create(struct
> fsl_dcu_drm_device *fsl_dev)
>  
>  	drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs);
>  
> +	drm_crtc_enable_color_mgmt(crtc, 0, false, 256);
> +	drm_mode_crtc_set_gamma_size(crtc, 256);
> +
>  	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 092aaec..e662890 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,20 @@ static const struct regmap_config fsl_dcu_regmap_config = {
>  	.volatile_reg = fsl_dcu_drm_is_volatile_reg,
>  };
>  
> +/*
> + * force using little endian here since ls1021a's gamma regs are little
> + * endian while the other regs are big endian, and all vf610s's regs
> + * are little endian
> + */
> +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;
> @@ -327,7 +341,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];
> @@ -370,6 +384,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) {

We should not fail here, since one could use a old device tree.

Just invert the if (to if (res)), and move all the gamma regmap setup
below into this if. And above, when enabling gamma, check if
fsl_dev->regmap_gamma is not null.

--
Stefan

> +		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_gamma_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..2610b6c 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;
_______________________________________________
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