From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/2] drm/i915: Streamline VLV forcewake handling Date: Fri, 28 Feb 2014 16:51:16 +0200 Message-ID: <20140228145116.GQ3852@intel.com> References: <1393254129-438-1-git-send-email-ville.syrjala@linux.intel.com> <1393531641-12608-1-git-send-email-ville.syrjala@linux.intel.com> <53109CB0.7050805@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 3FEDFFB902 for ; Fri, 28 Feb 2014 06:51:23 -0800 (PST) Content-Disposition: inline In-Reply-To: <53109CB0.7050805@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: "S, Deepak" Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Feb 28, 2014 at 07:56:56PM +0530, S, Deepak wrote: > = > = > On 2/28/2014 1:37 AM, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrj=E4l=E4 > > > > It occured to me that when we're trying to wake up both render > > and media wells on VLV, we might end up calling the low level > > force_wake_get/put two times even though one call would be > > enough. Make that happen by figuring out which wells really > > need to be woken up based on the forcewake counts. > > > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 70 +++++++++++++++-------------= --------- > > 1 file changed, 29 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915= /intel_uncore.c > > index dacb751..4119ddc 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -251,16 +251,16 @@ void vlv_force_wake_get(struct drm_i915_private *= dev_priv, > > unsigned long irqflags; > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - if (FORCEWAKE_RENDER & fw_engine) { > > - if (dev_priv->uncore.fw_rendercount++ =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, > > - FORCEWAKE_RENDER); > > - } > > - if (FORCEWAKE_MEDIA & fw_engine) { > > - if (dev_priv->uncore.fw_mediacount++ =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, > > - FORCEWAKE_MEDIA); > > - } > > + > > + if (fw_engine & FORCEWAKE_RENDER && > > + dev_priv->uncore.fw_rendercount++ !=3D 0) > > + fw_engine &=3D ~FORCEWAKE_RENDER; > > + if (fw_engine & FORCEWAKE_MEDIA && > > + dev_priv->uncore.fw_mediacount++ !=3D 0) > > + fw_engine &=3D ~FORCEWAKE_MEDIA; > = > Should we add WARN_ON? I think it will help us if we have forcewake = > count mismatch? > = > Other than this. Patch looks good. > Reviewed-by:Deepak S I dropped the WARNs since we didn't have them on other platforms either. But if people think they might help, I'm not opposed to keeping them. > = > > + if (fw_engine) > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine); > > > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > } > > @@ -272,19 +272,15 @@ void vlv_force_wake_put(struct drm_i915_private *= dev_priv, > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > - if (FORCEWAKE_RENDER & fw_engine) { > > - WARN_ON(dev_priv->uncore.fw_rendercount =3D=3D 0); > > - if (--dev_priv->uncore.fw_rendercount =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_put(dev_priv, > > - FORCEWAKE_RENDER); > > - } > > + if (fw_engine & FORCEWAKE_RENDER && > > + --dev_priv->uncore.fw_rendercount !=3D 0) > > + fw_engine &=3D ~FORCEWAKE_RENDER; > > + if (fw_engine & FORCEWAKE_MEDIA && > > + --dev_priv->uncore.fw_mediacount !=3D 0) > > + fw_engine &=3D ~FORCEWAKE_MEDIA; > > > > - if (FORCEWAKE_MEDIA & fw_engine) { > > - WARN_ON(dev_priv->uncore.fw_mediacount =3D=3D 0); > > - if (--dev_priv->uncore.fw_mediacount =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_put(dev_priv, > > - FORCEWAKE_MEDIA); > > - } > > + if (fw_engine) > > + dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine); > > > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > } > > @@ -502,27 +498,19 @@ gen6_read##x(struct drm_i915_private *dev_priv, o= ff_t reg, bool trace) { \ > > static u##x \ > > vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace)= { \ > > unsigned fwengine =3D 0; \ > > - unsigned fwcount; \ > > REG_READ_HEADER(x); \ > > - if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) { \ > > - fwengine =3D FORCEWAKE_RENDER; \ > > - fwcount =3D dev_priv->uncore.fw_rendercount; \ > > - } \ > > - else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) { \ > > - fwengine =3D FORCEWAKE_MEDIA; \ > > - fwcount =3D dev_priv->uncore.fw_mediacount; \ > > + if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) { \ > > + if (dev_priv->uncore.fw_rendercount =3D=3D 0) \ > > + fwengine =3D FORCEWAKE_RENDER; \ > > + } else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) { \ > > + if (dev_priv->uncore.fw_mediacount =3D=3D 0) \ > > + fwengine =3D FORCEWAKE_MEDIA; \ > > } \ > > - if (fwengine !=3D 0) { \ > > - if (fwcount =3D=3D 0) \ > > - (dev_priv)->uncore.funcs.force_wake_get(dev_priv, \ > > - fwengine); \ > > - val =3D __raw_i915_read##x(dev_priv, reg); \ > > - if (fwcount =3D=3D 0) \ > > - (dev_priv)->uncore.funcs.force_wake_put(dev_priv, \ > > - fwengine); \ > > - } else { \ > > - val =3D __raw_i915_read##x(dev_priv, reg); \ > > - } \ > > + if (fwengine) \ > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \ > > + val =3D __raw_i915_read##x(dev_priv, reg); \ > > + if (fwengine) \ > > + dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \ > > REG_READ_FOOTER; \ > > } > > > > -- = Ville Syrj=E4l=E4 Intel OTC