[PATCH v2 1/5] drm: Add helpers to kick off self refresh mode in drivers
Daniel Vetter
daniel at ffwll.ch
Tue Apr 2 07:49:00 UTC 2019
On Mon, Apr 01, 2019 at 09:49:30AM -0400, Sean Paul wrote:
> On Fri, Mar 29, 2019 at 08:21:31PM +0100, Daniel Vetter wrote:
> > On Fri, Mar 29, 2019 at 7:10 PM Sean Paul <sean at poorly.run> wrote:
> > >
> > > 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.
> >
> > Hm, question is: How many hooks do you need? Just something on the
> > connector, or on the encoder, or everywhere?
>
> bridge/encoder/crtc all do special things during SR transitions, I don't think
> connector is necessary. This is the same for any .sr_disable function, everyone
> would need to implement it.
Hm, that's a lot of new callbacks ...
> > And how do you handle the
> > various state transitions. On the disable side we have:
> > - active on -> active off, no sr (userspace disables crtc)
> > - active on, sr off -> active ooff, sr on (sr timer fires and suspends crtc)
> > - active off, sr on -> active off, sr off (userspace disable crtc
> > while crtc is in sr)
> > These are all "logical active on" -> "something" transitions where we
> > disable something (crtc, or display or both)
> >
> > So in a way you'd need 3 hooks here for the full matrix.
> > And they all
> > kinda disable something. On the enable side we have:
> > - active off, sr off -> active on, sr off (userspace enables crtc)
> > - active off, sr on -> active on, sr off (userspace does a pageflip, stops sr)
> > Here we either enable the crtc (display already on) or both. Since we
> > only go into sr with the timer there's no 3rd case of only enabling
> > the display. So still asymetric, even with lots more hooks.
>
> We don't need the (active off, sr on) -> (active off, sr off) (third) case
> above, it's the same as the first. Just doing a full disable is sufficient,
> so you would have symmetry in the enable/disable calls and asymmetry in the
> sr calls. This is similar to enabling a plane, or turning other HW features on
> while enabled. SR is after all just a feature of the hardware.
Hm yeah I guess we can treat it like plane disabling, which implicitly
happens in crtc->disable too. Or the implicit plane enable in crtc->enable
(although that case doesn't exist for sr, since we never go directly into
sr).
> > If you want the full matrix, there's going to be a _lot_ of hooks. I
> > think slightly more awkward driver, but less hooks is better. Hence
> > the slightly awkward middle ground of a special disable_from_sr hook.
> > But maybe there's a better option somewhere else ...
>
> There's really no reason to even have the sr_disable function. The .disable
> function in the driver will already need special casing to detect psr_entry
> vs full disable, so it'd be better to just call disable twice. The .sr_disable
> function would always just do a full disable (ie: the .disable implementation
> without the sr checks at the top).
>
> So the debate should be: add sr_enable/disable pair of hooks, or overload
> disable with asymmetry (current implementation).
I guess that means we're back to no new hooks, and the driver just dtrt
in the existing hooks with the state transition bits we have? I thought
the issue with that is that we can't get at all the right bits, hence the
sr_disable special case hook.
Or is your plan to roll out a full new set of hooks, equipped with
old/new_state for everything? I think we'd only need old/new_state for the
object at hand, since with the old_state you can get at drm_atomic_state,
which allows you to get anything else really.
Or should we just add drm_atomic_state *state to all these hooks? That'd
probably the most flexible long-term thing. Could even be done with cocci,
so we don't need new atomic_disable2 calls and silly things like that.
-Daniel
>
> Sean
>
> > -Daniel
> >
> > >
> > > 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
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list