* [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.