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

2016-11-04 Thread Meng Yi 
Hi Stefan,

> 
> 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 just did few more test using libdrm which had been modified for testing 
gamma. 
And I find you are right. It's my negligence that just tested one case for 
gamma using all 0xff;


.

> > drm_crtc_send_vblank_event(crtc, event);
> > spin_unlock_irq(>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."
> 

Yes, now gamma correction function behavior weird.
When setting gamma table within crtc enable during initializing, it works 
correctly. And not right during runtime so far.


Thanks,
Meng

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

2016-10-28 Thread Stefan Agner 
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 
> Signed-off-by: Meng Yi 

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 at 2ce {
>   compatible = "fsl,ls1021a-dcu";
> - reg = <0x0 0x2ce 0x0 0x1>;
> + reg = <0x0 0x2ce 0x0 0x2000>,
> +   <0x0 0x2ce2000 0x0 0x2000>,
> +   <0x0 0x2ce4000 0x0 0xc00>,
> +   <0x0 0x2ce4c00 0x0 0x400>;
> + reg-names = "regs", "palette", "gamma", "cursor";
>   clocks = <_clk 0>, <_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,
>

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

2016-10-27 Thread Meng Yi 
> Subject: Re: [PATCH v5] drm/fsl-dcu: Implement gamma_lut atomic crtc
> properties
> 
> 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 
> > Signed-off-by: Meng Yi 
> > ---
> > 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"
> 
> Looks good to me, device tree folks?
> 
> --
> Stefan
> 

Tested-by: Meng Yi 

On LS1021A-TWR board.

Meng

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

2016-10-08 Thread Rob Herring 
On Wed, Sep 28, 2016 at 04:24:29PM +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
> 
> 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 
> Signed-off-by: Meng Yi 
> ---
> 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 +++-

Acked-by: Rob Herring 

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

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

2016-10-08 Thread Meng Yi 
Hi Stefan,

> > @@ -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"
> 
> Looks good to me, device tree folks?
> 

We had on holidays and sorry for reply late, device tree's mailing list and 
maintainers are included.

Best Regards,
Meng

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

2016-09-30 Thread Stefan Agner 
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 
> Signed-off-by: Meng Yi 
> ---
> 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"

Looks good to me, device tree folks?

--
Stefan

>  - 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 at 2ce {
>   compatible = "fsl,ls1021a-dcu";
> - reg = <0x0 0x2ce 0x0 0x1>;
> + reg = <0x0 0x2ce 0x0 0x2000>,
> +   <0x0 0x2ce2000 0x0 0x2000>,
> +   <0x0 0x2ce4000 0x0 0xc00>,
> +   <0x0 0x2ce4c00 0x0 0x400>;
> + reg-names = "regs", "palette", "gamma", "cursor";
>   clocks = <_clk 0>, <_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);
> + }
> +
> + 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(>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);
>  }
>  
>  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 =

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

2016-09-28 Thread 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

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 
Signed-off-by: Meng Yi 
---
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 at 2ce {
compatible = "fsl,ls1021a-dcu";
-   reg = <0x0 0x2ce 0x0 0x1>;
+   reg = <0x0 0x2ce 0x0 0x2000>,
+ <0x0 0x2ce2000 0x0 0x2000>,
+ <0x0 0x2ce4000 0x0 0xc00>,
+ <0x0 0x2ce4c00 0x0 0x400>;
+   reg-names = "regs", "palette", "gamma", "cursor";
clocks = <_clk 0>, <_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);
+   }
+
+   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(>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);
 }

 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,