[PATCH v2 1/5] drm: Add helpers to kick off self refresh mode in drivers
Daniel Vetter
daniel at ffwll.ch
Tue Apr 2 16:05:48 UTC 2019
On Tue, Apr 2, 2019 at 3:24 PM Sean Paul <sean at poorly.run> wrote:
>
> On Tue, Apr 02, 2019 at 09:49:00AM +0200, Daniel Vetter wrote:
> > 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.
>
> I don't think we even need to pass the state to the sr hooks, just add
>
> void self_refresh_enter(struct drm_<type> *<name>);
> void self_refresh_exit(struct drm_<type> *<name>);
>
> to the funcs vtable for crtc/encoder/bridge.
>
> Of course it's not _quite_ as straightforward as that :)
>
> With the current model, the powerdown/powerup order of components is implicitly
> broken. With this new model, it's much more obvious, this is easiest to
> illustrate with bridges, but it's true for crtcs and encoders as well.
>
> Assume you have the following bridge chain:
>
> ENC0 (not SR-aware)
> -> BR0 (SR-aware)
> -> BR1 (not SR-aware)
> -> BR2 (SR-aware)
> -> CON0
>
> An SR-enter transition would be:
> BR2->self_refresh_enter
> BR1->disable
> BR0->self_refresh_enter
> ENC0->disable
> BR1->post_disable
>
> SR-exit is:
> BR1->pre_enable
> ENC0->enable
> BR0->self_refresh_exit
> BR1->enable
> BR2->self_refresh_exit
>
> Disabling from SR becomes:
> BR2->disable
> BR0->disable
> BR2->post_disable
> BR0->post_disable
>
> So I'm starting to question falling back on disable. I think it was a fine
> choice when we would exit psr before disable (ie: v1), but I think it might be
> too complicated now. We could make BR2 and BR0 do the right thing on
> disable-from-SR, but I'm worried that mixing up the order for SR-unaware devices
> (ENC0/BR1) might cause issues.
>
> Perhaps we should scale this back and just treat self_refresh as its own thing
> and not go through the enable/disable path at all. Devices which are not
> SR-aware stay on (which has it's own issues if BR1 underflows because it's
> expecting video from BR0). Maybe we have to ensure the entire pipe is SR-aware
> before we do an SR-enter.
Imo if the driver tries to enable SR on a pipe where some pieces
aren't SR aware, that's a driver bug. And if you really want to
implement the above sequence (well, need to implement it), then I
agree that helpers aren't the thing you're looking for and you should
just roll your own modeset code.
But we started all this assuming that you're hw isn't in the need of
the full state matrix with hooks for everything, hence that maybe a
helper would make sense. It feels a bit like the discussion lost
contact with the (driver) reality ...
> Thoughts?
... so imo if we can help out drivers by repurposing the existing
hooks to cover most cases, with a few special cases in callbacks, then
we can roll these helpers. If that doesn't happen, then probably
better if each driver just rolls their own sr enter/exit code and
calls it done. It's not like we don't allow subclassing of states or
also vtable hooks where you could just add more of your own stuff as
you see fit. But I thought sr for most devices would amount to a)
shutting the pipe down b) some special casing to keep the display
alive and nothing else. But now it sounds like you need hooks for
everything, which probably doesnt make sense to cover in the helpers.
-Daniel
>
> Sean
>
> >
> > 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
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list