[PATCH v2 1/5] drm: Add helpers to kick off self refresh mode in drivers
Daniel Vetter
daniel at ffwll.ch
Fri Mar 29 08:21:10 UTC 2019
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
>
> Thanks for the review!
>
>
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 2453678d1186..c659db105133 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -30,6 +30,7 @@
> > > #include <drm/drm_atomic_uapi.h>
> > > #include <drm/drm_plane_helper.h>
> > > #include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_self_refresh_helper.h>
> > > #include <drm/drm_writeback.h>
> > > #include <drm/drm_damage_helper.h>
> > > #include <linux/dma-fence.h>
> > > @@ -950,10 +951,33 @@ int drm_atomic_helper_check(struct drm_device *dev,
> > > if (state->legacy_cursor_update)
> > > state->async_update = !drm_atomic_helper_async_check(dev, state);
> > >
> > > + drm_self_refresh_helper_alter_state(state);
> > > +
> > > return ret;
> > > }
> > > EXPORT_SYMBOL(drm_atomic_helper_check);
> > >
> > > +static bool
> > > +crtc_needs_disable(struct drm_crtc_state *old_state,
> > > + struct drm_crtc_state *new_state)
> > > +{
> > > + /*
> > > + * No new_state means the crtc is off, so the only criteria is whether
> > > + * it's currently active or in self refresh mode.
> > > + */
> > > + if (!new_state)
> > > + return drm_atomic_crtc_effectively_active(old_state);
> > > +
> > > + /*
> > > + * We need to run through the crtc_funcs->disable() function if the crtc
> > > + * is currently on, if it's transitioning to self refresh mode, or if
> > > + * it's in self refresh mode and needs to be fully disabled.
> > > + */
> > > + return old_state->active ||
> > > + (old_state->self_refresh_active && !new_state->enable) ||
> > > + new_state->self_refresh_active;
> > > +}
> > > +
> > > static void
> > > disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > > {
> > > @@ -974,7 +998,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > >
> > > old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
> > >
> > > - if (!old_crtc_state->active ||
> > > + if (new_conn_state->crtc)
> > > + new_crtc_state = drm_atomic_get_new_crtc_state(
> > > + old_state,
> > > + new_conn_state->crtc);
> > > + else
> > > + new_crtc_state = NULL;
> > > +
> > > + if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> > > !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> >
> > I have a really hard time parsing the logic here and in the helper, since
> > it's also spread around. I think your helper should also include the
> > needs_modeset check. Plus your comments look a bit strange - e.g. with the
> > double negation the reason for the old_crtc->active check isn't that we
> > need to disable if it's active, but that we don't need to disable if it's
> > not active (you combine with || here, not &&).
> >
> > But aside from all that, why do you even change stuff here? The self
> > refresh worker changes ->active to false, which means needs_modeset will
> > fire, and no change should be needed in the commit code at all.
>
> The issue is that we need to go through disable if we're currently in SR and
> need to turn off (ie: old->active=false old->self_refresh_active=true
> new->active=false). In this case, needs_modeset returns false.
>
> So yeah, I think the new->self_refresh_active check above is probably redundant
> (will need to double check that), but we do need to ensure that we go through a
> full disable if we're in SR.
Hm right, totally missed that case. Annoying, since it complicates thing
(and makes the code asymetric since we'll never go directly into self
refresh on the enable side).
> On the enable side, you're right, we don't need to change anything.
>
> >
> > That's why I suggested the trick with
> > drm_atomic_crtc_effectively_active(), it allows us to keep the entire
> > check/commit driver code as-is.
> >
> > > continue;
> > >
> > > @@ -1018,7 +1049,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > > if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> > > continue;
> > >
> > > - if (!old_crtc_state->active)
> > > + if (!crtc_needs_disable(old_crtc_state, new_crtc_state))
> > > continue;
> > >
> > > funcs = crtc->helper_private;
> > > @@ -2948,6 +2979,7 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
> > > return -ENOMEM;
> > >
> > > state->acquire_ctx = ctx;
> > > +
> > > ret = __drm_atomic_helper_set_config(set, state);
> > > if (ret != 0)
> > > goto fail;
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > 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.
> > > }
> > > EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 4eb81f10bc54..d2085332172b 100644
>
> /snip
>
> > > +
> > > +static void drm_self_refresh_helper_entry_work(struct work_struct *work)
> > > +{
> > > + struct drm_self_refresh_state *sr_state = container_of(
> > > + to_delayed_work(work),
> > > + struct drm_self_refresh_state, entry_work);
> > > + struct drm_crtc *crtc = sr_state->crtc;
> > > + struct drm_device *dev = crtc->dev;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > + struct drm_atomic_state *state;
> > > + struct drm_connector *conn;
> > > + struct drm_connector_state *conn_state;
> > > + struct drm_crtc_state *crtc_state;
> > > + int i, ret;
> > > +
> > > + drm_modeset_acquire_init(&ctx, 0);
> > > +
> > > + state = drm_atomic_state_alloc(dev);
> > > + if (!state) {
> > > + ret = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > +retry:
> > > + state->acquire_ctx = &ctx;
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > + if (IS_ERR(crtc_state)) {
> > > + ret = PTR_ERR(crtc_state);
> > > + goto out;
> > > + }
> > > +
> > > + if (!crtc_state->enable)
> > > + goto out;
> > > +
> > > + ret = drm_atomic_add_affected_connectors(state, crtc);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + crtc_state->active = false;
> > > + crtc_state->self_refresh_active = true;
> > > +
> > > + for_each_new_connector_in_state(state, conn, conn_state, i) {
> > > + if (!conn_state->self_refresh_aware)
> > > + goto out;
> > > +
> > > + conn_state->self_refresh_changed = true;
> > > + conn_state->self_refresh_active = true;
> > > + }
> >
> > I wonder whether we should reject the timer here before going ahead.
> > There's ofc still a race, but if we're unlucky and the self fresh work
> > fires at the same time as userspace does a new atomic commit we could end
> > up with the following sequence:
> > 1. self-refresh work starts
> > 2. userspace does ioctls, starts to grab some locks
> > 3. self-refresh grabs sets up the acquire ctx
> > 4. atomic ioctl pulls through, does a commit, blocking the self-refresh
> > work
> > 5. self-refresh work finally gets all the locks, goes ahead and puts the
> > display into self-refresh.
> >
> > Not sure how much we care about this really, or whether delaying the timer
> > from all atomic_check (including TEST_ONLY) is enough to paper over this.
> >
>
> Hmm, yeah I suppose if the entry timer period was the same as some periodic
> animation (like a fancy blinking cursor), you could get some nasty jank.
>
> As you say it's not avoidable and the bulk of the time here will be doing the
> actual entry instead of the setup. So if there is an unfortunate collision,
> we're going to hit it more often than not even with the timer check. So I'm
> having a hard time getting excited about this.
>
> That said, I'll write the code to check the timer and see how it looks :)
>
> > > +
> > > + ret = drm_atomic_commit(state);
> > > + if (ret)
> > > + goto out;
> > > +
> > > +out:
> > > + if (ret == -EDEADLK) {
> > > + drm_atomic_state_clear(state);
> > > + ret = drm_modeset_backoff(&ctx);
> > > + if (!ret)
> > > + goto retry;
> > > + }
> > > +
> > > + drm_atomic_state_put(state);
> > > + drm_modeset_drop_locks(&ctx);
> > > + drm_modeset_acquire_fini(&ctx);
> > > +}
> > > +
> > > +/**
> > > + * drm_self_refresh_helper_alter_state - Alters the atomic state for SR exit
> > > + * @state: the state currently being checked
> > > + *
> > > + * Called at the end of atomic check. This function checks the state for flags
> > > + * incompatible with self refresh exit and changes them. This is a bit
> > > + * disingenuous since userspace is expecting one thing and we're giving it
> > > + * another. However in order to keep self refresh entirely hidden from
> > > + * userspace, this is required.
> > > + *
> > > + * At the end, we queue up the self refresh entry work so we can enter PSR after
> > > + * the desired delay.
> > > + */
> > > +void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state)
> > > +{
> > > + struct drm_crtc *crtc;
> > > + struct drm_crtc_state *crtc_state;
> > > + int i;
> > > +
> > > + if (state->async_update) {
> > > + for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> > > + if (crtc_state->self_refresh_active) {
> > > + state->async_update = false;
> > > + break;
> > > + }
> > > + }
> > > + }
> > > + if (!state->allow_modeset) {
> > > + for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> > > + if (crtc_state->self_refresh_active) {
> > > + state->allow_modeset = true;
> > > + break;
> > > + }
> > > + }
> > > + }
> >
> > I think you can flatten the above two to:
> >
> > for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> > if (crtc_state->self_refresh_active) {
> > state->allow_modeset = true;
> > state->async_update = false;
> > break;
> > }
> > }
>
> D'oh! I've stared at this SR code for sooo long, I guess I got highway hypnosis
> on this bit.
>
> >
> > Plus a comment that this breaks uapi, but who cares:
> >
> > /* This breaks uapi, but we want self-refresh to be as transparent
> > * to userspace as possible. Plus the assumption is that with the
> > * display still on, the modeset will take at most a few vblanks,
> > * and that's still ok. */
> >
> > > +
> > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > + struct drm_self_refresh_state *sr_state;
> > > +
> > > + /* Don't trigger the entry timer when we're already in SR */
> > > + if (crtc_state->self_refresh_active)
> > > + continue;
> > > +
> > > + sr_state = crtc->self_refresh_state;
> > > + mod_delayed_work(system_wq, &sr_state->entry_work,
> > > + msecs_to_jiffies(sr_state->entry_delay_ms));
> >
> > This bit needs to be in atomic_commit, not atomic check. Or at least it
> > feels a bit wrong to delay self-refresh if userspace does a few TEST_ONLY
> > commits.
>
> IMO we should be erring on the side of being overly cautious about entering PSR
> since it's a pretty big commitment. Doing this in atomic check is a signal that
> userspace is at least interested in poking at KMS and we should back off until
> its done.
>
> >
> > If you feel like we should do that, then we need a comment here, since
> > it's clearly against all the "never touch anything outside of the free
> > standing state structures from atomic_check code" rules.
>
> Good point, I'll add a comment to this effect. We're breaking all the rules
> 'round here :)
>
> >
> > > + }
> > > +}
> > > +EXPORT_SYMBOL(drm_self_refresh_helper_alter_state);
>
> /snip
>
> > > +/**
> > > + * drm_atomic_crtc_effectively_active - compute whether crtc is actually active
> > > + * @state: &drm_crtc_state for the CRTC
> > > + *
> > > + * When in self refresh mode, the crtc_state->active value will be false, since
> > > + * the crtc is off. However in some cases we're interested in whether the crtc
> > > + * is active, or effectively active (ie: it's connected to an active display).
> > > + * In these cases, use this function instead of just checking active.
> > > + */
> > > +static inline bool
> > > +drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state)
> > > +{
> > > + return state->active || state->self_refresh_active;
> > > +}
> > > +
> > > #endif /* DRM_ATOMIC_H_ */
> > > 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?
> > > + */
> > > + 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.
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index f7c3022dbdf4..208d68129b4c 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -53,6 +53,7 @@ struct drm_mode_set;
> > > struct drm_file;
> > > struct drm_clip_rect;
> > > struct drm_printer;
> > > +struct drm_self_refresh_state;
> > > struct device_node;
> > > struct dma_fence;
> > > struct edid;
> > > @@ -299,6 +300,17 @@ struct drm_crtc_state {
> > > */
> > > bool vrr_enabled;
> > >
> > > + /**
> > > + * @self_refresh_active:
> > > + *
> > > + * Used by the self refresh helpers to denote when a self refresh
> > > + * transition is occuring. This will be set on enable/disable callbacks
> > > + * when self refresh is being enabled or disabled. In some cases, it may
> > > + * not be desirable to fully shut off the crtc during self refresh.
> > > + * CRTC's can inspect this flag and determine the best course of action.
> > > + */
> > > + bool self_refresh_active;
> > > +
> > > /**
> > > * @event:
> > > *
> > > @@ -1087,6 +1099,13 @@ struct drm_crtc {
> > > * The name of the CRTC's fence timeline.
> > > */
> > > char timeline_name[32];
> > > +
> > > + /**
> > > + * @self_refresh_state: Holds the state for the self refresh helpers
> > > + *
> > > + * Initialized via drm_self_refresh_helper_register().
> > > + */
> > > + struct drm_self_refresh_state *self_refresh_state;
> >
> > Can we drop the _state here? In atomic structures with _state suffix have
> > very specific meaning&semantics, this isn't one of them. s/state/date/ is
>
> data might be even better ;)
>
> > imo clearer in the description too.
>
> Yeah, good suggestion.
>
> >
> > > };
> > >
> > > /**
> > > diff --git a/include/drm/drm_self_refresh_helper.h b/include/drm/drm_self_refresh_helper.h
> > > new file mode 100644
> > > index 000000000000..015dacb8a807
> > > --- /dev/null
> > > +++ b/include/drm/drm_self_refresh_helper.h
> > > @@ -0,0 +1,23 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright (C) 2019 Google, Inc.
> > > + *
> > > + * Authors:
> > > + * Sean Paul <seanpaul at chromium.org>
> > > + */
> > > +#ifndef DRM_SELF_REFRESH_HELPER_H_
> > > +#define DRM_SELF_REFRESH_HELPER_H_
> > > +
> > > +struct drm_atomic_state;
> > > +struct drm_connector;
> > > +struct drm_device;
> > > +struct drm_self_refresh_state;
> > > +struct drm_modeset_acquire_ctx;
> > > +
> > > +void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state);
> > > +
> > > +int drm_self_refresh_helper_register(struct drm_crtc *crtc,
> > > + unsigned int entry_delay_ms);
> > > +
> > > +void drm_self_refresh_helper_unregister(struct drm_crtc *crtc);
> > > +#endif
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> >
> > 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.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list