Re: [PATCH v2 1/5] drm: Add helpers to kick off self refresh mode in drivers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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@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@ffwll.ch>
> > > > > > > > > > Cc: Jose Souza <jose.souza@intel.com>
> > > > > > > > > > Cc: Zain Wang <wzz@rock-chips.com>
> > > > > > > > > > Cc: Tomasz Figa <tfiga@chromium.org>
> > > > > > > > > > Signed-off-by: Sean Paul <seanpaul@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 would suggest all new hooks should be specified as
do_stuff(struct drm_atomic_state *state, struct drm_foo *foo);
That way there is less confusion how to get at other states
besides the old/new foo states explicitly passed in.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel