drm/i915: Replace WARN inside I915_READ64_2x32 with retry loop
diff mbox

Message ID 1436950242-4738-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson July 15, 2015, 8:50 a.m. UTC
Since we may conceivably encounter situations where the upper part of the
64bit register changes between reads, for example when a timestamp
counter overflows, change the WARN into a retry loop.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Micha? Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Chris Wilson July 28, 2015, 1:53 p.m. UTC | #1
On Wed, Jul 15, 2015 at 09:50:42AM +0100, Chris Wilson wrote:
> Since we may conceivably encounter situations where the upper part of the
> 64bit register changes between reads, for example when a timestamp
> counter overflows, change the WARN into a retry loop.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Micha? Winiarski <michal.winiarski@intel.com>

Micha?, as you correctly predicted this WARN will be hit in the wild
after adjusting reg_read_ioctl(TIMESTAMP).

Anyone care to review this patch?

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ee53873ab252..cfddd69bd06c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3484,15 +3484,14 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
>  #define I915_READ64(reg)	dev_priv->uncore.funcs.mmio_readq(dev_priv, (reg), true)
>  
>  #define I915_READ64_2x32(lower_reg, upper_reg) ({			\
> -		u32 upper = I915_READ(upper_reg);			\
> -		u32 lower = I915_READ(lower_reg);			\
> -		u32 tmp = I915_READ(upper_reg);				\
> -		if (upper != tmp) {					\
> -			upper = tmp;					\
> -			lower = I915_READ(lower_reg);			\
> -			WARN_ON(I915_READ(upper_reg) != upper);		\
> -		}							\
> -		(u64)upper << 32 | lower; })
> +	u32 upper, lower, tmp;						\
> +	tmp = I915_READ(upper_reg);					\
> +	do {								\
> +		upper = tmp;						\
> +		lower = I915_READ(lower_reg);				\
> +		tmp = I915_READ(upper_reg);				\
> +	} while (upper != tmp);						\
> +	(u64)upper << 32 | lower; })
>  
>  #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
>  #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
Daniel Vetter July 29, 2015, 8:22 a.m. UTC | #2
On Tue, Jul 28, 2015 at 02:53:16PM +0100, Chris Wilson wrote:
> On Wed, Jul 15, 2015 at 09:50:42AM +0100, Chris Wilson wrote:
> > Since we may conceivably encounter situations where the upper part of the
> > 64bit register changes between reads, for example when a timestamp
> > counter overflows, change the WARN into a retry loop.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Micha? Winiarski <michal.winiarski@intel.com>
> 
> Micha?, as you correctly predicted this WARN will be hit in the wild
> after adjusting reg_read_ioctl(TIMESTAMP).
> 
> Anyone care to review this patch?

Picked up for -fixes, thanks for the patch.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ee53873ab252..cfddd69bd06c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3484,15 +3484,14 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
> >  #define I915_READ64(reg)	dev_priv->uncore.funcs.mmio_readq(dev_priv, (reg), true)
> >  
> >  #define I915_READ64_2x32(lower_reg, upper_reg) ({			\
> > -		u32 upper = I915_READ(upper_reg);			\
> > -		u32 lower = I915_READ(lower_reg);			\
> > -		u32 tmp = I915_READ(upper_reg);				\
> > -		if (upper != tmp) {					\
> > -			upper = tmp;					\
> > -			lower = I915_READ(lower_reg);			\
> > -			WARN_ON(I915_READ(upper_reg) != upper);		\
> > -		}							\
> > -		(u64)upper << 32 | lower; })
> > +	u32 upper, lower, tmp;						\
> > +	tmp = I915_READ(upper_reg);					\
> > +	do {								\
> > +		upper = tmp;						\
> > +		lower = I915_READ(lower_reg);				\
> > +		tmp = I915_READ(upper_reg);				\
> > +	} while (upper != tmp);						\
> > +	(u64)upper << 32 | lower; })
> >  
> >  #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
> >  #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ee53873ab252..cfddd69bd06c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3484,15 +3484,14 @@  int intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
 #define I915_READ64(reg)	dev_priv->uncore.funcs.mmio_readq(dev_priv, (reg), true)
 
 #define I915_READ64_2x32(lower_reg, upper_reg) ({			\
-		u32 upper = I915_READ(upper_reg);			\
-		u32 lower = I915_READ(lower_reg);			\
-		u32 tmp = I915_READ(upper_reg);				\
-		if (upper != tmp) {					\
-			upper = tmp;					\
-			lower = I915_READ(lower_reg);			\
-			WARN_ON(I915_READ(upper_reg) != upper);		\
-		}							\
-		(u64)upper << 32 | lower; })
+	u32 upper, lower, tmp;						\
+	tmp = I915_READ(upper_reg);					\
+	do {								\
+		upper = tmp;						\
+		lower = I915_READ(lower_reg);				\
+		tmp = I915_READ(upper_reg);				\
+	} while (upper != tmp);						\
+	(u64)upper << 32 | lower; })
 
 #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
 #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)