[2/3] drm/i915: run the full gem hw init after a gpu reset

Commit Message

Daniel Vetter July 3, 2013, 8:07 a.m. UTC

Otherwise we can't resurrect the bsd ring on gm45 completely. I've
discovered this trick because module reload recovered the bsd ring,
making it clear that it's not an issue with the hw or our reset code.

While touching the code update the comment to be a bit clearer about
what we actually want to reinitialize and which pieces we better leave
untouched.

Note: In my opinion this is not a stable candidate - historically such
seemingly correct looking changes in the reset code have lead to big
heaps of scary hilarity.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65495
Tested-by: lu hua <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

Comments

Chris Wilson July 3, 2013, 8:43 a.m. UTC | #1

On Wed, Jul 03, 2013 at 10:07:57AM +0200, Daniel Vetter wrote:
> Otherwise we can't resurrect the bsd ring on gm45 completely. I've
> discovered this trick because module reload recovered the bsd ring,
> making it clear that it's not an issue with the hw or our reset code.
> 
> While touching the code update the comment to be a bit clearer about
> what we actually want to reinitialize and which pieces we better leave
> untouched.
> 
> Note: In my opinion this is not a stable candidate - historically such
> seemingly correct looking changes in the reset code have lead to big
> heaps of scary hilarity.

At the expense of leaking all allocated state from earlier. The magic
ingredient here I guess is the resetting of the HWS.
-Chris

Daniel Vetter July 3, 2013, 9:22 a.m. UTC | #2

On Wed, Jul 03, 2013 at 09:43:32AM +0100, Chris Wilson wrote:
> On Wed, Jul 03, 2013 at 10:07:57AM +0200, Daniel Vetter wrote:
> > Otherwise we can't resurrect the bsd ring on gm45 completely. I've
> > discovered this trick because module reload recovered the bsd ring,
> > making it clear that it's not an issue with the hw or our reset code.
> > 
> > While touching the code update the comment to be a bit clearer about
> > what we actually want to reinitialize and which pieces we better leave
> > untouched.
> > 
> > Note: In my opinion this is not a stable candidate - historically such
> > seemingly correct looking changes in the reset code have lead to big
> > heaps of scary hilarity.
> 
> At the expense of leaking all allocated state from earlier. The magic
> ingredient here I guess is the resetting of the HWS.

Meh, I've tried to forget how broken our init sequence is. I still think
sharing the code is probably the right thing instead of adding a bunch of
magic calls. I'll have a look how we could fix this best.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ed9262c..cdb6790 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -962,28 +962,15 @@  int i915_reset(struct drm_device *dev)
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
 			!dev_priv->mm.suspended) {
-		struct intel_ring_buffer *ring;
-		int i;
-
-		dev_priv->mm.suspended = 0;
-
-		i915_gem_init_swizzling(dev);
-
-		for_each_ring(ring, dev_priv, i)
-			ring->init(ring);
-
-		i915_gem_context_init(dev);
-		if (dev_priv->mm.aliasing_ppgtt) {
-			ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
-			if (ret)
-				i915_gem_cleanup_aliasing_ppgtt(dev);
-		}
+		int ret;
 
 		/*
-		 * It would make sense to re-init all the other hw state, at
-		 * least the rps/rc6/emon init done within modeset_init_hw. For
-		 * some unknown reason, this blows up my ilk, so don't.
+		 * We have to be carefuly to not reinit hw state which is not
+		 * reset. E.g. reinitializing rps/rc6 on ilk here results in a
+		 * dead machine.
 		 */
+		ret = i915_gem_init_hw(dev);
+		WARN(ret != 0, "gem hw init failed after reset: %i\n", ret);
 
 		mutex_unlock(&dev->struct_mutex);