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

[Sean Paul <sean@poorly.run>]

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?

> 
> > > >  }
> > > >  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.

As mentioned above, we'll need these hooks on everything in the pipeline to be
fully covered.

> > > > +	 */
> > > > +	bool self_refresh_changed;
> > > > +
> > > > +	/**
> > > > +	 * @self_refresh_active:
> > > > +	 *
> > > > +	 * Used by the self refresh (SR) helpers to denote when the display
> > > > +	 * should be self refreshing. If your connector is SR-capable, check
> > > > +	 * this flag in .disable(). If it is true, instead of shutting off the
> > > > +	 * panel, put it into self refreshing mode.
> > > > +	 */
> > > > +	bool self_refresh_active;
> > > > +
> > > > +	/**
> > > > +	 * @self_refresh_aware:
> > > > +	 *
> > > > +	 * This tracks whether a connector is aware of the self refresh state.
> > > > +	 * It should be set to true for those connector implementations which
> > > > +	 * understand the self refresh state. This is needed since the crtc
> > > > +	 * registers the self refresh helpers and it doesn't know if the
> > > > +	 * connectors downstream have implemented self refresh entry/exit.
> > > 
> > > Maybe good to clarify whether drivers can adjust this at runtime from
> > > their atomic_check code? I think they can ...
> > 
> > Will do. Yes they can, that's exactly what this is for. Since not all panels
> > connected to a device can support SR, and you could conceivably hotplug between
> > the two, we want a way to avoid turning off the circuitry upstream of the panel.
> > 
> > > 
> > > > +	 */
> > > > +	bool self_refresh_aware;
> > > > +
> > > >  	/**
> > > >  	 * @picture_aspect_ratio: Connector property to control the
> > > >  	 * HDMI infoframe aspect ratio setting.

/snip

> > > 
> > > Aside from the bikesheds and suggestions, looks all good to me. I think
> > > it'd be good to also have an ack from some other soc display maintainer
> > > that this is useful (Tomi, Laurent, Liviu, or someone else). It's always
> > > good to check a design against a handful of actual drivers instead of some
> > > moonshot clean sheet design :-)
> > 
> > Yeah, would be nice to get a third set of eyes. I'm planning on using this
> > for msm dsi as well, so I've been keeping that in the back of my head while
> > working on this.
> > 
> > > 
> > > But there's a big one: Would be really lovely to have selftests for this
> > > stuff, kinda as a start for making sure we have a solid model of the
> > > atomic helpers ...
> > 
> > I got so far through the review without any Big Items (well except for the
> > complete redesign between v1 and v2 (but I kind of expected that))! I've never
> > done any selftests, so this will be a good chance to get familiar with them.
> 
> Awesome. It might be a bit more work since no one yet mocked enough of
> drm_device to run the atomic helpers, but for a basic self-refresh
> testcase I don't think you need much really. Plus we've made almost all
> callbacks optional, the minimal mock objects should be rather trivial.

It'll be a fun experiment :)

> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> 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