drm/i915: Add a way to test the modeset done during gpu reset.

Add force_reset_modeset_test as a parameter to force the modeset path during gpu reset. This allows a IGT test to set the knob and trigger a hang to force the gpu reset, even on platforms that wouldn't otherwise require it.

Changes since v1:
- Split out fix to separate commit.

Signed-off-by Maarten Lankhorst <[email protected]>

Testcase: drv_hangman.reset-with-forced-modeset

---
 drivers/gpu/drm/i915/i915_params.c   |  6 ++++++
 drivers/gpu/drm/i915/i915_params.h   |  1 +
 drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++++++++------------
 3 files changed, 35 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_params.c +6 -0

index 1779f02e6df8..80ce581793dc 100644
@@ -45,6 +45,7 @@ struct i915_params i915 __read_mostly = {
 	.fastboot = 0,
 	.prefault_disable = 0,
 	.load_detect_test = 0,
+	.force_reset_modeset_test = 0,
 	.reset = true,
 	.invert_brightness = 0,
 	.disable_display = 0,

@@ -158,6 +159,11 @@ MODULE_PARM_DESC(load_detect_test,
 	"Force-enable the VGA load detect code for testing (default:false). "
 	"For developers only.");
 
+module_param_named_unsafe(force_reset_modeset_test, i915.force_reset_modeset_test, bool, 0600);
+MODULE_PARM_DESC(force_reset_modeset_test,
+	"Force a modeset during gpu reset for testing (default:false). "
+	"For developers only.");
+
 module_param_named_unsafe(invert_brightness, i915.invert_brightness, int, 0600);
 MODULE_PARM_DESC(invert_brightness,
 	"Invert backlight brightness "

drivers/gpu/drm/i915/i915_params.h +1 -0

index 02bc27804291..3934c4300427 100644
@@ -55,6 +55,7 @@ struct i915_params {
 	bool fastboot;
 	bool prefault_disable;
 	bool load_detect_test;
+	bool force_reset_modeset_test;
 	bool reset;
 	bool disable_display;
 	bool enable_guc_submission;

drivers/gpu/drm/i915/intel_display.c +7 -2

index 2f76efc86417..faf2da08778f 100644
@@ -3160,6 +3160,7 @@ __intel_display_resume(struct drm_device *dev,
 
 void intel_prepare_reset(struct drm_device *dev)
 {
+	bool requires_display_reset = true;
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx *pctx;
 	int ret;

@@ -3169,8 +3170,12 @@ void intel_prepare_reset(struct drm_device *dev)
 		return;
 
 	/* reset doesn't touch the display */
-	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
-		return;
+	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) {
+		if (!i915.force_reset_modeset_test)
+			return;
+
+		requires_display_reset = false;
+	}
 
 	drm_modeset_lock_all(dev);
 	pctx = dev->mode_config.acquire_ctx;

drivers/gpu/drm/i915/intel_display.c +4 -11

 }
 
 void intel_finish_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	bool hpd_init;
 	int ret;
 
 	/*

@@ -3220,22 +3228,27 @@ void intel_finish_reset(struct drm_device *dev)
 
 	/* reset doesn't touch the display */
 	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) {
-		/*
-		 * Flips in the rings have been nuked by the reset,
-		 * so update the base address of all primary
-		 * planes to the the last fb to make sure we're
-		 * showing the correct fb after a reset.
-		 *
-		 * 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);
-		return;
+		hpd_init = false;
+
+		if (!state) {

drivers/gpu/drm/i915/intel_display.c +5 -1

+			return;
+		}
 	} else {
 		/*
 		 * The display has been reset as well,
 		 * so need a full re-initialization.
 		 */
+		hpd_init = true;
 		intel_runtime_pm_disable_interrupts(dev_priv);
 		intel_runtime_pm_enable_interrupts(dev_priv);
 

@@ -3255,7 +3268,8 @@ void intel_finish_reset(struct drm_device *dev)
 	if (ret)
 		DRM_ERROR("Restoring old state failed with %i\n", ret);
 
-	intel_hpd_init(dev_priv);
+	if (hpd_init)
+		intel_hpd_init(dev_priv);
 
 	drm_modeset_unlock_all(dev);
 }

Signature · 1 line

2.1.0

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.

Signed-off-by Maarten Lankhorst <[email protected]>

Fixes e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")

Cc [email protected]

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.

Signed-off-by Maarten Lankhorst <[email protected]>

Fixes e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")

Cc [email protected]

---
 drivers/gpu/drm/i915/intel_display.c | 123 +++++++++++++++++++++++------------
 1 file changed, 83 insertions(+), 40 deletions(-)

drivers/gpu/drm/i915/intel_display.c +2 -0

index feb7028341b8..2f76efc86417 100644
@@ -3126,8 +3126,44 @@ static void intel_update_primary_planes(struct drm_device *dev)
 	}
 }
 
