[Intel-gfx] [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Noralf Trønnes]

Den 21.03.2017 12.22, skrev Daniel Vetter:
> The trouble here is that it does multiple atomic commits under one
> drm_modeset_lock_all, which breaks the behind-the-scenes acquire
> context magic that function pulls off. It's much better to have one
> overall atomic commit. That we still have multiple atomic commits
> prevents us from adding some pretty useful debug checks to the atomic
> machinery.
>
> Hence it is really a bad idea to call the legacy
> drm_crtc_force_disable_all() function. There's 2 atomic drivers using
> this still, nouveau and tinydrm. To fix this, introduce a new
> drm_atomic_helper_shutdown() by extracting the code from i915.
>
> While at it improve kernel-doc and catch future offenders by
> sprinkling a WARN_ON into the legacy function. We should probably move
> those into the legacy modeset helpers, too ...
>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Noralf Trønnes <noralf at tronnes.org>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c         | 42 +++++++++++++++++++++++++++--
>   drivers/gpu/drm/drm_crtc.c                  |  7 +++++
>   drivers/gpu/drm/i915/i915_drv.c             | 20 +-------------
>   drivers/gpu/drm/nouveau/nouveau_display.c   |  8 ++++--
>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |  4 +--
>   include/drm/drm_atomic_helper.h             |  1 +
>   6 files changed, 57 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index b4dcb2559e09..5f401519d03c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2444,7 +2444,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
>    * that they are connected to.
>    *
>    * This is used for example in suspend/resume to disable all currently active
> - * functions when suspending.
> + * functions when suspending. If you just want to shut down everything at e.g.
> + * driver unload, look at drm_atomic_helper_shutdown().
>    *
>    * Note that if callers haven't already acquired all modeset locks this might
>    * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> @@ -2453,7 +2454,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
>    * 0 on success or a negative error code on failure.
>    *
>    * See also:
> - * drm_atomic_helper_suspend(), drm_atomic_helper_resume()
> + * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> + * drm_atomic_helper_shutdown().
>    */
>   int drm_atomic_helper_disable_all(struct drm_device *dev,
>   				  struct drm_modeset_acquire_ctx *ctx)
> @@ -2518,6 +2520,42 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>   EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>   
>   /**
> + * drm_atomic_helper_shutdown - shutdown all CRTC
> + * @dev: DRM device
> + *
> + * This shuts down all CRTC, which is useful for driver unloading. Shutdown on
> + * suspend should instead be handled with drm_atomic_helper_suspend(), since
> + * that also takes a snapshot of the modeset state to be restored on resume.
> + *
> + * This is just a convenience wrapper around drm_atomic_helper_disable_all(),
> + * and it is the atomic version of drm_crtc_force_disable().
> + */
> +void drm_atomic_helper_shutdown(struct drm_device *dev)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	while (1) {
> +		ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +		if (!ret)
> +			ret = drm_atomic_helper_disable_all(dev, &ctx);
> +
> +		if (ret != -EDEADLK)
> +			break;
> +
> +		drm_modeset_backoff(&ctx);
> +	}
> +
> +	if (ret)
> +		DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_shutdown);
> +
> +/**
>    * drm_atomic_helper_suspend - subsystem-level suspend helper
>    * @dev: DRM device
>    *
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e2974d3c92e7..660b4c8715de 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -94,6 +94,8 @@ EXPORT_SYMBOL(drm_crtc_from_index);
>    * drm_crtc_force_disable - Forcibly turn off a CRTC
>    * @crtc: CRTC to turn off
>    *
> + * Note: This should only be used by non-atomic legacy drivers.
> + *
>    * Returns:
>    * Zero on success, error code on failure.
>    */
> @@ -103,6 +105,8 @@ int drm_crtc_force_disable(struct drm_crtc *crtc)
>   		.crtc = crtc,
>   	};
>   
> +	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
> +
>   	return drm_mode_set_config_internal(&set);
>   }
>   EXPORT_SYMBOL(drm_crtc_force_disable);
> @@ -114,6 +118,9 @@ EXPORT_SYMBOL(drm_crtc_force_disable);
>    * Drivers may want to call this on unload to ensure that all displays are
>    * unlit and the GPU is in a consistent, low power state. Takes modeset locks.
>    *
> + * Note: This should only be used by non-atomic legacy drivers. For an atomic
> + * version look at drm_atomic_helper_shutdown().
> + *
>    * Returns:
>    * Zero on success, error code on failure.
>    */

<snip>

> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> index 6a257dd08ee0..bbb85e0fc153 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> @@ -251,7 +251,7 @@ static void tinydrm_unregister(struct tinydrm_device *tdev)
>   {
>   	struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
>   
> -	drm_crtc_force_disable_all(tdev->drm);
> +	drm_atomic_helper_shutdown(tdev->dev);

It's tdev->drm both here and the next one.

To me 'dev' is struct device, but maybe for drm devs sanity I should have
used dev instead...

With that fixed:
Acked-by: Noralf Trønnes <noralf at tronnes.org>

>   	/* don't restore fbdev in lastclose, keep pipeline disabled */
>   	tdev->fbdev_cma = NULL;
>   	drm_dev_unregister(tdev->drm);
> @@ -302,7 +302,7 @@ EXPORT_SYMBOL(devm_tinydrm_register);
>    */
>   void tinydrm_shutdown(struct tinydrm_device *tdev)
>   {
> -	drm_crtc_force_disable_all(tdev->drm);
> +	drm_atomic_helper_shutdown(tdev->dev);

tdev->drm

>   }
>   EXPORT_SYMBOL(tinydrm_shutdown);
>   
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index dc16274987c7..969f7237f1fc 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -104,6 +104,7 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
>   
>   int drm_atomic_helper_disable_all(struct drm_device *dev,
>   				  struct drm_modeset_acquire_ctx *ctx);
> +void drm_atomic_helper_shutdown(struct drm_device *dev);
>   struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
>   int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
>   					      struct drm_modeset_acquire_ctx *ctx);