From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: prevent gt fifo count underflow Date: Wed, 14 May 2014 16:35:42 +0300 Message-ID: <20140514133542.GL18465@intel.com> References: <1400069454-6945-1-git-send-email-mika.kuoppala@intel.com> <1400073482-15056-1-git-send-email-mika.kuoppala@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 86B9E6E04F for ; Wed, 14 May 2014 06:35:51 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1400073482-15056-1-git-send-email-mika.kuoppala@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Mika Kuoppala Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > --- > 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/i= ntel_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_i= 915_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 =3D 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 =3D __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIE= S_MASK; > } > if (WARN_ON(loop < 0 && fifo <=3D 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 <=3D GT_FIFO_NUM_RESERVED_ENTRIES'. > - ++ret; > + return true; > dev_priv->uncore.fifo_count =3D 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, of= f_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, bo= ol trace) { \ > - u32 __fifo_ret =3D 0; \ > + bool __fifo_failed =3D false; \ > REG_WRITE_HEADER; \ > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > - __fifo_ret =3D __gen6_gt_wait_for_fifo(dev_priv); \ > + __fifo_failed =3D __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, of= f_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, boo= l trace) { \ > - u32 __fifo_ret =3D 0; \ > + bool __fifo_failed =3D false; \ > REG_WRITE_HEADER; \ > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > - __fifo_ret =3D __gen6_gt_wait_for_fifo(dev_priv); \ > + __fifo_failed =3D __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=E4l=E4 Intel OTC