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

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

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Cc: drm-intel-fixes@lists.freedesktop.org
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1779f02e6df8..80ce581793dc 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -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 "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 02bc27804291..3934c4300427 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af74cdba7081..acca08797123 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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))
 		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;
+
 	/*
 	 * 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);
-- 
2.1.0

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

[PATCH i-g-t] drv_hangman: Add test for modeset during gpu reset.

[Maarten Lankhorst]

This adds a test that forces a modeset during gpu reset, which has broken
a few times in the past during the atomic rework.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
---
diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
index f4cdadb80e02..8a17d7acff1b 100644
--- a/tests/drv_hangman.c
+++ b/tests/drv_hangman.c
@@ -324,6 +324,22 @@ static void hangcheck_unterminated(void)
 	close(fd);
 }
 
+static void reset_with_forced_modeset(void)
+{
+	igt_hang_ring_t hang;
+	int fd = drm_open_driver(DRIVER_INTEL);
+
+	igt_require_hang_ring(fd, I915_EXEC_DEFAULT);
+
+	igt_set_module_param_int("force_reset_modeset_test", 1);
+
+	hang = igt_hang_ring(fd, I915_EXEC_DEFAULT);
+	igt_post_hang_ring(fd, hang);
+
+	igt_set_module_param_int("force_reset_modeset_test", 0);
+	close(fd);
+}
+
 igt_main
 {
 	const struct intel_execution_engine *e;
@@ -353,4 +369,7 @@ igt_main
 
 	igt_subtest("hangcheck-unterminated")
 		hangcheck_unterminated();
+
+	igt_subtest("reset-with-forced-modeset")
+		reset_with_forced_modeset();
 }

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

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

[Ville Syrjälä]

On Tue, Apr 05, 2016 at 12:56:37PM +0200, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Cc: drm-intel-fixes@lists.freedesktop.org

Seems like this should be two patches; One to fix the locking and
another to add the debug knob.

> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 1779f02e6df8..80ce581793dc 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -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 "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 02bc27804291..3934c4300427 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af74cdba7081..acca08797123 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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))
>  		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;

Can't we just run the full code on g4x+ anyway? Would leave the code
much simpler.

> +	}
>  
>  	drm_modeset_lock_all(dev);
> +	pctx = dev->mode_config.acquire_ctx;

pctx makes me think of power context.

> +
>  	/*
>  	 * 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);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Add a way to test the modeset done during gpu reset.

[Patchwork]

== Series Details ==

Series: drm/i915: Add a way to test the modeset done during gpu reset.
URL   : https://patchwork.freedesktop.org/series/5314/
State : failure

== Summary ==

Series 5314v1 drm/i915: Add a way to test the modeset done during gpu reset.
http://patchwork.freedesktop.org/api/1.0/series/5314/revisions/1/mbox/

Test drv_getparams_basic:
        Subgroup basic-subslice-total:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_ctx_basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_exec_basic:
        Subgroup gtt-bsd:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup readonly-default:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup readonly-vebox:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_exec_nop:
        Subgroup basic:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_exec_store:
        Subgroup basic-default:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_exec_whisper:
        Subgroup basic:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test gem_mmap_gtt:
        Subgroup basic-read-write-distinct:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-write-gtt:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_ringfill:
        Subgroup basic-default:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_storedw_loop:
        Subgroup basic-blt:
                pass       -> DMESG-FAIL (bsw-nuc-2)
        Subgroup basic-render:
                pass       -> SKIP       (bsw-nuc-2)
Test kms_addfb_basic:
        Subgroup addfb25-framebuffer-vs-set-tiling:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup addfb25-x-tiled-mismatch:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup addfb25-y-tiled:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup bad-pitch-1024:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup bad-pitch-63:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-x-tiled:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-y-tiled:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup bo-too-small-due-to-tiling:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup clobberred-modifier:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup no-handle:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup unused-modifier:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (snb-x220t)
                pass       -> FAIL       (bsw-nuc-2)
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> SKIP       (ilk-hp8440p)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-FAIL (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> DMESG-FAIL (bsw-nuc-2)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> DMESG-FAIL (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test prime_self_import:
        Subgroup basic-llseek-bad:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-llseek-size:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-with_fd_dup:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:153  pass:88   dwarn:20  dfail:4   fail:1   skip:39 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:130  dwarn:1   dfail:0   fail:0   skip:65 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 
BOOT FAILED for bdw-nuci7

Results at /archive/results/CI_IGT_test/Patchwork_1800/

48d2a0497cd1d0630fbb59fb7871e4d678458307 drm-intel-nightly: 2016y-04m-05d-12h-30m-42s UTC integration manifest
e4dd19c811e4ce73edf2919e7244f5484ac76265 drm/i915: Add a way to test the modeset done during gpu reset.

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

[PATCH v2 1/2] drm/i915: Fix modeset handling during gpu reset.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Cc: drm-intel-fixes@lists.freedesktop.org
---
 drivers/gpu/drm/i915/intel_display.c | 123 +++++++++++++++++++++++------------
 1 file changed, 83 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index feb7028341b8..2f76efc86417 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3126,8 +3126,44 @@ 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)
 {
+	struct drm_atomic_state *state;
+	struct drm_modeset_acquire_ctx *pctx;
+	int ret;
+
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev))
 		return;
@@ -3137,16 +3173,39 @@ void intel_prepare_reset(struct drm_device *dev)
 		return;
 
 	drm_modeset_lock_all(dev);
+	pctx = dev->mode_config.acquire_ctx;
+
 	/*
 	 * 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.
-	 */
-	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);
 
