[PATCH 0/3 v3] GTFIFODBG

[PATCH 0/3 v3] GTFIFODBG

[Ben Widawsky]

Someday I will get simple patches in one version... Alas that day is not
today.

Ben Widawsky (3):
  drm/i915: use gtfifodbg
  drm/i915: catch gtfifo errors on forcewake_put
  drm/i915: check gtfifodbg after possibly failed writes

 drivers/gpu/drm/i915/i915_drv.c      |   30 +++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/i915_reg.h      |    6 ++++++
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
 4 files changed, 40 insertions(+), 6 deletions(-)

-- 
1.7.9

[PATCH 1/3] drm/i915: use gtfifodbg

[Ben Widawsky]

Add register definitions for GTFIFODBG, and clear it during init time to
make sure state is correct.

This register tells us if either a read, or a write occurred while the
fifo was full. It seems like bit 2 is an OR of bit 0 and bit 1, so we
check that as well, but the documents are not quite clear.

Reviewed-by (v1): Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by (v1): Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h      |    6 ++++++
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c3afb78..1410fc4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3614,6 +3614,12 @@
 #define  ECOBUS					0xa180
 #define    FORCEWAKE_MT_ENABLE			(1<<5)
 
+#define  GTFIFODBG				0x120000
+#define    GT_FIFO_CPU_ERROR_MASK		7
+#define    GT_FIFO_OVFERR			(1<<2)
+#define    GT_FIFO_IAWRERR			(1<<1)
+#define    GT_FIFO_IARDERR			(1<<0)
+
 #define  GT_FIFO_FREE_ENTRIES			0x120008
 #define    GT_FIFO_NUM_RESERVED_ENTRIES		20
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ebe71ed..4f1205e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8204,6 +8204,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
 	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 	u32 pcu_mbox, rc6_mask = 0;
+	u32 gtfifodbg;
 	int cur_freq, min_freq, max_freq;
 	int i;
 
@@ -8215,6 +8216,13 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	 */
 	I915_WRITE(GEN6_RC_STATE, 0);
 	mutex_lock(&dev_priv->dev->struct_mutex);
+
+	/* Clear the DBG now so we don't confuse earlier errors */
+	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
+		DRM_ERROR("GT fifo had a previous error %x\n", gtfifodbg);
+		I915_WRITE(GTFIFODBG, gtfifodbg);
+	}
+
 	gen6_gt_force_wake_get(dev_priv);
 
 	/* disable the counters and set deterministic thresholds */
-- 
1.7.9

[PATCH 2/3] drm/i915: catch gtfifo errors on forcewake_put

[Ben Widawsky]

This is similar to a patch I wrote several months ago. It's been updated
for the new FORCEWAKE_MT. As recommended by Chris Wilson, use WARN()
instead of DRM_ERROR, so we can get a backtrace.

This shouldn't impact performance too much as the extra register read
can replace the POSTING_READ we had previously.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1658cfd..a8b6a78 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -379,16 +379,27 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 		dev_priv->display.force_wake_get(dev_priv);
 }
 
+static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
+{
+	u32 gtfifodbg;
+	gtfifodbg = I915_READ_NOTRACE(GTFIFODBG);
+	if (WARN(gtfifodbg & GT_FIFO_CPU_ERROR_MASK,
+	     "MMIO read or write has been dropped %x\n", gtfifodbg))
+		I915_WRITE_NOTRACE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);
+}
+
 void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
-	POSTING_READ(FORCEWAKE);
+	/* The below doubles as a POSTING_READ */
+	gen6_gt_check_fifodbg(dev_priv);
 }
 
 void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, (1<<16) | 0);
-	POSTING_READ(FORCEWAKE_MT);
+	/* The below doubles as a POSTING_READ */
+	gen6_gt_check_fifodbg(dev_priv);
 }
 
 /*
-- 
1.7.9

[PATCH 3/3] drm/i915: check gtfifodbg after possibly failed writes

[Ben Widawsky]

If we don't have a sufficient number of free entries in the FIFO, we
proceed to do a write anyway. With this check we should have a clue if
that write actually failed or not.

After some discussion with Daniel Vetter regarding his original
complaint, we agreed upon this.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   15 ++++++++++++---
 drivers/gpu/drm/i915/i915_drv.h |    2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a8b6a78..8276a0d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -413,8 +413,10 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 		dev_priv->display.force_wake_put(dev_priv);
 }
 
-void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
+int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
+	int ret = 0;
+
 	if (dev_priv->gt_fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) {
 		int loop = 500;
 		u32 fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
@@ -422,10 +424,13 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 			udelay(10);
 			fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
 		}
-		WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES);
+		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
+			++ret;
 		dev_priv->gt_fifo_count = fifo;
 	}
 	dev_priv->gt_fifo_count--;
+
+	return ret;
 }
 
 static int i915_drm_freeze(struct drm_device *dev)
@@ -960,11 +965,15 @@ __i915_read(64, q)
 
 #define __i915_write(x, y) \
 void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
+	u32 __fifo_ret = 0; \
 	trace_i915_reg_rw(true, reg, val, sizeof(val)); \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__gen6_gt_wait_for_fifo(dev_priv); \
+		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
 	write##y(val, dev_priv->regs + reg); \
+	if (unlikely(__fifo_ret)) { \
+		gen6_gt_check_fifodbg(dev_priv); \
+	} \
 }
 __i915_write(8, b)
 __i915_write(16, w)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f02a5f5..ad6408f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1350,7 +1350,7 @@ extern void intel_display_print_error_state(struct seq_file *m,
  */
 void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
-void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
+int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(dev_priv, reg) \
-- 
1.7.9

Re: [PATCH 0/3 v3] GTFIFODBG

[Daniel Vetter]

On Thu, Feb 09, 2012 at 10:15:17AM +0100, Ben Widawsky wrote:
> Someday I will get simple patches in one version... Alas that day is not
> today.
> 
> Ben Widawsky (3):
>   drm/i915: use gtfifodbg
>   drm/i915: catch gtfifo errors on forcewake_put
>   drm/i915: check gtfifodbg after possibly failed writes

I've slurped these three in for -next, thanks for the patches. There's a
quibble though from your grumpy maintainer: Please keep a proper patch
revision log in the commit message and don't just drop it randomly. In
this case the r-b (v1) is pretty pointless because you don't say what
you've changed (and that these patches aren't v1) ...

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/3 v3] GTFIFODBG

[Chris Wilson]

On Sat, 11 Feb 2012 17:22:05 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 09, 2012 at 10:15:17AM +0100, Ben Widawsky wrote:
> > Someday I will get simple patches in one version... Alas that day is not
> > today.
> > 
> > Ben Widawsky (3):
> >   drm/i915: use gtfifodbg
> >   drm/i915: catch gtfifo errors on forcewake_put
> >   drm/i915: check gtfifodbg after possibly failed writes
> 
> I've slurped these three in for -next, thanks for the patches. There's a
> quibble though from your grumpy maintainer: Please keep a proper patch
> revision log in the commit message and don't just drop it randomly. In
> this case the r-b (v1) is pretty pointless because you don't say what
> you've changed (and that these patches aren't v1) ...

Daniel, can you add my reviewed-by to that series?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/3 v3] GTFIFODBG

[Daniel Vetter]

On Sat, Feb 11, 2012 at 08:00:39PM +0000, Chris Wilson wrote:
> On Sat, 11 Feb 2012 17:22:05 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 09, 2012 at 10:15:17AM +0100, Ben Widawsky wrote:
> > > Someday I will get simple patches in one version... Alas that day is not
> > > today.
> > > 
> > > Ben Widawsky (3):
> > >   drm/i915: use gtfifodbg
> > >   drm/i915: catch gtfifo errors on forcewake_put
> > >   drm/i915: check gtfifodbg after possibly failed writes
> > 
> > I've slurped these three in for -next, thanks for the patches. There's a
> > quibble though from your grumpy maintainer: Please keep a proper patch
> > revision log in the commit message and don't just drop it randomly. In
> > this case the r-b (v1) is pretty pointless because you don't say what
> > you've changed (and that these patches aren't v1) ...
> 
> Daniel, can you add my reviewed-by to that series?

