[v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties

Commit Message

Stefan Agner Sept. 7, 2016, 5:05 p.m. UTC

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 <[e-mail]>
> Signed-off-by: Meng Yi <[e-mail]>
> ---
> 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(-)
> 

Please also extend the documentation of reg and reg-names above.

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

Looks good to me, I feel splitting up the register map makes sense
anyway. It is also documented that way:

36.4 Memory Map
Table 36-5. Memory map

Parameter                        Address Range
Register address space           0x0000 – 0x1FFF
Palette/Tile address space       0x2000 – 0x3FFF
Gamma_R address space            0x4000 – 0x43FF
Gamma_G address space            0x4400 – 0x47FF
Gamma_B address space            0x4800 – 0x4BFF
Cursor address space             0x4C00 – 0x4FFF

Can I have an Ack from the device tree maintainers here?

<snip>

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

<snip>

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

I would like to have a comment here why we force little endian.

> +
> +	.volatile_reg = fsl_dcu_drm_is_volatile_reg,
> +};
> +

<snip>

> @@ -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);
> +	}
> +

Mark, what do you think, is this a reasonable approach to work around
this errata?

--
Stefan

Comments

Meng Yi Sept. 8, 2016, 2:45 a.m. UTC | #1

> 

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

> 

> Looks good to me, I feel splitting up the register map makes sense anyway. It is

> also documented that way:

> 

> 36.4 Memory Map

> Table 36-5. Memory map

> 

> Parameter                        Address Range

> Register address space           0x0000 – 0x1FFF

> Palette/Tile address space       0x2000 – 0x3FFF

> Gamma_R address space            0x4000 – 0x43FF

> Gamma_G address space            0x4400 – 0x47FF

> Gamma_B address space            0x4800 – 0x4BFF

> Cursor address space             0x4C00 – 0x4FFF


Thanks!^_^

> 

> Can I have an Ack from the device tree maintainers here?

> 

> <snip>

> 

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

> {

> 

> <snip>

> 

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

> 

> I would like to have a comment here why we force little endian.

> 


Oh, I was going to do that, but forgotten anyway.

> > +

> > +	.volatile_reg = fsl_dcu_drm_is_volatile_reg, };

> > +

> 

> <snip>

> 

> > @@ -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);

> > +	}

> > +

> 

> Mark, what do you think, is this a reasonable approach to work around this

> errata?


I was thinking is it possible to define a different endianness for each register map.

Best Regards,
Meng

Patch

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;