[PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[ville.syrjala]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This function would call drm_modeset_lock_all, while the suspend/resume
functions already have their own locking. Fix this by factoring out
__intel_display_resume, and calling the atomic helpers for duplicating
atomic state and disabling all crtc's during suspend.

Changes since v1:
- Deal with -EDEADLK right after lock_all and clean up calls
  to hw readout.
- Always take all modeset locks so updates during gpu reset are blocked.
Changes since v2:
- Fix deadlock in intel_update_primary_planes.
- Move WARN_ON(EDEADLK) to __intel_display_resume.
- pctx -> ctx
- only call __intel_display_resume on success in intel_display_resume.
Changes since v3:
- Rebase on top of dev_priv -> dev change.
- Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
Changes since v4 [by vsyrjala]:
- Deal with skip_intermediate_wm
- Update comment w.r.t. mode_config.mutex vs. ->detect()
- Rebase due to INTEL_GEN() etc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Cc: stable@vger.kernel.org
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 170 ++++++++++++++++++++++-------------
 2 files changed, 111 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index feec00f769e1..94a333c44bee 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1840,6 +1840,7 @@ struct drm_i915_private {
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
 	struct drm_atomic_state *modeset_restore_state;
+	struct drm_modeset_acquire_ctx reset_ctx;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_ggtt ggtt; /* VM representing the global address space */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9cbf5431c1e3..1e2fb444eb93 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3093,40 +3093,110 @@ static void intel_update_primary_planes(struct drm_device *dev)
 
 	for_each_crtc(dev, crtc) {
 		struct intel_plane *plane = to_intel_plane(crtc->primary);
-		struct intel_plane_state *plane_state;
-
-		drm_modeset_lock_crtc(crtc, &plane->base);
-		plane_state = to_intel_plane_state(plane->base.state);
+		struct intel_plane_state *plane_state =
+			to_intel_plane_state(plane->base.state);
 
 		if (plane_state->visible)
 			plane->update_plane(&plane->base,
 					    to_intel_crtc_state(crtc->state),
 					    plane_state);
+	}
+}
+
+static int
+__intel_display_resume(struct drm_device *dev,
+		       struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i, ret;
 
-		drm_modeset_unlock_crtc(crtc);
+	intel_modeset_setup_hw_state(dev);
+	i915_redisable_vga(dev);
+
+	if (!state)
+		return 0;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * Force recalculation even if we restore
+		 * current state. With fast modeset this may not result
+		 * in a modeset when the state is compatible.
+		 */
+		crtc_state->mode_changed = true;
 	}
+
+	/* ignore any reset values/BIOS leftovers in the WM registers */
+	to_intel_atomic_state(state)->skip_intermediate_wm = true;
+
+	ret = drm_atomic_commit(state);
+
+	WARN_ON(ret == -EDEADLK);
+	return ret;
 }
 
 void intel_prepare_reset(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = &dev_priv->drm;
+	struct drm_modeset_acquire_ctx *ctx = &dev_priv->reset_ctx;
+	struct drm_atomic_state *state;
+	int ret;
+
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev_priv))
 		return;
 
-	/* reset doesn't touch the display */
+	/*
+	 * Need mode_config.mutex so that we don't
+	 * trample ongoing ->detect() and whatnot.
+	 */
+	mutex_lock(&dev->mode_config.mutex);
+	drm_modeset_acquire_init(ctx, 0);
+	while (1) {
+		ret = drm_modeset_lock_all_ctx(dev, ctx);
+		if (ret != -EDEADLK)
+			break;
+
+		drm_modeset_backoff(ctx);
+	}
+
+	/* reset doesn't touch the display, but flips might get nuked anyway, */
 	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
 		return;
 
