[PATCH 1/2] drm/i915: gtfifodbg in error state

[PATCH 1/2] drm/i915: gtfifodbg in error state

[Ben Widawsky]

 - Add register definitions for GTFIFODBG.
 - Capture it at error time.
 - Print it from debugfs (with whitespace fix).

This register tells us if either a read, or a write occured while the
fifo was full.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    3 ++-
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/i915_irq.c      |    1 +
 drivers/gpu/drm/i915/i915_reg.h      |    6 ++++++
 drivers/gpu/drm/i915/intel_display.c |    3 +++
 5 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6c3be86..f91a5d4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -760,7 +760,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	seq_printf(m, "EIR: 0x%08x\n", error->eir);
 	seq_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
 	if (INTEL_INFO(dev)->gen >= 6) {
-		seq_printf(m, "ERROR: 0x%08x\n", error->error);
+		seq_printf(m, "ERROR:   0x%08x\n", error->error);
+		seq_printf(m, "FIFODBG: 0x%08x\n", error->gtfifodbg);
 		seq_printf(m, "Blitter command stream:\n");
 		seq_printf(m, "  ACTHD:    0x%08x\n", error->bcs_acthd);
 		seq_printf(m, "  IPEIR:    0x%08x\n", error->bcs_ipeir);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f02a5f5..f02a57d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -173,6 +173,7 @@ struct drm_i915_error_state {
 	u32 seqno;
 	u64 bbaddr;
 	u64 fence[I915_MAX_NUM_FENCES];
+	u32 gtfifodbg;
 	struct timeval time;
 	struct drm_i915_error_object {
 		int page_count;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5d433fc..d99592c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -918,6 +918,7 @@ static void i915_capture_error_state(struct drm_device *dev)
 	error->error = 0;
 	if (INTEL_INFO(dev)->gen >= 6) {
 		error->error = I915_READ(ERROR_GEN6);
+		error->gtfifodbg = I915_READ(GTFIFODBG);
 
 		error->bcs_acthd = I915_READ(BCS_ACTHD);
 		error->bcs_ipehr = I915_READ(BCS_IPEHR);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c3afb78..1231c8e 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_OVFERR			(1<<2)
+#define    GT_FIFO_CPU_ERROR_MASK		3
+#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..537f029 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8215,6 +8215,9 @@ 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 */
+	I915_WRITE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);
+
 	gen6_gt_force_wake_get(dev_priv);
 
 	/* disable the counters and set deterministic thresholds */
-- 
1.7.9

[PATCH 2/2] 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.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1658cfd..3f88ca8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -381,12 +381,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 
 void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
+	u32 gtfifodbg = I915_READ(GTFIFODBG);
+	WARN(gtfifodbg & GT_FIFO_CPU_ERROR_MASK,
+	     "MMIO read or write has been dropped %x\n", gtfifodbg);
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
 	POSTING_READ(FORCEWAKE);
 }
 
 void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
 {
+	u32 gtfifodbg = I915_READ(GTFIFODBG);
+	WARN(gtfifodbg & GT_FIFO_CPU_ERROR_MASK,
+	     "MMIO read or write has been dropped %x\n", gtfifodbg);
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, (1<<16) | 0);
 	POSTING_READ(FORCEWAKE_MT);
 }
-- 
1.7.9

Re: [PATCH 1/2] drm/i915: gtfifodbg in error state

[Chris Wilson]

On Fri,  3 Feb 2012 14:31:40 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
>  - Add register definitions for GTFIFODBG.
>  - Capture it at error time.
>  - Print it from debugfs (with whitespace fix).
> 
> This register tells us if either a read, or a write occured while the
> fifo was full.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Fri,  3 Feb 2012 14:31:41 -0800, Ben Widawsky <ben@bwidawsk.net> 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.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Can we use the read of GTFIFODBG in place of the POSTING_READ? And do we
really need to watch all the GTFIFODBG reads in the trace, or can I
squelch that noise with a I915_READ_NOTRACE?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Ben Widawsky]

On Fri, Feb 03, 2012 at 11:10:09PM +0000, Chris Wilson wrote:
> On Fri,  3 Feb 2012 14:31:41 -0800, Ben Widawsky <ben@bwidawsk.net> 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.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Can we use the read of GTFIFODBG in place of the POSTING_READ? And do we
> really need to watch all the GTFIFODBG reads in the trace, or can I
> squelch that noise with a I915_READ_NOTRACE?
> -Chris

TBH, I don't really understand POSTING_READ that well. If it just
requires that any read request made it through to the MCH (or whatever),
then sure I can replace the POSTING_READ. Obviously though this is
reading GTFIFODBG, and not FORCEWAKE, so you tell me.

As for READ_NOTRACE, I'm all for that. I'll wait to reroll the patch
until you respond to the above though.

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

[Chris Wilson]

On Fri, 3 Feb 2012 18:15:33 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Feb 03, 2012 at 11:10:09PM +0000, Chris Wilson wrote:
> > On Fri,  3 Feb 2012 14:31:41 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> TBH, I don't really understand POSTING_READ that well. If it just
> requires that any read request made it through to the MCH (or whatever),
> then sure I can replace the POSTING_READ. Obviously though this is
> reading GTFIFODBG, and not FORCEWAKE, so you tell me.

The posting read is simply a PCI write/read ordering barrier. It can be
a read of any mmio address and before it is performed all previous writes
to mmio addresses must have been sent along the PCI and no subsequent write
is allowed to emitted ahead of the read. Hmm, so since the writes are in
fact weakly ordered (they are allowed to be emitted in any order so long
as they not reordered past a read), we would strictly need a barrier in
front of the write to disable forcewake if it were not for the buffering
performed by the chip of mmio writes under rc6.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: gtfifodbg in error state

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 395 bytes --]

On Fri, Feb 3, 2012 at 20:31, Ben Widawsky <ben@bwidawsk.net> wrote:

>  - Add register definitions for GTFIFODBG.
>  - Capture it at error time.
>  - Print it from debugfs (with whitespace fix).
>
> This register tells us if either a read, or a write occured while the
> fifo was full.
>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

-- 
Eugeni Dodonov
 <http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 756 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 2/2 v2] 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

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

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

Re: [PATCH 1/2] drm/i915: gtfifodbg in error state

[Daniel Vetter]

