Re: [PATCH 2/5] drm/i915: Read FORCEWAKE registers with I915_READ_FW()

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/i915: Read FORCEWAKE registers with I915_READ_FW()
Date: Thu, 22 Oct 2015 16:50:15 +0300	[thread overview]
Message-ID: <20151022135015.GS26517@intel.com> (raw)
In-Reply-To: <20151022133733.GZ16848@phenom.ffwll.local>

On Thu, Oct 22, 2015 at 03:37:33PM +0200, Daniel Vetter wrote:
> On Thu, Oct 22, 2015 at 03:34:57PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Change FORCEWAKE & co. reads for the error state to use I915_READ_FW().
> > Reading a FORCEWAKE register using a function that can frob forcewake
> > just seems wrong.
> > 
> > There is a check to skip grabbing the forcewake for accessing FORCEWAKE
> > in intel_uncore.c, but there's no such check for FORCEWAKE_MT. So no
> > idea what is currently happening with FORCEWAKE_MT reads. FORCEWAKE_VLV
> > is fortunately outside the forcewake range anyway, so no actual issue
> > with that one.
> > 
> > So let's just make the rule that you can access FORCEWAKE registers with
> 
> s/can/cannot/ I presume?

Yes.

> With that changed
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > the normal I915_READ() stuff, and we can drop the extra FORCEWAKE check
> > from NEEDS_FORCEWAKE(). While at it use NEEDS_FORCEWAKE() on BDW, where
> > it was skipped for whatever bikeshed reason that I've already forgotten.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 6 +++---
> >  drivers/gpu/drm/i915/intel_uncore.c   | 5 ++---
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 2f04e4f..de86f26 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1181,7 +1181,7 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
> >  	if (IS_VALLEYVIEW(dev)) {
> >  		error->gtier[0] = I915_READ(GTIER);
> >  		error->ier = I915_READ(VLV_IER);
> > -		error->forcewake = I915_READ(FORCEWAKE_VLV);
> > +		error->forcewake = I915_READ_FW(FORCEWAKE_VLV);
> >  	}
> >  
> >  	if (IS_GEN7(dev))
> > @@ -1193,14 +1193,14 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
> >  	}
> >  
> >  	if (IS_GEN6(dev)) {
> > -		error->forcewake = I915_READ(FORCEWAKE);
> > +		error->forcewake = I915_READ_FW(FORCEWAKE);
> >  		error->gab_ctl = I915_READ(GAB_CTL);
> >  		error->gfx_mode = I915_READ(GFX_MODE);
> >  	}
> >  
> >  	/* 2: Registers which belong to multiple generations */
> >  	if (INTEL_INFO(dev)->gen >= 7)
> > -		error->forcewake = I915_READ(FORCEWAKE_MT);
> > +		error->forcewake = I915_READ_FW(FORCEWAKE_MT);
> >  
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		error->derrmr = I915_READ(DERRMR);
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 8dfeac9..dca0979 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -513,8 +513,7 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
> >  }
> >  
> >  /* We give fast paths for the really cool registers */
> > -#define NEEDS_FORCE_WAKE(reg) \
> > -	 ((reg) < 0x40000 && (reg) != FORCEWAKE)
> > +#define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
> >  
> >  #define REG_RANGE(reg, start, end) ((reg) >= (start) && (reg) < (end))
> >  
> > @@ -918,7 +917,7 @@ static void \
> >  gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> >  	GEN6_WRITE_HEADER; \
> >  	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
> > -	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) \
> > +	if (NEEDS_FORCE_WAKE(reg) && !is_gen8_shadowed(dev_priv, reg)) \
> >  		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> >  	__raw_i915_write##x(dev_priv, reg, val); \
> >  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > -- 
> > 2.4.9
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx