All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: prevent gt fifo count underflow
@ 2014-05-14 12:10 Mika Kuoppala
  2014-05-14 13:18 ` Mika Kuoppala
  0 siblings, 1 reply; 5+ messages in thread
From: Mika Kuoppala @ 2014-05-14 12:10 UTC (permalink / raw)
  To: intel-gfx

If we get the final value of zero as a count of free
entries available, we will underflow our own fifo_count
and then it will take a long time before we check things again.
Admittedly we are in trouble already if we get into this situation,
but prevent the underflow by checking against zero before decreasing
fifo_count.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 76dc185..159f8ab 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -174,7 +174,7 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 		}
 		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
 			++ret;
-		dev_priv->uncore.fifo_count = fifo;
+		dev_priv->uncore.fifo_count = fifo ?: 1;
 	}
 	dev_priv->uncore.fifo_count--;
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] drm/i915: prevent gt fifo count underflow
  2014-05-14 12:10 [PATCH] drm/i915: prevent gt fifo count underflow Mika Kuoppala
@ 2014-05-14 13:18 ` Mika Kuoppala
  2014-05-14 13:35   ` Ville Syrjälä
  0 siblings, 1 reply; 5+ messages in thread
From: Mika Kuoppala @ 2014-05-14 13:18 UTC (permalink / raw)
  To: intel-gfx

If we get the final value of zero as a count of free
entries available, we will underflow our own fifo_count
and then it will take a long time before we check things again.
Admittedly we are in trouble already if we get into this situation,
but prevent the underflow by returning early.

v2: Less convoluted control flow (Daniel Vetter)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 76dc185..bf1b661 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -154,10 +154,8 @@ static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
 		gen6_gt_check_fifodbg(dev_priv);
 }
 
-static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
+static bool __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
-	int ret = 0;
-
 	/* On VLV, FIFO will be shared by both SW and HW.
 	 * So, we need to read the FREE_ENTRIES everytime */
 	if (IS_VALLEYVIEW(dev_priv->dev))
@@ -173,12 +171,12 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
 		}
 		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
-			++ret;
+			return true;
 		dev_priv->uncore.fifo_count = fifo;
 	}
 	dev_priv->uncore.fifo_count--;
 
-	return ret;
+	return false;
 }
 
 static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
@@ -642,13 +640,13 @@ gen5_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 #define __gen6_write(x) \
 static void \
 gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
-	u32 __fifo_ret = 0; \
+	bool __fifo_failed = false; \
 	REG_WRITE_HEADER; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
+		__fifo_failed = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
 	__raw_i915_write##x(dev_priv, reg, val); \
-	if (unlikely(__fifo_ret)) { \
+	if (unlikely(__fifo_failed)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
 	REG_WRITE_FOOTER; \
@@ -657,14 +655,14 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 #define __hsw_write(x) \
 static void \
 hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
-	u32 __fifo_ret = 0; \
+	bool __fifo_failed = false; \
 	REG_WRITE_HEADER; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
+		__fifo_failed = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
 	hsw_unclaimed_reg_clear(dev_priv, reg); \
 	__raw_i915_write##x(dev_priv, reg, val); \
-	if (unlikely(__fifo_ret)) { \
+	if (unlikely(__fifo_failed)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
 	hsw_unclaimed_reg_check(dev_priv, reg); \
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: prevent gt fifo count underflow
  2014-05-14 13:18 ` Mika Kuoppala
@ 2014-05-14 13:35   ` Ville Syrjälä
  2014-05-14 13:48     ` Chris Wilson
  2014-05-14 14:22     ` [PATCH v3] " Mika Kuoppala
  0 siblings, 2 replies; 5+ messages in thread
From: Ville Syrjälä @ 2014-05-14 13:35 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Wed, May 14, 2014 at 04:18:02PM +0300, Mika Kuoppala wrote:
> If we get the final value of zero as a count of free
> entries available, we will underflow our own fifo_count
> and then it will take a long time before we check things again.
> Admittedly we are in trouble already if we get into this situation,
> but prevent the underflow by returning early.
> 
> v2: Less convoluted control flow (Daniel Vetter)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c |   20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 76dc185..bf1b661 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -154,10 +154,8 @@ static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
>  		gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> -static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> +static bool __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  {
> -	int ret = 0;
> -
>  	/* On VLV, FIFO will be shared by both SW and HW.
>  	 * So, we need to read the FREE_ENTRIES everytime */
>  	if (IS_VALLEYVIEW(dev_priv->dev))
> @@ -173,12 +171,12 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
>  		}
>  		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))

Maybe kill the 'loop<0' check while at it. It's redundant and IMO just
makes things less obvious.

Also I don't get why we first check for
'fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES', but then the while
loop checks for 'fifo <= GT_FIFO_NUM_RESERVED_ENTRIES'.

> -			++ret;
> +			return true;
>  		dev_priv->uncore.fifo_count = fifo;

We no longer update fifo_count on failure. Not really a problem, but
since we've already done all the work maybe we should still update it.

>  	}
>  	dev_priv->uncore.fifo_count--;
>  
> -	return ret;
> +	return false;
>  }
>  
>  static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
> @@ -642,13 +640,13 @@ gen5_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  #define __gen6_write(x) \
>  static void \
>  gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> -	u32 __fifo_ret = 0; \
> +	bool __fifo_failed = false; \
>  	REG_WRITE_HEADER; \
>  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
> +		__fifo_failed = __gen6_gt_wait_for_fifo(dev_priv); \
>  	} \
>  	__raw_i915_write##x(dev_priv, reg, val); \
> -	if (unlikely(__fifo_ret)) { \
> +	if (unlikely(__fifo_failed)) { \
>  		gen6_gt_check_fifodbg(dev_priv); \
>  	} \
>  	REG_WRITE_FOOTER; \
> @@ -657,14 +655,14 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  #define __hsw_write(x) \
>  static void \
>  hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> -	u32 __fifo_ret = 0; \
> +	bool __fifo_failed = false; \
>  	REG_WRITE_HEADER; \
>  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
> +		__fifo_failed = __gen6_gt_wait_for_fifo(dev_priv); \
>  	} \
>  	hsw_unclaimed_reg_clear(dev_priv, reg); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
> -	if (unlikely(__fifo_ret)) { \
> +	if (unlikely(__fifo_failed)) { \
>  		gen6_gt_check_fifodbg(dev_priv); \
>  	} \
>  	hsw_unclaimed_reg_check(dev_priv, reg); \
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: prevent gt fifo count underflow
  2014-05-14 13:35   ` Ville Syrjälä
