[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191001192015.GB21064@pendragon.ideasonboard.com>
Date: Tue, 1 Oct 2019 22:20:15 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ezequiel Garcia <ezequiel@...labora.com>
Cc: Jacopo Mondi <jacopo+renesas@...ndi.org>,
kieran.bingham+renesas@...asonboard.com, geert@...ux-m68k.org,
horms@...ge.net.au, uli+renesas@...nd.eu,
VenkataRajesh.Kalakodima@...bosch.com, airlied@...ux.ie,
daniel@...ll.ch, muroya@....co.jp, koji.matsuoka.xm@...esas.com,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org,
Harsha.ManjulaMallikarjun@...bosch.com,
Doug Anderson <dianders@...gle.com>,
Heiko Stuebner <heiko@...ech.de>
Subject: Re: [PATCH v4 8/9] drm: rcar-du: kms: Update CMM in atomic commit
tail
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@...nd.eu>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.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.
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists