[Intel-gfx] [PATCH] drm/i915: prevent gt fifo count underflow

[Chris Wilson]

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 at 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