@ 2014-05-14 13:48     ` Chris Wilson
  2014-05-14 14:22     ` [PATCH v3] " Mika Kuoppala
  1 sibling, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2014-05-14 13:48 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Wed, May 14, 2014 at 04:35:42PM +0300, Ville Syrjälä wrote:
> On Wed, May 14, 2014 at 04:18:02PM +0300, Mika Kuoppala wrote:
> > If we get the final value of zero as a count of free
> > entries available, we will underflow our own fifo_count
> > and then it will take a long time before we check things again.
> > Admittedly we are in trouble already if we get into this situation,
> > but prevent the underflow by returning early.
> > 
> > v2: Less convoluted control flow (Daniel Vetter)
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c |   20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 76dc185..bf1b661 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -154,10 +154,8 @@ static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
> >  		gen6_gt_check_fifodbg(dev_priv);
> >  }
> >  
> > -static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> > +static bool __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> >  {
> > -	int ret = 0;
> > -
> >  	/* On VLV, FIFO will be shared by both SW and HW.
> >  	 * So, we need to read the FREE_ENTRIES everytime */
> >  	if (IS_VALLEYVIEW(dev_priv->dev))
> > @@ -173,12 +171,12 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> >  			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> >  		}
> >  		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
> 
> Maybe kill the 'loop<0' check while at it. It's redundant and IMO just
> makes things less obvious.
> 
> Also I don't get why we first check for
> 'fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES', but then the while
> loop checks for 'fifo <= GT_FIFO_NUM_RESERVED_ENTRIES'.

No good reason, I thought a little bit of hysteresis would be useful.
 
> > -			++ret;
> > +			return true;
> >  		dev_priv->uncore.fifo_count = fifo;
> 
> We no longer update fifo_count on failure. Not really a problem, but
> since we've already done all the work maybe we should still update it.

It doesn't matter, the value will still trigger the loop again, so by
ignoring it we can keep the code neater.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH v3] drm/i915: prevent gt fifo count underflow
  2014-05-14 13:35   ` Ville Syrjälä
  2014-05-14 13:48     ` Chris Wilson
@ 2014-05-14 14:22     ` Mika Kuoppala
  1 sibling, 0 replies; 5+ messages in thread
From: Mika Kuoppala @ 2014-05-14 14:22 UTC (permalink / raw)
  To: intel-gfx

If we get the final value of zero as a count of free
entries available, we will underflow our own fifo_count
and then it will take a long time before we check things again.
Admittedly we are in trouble already if we get into this situation,
but prevent the underflow by returning early.

v2: Less convoluted control flow (Daniel Vetter)

v3: Kill redundant loop<0 check (Ville Syrjälä)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 76dc185..72f74d0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -154,10 +154,8 @@ static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
 		gen6_gt_check_fifodbg(dev_priv);
 }
 
-static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
+static bool __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
-	int ret = 0;
-
 	/* On VLV, FIFO will be shared by both SW and HW.
 	 * So, we need to read the FREE_ENTRIES everytime */
 	if (IS_VALLEYVIEW(dev_priv->dev))
@@ -172,13 +170,14 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 			udelay(10);
 			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
 		}
-		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
-			++ret;
+		if (WARN_ON(fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
+			return true;
+
 		dev_priv->uncore.fifo_count = fifo;
 	}
 	dev_priv->uncore.fifo_count--;
 
-	return ret;
+	return false;
 }
 
 static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
@@ -642,13 +641,13 @@ gen5_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 #define __gen6_write(x) \
 static void \
 gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
-	u32 __fifo_ret = 0; \
+	bool __fifo_failed = false; \
 	REG_WRITE_HEADER; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
+		__fifo_failed = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
 	__raw_i915_write##x(dev_priv, reg, val); \
-	if (unlikely(__fifo_ret)) { \
+	if (unlikely(__fifo_failed)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
 	REG_WRITE_FOOTER; \
@@ -657,14 +656,14 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 #define __hsw_write(x) \
 static void \
 hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
-	u32 __fifo_ret = 0; \
+	bool __fifo_failed = false; \
 	REG_WRITE_HEADER; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
+		__fifo_failed = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
 	hsw_unclaimed_reg_clear(dev_priv, reg); \
 	__raw_i915_write##x(dev_priv, reg, val); \
-	if (unlikely(__fifo_ret)) { \
+	if (unlikely(__fifo_failed)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
 	hsw_unclaimed_reg_check(dev_priv, reg); \
-- 
1.7.9.5

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-05-14 14:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-14 12:10 [PATCH] drm/i915: prevent gt fifo count underflow Mika Kuoppala
2014-05-14 13:18 ` Mika Kuoppala
2014-05-14 13:35   ` Ville Syrjälä
2014-05-14 13:48     ` Chris Wilson
2014-05-14 14:22     ` [PATCH v3] " Mika Kuoppala

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.