+static int
+__intel_display_resume(struct drm_device *dev,

drivers/gpu/drm/i915/intel_display.c +8 -0

+
+		intel_modeset_setup_hw_state(dev);
+		i915_redisable_vga(dev);
+	}
+
+	if (!state)
+		return 0;
+

drivers/gpu/drm/i915/intel_display.c +6 -0

+}
+
 void intel_prepare_reset(struct drm_device *dev)
 {
+	struct drm_atomic_state *state;
+	struct drm_modeset_acquire_ctx *pctx;
+	int ret;
+
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev))
 		return;

drivers/gpu/drm/i915/intel_display.c +30 -6

+
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	intel_display_suspend(dev);
+
+	state = drm_atomic_helper_duplicate_state(dev, pctx);
+	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, pctx);
+	if (ret) {
+		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+		goto err;
+	}
+
+	to_i915(dev)->modeset_restore_state = state;
+	return;
+
+err:
+	drm_atomic_state_free(state);
 }
 
 void intel_finish_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	int ret;
 
 	/*
 	 * Flips in the rings will be nuked by the reset,

@@ -3172,23 +3231,29 @@ void intel_finish_reset(struct drm_device *dev)
 		 */
 		intel_update_primary_planes(dev);
 		return;
-	}
+	} 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);
 
-	/*
-	 * The display has been reset as well,
-	 * so need a full re-initialization.
-	 */

drivers/gpu/drm/i915/intel_display.c +10 -31

-	spin_lock_irq(&dev_priv->irq_lock);
-	if (dev_priv->display.hpd_irq_setup)
-		dev_priv->display.hpd_irq_setup(dev);
-	spin_unlock_irq(&dev_priv->irq_lock);
+	dev_priv->modeset_restore_state = NULL;
+	if (state)
+		state->acquire_ctx = dev->mode_config.acquire_ctx;
 
-	intel_display_resume(dev);
+	ret = __intel_display_resume(dev, state, NULL);
+	if (ret)
+		DRM_ERROR("Restoring old state failed with %i\n", ret);
 
 	intel_hpd_init(dev_priv);
 

@@ -15885,6 +15950,8 @@ void intel_display_resume(struct drm_device *dev)
 	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

@@ -15897,32 +15964,8 @@ void intel_display_resume(struct drm_device *dev)
 
 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;
-
-		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);
-	}
+	if (!ret)
+		ret = __intel_display_resume(dev, state, &setup);
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);

Signature · 1 line

2.1.0

Add force_reset_modeset_test as a parameter to force the modeset path during gpu reset. This allows a IGT test to set the knob and trigger a hang to force the gpu reset.

Signed-off-by Maarten Lankhorst <[email protected]>

Fixes e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")

Cc [email protected]

---
 drivers/gpu/drm/i915/i915_params.c   |   6 ++
 drivers/gpu/drm/i915/i915_params.h   |   1 +
 drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++++++++-----------
 3 files changed, 116 insertions(+), 52 deletions(-)

drivers/gpu/drm/i915/i915_params.c +6 -0

