diff mbox series

[v3,13/14] drm: rcar-du: kms: Update CMM in atomic commit tail

Message ID 20190825135154.11488-14-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series drm: rcar-du: Add Color Management Module (CMM) | expand

Commit Message

Jacopo Mondi Aug. 25, 2019, 1:51 p.m. UTC 
Update CMM settings at in the atomic commit tail helper method.

The CMM is updated with new gamma values provided to the driver
in the GAMMA_LUT blob property.

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Laurent Pinchart Aug. 27, 2019, midnight UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Sun, Aug 25, 2019 at 03:51:53PM +0200, Jacopo Mondi wrote:
> Update CMM settings at in the atomic commit tail helper method.
> 
> The CMM is updated with new gamma values provided to the driver
> in the GAMMA_LUT blob property.
> 
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 61ca1d3c379a..047fdb982a11 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/wait.h>
>  
> +#include "rcar_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
> @@ -368,6 +369,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>   * Atomic Check and Update
>   */
>  
> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> +					     struct drm_crtc_state *old_state)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	struct rcar_cmm_config cmm_config = {};
> +
> +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> +		return;
> +
> +	if (!crtc->state->gamma_lut) {
> +		cmm_config.lut.enable = false;
> +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +
> +		return;
> +	}
> +
> +	cmm_config.lut.enable = true;
> +	cmm_config.lut.table = (struct drm_color_lut *)
> +			       crtc->state->gamma_lut->data;
> +
> +	/* Set LUT table size to 0 if entries should not be updated. */
> +	if (!old_state->gamma_lut ||
> +	    old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> +		cmm_config.lut.size = crtc->state->gamma_lut->length
> +				    / sizeof(cmm_config.lut.table[0]);

It has just occurred to me that the hardware only support LUTs of
exactly 256 entries. Should we remove cmm_config.lut.size (simplifying
the code in the CMM driver), and add a check to the CRTC .atomic_check()
handler to reject invalid LUTs ? Sorry for not having caught this
earlier.

> +	else
> +		cmm_config.lut.size = 0;
> +
> +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +}
> +
>  static int rcar_du_atomic_check(struct drm_device *dev,
>  				struct drm_atomic_state *state)
>  {
> @@ -410,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  			rcdu->dpad1_source = rcrtc->index;
>  	}
>  
> +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> +
>  	/* Apply the atomic update. */
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  	drm_atomic_helper_commit_planes(dev, old_state,
Laurent Pinchart Aug. 27, 2019, 12:19 a.m. UTC | #2 
On Tue, Aug 27, 2019 at 03:00:17AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Sun, Aug 25, 2019 at 03:51:53PM +0200, Jacopo Mondi wrote:
> > Update CMM settings at in the atomic commit tail helper method.
> > 
> > The CMM is updated with new gamma values provided to the driver
> > in the GAMMA_LUT blob property.
> > 
> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 61ca1d3c379a..047fdb982a11 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/wait.h>
> >  
> > +#include "rcar_cmm.h"
> >  #include "rcar_du_crtc.h"
> >  #include "rcar_du_drv.h"
> >  #include "rcar_du_encoder.h"
> > @@ -368,6 +369,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> >   * Atomic Check and Update
> >   */
> >  
> > +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> > +					     struct drm_crtc_state *old_state)
> > +{
> > +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > +	struct rcar_cmm_config cmm_config = {};
> > +
> > +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> > +		return;
> > +
> > +	if (!crtc->state->gamma_lut) {
> > +		cmm_config.lut.enable = false;
> > +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > +
> > +		return;
> > +	}
> > +
> > +	cmm_config.lut.enable = true;
> > +	cmm_config.lut.table = (struct drm_color_lut *)
> > +			       crtc->state->gamma_lut->data;
> > +
> > +	/* Set LUT table size to 0 if entries should not be updated. */
> > +	if (!old_state->gamma_lut ||
> > +	    old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> > +		cmm_config.lut.size = crtc->state->gamma_lut->length
> > +				    / sizeof(cmm_config.lut.table[0]);
> 
> It has just occurred to me that the hardware only support LUTs of
> exactly 256 entries. Should we remove cmm_config.lut.size (simplifying
> the code in the CMM driver), and add a check to the CRTC .atomic_check()
> handler to reject invalid LUTs ? Sorry for not having caught this
> earlier.

Just an additional comment, if we drop the size field, then the
cmm_config.lut.table pointer should be set to NULL when the LUT contents
don't need to be updated.

> > +	else
> > +		cmm_config.lut.size = 0;
> > +
> > +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > +}
> > +
> >  static int rcar_du_atomic_check(struct drm_device *dev,
> >  				struct drm_atomic_state *state)
> >  {
> > @@ -410,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  			rcdu->dpad1_source = rcrtc->index;
> >  	}
> >  
> > +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> > +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> > +
> >  	/* Apply the atomic update. */
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Jacopo Mondi Aug. 27, 2019, 2:44 p.m. UTC | #3 
HI Laurent,

On Tue, Aug 27, 2019 at 03:19:27AM +0300, Laurent Pinchart wrote:
> On Tue, Aug 27, 2019 at 03:00:17AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Sun, Aug 25, 2019 at 03:51:53PM +0200, Jacopo Mondi wrote:
> > > Update CMM settings at in the atomic commit tail helper method.
> > >
> > > The CMM is updated with new gamma values provided to the driver
> > > in the GAMMA_LUT blob property.
> > >
> > > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > index 61ca1d3c379a..047fdb982a11 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/of_platform.h>
> > >  #include <linux/wait.h>
> > >
> > > +#include "rcar_cmm.h"
> > >  #include "rcar_du_crtc.h"
> > >  #include "rcar_du_drv.h"
> > >  #include "rcar_du_encoder.h"
> > > @@ -368,6 +369,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> > >   * Atomic Check and Update
> > >   */
> > >
> > > +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> > > +					     struct drm_crtc_state *old_state)
> > > +{
> > > +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > > +	struct rcar_cmm_config cmm_config = {};
> > > +
> > > +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> > > +		return;
> > > +
> > > +	if (!crtc->state->gamma_lut) {
> > > +		cmm_config.lut.enable = false;
> > > +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	cmm_config.lut.enable = true;
> > > +	cmm_config.lut.table = (struct drm_color_lut *)
> > > +			       crtc->state->gamma_lut->data;
> > > +
> > > +	/* Set LUT table size to 0 if entries should not be updated. */
> > > +	if (!old_state->gamma_lut ||
> > > +	    old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> > > +		cmm_config.lut.size = crtc->state->gamma_lut->length
> > > +				    / sizeof(cmm_config.lut.table[0]);
> >
> > It has just occurred to me that the hardware only support LUTs of

Where did you find this strict requirement ? I have tried programming
less than 256 entries in the 1-D LUT table, and it seems to me things
are working fine (from a visual inspection of the output image, I
don't see much differences from when I program the full table, maybe
that's an indication something is bad?)

Thanks
   j
> > exactly 256 entries. Should we remove cmm_config.lut.size (simplifying
> > the code in the CMM driver), and add a check to the CRTC .atomic_check()
> > handler to reject invalid LUTs ? Sorry for not having caught this
> > earlier.
>
> Just an additional comment, if we drop the size field, then the
> cmm_config.lut.table pointer should be set to NULL when the LUT contents
> don't need to be updated.
>
> > > +	else
> > > +		cmm_config.lut.size = 0;
> > > +
> > > +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> > > +}
> > > +
> > >  static int rcar_du_atomic_check(struct drm_device *dev,
> > >  				struct drm_atomic_state *state)
> > >  {
> > > @@ -410,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > >  			rcdu->dpad1_source = rcrtc->index;
> > >  	}
> > >
> > > +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> > > +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> > > +
> > >  	/* Apply the atomic update. */
> > >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > >  	drm_atomic_helper_commit_planes(dev, old_state,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Aug. 27, 2019, 4:38 p.m. UTC | #4 
Hi Jacopo,

On Tue, Aug 27, 2019 at 04:44:21PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 27, 2019 at 03:19:27AM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 27, 2019 at 03:00:17AM +0300, Laurent Pinchart wrote:
> >> On Sun, Aug 25, 2019 at 03:51:53PM +0200, Jacopo Mondi wrote:
> >>> Update CMM settings at in the atomic commit tail helper method.
> >>>
> >>> The CMM is updated with new gamma values provided to the driver
> >>> in the GAMMA_LUT blob property.
> >>>
> >>> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 35 +++++++++++++++++++++++++++
> >>>  1 file changed, 35 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> index 61ca1d3c379a..047fdb982a11 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> @@ -22,6 +22,7 @@
> >>>  #include <linux/of_platform.h>
> >>>  #include <linux/wait.h>
> >>>
> >>> +#include "rcar_cmm.h"
> >>>  #include "rcar_du_crtc.h"
> >>>  #include "rcar_du_drv.h"
> >>>  #include "rcar_du_encoder.h"
> >>> @@ -368,6 +369,37 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> >>>   * Atomic Check and Update
> >>>   */
> >>>
> >>> +static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
> >>> +					     struct drm_crtc_state *old_state)
> >>> +{
> >>> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> >>> +	struct rcar_cmm_config cmm_config = {};
> >>> +
> >>> +	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
> >>> +		return;
> >>> +
> >>> +	if (!crtc->state->gamma_lut) {
> >>> +		cmm_config.lut.enable = false;
> >>> +		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> >>> +
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	cmm_config.lut.enable = true;
> >>> +	cmm_config.lut.table = (struct drm_color_lut *)
> >>> +			       crtc->state->gamma_lut->data;
> >>> +
> >>> +	/* Set LUT table size to 0 if entries should not be updated. */
> >>> +	if (!old_state->gamma_lut ||
> >>> +	    old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
> >>> +		cmm_config.lut.size = crtc->state->gamma_lut->length
> >>> +				    / sizeof(cmm_config.lut.table[0]);
> >>
> >> It has just occurred to me that the hardware only support LUTs of
> 
> Where did you find this strict requirement ? I have tried programming
> less than 256 entries in the 1-D LUT table, and it seems to me things
> are working fine (from a visual inspection of the output image, I
> don't see much differences from when I program the full table, maybe
> that's an indication something is bad?)

Or maybe a previous write of the full 256 entries has initialised the
LUT correctly ?

There's no hardware register telling how many LUT entries the hardware
should use, and the documentation makes it quite clear that the LUT
contains 256 entries. It is indexed by the values of the 8-bit pixel
components, so it has to be written fully.

> >> exactly 256 entries. Should we remove cmm_config.lut.size (simplifying
> >> the code in the CMM driver), and add a check to the CRTC .atomic_check()
> >> handler to reject invalid LUTs ? Sorry for not having caught this
> >> earlier.
> >
> > Just an additional comment, if we drop the size field, then the
> > cmm_config.lut.table pointer should be set to NULL when the LUT contents
> > don't need to be updated.
> >
> >>> +	else
> >>> +		cmm_config.lut.size = 0;
> >>> +
> >>> +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> >>> +}
> >>> +
> >>>  static int rcar_du_atomic_check(struct drm_device *dev,
> >>>  				struct drm_atomic_state *state)
> >>>  {
> >>> @@ -410,6 +442,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >>>  			rcdu->dpad1_source = rcrtc->index;
> >>>  	}
> >>>
> >>> +	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
> >>> +		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
> >>> +
> >>>  	/* Apply the atomic update. */
> >>>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >>>  	drm_atomic_helper_commit_planes(dev, old_state,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 61ca1d3c379a..047fdb982a11 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -22,6 +22,7 @@ 
 #include <linux/of_platform.h>
 #include <linux/wait.h>
 
+#include "rcar_cmm.h"
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
 #include "rcar_du_encoder.h"
@@ -368,6 +369,37 @@  rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
  * Atomic Check and Update
  */
 
+static void rcar_du_atomic_commit_update_cmm(struct drm_crtc *crtc,
+					     struct drm_crtc_state *old_state)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	struct rcar_cmm_config cmm_config = {};
+
+	if (!rcrtc->cmm || !crtc->state->color_mgmt_changed)
+		return;
+
+	if (!crtc->state->gamma_lut) {
+		cmm_config.lut.enable = false;
+		rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+
+		return;
+	}
+
+	cmm_config.lut.enable = true;
+	cmm_config.lut.table = (struct drm_color_lut *)
+			       crtc->state->gamma_lut->data;
+
+	/* Set LUT table size to 0 if entries should not be updated. */
+	if (!old_state->gamma_lut ||
+	    old_state->gamma_lut->base.id != crtc->state->gamma_lut->base.id)
+		cmm_config.lut.size = crtc->state->gamma_lut->length
+				    / sizeof(cmm_config.lut.table[0]);
+	else
+		cmm_config.lut.size = 0;
+
+	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
+
 static int rcar_du_atomic_check(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
@@ -410,6 +442,9 @@  static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 			rcdu->dpad1_source = rcrtc->index;
 	}
 
+	for_each_old_crtc_in_state(old_state, crtc, crtc_state, i)
+		rcar_du_atomic_commit_update_cmm(crtc, crtc_state);
+
 	/* Apply the atomic update. */
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 	drm_atomic_helper_commit_planes(dev, old_state,