On Fri, Feb 03, 2012 at 02:31:40PM -0800, Ben Widawsky wrote:
>  - Add register definitions for GTFIFODBG.
>  - Capture it at error time.
>  - Print it from debugfs (with whitespace fix).
> 
> This register tells us if either a read, or a write occured while the
> fifo was full.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |    3 ++-
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/i915_irq.c      |    1 +
>  drivers/gpu/drm/i915/i915_reg.h      |    6 ++++++
>  drivers/gpu/drm/i915/intel_display.c |    3 +++
>  5 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6c3be86..f91a5d4 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -760,7 +760,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
>  	seq_printf(m, "EIR: 0x%08x\n", error->eir);
>  	seq_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
>  	if (INTEL_INFO(dev)->gen >= 6) {
> -		seq_printf(m, "ERROR: 0x%08x\n", error->error);
> +		seq_printf(m, "ERROR:   0x%08x\n", error->error);
> +		seq_printf(m, "FIFODBG: 0x%08x\n", error->gtfifodbg);
>  		seq_printf(m, "Blitter command stream:\n");
>  		seq_printf(m, "  ACTHD:    0x%08x\n", error->bcs_acthd);
>  		seq_printf(m, "  IPEIR:    0x%08x\n", error->bcs_ipeir);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f02a5f5..f02a57d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -173,6 +173,7 @@ struct drm_i915_error_state {
>  	u32 seqno;
>  	u64 bbaddr;
>  	u64 fence[I915_MAX_NUM_FENCES];
> +	u32 gtfifodbg;
>  	struct timeval time;
>  	struct drm_i915_error_object {
>  		int page_count;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5d433fc..d99592c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -918,6 +918,7 @@ static void i915_capture_error_state(struct drm_device *dev)
>  	error->error = 0;
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		error->error = I915_READ(ERROR_GEN6);
> +		error->gtfifodbg = I915_READ(GTFIFODBG);
>  
>  		error->bcs_acthd = I915_READ(BCS_ACTHD);
>  		error->bcs_ipehr = I915_READ(BCS_IPEHR);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c3afb78..1231c8e 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_OVFERR			(1<<2)
> +#define    GT_FIFO_CPU_ERROR_MASK		3

Reading configdb it sounds like bit 2 is set when either bit 1 or bit 0 is
set, but I might be wrong so imo it's better to check for all three bits.

> +#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..537f029 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8215,6 +8215,9 @@ 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 */
> +	I915_WRITE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);

I think we shouldn't silently clear errors here. Simplest might be a small
inline helper to WARN_ON dirt in the gt fifo db reg - could be used in the
later patches.

> +
>  	gen6_gt_force_wake_get(dev_priv);
>  
>  	/* disable the counters and set deterministic thresholds */
> -- 
> 1.7.9
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm/i915: gtfifodbg in error state

[Chris Wilson]

On Tue, 7 Feb 2012 10:28:30 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Feb 03, 2012 at 02:31:40PM -0800, Ben Widawsky wrote:
> >  - Add register definitions for GTFIFODBG.
> >  - Capture it at error time.
> >  - Print it from debugfs (with whitespace fix).
> > 
> > This register tells us if either a read, or a write occured while the
> > fifo was full.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |    3 ++-
> >  drivers/gpu/drm/i915/i915_drv.h      |    1 +
> >  drivers/gpu/drm/i915/i915_irq.c      |    1 +
> >  drivers/gpu/drm/i915/i915_reg.h      |    6 ++++++
> >  drivers/gpu/drm/i915/intel_display.c |    3 +++
> >  5 files changed, 13 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 6c3be86..f91a5d4 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -760,7 +760,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
> >  	seq_printf(m, "EIR: 0x%08x\n", error->eir);
> >  	seq_printf(m, "PGTBL_ER: 0x%08x\n", error->pgtbl_er);
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> > -		seq_printf(m, "ERROR: 0x%08x\n", error->error);
> > +		seq_printf(m, "ERROR:   0x%08x\n", error->error);
> > +		seq_printf(m, "FIFODBG: 0x%08x\n", error->gtfifodbg);
> >  		seq_printf(m, "Blitter command stream:\n");
> >  		seq_printf(m, "  ACTHD:    0x%08x\n", error->bcs_acthd);
> >  		seq_printf(m, "  IPEIR:    0x%08x\n", error->bcs_ipeir);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f02a5f5..f02a57d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -173,6 +173,7 @@ struct drm_i915_error_state {
> >  	u32 seqno;
> >  	u64 bbaddr;
> >  	u64 fence[I915_MAX_NUM_FENCES];
> > +	u32 gtfifodbg;
> >  	struct timeval time;
> >  	struct drm_i915_error_object {
> >  		int page_count;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 5d433fc..d99592c 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -918,6 +918,7 @@ static void i915_capture_error_state(struct drm_device *dev)
> >  	error->error = 0;
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		error->error = I915_READ(ERROR_GEN6);
> > +		error->gtfifodbg = I915_READ(GTFIFODBG);
> >  
> >  		error->bcs_acthd = I915_READ(BCS_ACTHD);
> >  		error->bcs_ipehr = I915_READ(BCS_IPEHR);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c3afb78..1231c8e 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_OVFERR			(1<<2)
> > +#define    GT_FIFO_CPU_ERROR_MASK		3
> 
> Reading configdb it sounds like bit 2 is set when either bit 1 or bit 0 is
> set, but I might be wrong so imo it's better to check for all three bits.
> 
> > +#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..537f029 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8215,6 +8215,9 @@ 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 */
> > +	I915_WRITE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);
> 
> I think we shouldn't silently clear errors here. Simplest might be a small
> inline helper to WARN_ON dirt in the gt fifo db reg - could be used in the
> later patches.

This reminds me, we have a few other error registers we should scrub upon init, along with the warning that something bad happened earlier.
-Chris
> 
> > +
> >  	gen6_gt_force_wake_get(dev_priv);
> >  
> >  	/* disable the counters and set deterministic thresholds */
> > -- 
> > 1.7.9
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Sat, Feb 04, 2012 at 11:48:43AM -0800, 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
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Looks good and because we can drop the posting read it shouldn't have any
perf impact. But we're not checking anything on the write side, which is
actually used the most under normal gem loads (tail updates). Because the
write path is rather perf critical, I think we should only check for
errors in the gt fifo reg if the wait_for_fifo times out. Can you whip up
that patch, too?

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