[PATCH 2/3] drm/atomic-helper: Implement subsystem-level suspend/resume

[Daniel Vetter]

On Tue, Aug 11, 2015 at 01:37:44PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding at nvidia.com>
> 
> Provide subsystem-level suspend and resume helpers that can be used to
> implement suspend/resume on atomic mode-setting enabled drivers.
> 
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 191 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |   5 +
>  2 files changed, 196 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f842499ca79e..aface21b713e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1751,6 +1751,197 @@ backoff:
>  EXPORT_SYMBOL(drm_atomic_helper_set_config);
>  
>  /**
> + * drm_atomic_helper_disable_all - disable all currently active outputs
> + * @dev: DRM device
> + *
> + * Loops through all connectors, finding those that aren't turned off and then
> + * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> + * that they are connected to.

Imo the subordinate helpers here should also mention how they're used in
the suspend/resume helpers and reference them (soonish all those function
references will be hyperlinks, making them even more useful). Same for the
other subordinate helpers (also duplicate_state from the previous patch).

> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int drm_atomic_helper_disable_all(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_connector_state *conn_state;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_connector *conn;
> +	unsigned int i;
> +	int err;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state->allow_modeset = true;
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	err = drm_modeset_lock(&config->connection_mutex, &ctx);
> +	if (err < 0)
> +		goto free;

Same comment about locking as with the duplicate_state helper.

> +
> +	drm_for_each_connector(conn, dev) {
> +		struct drm_crtc *crtc = conn->state->crtc;
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			err = PTR_ERR(crtc_state);
> +			goto unlock;
> +		}
> +
> +		err = drm_atomic_add_affected_connectors(state, crtc);
> +		if (err < 0)
> +			goto unlock;
> +
> +		conn->dpms = DRM_MODE_DPMS_OFF;
> +		crtc_state->active = false;
> +	}
> +
> +	err = drm_atomic_commit(state);
> +	if (err < 0) {
> +		/* restore DPMS mode */
> +		for_each_connector_in_state(state, conn, conn_state, i)
> +			conn->dpms = DRM_MODE_DPMS_ON;
> +
> +		if (err == -EDEADLK) {
> +			drm_atomic_state_clear(state);
> +			drm_modeset_backoff(&ctx);
> +			goto retry;
> +		}
> +	}
> +
> +unlock:
> +	drm_modeset_drop_locks(&ctx);
> +free:
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	if (err < 0)
> +		drm_atomic_state_free(state);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_disable_all);
> +
> +/**
> + * drm_atomic_helper_restore_commit - restores an atomic state object
> + * @state: atomic state object
> + *
> + * Takes an atomic state object for which no locks have been taken (such as
> + * atomic state objects obtained from drm_atomic_helper_duplicate_state()),
> + * grabs all the necessary locks, commits the state and releases the locks
> + * while gracefully handling deadlocks.

I think this also needs some warnings that reusing atomic states while
locks have been droppped could be risky, similar to duplicate_state().

> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +static int drm_atomic_helper_restore_commit(struct drm_atomic_state *state)
> +{
> +	struct drm_mode_config *config = &state->dev->mode_config;
> +	struct drm_plane_state *plane_state;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +	int err;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state->allow_modeset = true;
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	err = drm_modeset_lock(&config->connection_mutex, &ctx);
> +	if (err < 0)
> +		goto fini;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		err = drm_modeset_lock(&crtc->mutex, &ctx);
> +		if (err < 0)
> +			goto unlock;
> +	}
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		err = drm_modeset_lock(&plane->mutex, &ctx);
> +		if (err < 0)
> +			goto unlock;
> +	}
> +
> +	err = drm_atomic_commit(state);
> +	if (err == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +unlock:
> +	drm_modeset_drop_locks(&ctx);
> +fini:
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return err;
> +}
> +
> +/**
> + * drm_atomic_helper_suspend - subsystem-level suspend helper
> + * @dev: DRM device
> + *
> + * Duplicates the current atomic state, disables all active outputs and then
> + * returns a pointer to the original atomic state to the caller. Drivers can
> + * pass this pointer to the drm_atomic_helper_resume() helper upon resume to
> + * restore the output configuration that was active at the time the system
> + * entered suspend.
> + *
> + * Returns: A pointer to a copy of the state before suspend on success or an
> + * ERR_PTR()-encoded error code on failure. Drivers should store the returned
> + * atomic state object and pass it to the drm_atomic_helper_resume() helper
> + * upon resume.

Style-nit: There should be a newline after "Returns:" for consistency with
other kerneldoc in drm.

> + */
> +struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
> +{
> +	struct drm_atomic_state *state;
> +	int err;
> +
> +	state = drm_atomic_helper_duplicate_state(dev);
> +	if (IS_ERR(state))
> +		return state;
> +
> +	err = drm_atomic_helper_disable_all(dev);
> +	if (err < 0) {
> +		drm_atomic_state_free(state);

This will hit a locking WARN_ON. Maybe better to pull out the
drm_modset_lock all from these two functions and just pass them the
acquire ctx? Otoh drm-misc already has a patch from Maarten to remove that
locking check since its violated with drm dp mst stuff iirc.

> +		return ERR_PTR(err);
> +	}
> +
> +	return state;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_suspend);
> +
> +/**
> + * drm_atomic_helper_resume - subsystem-level resume helper
> + * @dev: DRM device
> + * @state: atomic state to resume to
> + *
> + * Takes an atomic state object, grabs all the necessary locks and commits it.
> + * This can be used in conjunction with the drm_atomic_helper_suspend() helper
> + * to implement suspend/resume for atomic mode-setting enabled drivers.

Kerneldoc should also mention that it calls reset() first to make sure sw
and hw state agree again. Otherwise the atomic_commit might go "well
there's nothing to do here", so it's a crucial step.

Looks great otherwise.
-Daniel

> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int drm_atomic_helper_resume(struct drm_device *dev,
> +				    struct drm_atomic_state *state)
> +{
> +	drm_mode_config_reset(dev);
> +
> +	return drm_atomic_helper_restore_commit(state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_resume);
> +
> +/**
>   * drm_atomic_helper_crtc_set_property - helper for crtc properties
>   * @crtc: DRM crtc
>   * @property: DRM property
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 5d27612997ce..080f131c9f2e 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -75,6 +75,11 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  int drm_atomic_helper_disable_plane(struct drm_plane *plane);
>  int drm_atomic_helper_set_config(struct drm_mode_set *set);
>  
> +int drm_atomic_helper_disable_all(struct drm_device *dev);
> +struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
> +int drm_atomic_helper_resume(struct drm_device *dev,
> +			     struct drm_atomic_state *state);
> +
>  int drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>  					struct drm_property *property,
>  					uint64_t val);
> -- 
> 2.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch