On Fri, Mar 01, 2019 at 09:18:13AM +0100, Daniel Vetter wrote:
> On Thu, Feb 28, 2019 at 04:09:30PM -0500, Sean Paul wrote:
> > From: Sean Paul <seanp...@chromium.org>
> >
> > This patch adds a new drm helper library to help drivers implement
> > PSR. Drivers choosing to use it will register connectors with
> > PSR-capable displays connected and will receive callbacks when it's time
> > to enter or exit PSR.
> >
> > 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 save the current atomic state and issue a new state which
> > has the PSR-enabled pipes turned off. On the next update, the drm core
> > will poke the PSR helpers to restore the saved state to the driver before
> > servicing said update.
> >
> > From the driver's perspective, this works like a regular disable/enable
> > cycle. The driver need only check the 'psr_transition' state in
> > connector_state and keep the panel turned on when in .disable(), while
> > everything else will cycle off as normal. If drivers want more control,
> > they can use the psr_transition state to enter a low-power state to
> > minimize PSR exit time.
> >
> > While this carries the PSR moniker, it is not specific to the
> > DisplayPort technology. This can be used for power savings with other
> > types of self refresh, such as MIPI command mode.
> >
> > Cc: Zain Wang <w...@rock-chips.com>
> > Cc: Tomasz Figa <tf...@chromium.org>
> > Signed-off-by: Sean Paul <seanp...@chromium.org>
/snip to the good part
>
> So for the high-level design, before looking at the implementation
> details:
> - I agree with Jose that a per-crtc state sounds more like what we want,
> instead of global. Just from a conceptual point.
Agreed this would be better. I was mostly worried about resource sharing
between crtcs. ie: If 2 crtcs share a clock, 1 goes to PSR and the other
changes the rate to something incompatible. However I suppose the driver would
be responsible for sussing this all out, so sure.
It'll be nice to avoid the global lock for psr.
> - We can imo flat-out require that your driver is using the fancy new
> dirtyfb helpers to remap the dirtyfb call to an atomic update. It's the
> clean design for sw controlled manual update displays anyway.
> - That means your single entry points (instead of trying to catch all of
> them like above, and missing some like you do) are ->atomic_check and
> ->atomic_commit. That also gives you the clean hook into helpers.
The reason I did this was because new state was being calculated off the
disabled state. So the psr disable commit needed to precede the atomic
check. However, with your suggestions below I think we can avoid that :)
> - The duplicated state only works when no one else is looking, it's very
> fragile. single-threaded (from a driver pov) suspend/resume is ok, I
> don't think it's a good idea to expand this.
Yeah, fair. I was trying to stay out of the way of the existing code, but I'll
integrate more tightly.
>
> Here's what I'd do:
> - Stuff a new self_refresh_active state into drm_crtc_state (not sure we
> should call this psr or self_refresh - latter is imo better name since
> not associated with DP, but longer).
I struggled with this too, naming is hard.
>
> - Use crtc_state->self_refresh_active and crtc_state->active to compute
> the reported active state to userspace in the get_prop functions. This
> because doing a mass search&replace over all drivers is too much.
> Something like
>
> real_active == active || self_refresh
>
> - In duplicate_state always reset the state
> active = real_active();
> self_refresh = false;
> to disable self refresh and set active to what userspace the "ACTIVE"
> property should be.
We'll also have to alter the flags when they don't include
DRM_MODE_ATOMIC_ALLOW_MODESET, and async commits would need to go through the
synchronous path. Conversely, non-blocking commits will get back to userspace
more quickly with this design.
This is the primary reason I kept the psr disable in a separate commit, so the
original request could go through as intended (albeit a bit delayed, but there's
no getting around that).
>
> - self_refresh state in connector_state sounds like a good idea
>
> - Your idle worker would do a normal atomic commit, no evil state
> duplicate, and then set:
> self_fresh = active;
> active = false;
> Handling races left as an exercise :-)
>
> - Somewhere in the commit helpers put a function to arm your idle work
> (per-crtc imo simplest).
>
> This should give you a clean lasagne going from core ioctl -> core atomic
> -> helpers <-> drivers. And not the spaghetti sprawl of finely sprinkling
> the self refresh helpers all over.
Alright, I'll mock this up and see if it floats. Thanks for the detailed
feedback!
Sean
>
> Aside: Going snowboarding next week, so if this is a bonghits idea I'm
> afraid will take 1 week until I can spin something new :-)
>
> Cheers, Daniel
>
> > diff --git a/drivers/gpu/drm/drm_psr_helper.c
> > b/drivers/gpu/drm/drm_psr_helper.c
> > new file mode 100644
> > index 000000000000..0b57a2a53075
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_psr_helper.c
> > @@ -0,0 +1,343 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright (C) 2019 Google, Inc.
> > + *
> > + * Authors:
> > + * Sean Paul <seanp...@chromium.org>
> > + */
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_mode_config.h>
> > +#include <drm/drm_modeset_lock.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_psr_helper.h>
> > +#include <linux/bitops.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +
> > +/**
> > + * DOC: overview
> > + *
> > + * This helper library provides an easy way for drivers to leverage the
> > atomic
> > + * framework to implement panel self refresh (PSR) support. Drivers are
> > + * responsible for intializing and cleaning up the PSR helpers on
> > load/unload.
> > + * When drivers identify a display that supports self refreshing (eDP or
> > MIPI
> > + * command mode), it should register the affected connector with the PSR
> > + * helpers.
> > + *
> > + * Once a connector is registered, the PSR helpers will monitor activity
> > and
> > + * call back into the driver to enable/disable PSR as appropriate. The
> > best way
> > + * to think about this is that it's a DPMS on/off request with a flag set
> > in
> > + * state that tells you to disable/enable PSR on the panel instead of
> > power-
> > + * cycling it.
> > + *
> > + * Drivers may choose to fully disable their crtc/encoder/bridge hardware,
> > or
> > + * they can use the "psr_transition" flag in crtc and connector state if
> > they
> > + * want to enter low power mode without full disable (in case full
> > + * disable/enable is too slow).
> > + *
> > + * PSR will be deactivated if there are any atomic updates, even updates
> > that do
> > + * not affect the connectors which are self refreshing. Supporting this is
> > + * possible but non-trivial due to sharing of hardware resources.
> > Similarly, if
> > + * a crtc is driving multiple connectors, PSR will not be initiated on any
> > of
> > + * those connectors.
> > + */
> > +
> > +struct drm_psr_state {
> > + struct drm_device *dev;
> > + struct drm_modeset_lock mutex;
> > + struct delayed_work entry_work;
> > + struct drm_atomic_state *save_state;
> > + unsigned int entry_delay_ms;
> > +};
> > +
> > +static void drm_psr_helper_entry_work(struct work_struct *work)
> > +{
> > + struct drm_psr_state *psr_state = container_of(to_delayed_work(work),
> > + struct drm_psr_state,
> > + entry_work);
> > + struct drm_atomic_state *save_state;
> > + struct drm_device *dev = psr_state->dev;
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_atomic_state *state;
> > + struct drm_connector *conn;
> > + struct drm_connector_list_iter conn_iter;
> > + struct drm_connector_state *conn_state;
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *crtc_state;
> > + bool commit = false;
> > + int ret, i;
> > +
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > +
> > + ret = drm_modeset_lock(&psr_state->mutex, &ctx);
> > + if (ret)
> > + goto out;
> > +
> > + /*
> > + * The only way this can happen is if we schedule the worker while it's
> > + * already running and that timeout subsequently elapses. Since we hold
> > + * the psr_state->mutex when scheduling, we also know where the worker
> > + * is sitting in its execution (hint: look up). In this case, it's
> > + * possible for the entry worker to run twice for the same commit. Since
> > + * the hardware hasn't changed since the last save state, just kick out.
> > + */
> > + if (psr_state->save_state)
> > + goto out;
> > +
> > + state = drm_atomic_state_alloc(dev);
> > + if (!state) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + save_state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > +
> > + /*
> > + * Now that we have the current the HW state saved, we have to flip the
> > + * psr_transition bit so we know what type of enable we're dealing with
> > + * when coming back on.
> > + *
> > + * NOTE: We don't check conn->capable here since that could change out
> > + * from under us. We'll trust the atomic core to only call enable if
> > + * necessary (ie: only for those connectors/crtcs that currently have
> > + * psr enabled).
> > + */
> > + if (IS_ERR(save_state)) {
> > + ret = PTR_ERR(save_state);
> > + goto out;
> > + }
> > + for_each_new_connector_in_state(save_state, conn, conn_state, i) {
> > + if (!conn_state->crtc)
> > + continue;
> > + conn_state->psr_transition = true;
> > + }
> > + for_each_new_crtc_in_state(save_state, crtc, crtc_state, i) {
> > + if (!crtc_state->active)
> > + continue;
> > + crtc_state->psr_transition = true;
> > + }
> > +
> > + state->acquire_ctx = &ctx;
> > + drm_connector_list_iter_begin(psr_state->dev, &conn_iter);
> > + drm_for_each_connector_iter(conn, &conn_iter) {
> > + if (!conn->psr_capable)
> > + continue;
> > +
> > + conn_state = drm_atomic_get_connector_state(state, conn);
> > + if (IS_ERR(conn_state)) {
> > + ret = PTR_ERR(conn_state);
> > + drm_connector_list_iter_end(&conn_iter);
> > + goto out_free_state;
> > + }
> > +
> > + if (!conn_state->crtc)
> > + continue;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> > + if (IS_ERR(crtc_state)) {
> > + ret = PTR_ERR(crtc_state);
> > + drm_connector_list_iter_end(&conn_iter);
> > + goto out_free_state;
> > + }
> > +
> > + /* Don't use PSR if the crtc is driving multiple connectors */
> > + if (hweight_long(crtc_state->connector_mask) > 1)
> > + continue;
> > +
> > + commit = true;
> > + crtc_state->active = false;
> > + crtc_state->psr_transition = true;
> > + conn_state->psr_transition = true;
> > + }
> > + drm_connector_list_iter_end(&conn_iter);
> > +
> > + /* Nothing to commit, so just exit */
> > + if (!commit)
> > + goto out_free_state;
> > +
> > + ret = drm_atomic_commit(state);
> > + if (ret)
> > + goto out_free_state;
> > +
> > + psr_state->save_state = save_state;
> > + goto out;
> > +
> > +out_free_state:
> > + drm_atomic_state_put(save_state);
> > + drm_atomic_state_put(state);
> > +out:
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > +}
> > +
> > +static int
> > +drm_psr_helper_acquire_modeset_locks(struct drm_atomic_state *state,
> > + struct drm_modeset_acquire_ctx *ctx)
> > +{
> > + struct drm_mode_config *config = &state->dev->mode_config;
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *new_crtc_state;
> > + struct drm_plane *plane;
> > + struct drm_plane_state *new_plane_state;
> > + int ret, i;
> > +
> > + ret = drm_modeset_lock(&config->connection_mutex, ctx);
> > + if (ret)
> > + return ret;
> > +
> > + for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > + ret = drm_modeset_lock(&plane->mutex, ctx);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > + ret = drm_modeset_lock(&crtc->mutex, ctx);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * drm_psr_helper_flush - Restore the hardware to pre-PSR state
> > + * @dev: DRM device
> > + * @ctx: An acquire context to use for restoring the state
> > + *
> > + * This function should be called before every drm_atomic_commit to ensure
> > any
> > + * connectors that are currently self-refreshing revert back to being bus
> > + * driven. Drivers may call this function outside of the atomic hooks if
> > + * they wish to disable PSR pre-emptively (such as upon an input event or
> > when
> > + * GPU becomes active).
> > + *
> > + * If everything is successful, this function will schedule the PSR entry
> > worker
> > + * to enable PSR after the driver-specified timeout.
> > + *
> > + * If the PSR helper is not being used, this is a no-op.
> > + */
> > +int drm_psr_helper_flush(struct drm_device *dev,
> > + struct drm_modeset_acquire_ctx *ctx)
> > +{
> > + struct drm_mode_config *config = &dev->mode_config;
> > + struct drm_psr_state *psr_state = config->psr_state;
> > + int ret;
> > +
> > + if (!psr_state)
> > + return 0;
> > +
> > + ret = drm_modeset_lock(&psr_state->mutex, ctx);
> > + if (ret)
> > + return ret;
> > +
> > + if (!psr_state->save_state)
> > + goto out;
> > +
> > + ret = drm_psr_helper_acquire_modeset_locks(psr_state->save_state, ctx);
> > + if (ret)
> > + goto out;
> > +
> > + ret = drm_atomic_helper_commit_duplicated_state(psr_state->save_state,
> > + ctx);
> > +
> > +out:
> > + psr_state->save_state = NULL;
> > + if (!ret) {
> > + mod_delayed_work(system_wq, &psr_state->entry_work,
> > + msecs_to_jiffies(psr_state->entry_delay_ms));
> > + }
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_flush);
> > +
> > +/**
> > + * drm_psr_helper_register - Registers a connector with the PSR helpers
> > + * @connector: the connector which has a PSR-supported display attached
> > + *
> > + * Note that this can be called once on initialization for fixed panels, or
> > + * during enable/hotplug.
> > + */
> > +int drm_psr_helper_register(struct drm_connector *connector)
> > +{
> > + struct drm_mode_config *config = &connector->dev->mode_config;
> > + struct drm_psr_state *psr_state = config->psr_state;
> > +
> > + /* PSR helpers are uninitialized */
> > + if (WARN_ON(!psr_state))
> > + return -EINVAL;
> > +
> > + /* Acquired via psr_helper_flush */
> > + if (!drm_modeset_is_locked(&psr_state->mutex))
> > + return -EINVAL;
> > +
> > + connector->psr_capable = true;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_register);
> > +
> > +/**
> > + * drm_psr_helper_unregister - Unregisters a connector with the PSR helpers
> > + * @connector: the connector to unregister
> > + */
> > +int drm_psr_helper_unregister(struct drm_connector *connector)
> > +{
> > + struct drm_mode_config *config = &connector->dev->mode_config;
> > + struct drm_psr_state *psr_state = config->psr_state;
> > +
> > + /* PSR helpers are uninitialized */
> > + if (WARN_ON(!psr_state))
> > + return -EINVAL;
> > +
> > + /* Acquired via psr_helper_flush */
> > + if (!drm_modeset_is_locked(&psr_state->mutex))
> > + return -EINVAL;
> > +
> > + connector->psr_capable = false;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_unregister);
> > +
> > +/**
> > + * drm_psr_helper_init - Initializes the PSR helpers
> > + * @dev: DRM device
> > + * @entry_delay_ms: The amount of time to wait after an atomic commit
> > before
> > + * activating PSR
> > + *
> > + * Drivers using the PSR helpers must call this some time after mode_config
> > + * is initialized in order to make use of the PSR helpers. Typically
> > + * entry_delay_ms is a function of how quickly the hardware can enter/exit
> > PSR.
> > + */
> > +int drm_psr_helper_init(struct drm_device *dev, unsigned int
> > entry_delay_ms)
> > +{
> > + struct drm_mode_config *config = &dev->mode_config;
> > + struct drm_psr_state *psr_state;
> > +
> > + psr_state = kzalloc(sizeof(*psr_state), GFP_KERNEL);
> > + if (!psr_state)
> > + return -ENOMEM;
> > +
> > + INIT_DELAYED_WORK(&psr_state->entry_work, drm_psr_helper_entry_work);
> > + drm_modeset_lock_init(&psr_state->mutex);
> > + psr_state->dev = dev;
> > + psr_state->entry_delay_ms = entry_delay_ms;
> > +
> > + config->psr_state = psr_state;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_init);
> > +
> > +/**
> > + * drm_psr_helper_cleanup - De-initializes the PSR helpers
> > + * @dev: DRM device
> > + */
> > +void drm_psr_helper_cleanup(struct drm_device *dev)
> > +{
> > + struct drm_mode_config *config = &dev->mode_config;
> > +
> > + cancel_delayed_work_sync(&config->psr_state->entry_work);
> > + kfree(config->psr_state);
> > + config->psr_state = NULL;
> > +}
> > +EXPORT_SYMBOL(drm_psr_helper_cleanup);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index c8061992d6cb..9612b3d56f30 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -501,6 +501,17 @@ struct drm_connector_state {
> > /** @tv: TV connector state */
> > struct drm_tv_connector_state tv;
> >
> > + /**
> > + * @psr_transition:
> > + *
> > + * Used by the PSR helpers to denote when a PSR transition is occuring.
> > + * If your connector is PSR-capable, register it with the helpers and
> > + * check this flag in .enable() and .disable(). If it is true, instead
> > + * of shutting off the panel, put it in or take it out of self
> > + * refreshing mode.
> > + */
> > + bool psr_transition;
> > +
> > /**
> > * @picture_aspect_ratio: Connector property to control the
> > * HDMI infoframe aspect ratio setting.
> > @@ -993,6 +1004,17 @@ struct drm_connector {
> > */
> > struct drm_display_info display_info;
> >
> > + /**
> > + * @psr_capable:
> > + *
> > + * Set by the driver via drm_psr_helper_register(). Signals that this
> > + * connector (and associated pipe) is PSR capable and should be put in
> > + * low-power mode when it is inactive.
> > + *
> > + * Protected by &drm_mode_config.psr_state.mutex
> > + */
> > + bool psr_capable;
> > +
> > /** @funcs: connector control functions */
> > const struct drm_connector_funcs *funcs;
> >
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index f7c3022dbdf4..10acd4fc0991 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -299,6 +299,17 @@ struct drm_crtc_state {
> > */
> > bool vrr_enabled;
> >
> > + /**
> > + * @psr_transition:
> > + *
> > + * Used by the PSR helpers to denote when a PSR transition is occuring.
> > + * This will be set on enable/disable callbacks when PSR is being
> > + * enabled or disabled. In some cases, it may not be desirable to fully
> > + * shut off the crtc during PSR. CRTC's can inspect this flag and
> > + * determine the best course of action.
> > + */
> > + bool psr_transition;
> > +
> > /**
> > * @event:
> > *
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index 7f60e8eb269a..371b80d090ab 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -37,6 +37,7 @@ struct drm_atomic_state;
> > struct drm_mode_fb_cmd2;
> > struct drm_format_info;
> > struct drm_display_mode;
> > +struct drm_psr_state;
> >
> > /**
> > * struct drm_mode_config_funcs - basic driver provided mode setting
> > functions
> > @@ -900,6 +901,11 @@ struct drm_mode_config {
> > */
> > struct drm_atomic_state *suspend_state;
> >
> > + /**
> > + * @psr_state: Holds the state for the psr helper
> > + */
> > + struct drm_psr_state *psr_state;
> > +
> > const struct drm_mode_config_helper_funcs *helper_private;
> > };
> >
> > diff --git a/include/drm/drm_psr_helper.h b/include/drm/drm_psr_helper.h
> > new file mode 100644
> > index 000000000000..972c4ec98d05
> > --- /dev/null
> > +++ b/include/drm/drm_psr_helper.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright (C) 2019 Google, Inc.
> > + *
> > + * Authors:
> > + * Sean Paul <seanp...@chromium.org>
> > + */
> > +#ifndef DRM_PSR_HELPER_H_
> > +#define DRM_PSR_HELPER_H_
> > +
> > +struct drm_connector;
> > +struct drm_device;
> > +struct drm_psr_state;
> > +struct drm_modeset_acquire_ctx;
> > +
> > +int drm_psr_helper_flush(struct drm_device *dev,
> > + struct drm_modeset_acquire_ctx *ctx);
> > +
> > +int drm_psr_helper_register(struct drm_connector *connector);
> > +int drm_psr_helper_unregister(struct drm_connector *connector);
> > +
> > +int drm_psr_helper_init(struct drm_device *dev, unsigned int
> > entry_delay_ms);
> > +void drm_psr_helper_cleanup(struct drm_device *dev);
> > +#endif
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
>
> --
> 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
