[PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[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@linux.intel.com>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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.
  */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 95fb7d391788..e54800a3e52d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1306,8 +1306,6 @@ void i915_driver_unload(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
-	struct drm_modeset_acquire_ctx ctx;
-	int ret;
 
 	intel_fbdev_fini(dev);
 
@@ -1316,23 +1314,7 @@ void i915_driver_unload(struct drm_device *dev)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
-	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);
+	drm_atomic_helper_shutdown(dev);
 
 	intel_gvt_cleanup(dev_priv);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 8b3622afa530..b5a92386cc8e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -413,8 +413,12 @@ nouveau_display_fini(struct drm_device *dev, bool suspend)
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 
-	if (!suspend)
-		drm_crtc_force_disable_all(dev);
+	if (!suspend) {
+		if (drm_drv_uses_atomic_modeset(dev))
+			drm_atomic_helper_shutdown(dev);
+		else
+			drm_crtc_force_disable_all(dev);
+	}
 
 	/* Make sure that drm and hw vblank irqs get properly disabled. */
 	drm_for_each_crtc(crtc, dev)
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);
 	/* 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);
 }
 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);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: warning for drm/atomic: Introduce drm_atomic_helper_shutdown

[Patchwork]

== Series Details ==

Series: drm/atomic: Introduce drm_atomic_helper_shutdown
URL   : https://patchwork.freedesktop.org/series/21602/
State : warning

== Summary ==

Series 21602v1 drm/atomic: Introduce drm_atomic_helper_shutdown
https://patchwork.freedesktop.org/api/1.0/series/21602/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-bxt-t5700) fdo#100125
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> DMESG-WARN (fi-byt-n2820)

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 465s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 587s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 538s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 559s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 496s
fi-byt-n2820     total:278  pass:246  dwarn:1   dfail:0   fail:0   skip:31  time: 500s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 441s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 440s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 434s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 503s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 497s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 482s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 484s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 598s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 482s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 516s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 547s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 411s

84b98c6d4f5f6ce0ce17b8fd07629dfc71e7d829 drm-tip: 2017y-03m-21d-11h-08m-23s UTC integration manifest
d0988b8 drm/atomic: Introduce drm_atomic_helper_shutdown

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4243/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@linux.intel.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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@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);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

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

v2: Make it compile on arm drivers too (Noralf).

Acked-by: Noralf Trønnes <noralf@tronnes.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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.
  */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 03d9e45694c9..98b17070a123 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1307,8 +1307,6 @@ void i915_driver_unload(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
-	struct drm_modeset_acquire_ctx ctx;
-	int ret;
 
 	intel_fbdev_fini(dev);
 
@@ -1317,23 +1315,7 @@ void i915_driver_unload(struct drm_device *dev)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
-	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);
+	drm_atomic_helper_shutdown(dev);
 
 	intel_gvt_cleanup(dev_priv);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 8b3622afa530..b5a92386cc8e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -413,8 +413,12 @@ nouveau_display_fini(struct drm_device *dev, bool suspend)
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 
-	if (!suspend)
-		drm_crtc_force_disable_all(dev);
+	if (!suspend) {
+		if (drm_drv_uses_atomic_modeset(dev))
+			drm_atomic_helper_shutdown(dev);
+		else
+			drm_crtc_force_disable_all(dev);
+	}
 
 	/* Make sure that drm and hw vblank irqs get properly disabled. */
 	drm_for_each_crtc(crtc, dev)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
index 6a257dd08ee0..9546b4f8bf50 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->drm);
 	/* 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->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);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Maarten Lankhorst]

Op 21-03-17 om 16:26 schreef 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 ...
>
> v2: Make it compile on arm drivers too (Noralf).
>
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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);
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

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

v2: Make it compile on arm drivers too (Noralf).

v3: Correct kerneldoc to point at _disable_all().

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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..55c1f2b6a10b 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_all().
+ */
+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.
  */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 03d9e45694c9..98b17070a123 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1307,8 +1307,6 @@ void i915_driver_unload(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
-	struct drm_modeset_acquire_ctx ctx;
-	int ret;
 
 	intel_fbdev_fini(dev);
 
@@ -1317,23 +1315,7 @@ void i915_driver_unload(struct drm_device *dev)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
-	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);
+	drm_atomic_helper_shutdown(dev);
 
 	intel_gvt_cleanup(dev_priv);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 8b3622afa530..b5a92386cc8e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -413,8 +413,12 @@ nouveau_display_fini(struct drm_device *dev, bool suspend)
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 
-	if (!suspend)
-		drm_crtc_force_disable_all(dev);
+	if (!suspend) {
+		if (drm_drv_uses_atomic_modeset(dev))
+			drm_atomic_helper_shutdown(dev);
+		else
+			drm_crtc_force_disable_all(dev);
+	}
 
 	/* Make sure that drm and hw vblank irqs get properly disabled. */
 	drm_for_each_crtc(crtc, dev)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
