On Tue, Apr 2, 2019 at 6:47 PM Sean Paul <s...@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 <s...@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 <s...@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 <seanp...@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-s...@poorly.run
> > > > > > > > > > > > >
> > > > > > > > > > > > > Cc: Daniel Vetter <dan...@ffwll.ch>
> > > > > > > > > > > > > Cc: Jose Souza <jose.so...@intel.com>
> > > > > > > > > > > > > Cc: Zain Wang <w...@rock-chips.com>
> > > > > > > > > > > > > Cc: Tomasz Figa <tf...@chromium.org>
> > > > > > > > > > > > > Signed-off-by: Sean Paul <seanp...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to