-	drm_modeset_lock_all(&dev_priv->drm);
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	intel_display_suspend(&dev_priv->drm);
+	state = drm_atomic_helper_duplicate_state(dev, ctx);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		state = NULL;
+		DRM_ERROR("Duplicating state failed with %i\n", ret);
+		goto err;
+	}
+
+	ret = drm_atomic_helper_disable_all(dev, ctx);
+	if (ret) {
+		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+		goto err;
+	}
+
+	dev_priv->modeset_restore_state = state;
+	state->acquire_ctx = ctx;
+	return;
+
+err:
+	drm_atomic_state_free(state);
 }
 
 void intel_finish_reset(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = &dev_priv->drm;
+	struct drm_modeset_acquire_ctx *ctx = &dev_priv->reset_ctx;
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	int ret;
+
 	/*
 	 * Flips in the rings will be nuked by the reset,
 	 * so complete all pending flips so that user space
@@ -3138,6 +3208,8 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 	if (IS_GEN2(dev_priv))
 		return;
 
+	dev_priv->modeset_restore_state = NULL;
+
 	/* reset doesn't touch the display */
 	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv)) {
 		/*
@@ -3149,29 +3221,32 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 		 * FIXME: Atomic will make this obsolete since we won't schedule
 		 * CS-based flips (which might get lost in gpu resets) any more.
 		 */
-		intel_update_primary_planes(&dev_priv->drm);
-		return;
-	}
-
-	/*
-	 * The display has been reset as well,
-	 * so need a full re-initialization.
-	 */
-	intel_runtime_pm_disable_interrupts(dev_priv);
-	intel_runtime_pm_enable_interrupts(dev_priv);
+		intel_update_primary_planes(dev);
+	} else {
+		/*
+		 * The display has been reset as well,
+		 * so need a full re-initialization.
+		 */
+		intel_runtime_pm_disable_interrupts(dev_priv);
+		intel_runtime_pm_enable_interrupts(dev_priv);
 
-	intel_modeset_init_hw(&dev_priv->drm);
+		intel_modeset_init_hw(dev);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	if (dev_priv->display.hpd_irq_setup)
-		dev_priv->display.hpd_irq_setup(dev_priv);
-	spin_unlock_irq(&dev_priv->irq_lock);
+		spin_lock_irq(&dev_priv->irq_lock);
+		if (dev_priv->display.hpd_irq_setup)
+			dev_priv->display.hpd_irq_setup(dev_priv);
+		spin_unlock_irq(&dev_priv->irq_lock);
 
-	intel_display_resume(&dev_priv->drm);
+		ret = __intel_display_resume(dev, state);
+		if (ret)
+			DRM_ERROR("Restoring old state failed with %i\n", ret);
 
-	intel_hpd_init(dev_priv);
+		intel_hpd_init(dev_priv);
+	}
 
-	drm_modeset_unlock_all(&dev_priv->drm);
+	drm_modeset_drop_locks(ctx);
+	drm_modeset_acquire_fini(ctx);
+	mutex_unlock(&dev->mode_config.mutex);
 }
 
 static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
@@ -16163,9 +16238,10 @@ void intel_display_resume(struct drm_device *dev)
 	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
-	bool setup = false;
 
 	dev_priv->modeset_restore_state = NULL;
+	if (state)
+		state->acquire_ctx = &ctx;
 
 	/*
 	 * This is a cludge because with real atomic modeset mode_config.mutex
@@ -16176,43 +16252,17 @@ void intel_display_resume(struct drm_device *dev)
 	mutex_lock(&dev->mode_config.mutex);
 	drm_modeset_acquire_init(&ctx, 0);
 
-retry:
-	ret = drm_modeset_lock_all_ctx(dev, &ctx);
-
-	if (ret == 0 && !setup) {
-		setup = true;
-
-		intel_modeset_setup_hw_state(dev);
-		i915_redisable_vga(dev);
-	}
-
-	if (ret == 0 && state) {
-		struct drm_crtc_state *crtc_state;
-		struct drm_crtc *crtc;
-		int i;
-
-		state->acquire_ctx = &ctx;
-
-		/* ignore any reset values/BIOS leftovers in the WM registers */
-		to_intel_atomic_state(state)->skip_intermediate_wm = true;
-
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
-			/*
-			 * Force recalculation even if we restore
-			 * current state. With fast modeset this may not result
-			 * in a modeset when the state is compatible.
-			 */
-			crtc_state->mode_changed = true;
-		}
-
-		ret = drm_atomic_commit(state);
-	}
+	while (1) {
+		ret = drm_modeset_lock_all_ctx(dev, &ctx);
+		if (ret != -EDEADLK)
+			break;
 
-	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
-		goto retry;
 	}
 
