Re: [PATCH] drm/i915: fix hard-hangs while resetting the gpu on some ilks

Re: [PATCH] drm/i915: fix hard-hangs while resetting the gpu on some ilks

[Chris Wilson]

On Tue, 19 Jun 2012 16:15:30 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Now I still think that having unified hw frobbing paths between driver
> load, resume and gpu reset is highly desirable, so I've opted to just
> add a reset argument to disable the offending code for gpu resets. I
> have no idea why it actually blows up.

Did you look at whether it was an IRQ firing during the reset may have
been an issue? One of the patches I have is to disable interrupts across
the hw reset during i915_reset().

Otherwise, bisect the register writes int the offending
ironlake_enable_drps().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: fix hard-hangs while resetting the gpu on some ilks

[Daniel Vetter]

I admit up-front that this is ugly. This patch fixes fully reliable
hard-hang on my ilk when exercising the reset paths with the hangman
i-g-t tests. Both Chris Wilson and our QA couldn't reproduce this.
This regression has been introduced in

commit 1833b134454d5300d8a9d07b78876a20395f01a9
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed May 9 11:56:28 2012 +0100

    drm/i915: gen6_enable_rps() wants to be called after ring initialisation

specifically calling intel_modeset_init_hw instead of open-coding half
of it. After some trial and error (and figuring out the hard way that
the problem only manifests after a full reboot) I've tracked it down
to calling ironlake_enable_drps.

Now I still think that having unified hw frobbing paths between driver
load, resume and gpu reset is highly desirable, so I've opted to just
add a reset argument to disable the offending code for gpu resets. I
have no idea why it actually blows up.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c      |    4 ++--
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |    9 ++++++---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 238a521..d05fe30 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -640,7 +640,7 @@ static int i915_drm_thaw(struct drm_device *dev)
 		error = i915_gem_init_hw(dev);
 		mutex_unlock(&dev->struct_mutex);
 
-		intel_modeset_init_hw(dev);
+		intel_modeset_init_hw(dev, false);
 		drm_mode_config_reset(dev);
 		drm_irq_install(dev);
 
@@ -908,7 +908,7 @@ int i915_reset(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 
 		if (drm_core_check_feature(dev, DRIVER_MODESET))
-			intel_modeset_init_hw(dev);
+			intel_modeset_init_hw(dev, true);
 
 		drm_irq_uninstall(dev);
 		drm_irq_install(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ae4129b..04ad9c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1471,7 +1471,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
 #endif /* CONFIG_ACPI */
 
 /* modesetting */
-extern void intel_modeset_init_hw(struct drm_device *dev);
+extern void intel_modeset_init_hw(struct drm_device *dev, bool reset);
 extern void intel_modeset_init(struct drm_device *dev);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 308e1a2..e76d12c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6900,14 +6900,17 @@ static void ivb_pch_pwm_override(struct drm_device *dev)
 	I915_WRITE(BLC_PWM_PCH_CTL1, BLM_PCH_PWM_ENABLE | BLM_PCH_OVERRIDE_ENABLE);
 }
 
-void intel_modeset_init_hw(struct drm_device *dev)
+void intel_modeset_init_hw(struct drm_device *dev, bool reset)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	intel_init_clock_gating(dev);
 
 	if (IS_IRONLAKE_M(dev)) {
-		ironlake_enable_drps(dev);
+		/* Re-enabling drps on gpu reset hard-hangs some ilks for no
+		 * apparent reason. Hence avoid it. */
+		if (!reset)
+			ironlake_enable_drps(dev);
 		ironlake_enable_rc6(dev);
 		intel_init_emon(dev);
 	}
@@ -6979,7 +6982,7 @@ void intel_modeset_init(struct drm_device *dev)
 
 void intel_modeset_gem_init(struct drm_device *dev)
 {
-	intel_modeset_init_hw(dev);
+	intel_modeset_init_hw(dev, false);
 
 	intel_setup_overlay(dev);
 }
-- 
1.7.7.6

[PATCH] drm/i915: don't call modeset_init_hw in i915_reset

[Daniel Vetter]

It seems to blow up my ilk in all kinds of strange ways. And now that
we're no longer resetting the entire modeset state, it shouldn't be
necessary any longer.

This essentially reverts

commit f817586cebf1b946d1f327f9a596048efd6b64e9
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Apr 10 15:50:11 2012 +0200

    drm/i915: re-init modeset hw state after gpu reset

safe for the introduction of modeset_init_hw, that one is nice to
prevent code duplication between driver load and resume.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e41aade..ae2f6a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -906,9 +906,6 @@ int i915_reset(struct drm_device *dev)
 
 		mutex_unlock(&dev->struct_mutex);
 
-		if (drm_core_check_feature(dev, DRIVER_MODESET))
-			intel_modeset_init_hw(dev);
-
 		drm_irq_uninstall(dev);
 		drm_irq_install(dev);
 	} else {
-- 
1.7.7.6

Re: [PATCH] drm/i915: don't call modeset_init_hw in i915_reset

[Daniel Vetter]

On Tue, Jun 19, 2012 at 06:40:00PM +0200, Daniel Vetter wrote:
> It seems to blow up my ilk in all kinds of strange ways. And now that
> we're no longer resetting the entire modeset state, it shouldn't be
> necessary any longer.
> 
> This essentially reverts
> 
> commit f817586cebf1b946d1f327f9a596048efd6b64e9
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Apr 10 15:50:11 2012 +0200
> 
>     drm/i915: re-init modeset hw state after gpu reset
> 
> safe for the introduction of modeset_init_hw, that one is nice to
> prevent code duplication between driver load and resume.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Queued for -next with Chris' irc-comments incorporated.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48