[PATCH 0/3] GTFIFODBG

[PATCH 0/3] GTFIFODBG

[Ben Widawsky]

The GTFIFODBG registers gives us some info regarding the state of the
GT FIFO. In the following patches we therefore add a few things:
1. Add the register definition, and clear previous errors.
2. Check if any failures occurred when doing a forcewake_put.
3. Check if any failures occurred after reaching the low watermark of the
write threshold. This differs from #2 slightly in that there is an extra
error bit which seems to indicate a read error in the register. It is my
suspicion though that #2, and #3 will often be redundant, but since the
clearing of the register occurs during the check, you will only see one
message.

I've only done some basic checks for performance regressions, and found
none.

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 v2] 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.

v2: Check all 3 mask bits, and DRM_ERROR when we see one of the fifodbg
bits set on init. Remove error state portion as it's redundant with
later patches.

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 v3] 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, and it also no longer clears the debug
register as it may be helpful to get that for the error state.  Also
recommended by Chris Wilson, use WARN() instead of DRM_ERROR, so we can
get a backtrace.

v2: Replace POSTING_READ with I915_READ_NOTRACE(GTFIFODBG). Recommended
by Chris Wilson

v3: Pull the fifo check out into a helper function to reduce code
duplication

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

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

[Ben Widawsky]

On 02/07/2012 04:21 PM, Ben Widawsky wrote:
> This is similar to a patch I wrote several months ago. It's been updated
> for the new FORCEWAKE_MT, and it also no longer clears the debug
> register as it may be helpful to get that for the error state.  Also
> recommended by Chris Wilson, use WARN() instead of DRM_ERROR, so we can
> get a backtrace.

This comment is wrong. It once again clears the debug register. ;\

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

[Daniel Vetter]

On Tue, Feb 07, 2012 at 04:21:49PM +0100, Ben Widawsky wrote:
> This is similar to a patch I wrote several months ago. It's been updated
> for the new FORCEWAKE_MT, and it also no longer clears the debug
> register as it may be helpful to get that for the error state.  Also
> recommended by Chris Wilson, use WARN() instead of DRM_ERROR, so we can
> get a backtrace.
> 
> v2: Replace POSTING_READ with I915_READ_NOTRACE(GTFIFODBG). Recommended
> by Chris Wilson
> 
> v3: Pull the fifo check out into a helper function to reduce code
> duplication
> 
> 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..a7858a1 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);
> +	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);

I think we should move the write out of line and only do it when we're
actually catching an error.
-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

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

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

[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.