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.
> 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.
>
> 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).
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
