From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915/bdw: Don't use forcewake needlessly Date: Tue, 10 Dec 2013 23:30:10 +0100 Message-ID: <20131210223010.GU9804@phenom.ffwll.local> References: <1386681892-31586-1-git-send-email-ville.syrjala@linux.intel.com> <20131210194716.GB27093@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 87D6CFA470 for ; Tue, 10 Dec 2013 14:29:20 -0800 (PST) Received: by mail-ee0-f49.google.com with SMTP id c41so2527339eek.8 for ; Tue, 10 Dec 2013 14:29:19 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131210194716.GB27093@bwidawsk.net> 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: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Dec 10, 2013 at 11:47:17AM -0800, Ben Widawsky wrote: > On Tue, Dec 10, 2013 at 03:24:52PM +0200, ville.syrjala@linux.intel.com w= rote: > > From: Ville Syrj=E4l=E4 > > = > > Not all registers need forcewake even if they're not shadowed. > > Add the missing NEEDS_FORCE_WAKE() check to gen8_writeX() to > > avoid needless forcewake usage when writing eg. display > > registers. > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > I didn't test this at all, so be warned. > > = > > drivers/gpu/drm/i915/intel_uncore.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915= /intel_uncore.c > > index e63658e..78fe8d6 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -616,7 +616,8 @@ static bool is_gen8_shadowed(struct drm_i915_privat= e *dev_priv, u32 reg) > > #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 !is_gen8_shadowed(dev_priv, reg); \ > > + bool __needs_put =3D NEEDS_FORCE_WAKE(dev_priv, reg) && \ > > + !is_gen8_shadowed(dev_priv, reg); \ > > REG_WRITE_HEADER; \ > > if (__needs_put) { \ > > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > = > I would prefer if this was simply checking the display offset, as > opposed to NEEDS_FORCE_WAKE - since if someone adds to the macro we run > the risk of a potentially hard to track down bug. Yeah, I agree that an explicit check would be better since the forcewake stuff works a bit different on gen8+ -Daniel > = > In any case, the patch is functionally correct, and I'd prefer the > change, but up to you: > Reviewed-by: Ben Widawsky > = > > -- = > > 1.8.3.2 > > = > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > -- = > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch