[Intel-gfx] [PATCH] drm/i915: RMW register cycles considered evil

[Daniel Vetter]

On Mon, Jul 06, 2015 at 01:46:19PM +0100, Damien Lespiau wrote:
> On Mon, Jul 06, 2015 at 02:42:02PM +0200, Daniel Vetter wrote:
> > Especially for workarounds which is stuff that's almost impossible to
> > verify: The initial state from the firmware on boot-up and after
> > resume could be different, which will hide bugs when we do an RMW
> > cycle.
> > 
> > Hence never do them, and if it's required we need a special mask.
> > 
> > Cc: Damien Lespiau <damien.lespiau at intel.com>
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Nick Hoath <nicholas.hoath at intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> Eeek. Let's take the problem the other way around: have you verified
> it's OK to zero all those other fields?

Nope, but it's what we do for other workarounds (e.g. the ones we load
through the rings except for one case in the cxt switch wa) and on other
platforms. And in general we've moved away from RMW wherever we can since
it had too much surprises.

It's really just something I spotted while stumbling over a w/a patch for
hsw that we never merged - I don't like the inconsistency. And it has
bitten us in the past.

And yes I haven't done the audit here, but the fact that you suggest we
need one kind proves my point ;-)
-Daniel

> 
> -- 
> Damien
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 166ae51f5a5b..565f78d6a21d 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -57,7 +57,7 @@ static void gen9_init_clock_gating(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	/* WaEnableLbsSlaRetryTimerDecrement:skl */
> > -	I915_WRITE(BDW_SCRATCH1, I915_READ(BDW_SCRATCH1) |
> > +	I915_WRITE(BDW_SCRATCH1,
> >  		   GEN9_LBS_SLA_RETRY_TIMER_DECREMENT_ENABLE);
> >  }
> >  
> > @@ -72,18 +72,18 @@ static void skl_init_clock_gating(struct drm_device *dev)
> >  		 * WaDisableSDEUnitClockGating:skl
> >  		 * WaSetGAPSunitClckGateDisable:skl
> >  		 */
> > -		I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
> > +		I915_WRITE(GEN8_UCGCTL6,
> >  			   GEN8_GAPSUNIT_CLOCK_GATE_DISABLE |
> >  			   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
> >  
> >  		/* WaDisableVFUnitClockGating:skl */
> > -		I915_WRITE(GEN6_UCGCTL2, I915_READ(GEN6_UCGCTL2) |
> > +		I915_WRITE(GEN6_UCGCTL2,
> >  			   GEN6_VFUNIT_CLOCK_GATE_DISABLE);
> >  	}
> >  
> >  	if (INTEL_REVID(dev) <= SKL_REVID_D0) {
> >  		/* WaDisableHDCInvalidation:skl */
> > -		I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) |
> > +		I915_WRITE(GAM_ECOCHK,
> >  			   BDW_DISABLE_HDC_INVALIDATION);
> >  
> >  		/* WaDisableChickenBitTSGBarrierAckForFFSliceCS:skl */
> > @@ -93,7 +93,7 @@ static void skl_init_clock_gating(struct drm_device *dev)
> >  
> >  	if (INTEL_REVID(dev) <= SKL_REVID_E0)
> >  		/* WaDisableLSQCROPERFforOCL:skl */
> > -		I915_WRITE(GEN8_L3SQCREG4, I915_READ(GEN8_L3SQCREG4) |
> > +		I915_WRITE(GEN8_L3SQCREG4,
> >  			   GEN8_LQSC_RO_PERF_DIS);
> >  }
> >  
> > @@ -109,12 +109,12 @@ static void bxt_init_clock_gating(struct drm_device *dev)
> >  	 * GEN8_HDCUNIT_CLOCK_GATE_DISABLE_HDCREQ applies on 3x6 GT SKUs only.
> >  	 */
> >  	 /* WaDisableSDEUnitClockGating:bxt */
> > -	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
> > +	I915_WRITE(GEN8_UCGCTL6,
> >  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE |
> >  		   GEN8_HDCUNIT_CLOCK_GATE_DISABLE_HDCREQ);
> >  
> >  	/* FIXME: apply on A0 only */
> > -	I915_WRITE(TILECTL, I915_READ(TILECTL) | TILECTL_TLBPF);
> > +	I915_WRITE(TILECTL, TILECTL_TLBPF);
> >  }
> >  
> >  static void i915_pineview_get_mem_freq(struct drm_device *dev)
> > -- 
> > 2.1.4
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch