diff mbox series

[v4,8/9] drm: rcar-du: kms: Update CMM in atomic commit tail

Message ID 20190906135436.10622-9-jacopo+renesas@jmondi.org (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series [v4,1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation | expand

Commit Message

Jacopo Mondi Sept. 6, 2019, 1:54 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.

When resuming from system suspend, the DU driver is responsible for
reprogramming and enabling the CMM unit if it was in use at the time the
system entered the suspend state.  Force the color_mgmt_changed flag to
true if the DRM gamma lut color transformation property was set in the
CRTC state duplicated at suspend time, as the CMM gets reprogrammed only
if said flag is active in the rcar_du_atomic_commit_update_cmm() method.

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

Daniel could you have a look if resume bits are worth being moved to the
DRM core? The color_mgmt_changed flag is set to false when the state is
duplicated if I read the code correctly, but when this happens in a
suspend/resume sequence its value should probably be restored to true if
any color management property was set in the crtc state when system entered
suspend.

---

 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 20 ++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 38 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

--
2.23.0

Comments

Kieran Bingham Sept. 12, 2019, 9:51 a.m. UTC | #1 
Hi Jacopo,

On 06/09/2019 14:54, 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.
> 
> When resuming from system suspend, the DU driver is responsible for
> reprogramming and enabling the CMM unit if it was in use at the time the
> system entered the suspend state.  Force the color_mgmt_changed flag to
> true if the DRM gamma lut color transformation property was set in the
> CRTC state duplicated at suspend time, as the CMM gets reprogrammed only
> if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
> 
> 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>

Also throwing in an LGTM here.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



> ---
> 
> Daniel could you have a look if resume bits are worth being moved to the
> DRM core? The color_mgmt_changed flag is set to false when the state is
> duplicated if I read the code correctly, but when this happens in a
> suspend/resume sequence its value should probably be restored to true if
> any color management property was set in the crtc state when system entered
> suspend.
> 
> ---
> 
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 20 ++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 38 +++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 018480a8f35c..d1003d31cfaf 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
> 
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_fb_helper.h>
> @@ -482,6 +483,25 @@ static int rcar_du_pm_suspend(struct device *dev)
>  static int rcar_du_pm_resume(struct device *dev)
>  {
>  	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> +	struct drm_atomic_state *state = rcdu->ddev->mode_config.suspend_state;
> +	unsigned int i;
> +
> +	for (i = 0; i < rcdu->num_crtcs; ++i) {
> +		struct drm_crtc *crtc = &rcdu->crtcs[i].crtc;
> +		struct drm_crtc_state *crtc_state;
> +
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +		if (!crtc_state)
> +			continue;
> +
> +		/*
> +		 * Force re-enablement of CMM after system resume if any
> +		 * of the DRM color transformation properties was set in
> +		 * the state saved at system suspend time.
> +		 */
> +		if (crtc_state->gamma_lut)
> +			crtc_state->color_mgmt_changed = true;
> +	}
> 
>  	return drm_mode_config_helper_resume(rcdu->ddev);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 294630e56992..fc30fff0eb8d 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,40 @@ 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 = {};
> +	struct device *dev = rcrtc->dev->dev;
> +	struct drm_property_blob *lut_blob;
> +
> +	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;
> +	}
> +
> +	lut_blob = crtc->state->gamma_lut;
> +	if (lut_blob->length != (CM2_LUT_SIZE * sizeof(struct drm_color_lut))) {
> +		/*
> +		 * We only accept fully populated LUT tables;
> +		 * be loud here, otherwise the CMM gets silently ignored.
> +		 */
> +		dev_err(dev, "invalid gamma lut size of %lu bytes\n",
> +			lut_blob->length);
> +		return;
> +	}
> +
> +	cmm_config.lut.enable = true;
> +	cmm_config.lut.table = (struct drm_color_lut *)lut_blob->data;
> +
> +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +}
> +
>  static int rcar_du_atomic_check(struct drm_device *dev,
>  				struct drm_atomic_state *state)
>  {
> @@ -410,6 +445,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,
> --
> 2.23.0
>
Laurent Pinchart Sept. 20, 2019, 10:49 p.m. UTC | #2 
Hi Daniel,

On Fri, Sep 06, 2019 at 03:54:35PM +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.
> 
> When resuming from system suspend, the DU driver is responsible for
> reprogramming and enabling the CMM unit if it was in use at the time the
> system entered the suspend state.  Force the color_mgmt_changed flag to
> true if the DRM gamma lut color transformation property was set in the
> CRTC state duplicated at suspend time, as the CMM gets reprogrammed only
> if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
> 
> 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>
> ---
> 
> Daniel could you have a look if resume bits are worth being moved to the
> DRM core? The color_mgmt_changed flag is set to false when the state is
> duplicated if I read the code correctly, but when this happens in a
> suspend/resume sequence its value should probably be restored to true if
> any color management property was set in the crtc state when system entered
> suspend.

Gentle ping on this.

> ---
> 
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 20 ++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 38 +++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 018480a8f35c..d1003d31cfaf 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
> 
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_fb_helper.h>
> @@ -482,6 +483,25 @@ static int rcar_du_pm_suspend(struct device *dev)
>  static int rcar_du_pm_resume(struct device *dev)
>  {
>  	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
> +	struct drm_atomic_state *state = rcdu->ddev->mode_config.suspend_state;
> +	unsigned int i;
> +
> +	for (i = 0; i < rcdu->num_crtcs; ++i) {
> +		struct drm_crtc *crtc = &rcdu->crtcs[i].crtc;
> +		struct drm_crtc_state *crtc_state;
> +
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +		if (!crtc_state)
> +			continue;
> +
> +		/*
> +		 * Force re-enablement of CMM after system resume if any
> +		 * of the DRM color transformation properties was set in
> +		 * the state saved at system suspend time.
> +		 */
> +		if (crtc_state->gamma_lut)
> +			crtc_state->color_mgmt_changed = true;
> +	}
> 
>  	return drm_mode_config_helper_resume(rcdu->ddev);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 294630e56992..fc30fff0eb8d 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,40 @@ 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 = {};
> +	struct device *dev = rcrtc->dev->dev;
> +	struct drm_property_blob *lut_blob;
> +
> +	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;
> +	}
> +
> +	lut_blob = crtc->state->gamma_lut;
> +	if (lut_blob->length != (CM2_LUT_SIZE * sizeof(struct drm_color_lut))) {
> +		/*
> +		 * We only accept fully populated LUT tables;
> +		 * be loud here, otherwise the CMM gets silently ignored.
> +		 */
> +		dev_err(dev, "invalid gamma lut size of %lu bytes\n",
> +			lut_blob->length);
> +		return;
> +	}
> +
> +	cmm_config.lut.enable = true;
> +	cmm_config.lut.table = (struct drm_color_lut *)lut_blob->data;
> +
> +	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
> +}
> +
>  static int rcar_du_atomic_check(struct drm_device *dev,
>  				struct drm_atomic_state *state)
>  {
> @@ -410,6 +445,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,
Ezequiel Garcia Sept. 30, 2019, 8:53 p.m. UTC | #3 
+Doug, Heiko:

On Fri, 2019-09-06 at 15:54 +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.
> 
> When resuming from system suspend, the DU driver is responsible for
> reprogramming and enabling the CMM unit if it was in use at the time the
> system entered the suspend state.  Force the color_mgmt_changed flag to
> true if the DRM gamma lut color transformation property was set in the
> CRTC state duplicated at suspend time, as the CMM gets reprogrammed only
> if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
> 
> 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>
> ---
> 
> Daniel could you have a look if resume bits are worth being moved to the
> DRM core? The color_mgmt_changed flag is set to false when the state is
> duplicated if I read the code correctly, but when this happens in a
> suspend/resume sequence its value should probably be restored to true if
> any color management property was set in the crtc state when system entered
> suspend.
> 

Perhaps we can use the for_each_new_crtc_in_state() helper here,
and move it to the core like this:

--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3234,8 +3234,20 @@ int drm_atomic_helper_resume(struct
drm_device *dev,
                             struct drm_atomic_state *state)
 {
        struct drm_modeset_acquire_ctx ctx;
+       struct drm_crtc_state
*crtc_state;
+       struct drm_crtc *crtc;
+       unsigned int i;
        int err;
 
+       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+   
            /*
+                * Force re-enablement of CMM after system resume if any
+                * of the DRM color transformation properties
was set in
+                * the state saved at system suspend time.
+                */
+               if (crtc_state->gamma_lut)
+                    
   crtc_state->color_mgmt_changed = true;
+       }

This probably is wrong, and should be instead constrained to some
condition of some sort.

FWIW, the Rockchip DRM is going to need this as well.

Any ideas?

Thanks,
Ezequiel
Laurent Pinchart Oct. 1, 2019, 7:20 p.m. UTC | #4 
Hi Ezequiel,

On Mon, Sep 30, 2019 at 05:53:00PM -0300, Ezequiel Garcia wrote:
> +Doug, Heiko:
> 
> On Fri, 2019-09-06 at 15:54 +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.
> > 
> > When resuming from system suspend, the DU driver is responsible for
> > reprogramming and enabling the CMM unit if it was in use at the time the
> > system entered the suspend state.  Force the color_mgmt_changed flag to
> > true if the DRM gamma lut color transformation property was set in the
> > CRTC state duplicated at suspend time, as the CMM gets reprogrammed only
> > if said flag is active in the rcar_du_atomic_commit_update_cmm() method.
> > 
> > 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>
> > ---
> > 
> > Daniel could you have a look if resume bits are worth being moved to the
> > DRM core? The color_mgmt_changed flag is set to false when the state is
> > duplicated if I read the code correctly, but when this happens in a
> > suspend/resume sequence its value should probably be restored to true if
> > any color management property was set in the crtc state when system entered
> > suspend.
> 
> Perhaps we can use the for_each_new_crtc_in_state() helper here,
> and move it to the core like this:
> 
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3234,8 +3234,20 @@ int drm_atomic_helper_resume(struct
> drm_device *dev,
>                              struct drm_atomic_state *state)
>  {
>         struct drm_modeset_acquire_ctx ctx;
> +       struct drm_crtc_state
> *crtc_state;
> +       struct drm_crtc *crtc;
> +       unsigned int i;
>         int err;
>  
> +       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +   
>             /*
> +                * Force re-enablement of CMM after system resume if any
> +                * of the DRM color transformation properties
> was set in
> +                * the state saved at system suspend time.
> +                */
> +               if (crtc_state->gamma_lut)
> +                    
>    crtc_state->color_mgmt_changed = true;
> +       }
> 
> This probably is wrong, and should be instead constrained to some
> condition of some sort.
> 
> FWIW, the Rockchip DRM is going to need this as well.
> 
> Any ideas?

That's more or less what I had in mind, yes. The question is if
something like this would make sense. If there's a consensus it would, I
think it can be done as part of this series.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 018480a8f35c..d1003d31cfaf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -17,6 +17,7 @@ 
 #include <linux/slab.h>
 #include <linux/wait.h>

+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fb_helper.h>
@@ -482,6 +483,25 @@  static int rcar_du_pm_suspend(struct device *dev)
 static int rcar_du_pm_resume(struct device *dev)
 {
 	struct rcar_du_device *rcdu = dev_get_drvdata(dev);
+	struct drm_atomic_state *state = rcdu->ddev->mode_config.suspend_state;
+	unsigned int i;
+
+	for (i = 0; i < rcdu->num_crtcs; ++i) {
+		struct drm_crtc *crtc = &rcdu->crtcs[i].crtc;
+		struct drm_crtc_state *crtc_state;
+
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+		if (!crtc_state)
+			continue;
+
+		/*
+		 * Force re-enablement of CMM after system resume if any
+		 * of the DRM color transformation properties was set in
+		 * the state saved at system suspend time.
+		 */
+		if (crtc_state->gamma_lut)
+			crtc_state->color_mgmt_changed = true;
+	}

 	return drm_mode_config_helper_resume(rcdu->ddev);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 294630e56992..fc30fff0eb8d 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,40 @@  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 = {};
+	struct device *dev = rcrtc->dev->dev;
+	struct drm_property_blob *lut_blob;
+
+	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;
+	}
+
+	lut_blob = crtc->state->gamma_lut;
+	if (lut_blob->length != (CM2_LUT_SIZE * sizeof(struct drm_color_lut))) {
+		/*
+		 * We only accept fully populated LUT tables;
+		 * be loud here, otherwise the CMM gets silently ignored.
+		 */
+		dev_err(dev, "invalid gamma lut size of %lu bytes\n",
+			lut_blob->length);
+		return;
+	}
+
+	cmm_config.lut.enable = true;
+	cmm_config.lut.table = (struct drm_color_lut *)lut_blob->data;
+
+	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+}
+
 static int rcar_du_atomic_check(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
@@ -410,6 +445,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,