index 6a257dd08ee0..9546b4f8bf50 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->drm);
 	/* 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->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);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: failure for drm/atomic: Introduce drm_atomic_helper_shutdown (rev3)

[Patchwork]

== Series Details ==

Series: drm/atomic: Introduce drm_atomic_helper_shutdown (rev3)
URL   : https://patchwork.freedesktop.org/series/21602/
State : failure

== Summary ==

Series 21602v3 drm/atomic: Introduce drm_atomic_helper_shutdown
https://patchwork.freedesktop.org/api/1.0/series/21602/revisions/3/mbox/

Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> INCOMPLETE (fi-byt-j1900)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 460s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 582s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 540s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 558s
fi-byt-j1900     total:5    pass:4    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 437s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 445s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 429s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 514s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 494s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 485s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 476s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 595s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 485s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 512s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 542s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 419s
fi-byt-n2820 failed to collect. IGT log at Patchwork_4251/fi-byt-n2820/igt.log

d17b31b3444ecffa68be0c331a4b20078fd10ccf drm-tip: 2017y-03m-21d-17h-36m-49s UTC integration manifest
92e6b44 drm/atomic: Introduce drm_atomic_helper_shutdown

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4251/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1598 bytes --]

On 21/03/17 18:41, Daniel Vetter wrote:
> 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 ...
> 
> v2: Make it compile on arm drivers too (Noralf).
> 
> v3: Correct kerneldoc to point at _disable_all().
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

As a side note, I find it a bit odd that when fbdev is disabled, the
crtcs stays enabled even when DRM userspace app quits. Or is that just
omapdrm behavior? I presume not, as this shutdown on unload would not be
needed otherwise.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Daniel Vetter]

On Mon, Mar 27, 2017 at 10:15:12AM +0300, Tomi Valkeinen wrote:
> On 21/03/17 18:41, Daniel Vetter wrote:
> > 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 ...
> > 
> > v2: Make it compile on arm drivers too (Noralf).
> > 
> > v3: Correct kerneldoc to point at _disable_all().
> > 
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Acked-by: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> As a side note, I find it a bit odd that when fbdev is disabled, the
> crtcs stays enabled even when DRM userspace app quits. Or is that just
> omapdrm behavior? I presume not, as this shutdown on unload would not be
> needed otherwise.

With atomic we've stopped killing the entire CRTC when you the last
userspace reference for the framebuffer on the primary plane disappears,
but just shut down the primary plane. Assuming the driver can do that, we
fall back to full CRTC shutdown if not. That avoids a pile of flickering
and unecessary modesets.

But yeah downside is that the crtc is active even when unloading. Note
that with the fb refcounting years ago the CRTC (including planes) was
already kept alive when you have fbdev emulation enabled.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1237 bytes --]

On 27/03/17 10:43, Daniel Vetter wrote:

> With atomic we've stopped killing the entire CRTC when you the last
> userspace reference for the framebuffer on the primary plane disappears,
> but just shut down the primary plane. Assuming the driver can do that, we
> fall back to full CRTC shutdown if not. That avoids a pile of flickering
> and unecessary modesets.
> 
> But yeah downside is that the crtc is active even when unloading. Note

Not only that, but also when you stop a DRM userspace app and don't
start a new one, the crtc stays active. Maybe that's not so common
scenario, but I wouldn't be that surprised if on embedded world someone
only runs a DRM app when they're showing something, and no app in
between the runs.

What makes the behavior a bit funny is that if I boot without fbdev
emulation, the crtcs stay off (as they should). But run a DRM app, quit
it, and the crtcs are on.

This is a bit related to the other annoyance, which is that we don't
reset properties when a DRM app quits. I think the state of the HW
should be restored to exactly the same state as it was (including
turning off the crtcs). I'm planning to have a look at this whenever I
happen to find some time...

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Daniel Vetter]

On Tue, Mar 28, 2017 at 03:18:36PM +0300, Tomi Valkeinen wrote:
> On 27/03/17 10:43, Daniel Vetter wrote:
> 
> > With atomic we've stopped killing the entire CRTC when you the last
> > userspace reference for the framebuffer on the primary plane disappears,
> > but just shut down the primary plane. Assuming the driver can do that, we
> > fall back to full CRTC shutdown if not. That avoids a pile of flickering
> > and unecessary modesets.
> > 
> > But yeah downside is that the crtc is active even when unloading. Note
> 
> Not only that, but also when you stop a DRM userspace app and don't
> start a new one, the crtc stays active. Maybe that's not so common
> scenario, but I wouldn't be that surprised if on embedded world someone
> only runs a DRM app when they're showing something, and no app in
> between the runs.
> 
> What makes the behavior a bit funny is that if I boot without fbdev
> emulation, the crtcs stay off (as they should). But run a DRM app, quit
> it, and the crtcs are on.

Atm the recommendation is "don't do that". And keeping display state us
unchanged as possible is one of the things atomic was meant for: With
solid state tracking where sw state always matches hw state you can do
stuff like fastboot, i.e. take over the display state from firmware. Not a
common thing on embedded things, but rather important for i915. Ofc in
that case you can't use the reset helper, but instead need a hw state
readout logic.

> This is a bit related to the other annoyance, which is that we don't
> reset properties when a DRM app quits. I think the state of the HW
> should be restored to exactly the same state as it was (including
> turning off the crtcs). I'm planning to have a look at this whenever I
> happen to find some time...

I have a blog post with all the ideas from last time around we've
discussed this:

http://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html

Consensus was that we'll look at this again when someone has a real use
case that needs it. And then maybe still decide that they just need a
system compositor :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1351 bytes --]

On 28/03/17 17:22, Daniel Vetter wrote:

>> This is a bit related to the other annoyance, which is that we don't
>> reset properties when a DRM app quits. I think the state of the HW
>> should be restored to exactly the same state as it was (including
>> turning off the crtcs). I'm planning to have a look at this whenever I
>> happen to find some time...
> 
> I have a blog post with all the ideas from last time around we've
> discussed this:
> 
> http://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> 
> Consensus was that we'll look at this again when someone has a real use
> case that needs it. And then maybe still decide that they just need a
> system compositor :-)

Thanks for the link. I now see this is pretty messed up (but necessary)
stuff we have =).

The situation on our embedded devices is often a bit simpler: no system
compositors, one VT, (hopefully soon) no fbdev, and we have a clear
reset state.

Reading the text makes me think the state should be somehow explicitly
passed from one compositor to the other, which would keep that state
active on the kernel side, but if that's even sanely possible, I have no
idea...

I guess I'll have to deal with this in the userspace for now, and I'll
add a helper to kms++ library which will disable and reset everything it
can.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel