[PATCH v2 1/5] drm: Add helpers to kick off self refresh mode in drivers
Daniel Vetter
daniel at ffwll.ch
Wed Apr 3 06:52:32 UTC 2019
On Tue, Apr 2, 2019 at 6:47 PM Sean Paul <sean at poorly.run> wrote:
> On Tue, Apr 02, 2019 at 06:05:48PM +0200, Daniel Vetter wrote:
> > 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.
>
> If you assume that most bridges (aside from the "bridges" that represent shared
> silicon IP) can be arbitrarily mixed with most other bridges and drivers, then
> yeah, this is unavoidable and not something that's easily fixed since we'd need
> to make all SR-aware or at the very least audit them to make sure they don't
> foul up if the things around them go to sleep.
>
> > 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.
>
> For most everything upstream of the connector, you don't _need_ a hook since
> shutting them down is fine. However if your crtc takes a while to come back on
> (like with rockchip), then you start to want hooks everywhere to optimize
> things.
>
> I'll put this on the shelf and wait for a few more drivers to implement their
> own SR. Perhaps a pattern will emerge.
I did scroll through the rockchip implementation patches again. Adding
a few conditions seems not too onerous, repurposing the current hooks
works. And I think if we pimp the atomic hooks as ville suggested
(just the ones where we need it, we can be lazy), then I think we can
also get rid of the duplicated tracking in connectors and essentially
go with v2. So wondering a bit whether we managed to derail this
unecessarily? It seems to work ... And I think some state dependent
code flow is unavoidable anyway, whether you roll your own modeset
code or use the helpers. Most drivers have such "hacks" for something
somewhere.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list