[PATCH v2 1/5] drm: Add helpers to kick off self refresh mode in drivers
Sean Paul
sean at poorly.run
Fri Mar 29 18:10:11 UTC 2019
On Fri, Mar 29, 2019 at 04:36:32PM +0100, Daniel Vetter wrote:
> On Fri, Mar 29, 2019 at 09:16:59AM -0400, Sean Paul wrote:
> > On Fri, Mar 29, 2019 at 09:21:10AM +0100, Daniel Vetter wrote:
> > > On Thu, Mar 28, 2019 at 05:03:03PM -0400, Sean Paul wrote:
> > > > On Wed, Mar 27, 2019 at 07:15:00PM +0100, Daniel Vetter wrote:
> > > > > On Tue, Mar 26, 2019 at 04:44:54PM -0400, Sean Paul wrote:
> > > > > > From: Sean Paul <seanpaul at chromium.org>
> > > > > >
> > > > > > This patch adds a new drm helper library to help drivers implement
> > > > > > self refresh. Drivers choosing to use it will register crtcs and
> > > > > > will receive callbacks when it's time to enter or exit self refresh
> > > > > > mode.
> > > > > >
> > > > > > In its current form, it has a timer which will trigger after a
> > > > > > driver-specified amount of inactivity. When the timer triggers, the
> > > > > > helpers will submit a new atomic commit to shut the refreshing pipe
> > > > > > off. On the next atomic commit, the drm core will revert the self
> > > > > > refresh state and bring everything back up to be actively driven.
> > > > > >
> > > > > > From the driver's perspective, this works like a regular disable/enable
> > > > > > cycle. The driver need only check the 'self_refresh_active' and/or
> > > > > > 'self_refresh_changed' state in crtc_state and connector_state. It
> > > > > > should initiate self refresh mode on the panel and enter an off or
> > > > > > low-power state.
> > > > > >
> > > > > > Changes in v2:
> > > > > > - s/psr/self_refresh/ (Daniel)
> > > > > > - integrated the psr exit into the commit that wakes it up (Jose/Daniel)
> > > > > > - made the psr state per-crtc (Jose/Daniel)
> > > > > >
> > > > > > Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-2-sean@poorly.run
> > > > > >
> > > > > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > > > > Cc: Jose Souza <jose.souza at intel.com>
> > > > > > Cc: Zain Wang <wzz at rock-chips.com>
> > > > > > Cc: Tomasz Figa <tfiga at chromium.org>
> > > > > > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > > > > > ---
> > > > > > Documentation/gpu/drm-kms-helpers.rst | 9 +
> > > > > > drivers/gpu/drm/Makefile | 3 +-
> > > > > > drivers/gpu/drm/drm_atomic.c | 4 +
> > > > > > drivers/gpu/drm/drm_atomic_helper.c | 36 +++-
> > > > > > drivers/gpu/drm/drm_atomic_state_helper.c | 8 +
> > > > > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +-
> > > > > > drivers/gpu/drm/drm_self_refresh_helper.c | 212 ++++++++++++++++++++++
> > > > > > include/drm/drm_atomic.h | 15 ++
> > > > > > include/drm/drm_connector.h | 31 ++++
> > > > > > include/drm/drm_crtc.h | 19 ++
> > > > > > include/drm/drm_self_refresh_helper.h | 23 +++
> > > > > > 11 files changed, 360 insertions(+), 5 deletions(-)
> > > > > > create mode 100644 drivers/gpu/drm/drm_self_refresh_helper.c
> > > > > > create mode 100644 include/drm/drm_self_refresh_helper.h
> > > > > >
> >
> > /snip
> >
> > > > > > index 4985384e51f6..ec90c527deed 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > > > @@ -105,6 +105,10 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > > > > > state->commit = NULL;
> > > > > > state->event = NULL;
> > > > > > state->pageflip_flags = 0;
> > > > > > +
> > > > > > + /* Self refresh should be canceled when a new update is available */
> > > > > > + state->active = drm_atomic_crtc_effectively_active(state);
> > > > > > + state->self_refresh_active = false;
> > > > > > }
> > > > > > EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
> > > > > >
> > > > > > @@ -370,6 +374,10 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> > > > > >
> > > > > > /* Don't copy over a writeback job, they are used only once */
> > > > > > state->writeback_job = NULL;
> > > > > > +
> > > > > > + /* Self refresh should be canceled when a new update is available */
> > > > > > + state->self_refresh_changed = state->self_refresh_active;
> > > > > > + state->self_refresh_active = false;
> > > > >
> > > > > Why the duplication in self-refresh tracking? Connectors never have a
> > > > > different self-refresh state, and you can always look at the right
> > > > > crtc_state. Duplication just gives us the chance to screw up and get out
> > > > > of sync (e.g. if the crtc for a connector changes).
> > > > >
> > > >
> > > > On disable the crtc is cleared from connector_state, so we don't have access to
> > > > it. If I add the appropriate atomic_enable/disable hooks as suggested below, we
> > > > should be able to nuke these.
> > >
> > > Yeah we'd need the old state to look at the crtc and all that. Which is a
> > > lot more trickier.
> > >
> > > Since it's such a special case, should we have a dedicated callback for
> > > the direct self-refresh -> completely off transition? It'll be asymetric,
> > > but that's the nature of this I think.
> >
> > Right, the asymmetry is really annoying here. If the driver is SR-aware, it makes
> > sense since SR-active to disable is a real transition. However if the driver is
> > not SR-aware (ie: it just gets turned off when SR becomes active), the disable
> > function gets called twice without an enable. So that changes the "for every
> > enable there is a disable and vice versa" assumption.
> >
> > This is one of the benefits of the v1 design, SR was bolted on and no existing
> > rules (async/no_modeset/enable-disable pairs) were [explicitly] broken. That's
> > not to say it was better, it wasn't, but it was a big consideration.
> >
> > So, what to do.
> >
> > I really like the idea that drivers shouldn't have to be SR-aware to be involved
> > in the pipeline. So if we add a hook for this like you suggest, we could avoid
> > calling disable twice on anything not SR-aware. We would need to add the hook on
> > crtc/encoder/bridge to make sure you could mix n' match SR-aware and
> > non-SR-aware devices.
> >
> > It probably makes sense to just add matching SR hooks at this point. Since if
> > the driver is doing something special in disable, it'll need to do something
> > special in enable. It also reserves enable and disable for what they've
> > traditionally done. If a device is not SR-aware, it'll just fall back to the
> > full enable/disable and we'll make sure to not double up on the disable in the
> > helpers.
> >
> > So we'll keep symmetry, and avoid having an awful hook name like
> > disable_from_self_refresh.. yuck!
> >
> > Thoughts?
>
> I like the asymetry actually, it has grown on a bit while working out and
> pondering this :-)
>
I'm not quite there with you, I still think it's better to split it all out.
> Benefits:
> - we keep the 100% symmetry of enable/disable hooks
> - self-refresh aware connector code also gets a bit simpler I think: in
> the normal enable/disable hooks it can just check for
> connector->state->crtc->state->self_refresh_active for sr state changes
> while the pipe is logically staying on
> - the one asymmetric case due to this design where we disable the pipe
> harder has an awkward special hook, which gives us a great opportunity
> to explain why it's needed
> - nothing changes for non-sr aware drivers
> - also no need to duplicate sr state into connectors, since it's all
> fairly explit already in all three state transitions.
To be fair, only one of these is exclusive to asymmetry, and it's the one that
provides the opportunity to add a comment. If the sr functions are symmetric,
the code becomes much more "normal" and less deserving of the explanation.
The reason I would like to split out entry and exit is that it makes the driver
code a bit easier to rationalize. Currently we need to check the state at the
beginning of enable/disable to determine whether we want the full enable/disable
or the psr exit/enter. So the sr_disable function would really just be plain
old disable without the special casing at the top. In that case, we don't even
need the separate function, we could just limit disable calls only on those
objects which are effectively on (active || sr). That starts sounding a lot like
what we already have here.
Further, doing SR in enable/disable is really just legacy from v1 which tried to
keep as much the same as possible. Now that we're "in it", I think it makes
sense to go all in and make SR a first class citizen.
Sean
>
> - SR on can only happen if the logical crtc_state->active is on and stays on
> - SR can get disabled in 2 subcases
> - logical active state stays on -> handled with existing hooks
> - logical active state also goes off -> existing hooks all skip (because
> active=false -> active=false is a no-op), the special ->sr_disable
> takes care
>
> It feels like this is clean, integrates well with atomic helpers overall
> and it even makes sense. At least to my slightly oxygen deprived mind
> right now ...
>
> > > > > > }
> > > > > > EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
> > > > > >
> >
> > /snip
> >
> > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > > index c8061992d6cb..0ae7e812ec62 100644
> > > > > > --- a/include/drm/drm_connector.h
> > > > > > +++ b/include/drm/drm_connector.h
> > > > > > @@ -501,6 +501,37 @@ struct drm_connector_state {
> > > > > > /** @tv: TV connector state */
> > > > > > struct drm_tv_connector_state tv;
> > > > > >
> > > > > > + /**
> > > > > > + * @self_refresh_changed:
> > > > > > + *
> > > > > > + * Set true when self refresh status has changed. This is useful for
> > > > > > + * use in encoder/bridge enable where the old state is unavailable to
> > > > > > + * the driver and it needs to know whether the enable transition is a
> > > > > > + * full transition, or if it just needs to exit self refresh mode.
> > > > >
> > > > > Uh, we just need proper atomic callbacks with all the states available.
> > > > > Once you have one, you can get at the others.
> > > > >
> > > >
> > > > Well, sure, we could do that too :)
> > >
> > > tbh I'm not sure whether that's really better, the duplication just irks
> > > me. With a new callback for the special self-refresh disable (I guess we
> > > only need that on the connector), plus looking at
> > > connector->state->crtc->state->self_refresh, I think we'd be covered
> > > as-is? Or is there a corner case I'm still missing?
> > >
> >
> > I think we can remove self_refresh_changed/self_refresh_active if we implement
> > dedicated hooks for self_refresh_enter/exit. We'll want to keep
> > self_refresh_aware around since the presence of the callback implementations
> > does not imply the panel connected supports SR.
>
> Yup, self_refresh_aware is needed.
>
> > As mentioned above, we'll need these hooks on everything in the pipeline to be
> > fully covered.
>
> Let's just do the ->sr_disable hook for now. I don't think we need all the
> others really.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Sean Paul, Software Engineer, Google / Chromium OS
More information about the dri-devel
mailing list