+	if (!ret)
+		ret = __intel_display_resume(dev, state);
+
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 	mutex_unlock(&dev->mode_config.mutex);
-- 
2.7.4

Re: [PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Maarten Lankhorst]

Hey,

Op 05-08-16 om 22:28 schreef ville.syrjala@linux.intel.com:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> This function would call drm_modeset_lock_all, while the suspend/resume
> functions already have their own locking. Fix this by factoring out
> __intel_display_resume, and calling the atomic helpers for duplicating
> atomic state and disabling all crtc's during suspend.
>
> Changes since v1:
> - Deal with -EDEADLK right after lock_all and clean up calls
>   to hw readout.
> - Always take all modeset locks so updates during gpu reset are blocked.
> Changes since v2:
> - Fix deadlock in intel_update_primary_planes.
> - Move WARN_ON(EDEADLK) to __intel_display_resume.
> - pctx -> ctx
> - only call __intel_display_resume on success in intel_display_resume.
> Changes since v3:
> - Rebase on top of dev_priv -> dev change.
> - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> Changes since v4 [by vsyrjala]:
> - Deal with skip_intermediate_wm
> - Update comment w.r.t. mode_config.mutex vs. ->detect()
> - Rebase due to INTEL_GEN() etc.

Setting skip_intermediate_wm seems to have already been upstreamed and I missed it, but
this may blow up in .crtc_enable, which programs in the intermediate wm's which is used
until all planes are enabled.

I fear this may blow up in interesting ways. And it should probably be using
dev_priv->wm.distrust_bios_wm instead like on SKL.

~Maarten

Re: [PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Ville Syrjälä]

On Mon, Aug 08, 2016 at 09:52:49AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 05-08-16 om 22:28 schreef ville.syrjala@linux.intel.com:
> > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >
> > This function would call drm_modeset_lock_all, while the suspend/resume
> > functions already have their own locking. Fix this by factoring out
> > __intel_display_resume, and calling the atomic helpers for duplicating
> > atomic state and disabling all crtc's during suspend.
> >
> > Changes since v1:
> > - Deal with -EDEADLK right after lock_all and clean up calls
> >   to hw readout.
> > - Always take all modeset locks so updates during gpu reset are blocked.
> > Changes since v2:
> > - Fix deadlock in intel_update_primary_planes.
> > - Move WARN_ON(EDEADLK) to __intel_display_resume.
> > - pctx -> ctx
> > - only call __intel_display_resume on success in intel_display_resume.
> > Changes since v3:
> > - Rebase on top of dev_priv -> dev change.
> > - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> > Changes since v4 [by vsyrjala]:
> > - Deal with skip_intermediate_wm
> > - Update comment w.r.t. mode_config.mutex vs. ->detect()
> > - Rebase due to INTEL_GEN() etc.
> 
> Setting skip_intermediate_wm seems to have already been upstreamed and I missed it, but
> this may blow up in .crtc_enable, which programs in the intermediate wm's which is used
> until all planes are enabled.

What blows up and how?

Even if it can blow up we don't have any two stage wm stuff for pre-g4x at
this time anyway, so -ENOCARE at this point really.

> 
> I fear this may blow up in interesting ways. And it should probably be using
> dev_priv->wm.distrust_bios_wm instead like on SKL.

Sigh. How many ways do we need to do the same thing?

Anywyas, what we should really do is sanitize the current wms better
at readout time, and then we shouldn't need these flags at all.

-- 
Ville Syrj�l�
Intel OTC