| Message ID | 1400073482-15056-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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); \
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(-)