History fixed up, thanks for the reivew.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/3] drm/i915: check gtfifodbg after possibly failed writes

[Ben Widawsky]

On 02/08/2012 11:28 AM, Chris Wilson wrote:
> On Wed, 8 Feb 2012 11:15:58 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
>> Bit a bikeshed comment, but I prefer explicit control flow instead of
>> playing tricks with the short-circuiting behaviour of &&. Mind if you can
>> change that, too?
> 
> Whilst we're on the subject of bikesheds, having an explicit tracepoint
> for forcewake get/put would have answered a query I just had.
> -Chris
> 

I'll do it if you'll review it.

Re: [PATCH 3/3] drm/i915: check gtfifodbg after possibly failed writes

[Chris Wilson]

On Wed, 8 Feb 2012 11:15:58 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> Bit a bikeshed comment, but I prefer explicit control flow instead of
> playing tricks with the short-circuiting behaviour of &&. Mind if you can
> change that, too?

Whilst we're on the subject of bikesheds, having an explicit tracepoint
for forcewake get/put would have answered a query I just had.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: check gtfifodbg after possibly failed writes

[Daniel Vetter]

On Tue, Feb 07, 2012 at 04:21:50PM +0100, Ben Widawsky wrote:
> If we don't have a sufficient number of free entries in the FIFO, we
> proceed to do a write anyway. With this check we should have a clue if
> that write actually failed or not.
> 
> After some discussion with Daniel Vetter regarding his original
> complaint, we agreed upon this.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   15 ++++++++++++---
>  drivers/gpu/drm/i915/i915_drv.h |    2 +-
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a7858a1..ff5c3c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -413,8 +413,10 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  		dev_priv->display.force_wake_put(dev_priv);
>  }
>  
> -void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> +int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  {
> +	int ret = 0;
> +
>  	if (dev_priv->gt_fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) {
>  		int loop = 500;
>  		u32 fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
> @@ -422,10 +424,13 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  			udelay(10);
>  			fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
>  		}
> -		WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES);
> +		WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES &&
> +			++ret);

Bit a bikeshed comment, but I prefer explicit control flow instead of
playing tricks with the short-circuiting behaviour of &&. Mind if you can
change that, too?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 3/3] drm/i915: check gtfifodbg after possibly failed writes

[Ben Widawsky]

If we don't have a sufficient number of free entries in the FIFO, we
proceed to do a write anyway. With this check we should have a clue if
that write actually failed or not.

After some discussion with Daniel Vetter regarding his original
complaint, we agreed upon this.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   15 ++++++++++++---
 drivers/gpu/drm/i915/i915_drv.h |    2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a7858a1..ff5c3c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -413,8 +413,10 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 		dev_priv->display.force_wake_put(dev_priv);
 }
 
-void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
+int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
+	int ret = 0;
+
 	if (dev_priv->gt_fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) {
 		int loop = 500;
 		u32 fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
@@ -422,10 +424,13 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 			udelay(10);
 			fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
 		}
-		WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES);
+		WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES &&
+			++ret);
 		dev_priv->gt_fifo_count = fifo;
 	}
 	dev_priv->gt_fifo_count--;
+
+	return ret;
 }
 
 static int i915_drm_freeze(struct drm_device *dev)
@@ -960,11 +965,15 @@ __i915_read(64, q)
 
 #define __i915_write(x, y) \
 void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
+	u32 __fifo_ret = 0; \
 	trace_i915_reg_rw(true, reg, val, sizeof(val)); \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__gen6_gt_wait_for_fifo(dev_priv); \
+		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
 	write##y(val, dev_priv->regs + reg); \
+	if (unlikely(__fifo_ret)) { \
+		gen6_gt_check_fifodbg(dev_priv); \
+	} \
 }
 __i915_write(8, b)
 __i915_write(16, w)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f02a5f5..ad6408f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1350,7 +1350,7 @@ extern void intel_display_print_error_state(struct seq_file *m,
  */
 void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
-void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
+int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(dev_priv, reg) \
-- 
1.7.9