index 1779f02e6df8..80ce581793dc 100644
@@ -45,6 +45,7 @@ struct i915_params i915 __read_mostly = {
 	.fastboot = 0,
 	.prefault_disable = 0,
 	.load_detect_test = 0,
+	.force_reset_modeset_test = 0,
 	.reset = true,
 	.invert_brightness = 0,
 	.disable_display = 0,

@@ -158,6 +159,11 @@ MODULE_PARM_DESC(load_detect_test,
 	"Force-enable the VGA load detect code for testing (default:false). "
 	"For developers only.");
 
+module_param_named_unsafe(force_reset_modeset_test, i915.force_reset_modeset_test, bool, 0600);
+MODULE_PARM_DESC(force_reset_modeset_test,
+	"Force a modeset during gpu reset for testing (default:false). "
+	"For developers only.");
+
 module_param_named_unsafe(invert_brightness, i915.invert_brightness, int, 0600);
 MODULE_PARM_DESC(invert_brightness,
 	"Invert backlight brightness "

drivers/gpu/drm/i915/i915_params.h +1 -0

index 02bc27804291..3934c4300427 100644
@@ -55,6 +55,7 @@ struct i915_params {
 	bool fastboot;
 	bool prefault_disable;
 	bool load_detect_test;
+	bool force_reset_modeset_test;
 	bool reset;
 	bool disable_display;
 	bool enable_guc_submission;

drivers/gpu/drm/i915/intel_display.c +37 -0

index af74cdba7081..acca08797123 100644
@@ -3119,27 +3119,94 @@ static void intel_update_primary_planes(struct drm_device *dev)
 	}
 }
 
+static int
+__intel_display_resume(struct drm_device *dev,
+		       struct drm_atomic_state *state,
+		       bool *setup)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	if (!setup || !*setup) {
+		if (setup)
+			*setup = true;
+
+		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;
+	}
+
+	return drm_atomic_commit(state);
+}
+
 void intel_prepare_reset(struct drm_device *dev)
 {
+	bool requires_display_reset = true;
+	struct drm_atomic_state *state;
+	struct drm_modeset_acquire_ctx *pctx;
+	int ret;
+
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev))

drivers/gpu/drm/i915/intel_display.c +65 -50

+
 	/*
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	intel_display_suspend(dev);
+
+	state = drm_atomic_helper_duplicate_state(dev, pctx);
+	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, pctx);
+	if (ret) {
+		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+		goto err;
+	}
+
+	to_i915(dev)->modeset_restore_state = state;
+	return;
+
+err:
+	drm_atomic_state_free(state);
+	if (!requires_display_reset)
+		drm_modeset_unlock_all(dev);
 }
 
 void intel_finish_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	bool hpd_init;
+	int ret;
 
 	/*
 	 * Flips in the rings will be nuked by the reset,

@@ -3154,36 +3221,48 @@ void intel_finish_reset(struct drm_device *dev)
 
 	/* reset doesn't touch the display */
 	if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) {
+		hpd_init = false;
+
+		if (!state) {
+			/*
+			 * Flips in the rings have been nuked by the reset,
+			 * so update the base address of all primary
+			 * planes to the the last fb to make sure we're
+			 * showing the correct fb after a reset.
+			 *
+			 * 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);
+			return;
+		}
+	} else {
 		/*
-		 * Flips in the rings have been nuked by the reset,
-		 * so update the base address of all primary
-		 * planes to the the last fb to make sure we're
-		 * showing the correct fb after a reset.
-		 *
-		 * FIXME: Atomic will make this obsolete since we won't schedule
-		 * CS-based flips (which might get lost in gpu resets) any more.
+		 * The display has been reset as well,
+		 * so need a full re-initialization.
 		 */
-		intel_update_primary_planes(dev);
-		return;
-	}
+		hpd_init = true;
+		intel_runtime_pm_disable_interrupts(dev_priv);
+		intel_runtime_pm_enable_interrupts(dev_priv);
 
-	/*
-	 * 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);
 
-	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);
+		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);
-	spin_unlock_irq(&dev_priv->irq_lock);
+	dev_priv->modeset_restore_state = NULL;
+	if (state)
+		state->acquire_ctx = dev->mode_config.acquire_ctx;
 
-	intel_display_resume(dev);
+	ret = __intel_display_resume(dev, state, NULL);
+	if (ret)
+		DRM_ERROR("Restoring old state failed with %i\n", ret);
 
-	intel_hpd_init(dev_priv);
+	if (hpd_init)
+		intel_hpd_init(dev_priv);
 
 	drm_modeset_unlock_all(dev);
 }

@@ -15876,6 +15955,8 @@ void intel_display_resume(struct drm_device *dev)
 	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

@@ -15888,32 +15969,8 @@ void intel_display_resume(struct drm_device *dev)
 
 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;
-
-		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);
-	}
+	if (!ret)
+		ret = __intel_display_resume(dev, state, &setup);
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx