| Message ID | 20190825135154.11488-15-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: rcar-du: Add Color Management Module (CMM) | expand |
Hi Jacopo, (Question for Daniel below) Thank you for the patch. On Sun, Aug 25, 2019 at 03:51:54PM +0200, Jacopo Mondi wrote: > 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 any of the DRM color > transformation properties 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. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 +++++++++++++++++++++ > 1 file changed, 21 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..6e38495fb78f 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,26 @@ 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_existing_crtc_state(state, crtc); > + if (!crtc_state) > + continue; Shouldn't you get the new state here ? > + > + /* > + * 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->degamma_lut || > + crtc_state->ctm) We don't support degamma_lut or crm, so I would drop those. With these small issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Shouldn't we however squash this with the previous patch to avoid bisection issues ? > + crtc_state->color_mgmt_changed = true; Daniel, is this something that would make sense in the KMS core (or helpers) ? > + } > > return drm_mode_config_helper_resume(rcdu->ddev); > }
Hi Laurent, On Tue, Aug 27, 2019 at 03:05:17AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > (Question for Daniel below) > > Thank you for the patch. > > On Sun, Aug 25, 2019 at 03:51:54PM +0200, Jacopo Mondi wrote: > > 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 any of the DRM color > > transformation properties 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. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 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..6e38495fb78f 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,26 @@ 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_existing_crtc_state(state, crtc); > > + if (!crtc_state) > > + continue; > > Shouldn't you get the new state here ? > I have followed the drm_atomic_helper_suspend() call stack, that calls drm_atomic_helper_duplicate_state() which then assign the crtct state with drm_atomic_get_crtc_state(), where I read: crtc_state = drm_atomic_get_existing_crtc_state(state, crtc); ... state->crtcs[index].state = crtc_state; state->crtcs[index].old_state = crtc->state; state->crtcs[index].new_state = crtc_state; So state or new_state for the purpose of getting back the crtc state are the same if I'm not mistaken. > > + > > + /* > > + * 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->degamma_lut || > > + crtc_state->ctm) > > We don't support degamma_lut or crm, so I would drop those. yeah, I added them as it was less code to change when we'll support them. But for now they could be removed. > > With these small issues addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Shouldn't we however squash this with the previous patch to avoid > bisection issues ? > Which one in your opinion? "drm: rcar-du: kms: Update CMM in atomic commit tail" ? It seems to me they do quite different things though.. Thanks j > > + crtc_state->color_mgmt_changed = true; > > Daniel, is this something that would make sense in the KMS core (or > helpers) ? > > > + } > > > > return drm_mode_config_helper_resume(rcdu->ddev); > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Sep 05, 2019 at 12:58:09PM +0200, Jacopo Mondi wrote: > On Tue, Aug 27, 2019 at 03:05:17AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > (Question for Daniel below) > > > > Thank you for the patch. > > > > On Sun, Aug 25, 2019 at 03:51:54PM +0200, Jacopo Mondi wrote: > >> 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 any of the DRM color > >> transformation properties 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. > >> > >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 +++++++++++++++++++++ > >> 1 file changed, 21 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..6e38495fb78f 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,26 @@ 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_existing_crtc_state(state, crtc); > >> + if (!crtc_state) > >> + continue; > > > > Shouldn't you get the new state here ? > > I have followed the drm_atomic_helper_suspend() call stack, that calls > drm_atomic_helper_duplicate_state() which then assign the crtct state > with drm_atomic_get_crtc_state(), where I read: > > crtc_state = drm_atomic_get_existing_crtc_state(state, crtc); > ... > state->crtcs[index].state = crtc_state; > state->crtcs[index].old_state = crtc->state; > state->crtcs[index].new_state = crtc_state; > > So state or new_state for the purpose of getting back the crtc state > are the same if I'm not mistaken. It seems to be the case, but the documentation of drm_atomic_get_existing_crtc_state() states * This function is deprecated, @drm_atomic_get_old_crtc_state or * @drm_atomic_get_new_crtc_state should be used instead. I would thus use drm_atomic_get_new_crtc_state(). > >> + > >> + /* > >> + * 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->degamma_lut || > >> + crtc_state->ctm) > > > > We don't support degamma_lut or crm, so I would drop those. > > yeah, I added them as it was less code to change when we'll support > them. But for now they could be removed. > > > With these small issues addressed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Shouldn't we however squash this with the previous patch to avoid > > bisection issues ? > > Which one in your opinion? > "drm: rcar-du: kms: Update CMM in atomic commit tail" ? > It seems to me they do quite different things though.. Yes, but suspend/resume will be broken after 13/14 without 14/14. Not the end of the world, but not really nice if we need to bisect suspend/resume issues. > >> + crtc_state->color_mgmt_changed = true; > > > > Daniel, is this something that would make sense in the KMS core (or > > helpers) ? > > > >> + } > >> > >> return drm_mode_config_helper_resume(rcdu->ddev); > >> }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 018480a8f35c..6e38495fb78f 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,26 @@ 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_existing_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->degamma_lut || + crtc_state->ctm) + crtc_state->color_mgmt_changed = true; + } return drm_mode_config_helper_resume(rcdu->ddev); }
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 any of the DRM color transformation properties 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. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)