@@ -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);
-- 
2.1.0

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

[PATCH v2 2/2] drm/i915: Add a way to test the modeset done during gpu reset, v2.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
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(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 1779f02e6df8..80ce581793dc 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -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 "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 02bc27804291..3934c4300427 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f76efc86417..faf2da08778f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
@@ -3199,12 +3204,15 @@ void intel_prepare_reset(struct drm_device *dev)
 
 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;
 
 	/*
@@ -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) {
+			/*
+			 * 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 {
 		/*
 		 * 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);
 }
-- 
2.1.0

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

Re: [PATCH v2 1/2] drm/i915: Fix modeset handling during gpu reset.

[Maarten Lankhorst]

Op 11-04-16 om 08:23 schreef Maarten Lankhorst:
> 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 <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Cc: drm-intel-fixes@lists.freedesktop.org
>
Ping for review?

Also for "[PATCH i-g-t] drv_hangman: Add test for modeset during gpu reset." from the original series.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915: Fix modeset handling during gpu reset.

[Ville Syrjälä]

On Mon, Apr 11, 2016 at 08:23:33AM +0200, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> Cc: drm-intel-fixes@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/intel_display.c | 123 +++++++++++++++++++++++------------
>  1 file changed, 83 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index feb7028341b8..2f76efc86417 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3126,8 +3126,44 @@ 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;

This is getting nasty. How about dealing with -EDEADLK right after the
modeset_lock_all() in the caller, so that we don't have to be concerned
with getting called mutiple times here?

> +
> +		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);

To go along with my suggestion about dealing with -EDEADLK upfront,
we could then have a WARN_ON(ret == -EDEADLK) here to make sure we
didn't make a mess of things.

> +}
> +
>  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;
> @@ -3137,16 +3173,39 @@ void intel_prepare_reset(struct drm_device *dev)
>  		return;
>  
>  	drm_modeset_lock_all(dev);
> +	pctx = dev->mode_config.acquire_ctx;

Still looks like "power context" to me.

I was wondering if we should have a private context here, but I guess
that would fall over with some mode_config.acquire_ctx usage in the
display init code?

> +
>  	/*
>  	 * 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.
> -	 */
> -	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);
> +	}

This seems like an unrelated change. More to do with the second patch I
guess, though I'm still wondering if we can't just run through the
whole thing every time?

>  
> -	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);
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Add a way to test the modeset done during gpu reset, v2.

[Ville Syrjälä]

On Mon, Apr 11, 2016 at 08:23:34AM +0200, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> 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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 1779f02e6df8..80ce581793dc 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -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 "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 02bc27804291..3934c4300427 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2f76efc86417..faf2da08778f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
> @@ -3199,12 +3204,15 @@ void intel_prepare_reset(struct drm_device *dev)
>  
>  err:
>  	drm_atomic_state_free(state);
> +	if (!requires_display_reset)
> +		drm_modeset_unlock_all(dev);

Why do we want to unlock here?

>  }
>  
>  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) {
> +			/*
> +			 * 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);

For this I guess. Hmm. Maybe hang on to the lock anyway, and add a
new flag in dev_priv or something to indicate if we're performing
a display reset or not. I suppose we could check for
(pre-g4x || i915.whatever) on either side, but that could
be problematic if someone changes the modparam mid-flight.

So finish_reset could perhaps looks something like (assuming we don't
want to run through the whole display init sequence on g4x+):

{
	if (!dev_priv->display_reset) {
		intel_update_primary_planes();
		return;
	}

	if (pre-g4x) {
		...

		ret = display_resume();

		init_hpd();
	} else {
		ret = display_resume();
	}

	unlock_all();
}


> +			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);
>  }
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx