[PATCH 0/4] drm/i915: gen3/4 GPU reset stuff

[PATCH 0/4] drm/i915: gen3/4 GPU reset stuff

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I went ahead and quckly fixed the GPU reset for my 946gz. I had to add
a bit of code to restore the display side since that too gets clobbered.

I then noticed that according to the specs gen3 should work with the exact
same code, so I added the missing bits. g33 is slightly special, but should
be close enough. The spec says we should turn off all planes before the
reset on g33, but so far I didn't implement that part.

Tested with simulated hangs on my 946gz. Would be mildly intersting to
see how it fares on other machines, but alas I have none.

Ville Syrjälä (4):
  drm/i915: Fix gen4 GPU reset
  drm/i915: Restore the display config after a GPU reset on gen4
  drm/i915: Implement GPU reset for 915/945
  drm/i915: Implement GPU reset for g33

 drivers/gpu/drm/i915/i915_drv.c     |  8 +++++
 drivers/gpu/drm/i915/i915_reg.h     |  3 +-
 drivers/gpu/drm/i915/intel_uncore.c | 60 ++++++++++++++++++-------------------
 3 files changed, 40 insertions(+), 31 deletions(-)

-- 
1.8.5.5

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

[PATCH 1/4] drm/i915: Fix gen4 GPU reset

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On pre-ctg the reset bit directly controls the reset signal. We must
assert it for >=20usec and then deassert it. Bit 1 is a RO status bit
which should also go down when the reset is no longer asserted.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c | 38 ++++++++++++++-----------------------
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 543f23c..1346c1e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -83,6 +83,7 @@
 #define  GRDOM_RENDER	(1<<2)
 #define  GRDOM_MEDIA	(3<<2)
 #define  GRDOM_MASK	(3<<2)
+#define  GRDOM_RESET_STATUS (1<<1)
 #define  GRDOM_RESET_ENABLE (1<<0)
 
 #define ILK_GDSR 0x2ca4 /* MCHBAR offset */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 67385a9..f073fa0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -960,34 +960,24 @@ static int i965_reset_complete(struct drm_device *dev)
 {
 	u8 gdrst;
 	pci_read_config_byte(dev->pdev, I965_GDRST, &gdrst);
-	return (gdrst & GRDOM_RESET_ENABLE) == 0;
+	return (gdrst & GRDOM_RESET_STATUS) == 0;
 }
 
 static int i965_do_reset(struct drm_device *dev)
 {
-	int ret;
-
-	/*
-	 * Set the domains we want to reset (GRDOM/bits 2 and 3) as
-	 * well as the reset bit (GR/bit 0).  Setting the GR bit
-	 * triggers the reset; when done, the hardware will clear it.
-	 */
-	pci_write_config_byte(dev->pdev, I965_GDRST,
-			      GRDOM_RENDER | GRDOM_RESET_ENABLE);
-	ret =  wait_for(i965_reset_complete(dev), 500);
-	if (ret)
-		return ret;
-
-	pci_write_config_byte(dev->pdev, I965_GDRST,
-			      GRDOM_MEDIA | GRDOM_RESET_ENABLE);
-
-	ret =  wait_for(i965_reset_complete(dev), 500);
-	if (ret)
-		return ret;
-
+	/* assert reset for at least 20 usec */
+	pci_write_config_byte(dev->pdev, I965_GDRST, GRDOM_RESET_ENABLE);
+	udelay(20);
 	pci_write_config_byte(dev->pdev, I965_GDRST, 0);
 
-	return 0;
+	return wait_for(i965_reset_complete(dev), 500);
+}
+
+static int g4x_reset_complete(struct drm_device *dev)
+{
+	u8 gdrst;
+	pci_read_config_byte(dev->pdev, I965_GDRST, &gdrst);
+	return (gdrst & GRDOM_RESET_ENABLE) == 0;
 }
 
 static int g4x_do_reset(struct drm_device *dev)
@@ -997,7 +987,7 @@ static int g4x_do_reset(struct drm_device *dev)
 
 	pci_write_config_byte(dev->pdev, I965_GDRST,
 			      GRDOM_RENDER | GRDOM_RESET_ENABLE);
-	ret =  wait_for(i965_reset_complete(dev), 500);
+	ret =  wait_for(g4x_reset_complete(dev), 500);
 	if (ret)
 		return ret;
 
@@ -1007,7 +997,7 @@ static int g4x_do_reset(struct drm_device *dev)
 
 	pci_write_config_byte(dev->pdev, I965_GDRST,
 			      GRDOM_MEDIA | GRDOM_RESET_ENABLE);
-	ret =  wait_for(i965_reset_complete(dev), 500);
+	ret =  wait_for(g4x_reset_complete(dev), 500);
 	if (ret)
 		return ret;
 
-- 
1.8.5.5

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

[PATCH 2/4] drm/i915: Restore the display config after a GPU reset on gen4

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On pre-ctg GPU reset also resets the display hardware. Force a mode
restore after the GPU reset, and also re-init clock gating.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b948c71..2ec3796 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -794,6 +794,14 @@ int i915_reset(struct drm_device *dev)
 		if (INTEL_INFO(dev)->gen > 5)
 			intel_reset_gt_powersave(dev);
 
+		if (IS_GEN4(dev) && !IS_G4X(dev)) {
+			intel_init_clock_gating(dev);
+
+			drm_modeset_lock_all(dev);
+			intel_modeset_setup_hw_state(dev, true);
+			drm_modeset_unlock_all(dev);
+		}
+
 		intel_hpd_init(dev);
 	} else {
 		mutex_unlock(&dev->struct_mutex);
-- 
1.8.5.5

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

[PATCH 3/4] drm/i915: Implement GPU reset for 915/945

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

915/945 have the same reset registers as 965, so share the code.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  3 ++-
 drivers/gpu/drm/i915/i915_reg.h     |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c | 26 ++++++++++++++------------
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2ec3796..187f42c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -794,7 +794,8 @@ int i915_reset(struct drm_device *dev)
 		if (INTEL_INFO(dev)->gen > 5)
 			intel_reset_gt_powersave(dev);
 
-		if (IS_GEN4(dev) && !IS_G4X(dev)) {
+		if ((IS_GEN3(dev) && !IS_G33(dev)) ||
+		    (IS_GEN4(dev) && !IS_G4X(dev))) {
 			intel_init_clock_gating(dev);
 
 			drm_modeset_lock_all(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1346c1e..018fd9d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -78,7 +78,7 @@
 
 
 /* Graphics reset regs */
-#define I965_GDRST 0xc0 /* PCI config register */
+#define I915_GDRST 0xc0 /* PCI config register */
 #define  GRDOM_FULL	(0<<2)
 #define  GRDOM_RENDER	(1<<2)
 #define  GRDOM_MEDIA	(3<<2)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f073fa0..a043baf 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -956,27 +956,27 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
 	return 0;
 }
 
-static int i965_reset_complete(struct drm_device *dev)
+static int i915_reset_complete(struct drm_device *dev)
 {
 	u8 gdrst;
-	pci_read_config_byte(dev->pdev, I965_GDRST, &gdrst);
+	pci_read_config_byte(dev->pdev, I915_GDRST, &gdrst);
 	return (gdrst & GRDOM_RESET_STATUS) == 0;
 }
 
-static int i965_do_reset(struct drm_device *dev)
+static int i915_do_reset(struct drm_device *dev)
 {
 	/* assert reset for at least 20 usec */
-	pci_write_config_byte(dev->pdev, I965_GDRST, GRDOM_RESET_ENABLE);
+	pci_write_config_byte(dev->pdev, I915_GDRST, GRDOM_RESET_ENABLE);
 	udelay(20);
-	pci_write_config_byte(dev->pdev, I965_GDRST, 0);
+	pci_write_config_byte(dev->pdev, I915_GDRST, 0);
 
-	return wait_for(i965_reset_complete(dev), 500);
+	return wait_for(i915_reset_complete(dev), 500);
 }
 
 static int g4x_reset_complete(struct drm_device *dev)
 {
 	u8 gdrst;
-	pci_read_config_byte(dev->pdev, I965_GDRST, &gdrst);
+	pci_read_config_byte(dev->pdev, I915_GDRST, &gdrst);
 	return (gdrst & GRDOM_RESET_ENABLE) == 0;
 }
 
@@ -985,7 +985,7 @@ static int g4x_do_reset(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	pci_write_config_byte(dev->pdev, I965_GDRST,
+	pci_write_config_byte(dev->pdev, I915_GDRST,
 			      GRDOM_RENDER | GRDOM_RESET_ENABLE);
 	ret =  wait_for(g4x_reset_complete(dev), 500);
 	if (ret)
@@ -995,7 +995,7 @@ static int g4x_do_reset(struct drm_device *dev)
 	I915_WRITE(VDECCLK_GATE_D, I915_READ(VDECCLK_GATE_D) | VCP_UNIT_CLOCK_GATE_DISABLE);
 	POSTING_READ(VDECCLK_GATE_D);
 
-	pci_write_config_byte(dev->pdev, I965_GDRST,
+	pci_write_config_byte(dev->pdev, I915_GDRST,
 			      GRDOM_MEDIA | GRDOM_RESET_ENABLE);
 	ret =  wait_for(g4x_reset_complete(dev), 500);
 	if (ret)
@@ -1005,7 +1005,7 @@ static int g4x_do_reset(struct drm_device *dev)
 	I915_WRITE(VDECCLK_GATE_D, I915_READ(VDECCLK_GATE_D) & ~VCP_UNIT_CLOCK_GATE_DISABLE);
 	POSTING_READ(VDECCLK_GATE_D);
 
-	pci_write_config_byte(dev->pdev, I965_GDRST, 0);
+	pci_write_config_byte(dev->pdev, I915_GDRST, 0);
 
 	return 0;
 }
@@ -1065,8 +1065,10 @@ int intel_gpu_reset(struct drm_device *dev)
 	case 4:
 		if (IS_G4X(dev))
 			return g4x_do_reset(dev);
-		else
-			return i965_do_reset(dev);
+	case 3:
+		if (IS_G33(dev))
+			return -ENODEV;
+		return i915_do_reset(dev);
 	default: return -ENODEV;
 	}
 }
-- 
1.8.5.5

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

[PATCH 4/4] drm/i915: Implement GPU reset for g33

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

g33 seems to sit somewhere between the 915/945/965 style and the
g4x style. The bits look like g4x, but we still need to do a full
reset including display.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  3 +--
 drivers/gpu/drm/i915/intel_uncore.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 187f42c..b44cdb3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -794,8 +794,7 @@ int i915_reset(struct drm_device *dev)
 		if (INTEL_INFO(dev)->gen > 5)
 			intel_reset_gt_powersave(dev);
 
-		if ((IS_GEN3(dev) && !IS_G33(dev)) ||
-		    (IS_GEN4(dev) && !IS_G4X(dev))) {
+		if (IS_GEN3(dev) || (IS_GEN4(dev) && !IS_G4X(dev))) {
 			intel_init_clock_gating(dev);
 
 			drm_modeset_lock_all(dev);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index a043baf..de2d708 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -980,6 +980,14 @@ static int g4x_reset_complete(struct drm_device *dev)
 	return (gdrst & GRDOM_RESET_ENABLE) == 0;
 }
 
+static int g33_do_reset(struct drm_device *dev)
+{
+	/* FIXME spec says to turn off all planes and wait 1 usec before reset */
+
+	pci_write_config_byte(dev->pdev, I915_GDRST, GRDOM_RESET_ENABLE);
+	return wait_for(g4x_reset_complete(dev), 500);
+}
+
 static int g4x_do_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1067,7 +1075,7 @@ int intel_gpu_reset(struct drm_device *dev)
 			return g4x_do_reset(dev);
 	case 3:
 		if (IS_G33(dev))
-			return -ENODEV;
+			return g33_do_reset(dev);
 		return i915_do_reset(dev);
 	default: return -ENODEV;
 	}
-- 
1.8.5.5

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

Re: [PATCH 2/4] drm/i915: Restore the display config after a GPU reset on gen4

[Daniel Vetter]

On Mon, May 19, 2014 at 10:25:19PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On pre-ctg GPU reset also resets the display hardware. Force a mode
> restore after the GPU reset, and also re-init clock gating.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b948c71..2ec3796 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -794,6 +794,14 @@ int i915_reset(struct drm_device *dev)
>  		if (INTEL_INFO(dev)->gen > 5)
>  			intel_reset_gt_powersave(dev);
>  
> +		if (IS_GEN4(dev) && !IS_G4X(dev)) {
> +			intel_init_clock_gating(dev);
> +
> +			drm_modeset_lock_all(dev);
> +			intel_modeset_setup_hw_state(dev, true);
> +			drm_modeset_unlock_all(dev);

Locking inversion here. I think we need to push this down to the very end.
Also this leaves the interesting question of what happens with vblank
waits and friends ...
-Daniel

> +		}
> +
>  		intel_hpd_init(dev);
>  	} else {
>  		mutex_unlock(&dev->struct_mutex);
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: Restore the display config after a GPU reset on gen4

[Ville Syrjälä]

On Tue, May 20, 2014 at 10:03:44AM +0200, Daniel Vetter wrote:
> On Mon, May 19, 2014 at 10:25:19PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On pre-ctg GPU reset also resets the display hardware. Force a mode
> > restore after the GPU reset, and also re-init clock gating.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b948c71..2ec3796 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -794,6 +794,14 @@ int i915_reset(struct drm_device *dev)
> >  		if (INTEL_INFO(dev)->gen > 5)
> >  			intel_reset_gt_powersave(dev);
> >  
> > +		if (IS_GEN4(dev) && !IS_G4X(dev)) {
> > +			intel_init_clock_gating(dev);
> > +
> > +			drm_modeset_lock_all(dev);
> > +			intel_modeset_setup_hw_state(dev, true);
> > +			drm_modeset_unlock_all(dev);
> 
> Locking inversion here. I think we need to push this down to the very end.

We already dropped struct_mutex a bit earlier. Or was there some other
lock you were worried about?

> Also this leaves the interesting question of what happens with vblank
> waits and friends ...

Hmm. I guess we might have some kind of refcount problem on our hands w/
vblanks and whatnot. Might be I'd need to for_each_crtc() .crtc_disable()
before the reset. That would match the g33 spec's recommendation to
disable all planes, but it also does quite a bit more, maybe too much?
But I could try it at least.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/4] drm/i915: Restore the display config after a GPU reset on gen4

[Daniel Vetter]

On Tue, May 20, 2014 at 11:46:42AM +0300, Ville Syrjälä wrote:
> On Tue, May 20, 2014 at 10:03:44AM +0200, Daniel Vetter wrote:
> > On Mon, May 19, 2014 at 10:25:19PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On pre-ctg GPU reset also resets the display hardware. Force a mode
> > > restore after the GPU reset, and also re-init clock gating.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index b948c71..2ec3796 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -794,6 +794,14 @@ int i915_reset(struct drm_device *dev)
> > >  		if (INTEL_INFO(dev)->gen > 5)
> > >  			intel_reset_gt_powersave(dev);
> > >  
> > > +		if (IS_GEN4(dev) && !IS_G4X(dev)) {
> > > +			intel_init_clock_gating(dev);
> > > +
> > > +			drm_modeset_lock_all(dev);
> > > +			intel_modeset_setup_hw_state(dev, true);
> > > +			drm_modeset_unlock_all(dev);
> > 
> > Locking inversion here. I think we need to push this down to the very end.
> 
> We already dropped struct_mutex a bit earlier. Or was there some other
> lock you were worried about?

Oh right, was just confused by the mutex_unlock in the else branch.
> 
> > Also this leaves the interesting question of what happens with vblank
> > waits and friends ...
> 
> Hmm. I guess we might have some kind of refcount problem on our hands w/
> vblanks and whatnot. Might be I'd need to for_each_crtc() .crtc_disable()
> before the reset. That would match the g33 spec's recommendation to
> disable all planes, but it also does quite a bit more, maybe too much?
> But I could try it at least.

For full nasties we might need a intel_display_reset which does a few
things more and wraps the entire sequence in drm_modeset_lock_all so that
nothing can escape. Well except the vblank waiting because drm_irq.c. I
think we should grab the crtc->mutex in the vblank ioctl anyway for kms.

There's a few more trick bits:
- On ilk+ (but might as well do it for g4x+) we don't want to reprobe the
  interrupt since that breaks a few things like vblanks or pipe crc which
  we only enable at runtime. If we split this up we could just keep the
  irq reset for pre-g4x. Still leaves us with racy vblank, but meh. Do we
  have a vblank-vs-hagn testcase already?

- Full deadlock audit for the modeset locks to make sure the gpu reset
  work can actually execute.

- Probably more ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch