Re: [PATCH 31/40] drm/i916: Init chv workarounds at render ring init

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jul 30, 2014 at 12:35:49PM +0000, Barbalho, Rafael wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of ville.syrjala@linux.intel.com
> > Sent: Saturday, June 28, 2014 12:04 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 31/40] drm/i916: Init chv workarounds at render
> > ring init
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > My bsw is an unhappy camper if we delay the workaround init until
> > init_clock_gating(). Move a bunch of it to the render ring init.
> > 
> > FIXME: need to do this for all platforms since some of the registers
> >        also get clobbered at reset. Just need to figure out which
> >        registers those actually are. This patch is based on a
> >        slightly educated guess, but verifying on actual hw would
> >        be a good idea. Also should maybe move the init_clock_gating
> >        earlier too since we set up a bunch of clock gating stuff
> >        there that might be important for a properly working GT.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c         | 40 +++++++--------------------------
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 40
> > +++++++++++++++++++++++++++++++++
> >  2 files changed, 48 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 346dced..158c3f5 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5720,6 +5720,10 @@ static void valleyview_init_clock_gating(struct
> > drm_device *dev)
> >  	 * in the reporting of vblank events.
> >  	 */
> >  	I915_WRITE(VLV_GUNIT_CLOCK_GATE, GCFG_DIS);
> > +
> > +	/* WaDisableDopClockGating:chv (pre-production hw) */
> > +	I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> > +		   GEN6_EU_TCUNIT_CLOCK_GATE_DISABLE);
> >  }
> 
> Did you really mean to add a cherryview workaround to valleyview?

Nope. Not sure what happened with this patch.

Maybe it's best to wait and see what Arun comes up for BDW and once
that's sorted we deal with CHV (and all the other platforms).

> 
> > 
> >  static void cherryview_init_clock_gating(struct drm_device *dev)
> > @@ -5730,49 +5734,21 @@ static void cherryview_init_clock_gating(struct
> > drm_device *dev)
> > 
> >  	I915_WRITE(MI_ARB_VLV,
> > MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE);
> > 
> > -	/* WaDisablePartialInstShootdown:chv */
> > -	I915_WRITE(GEN8_ROW_CHICKEN,
> > -
> > _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE));
> > -
> > -	/* WaDisableThreadStallDopClockGating:chv */
> > -	I915_WRITE(GEN8_ROW_CHICKEN,
> > -		   _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE));
> > -
> > -	/* WaVSRefCountFullforceMissDisable:chv */
> > -	/* WaDSRefCountFullforceMissDisable:chv */
> > -	I915_WRITE(GEN7_FF_THREAD_MODE,
> > -		   I915_READ(GEN7_FF_THREAD_MODE) &
> > -		   ~(GEN8_FF_DS_REF_CNT_FFME |
> > GEN7_FF_VS_REF_CNT_FFME));
> > -
> > -	/* WaDisableSemaphoreAndSyncFlipWait:chv */
> > -	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
> > -
> > _MASKED_BIT_ENABLE(GEN8_RC_SEMA_IDLE_MSG_DISABLE));
> > -
> >  	/* WaDisableCSUnitClockGating:chv */
> >  	I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> >  		   GEN6_CSUNIT_CLOCK_GATE_DISABLE);
> > 
> > +	/* WaDisableDopClockGating:chv (pre-production hw) */
> 
> Shouldn't this be WaDisableTCUnitClock gating?
> 
> > +	I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> > +		   GEN6_EU_TCUNIT_CLOCK_GATE_DISABLE);
> > +
> >  	/* WaDisableSDEUnitClockGating:chv */
> >  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
> >  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
> > 
> > -	/* WaDisableSamplerPowerBypass:chv (pre-production hw) */
> > -	I915_WRITE(HALF_SLICE_CHICKEN3,
> > -
> > _MASKED_BIT_ENABLE(GEN8_SAMPLER_POWER_BYPASS_DIS));
> > -
> >  	/* WaDisableGunitClockGating:chv (pre-production hw) */
> >  	I915_WRITE(VLV_GUNIT_CLOCK_GATE,
> > I915_READ(VLV_GUNIT_CLOCK_GATE) |
> >  		   GINT_DIS);
> > -
> > -	/* WaDisableFfDopClockGating:chv (pre-production hw) */
> > -	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
> > -
> > _MASKED_BIT_ENABLE(GEN8_FF_DOP_CLOCK_GATE_DISABLE));
> > -
> > -	/* WaDisableDopClockGating:chv (pre-production hw) */
> > -	I915_WRITE(GEN7_ROW_CHICKEN2,
> > -		   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
> > -	I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> > -		   GEN6_EU_TCUNIT_CLOCK_GATE_DISABLE);
> >  }
> > 
> >  static void g4x_init_clock_gating(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index ceb1295..9e81c28 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -615,6 +615,43 @@ err:
> >  	return ret;
> >  }
> > 
> 
> I think we can share the cherryview_init_workarounds functions with broadwell
> and just call it gen8_init_workarounds with a bit of code to just enable the
> cheryview functions. I'll mark down the workarounds that are shared and the ones
> that are not. It will also simplify the bdw clock gating init functions.
> 
> > +static void cherryview_init_workarounds(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	/* WaDisablePartialInstShootdown:chv */
> 
> Applies to bdw.
> 
> > +	I915_WRITE(GEN8_ROW_CHICKEN,
> > +
> > _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE));
> > +
> > +	/* WaDisableThreadStallDopClockGating:chv */
> 
> Applies to bdw.
> 
> > +	I915_WRITE(GEN8_ROW_CHICKEN,
> > +		   _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE));
> > +
> > +	/* WaVSRefCountFullforceMissDisable:chv */
> > +	/* WaDSRefCountFullforceMissDisable:chv */
> 
> Applies to bdw.
> 
> > +	I915_WRITE(GEN7_FF_THREAD_MODE,
> > +		   I915_READ(GEN7_FF_THREAD_MODE) &
> > +		   ~(GEN8_FF_DS_REF_CNT_FFME |
> > GEN7_FF_VS_REF_CNT_FFME));
> > +
> > +	/* WaDisableSemaphoreAndSyncFlipWait:chv */
> 
> Chv specific.
> 
> > +	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
> > +
> > _MASKED_BIT_ENABLE(GEN8_RC_SEMA_IDLE_MSG_DISABLE));
> > +
> > +	/* WaDisableSamplerPowerBypass:chv (pre-production hw) */
> 
> Chv specific.
> 
> > +	I915_WRITE(HALF_SLICE_CHICKEN3,
> > +
> > _MASKED_BIT_ENABLE(GEN8_SAMPLER_POWER_BYPASS_DIS));
> > +
> > +	/* WaDisableFfDopClockGating:chv (pre-production hw) */
> 
> Chv specific.
> 
> > +	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
> > +
> > _MASKED_BIT_ENABLE(GEN8_FF_DOP_CLOCK_GATE_DISABLE));
> > +
> > +	/* WaDisableDopClockGating:chv (pre-production hw) */
> 
> This first register write as applies to broadwell.
> 
> > +	I915_WRITE(GEN7_ROW_CHICKEN2,
> > +		   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
> 
> I think we need a split here and add WaDisableTCUnitClockGating. This also shows up in
> the init clock gating function. Do we need to have it in both places?  This second register
> write also only applies to chv.
> 
> > +	I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> > +		   GEN6_EU_TCUNIT_CLOCK_GATE_DISABLE);
> > +}
> > +
> >  static int init_render_ring(struct intel_engine_cs *ring)
> >  {
> >  	struct drm_device *dev = ring->dev;
> > @@ -670,6 +707,9 @@ static int init_render_ring(struct intel_engine_cs
> > *ring)
> >  	if (HAS_L3_DPF(dev))
> >  		I915_WRITE_IMR(ring, ~GT_PARITY_ERROR(dev));
> > 
> > +	if (IS_CHERRYVIEW(dev))
> 
> If we modify the function init_workaround functions then we change from IS_CHERRYVIEW
> to IS_GEN8.
> 
> Thanks,
> Raf
> 
> > +		cherryview_init_workarounds(dev);
> > +
> >  	return ret;
> >  }
> > 
> > --
> > 1.8.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC