From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: fix forcewake counts for gen8 Date: Tue, 18 Feb 2014 18:42:51 +0200 Message-ID: <20140218164251.GU3852@intel.com> References: <1392738534-25485-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 mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id CBDE1FAB8E for ; Tue, 18 Feb 2014 08:43:21 -0800 (PST) Content-Disposition: inline In-Reply-To: <1392738534-25485-1-git-send-email-mika.kuoppala@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Mika Kuoppala Cc: intel-gfx@lists.freedesktop.org, miku@iki.fi, Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Tue, Feb 18, 2014 at 05:48:54PM +0200, Mika Kuoppala wrote: > Generic driver code gets forcewake explicitly by gen6_gt_force_wake_get(), > which keeps force wake counts before accessing low level fw get. > However the underlying gen8 register write function access low level > accessors directly. This leads to nested fw get from hardware, causing > forcewake ack clear errors and/or hangs. > = > Fix this by checking the forcewake count in gen8 accessors. > Also implement read accessors for gen8 to gain symmetry for > shadowed register access. > = > References: https://bugs.freedesktop.org/show_bug.cgi?id=3D74007 > Signed-off-by: Mika Kuoppala > Cc: Ben Widawsky > --- > drivers/gpu/drm/i915/intel_uncore.c | 74 +++++++++++++++++++++++------= ------ > 1 file changed, 49 insertions(+), 25 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/i= ntel_uncore.c > index c628414..089feaa 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -498,6 +498,45 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_= t reg, bool trace) { \ > REG_READ_FOOTER; \ > } > = > +static const u32 gen8_shadowed_regs[] =3D { > + FORCEWAKE_MT, > + GEN6_RPNSWREQ, > + GEN6_RC_VIDEO_FREQ, > + RING_TAIL(RENDER_RING_BASE), > + RING_TAIL(GEN6_BSD_RING_BASE), > + RING_TAIL(VEBOX_RING_BASE), > + RING_TAIL(BLT_RING_BASE), > + /* TODO: Other registers are not yet used */ > +}; > + > +static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg) > +{ > + int i; > + for (i =3D 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++) > + if (reg =3D=3D gen8_shadowed_regs[i]) > + return true; > + > + return false; > +} > + > +#define __gen8_read(x) \ > +static u##x \ > +gen8_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) {= \ > + bool __needs_put =3D reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg);= \ This isn't right. Shadowed registers still need forcewake for reads. > + REG_READ_HEADER(x); \ > + __needs_put &=3D dev_priv->uncore.forcewake_count =3D=3D 0; \ This looks a bit funky. I'm not sure I understood the issue correctly, but it looks like just a failure to obey forcewake_count in gen8_write. So I'd just fix gen8_write. Something like this: #define __gen8_write(x) \ static void \ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool= trace) { \ - bool __needs_put =3D reg < 0x40000 && !is_gen8_shadowed(dev_priv, r= eg); \ REG_WRITE_HEADER; \ - if (__needs_put) { \ - dev_priv->uncore.funcs.force_wake_get(dev_priv, \ - FORCEWAKE_ALL); \ - } \ - __raw_i915_write##x(dev_priv, reg, val); \ - if (__needs_put) { \ - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ - FORCEWAKE_ALL); \ + if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \ + if (dev_priv->uncore.forcewake_count =3D=3D 0) \ + dev_priv->uncore.funcs.force_wake_get(dev_priv, \ + FORCEWAKE_ALL= ); \ + __raw_i915_write##x(dev_priv, reg, val); \ + if (dev_priv->uncore.forcewake_count =3D=3D 0) \ + dev_priv->uncore.funcs.force_wake_put(dev_priv, \ + FORCEWAKE_ALL= ); \ + } else { \ + __raw_i915_write##x(dev_priv, reg, val); \ } \ REG_WRITE_FOOTER; \ } I also noticed that gen6 doesn't increment/decrement the forcewake_count here. We can follow that rule here too since we have the uncore lock around the whole operation. I see the VLV code has the ++/--. I think we should either add them ++/-- everywhere, or remove them from the VLV code, just to make the code more uniform. -- = Ville Syrj=E4l=E4 Intel OTC