Re: [PATCH] drm/i915/chv: Add new WA and remove pre-production ones

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

On Tue, Oct 28, 2014 at 03:48:24PM +0000, Arun Siluvery wrote:
> +WaForceEnableNonCoherent:chv
> +WaHdcDisableFetchWhenMasked:chv
> -WaDisableDopClockGating:chv
> -WaDisableSamplerPowerBypass:chv
> -WaDisableGunitClockGating:chv
> -WaDisableFfDopClockGating:chv
> -WaDisableDopClockGating:chv
> 
> WaDisablePartialInstShootdown:chv and
> WaDisableThreadStallDopClockGating:chv are related to the
> same register so combine them.

Please split into at least two patches (one to add new w/as and another to
remove old ones). Otherwise reverting is a pita in case we find that one
of the dropped w/as was actually still needed.

> 
> v2: Remove pre-production WA instead of restricting them
> based on revision id (Ville)
> 
> For: VIZ-4090
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         |  1 +
>  drivers/gpu/drm/i915/intel_pm.c         | 12 ------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 22 +++++++++++-----------
>  3 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 77fce96..9d39700 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5024,6 +5024,7 @@ enum punit_power_well {
>  /* GEN8 chicken */
>  #define HDC_CHICKEN0				0x7300
>  #define  HDC_FORCE_NON_COHERENT			(1<<4)
> +#define  HDC_DONOT_FETCH_MEM_WHEN_MASKED	(1<<11)
>  #define  HDC_FENCE_DEST_SLM_DISABLE		(1<<14)
>  
>  /* WaCatErrorRejectionIssue */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7a69eba..93db25f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5944,18 +5944,6 @@ static void cherryview_init_clock_gating(struct drm_device *dev)
>  	/* WaDisableSDEUnitClockGating:chv */
>  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
>  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
> -
> -	/* WaDisableGunitClockGating:chv (pre-production hw) */
> -	I915_WRITE(VLV_GUNIT_CLOCK_GATE, I915_READ(VLV_GUNIT_CLOCK_GATE) |
> -		   GINT_DIS);

OK

> -
> -	/* WaDisableFfDopClockGating:chv (pre-production hw) */
> -	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
> -		   _MASKED_BIT_ENABLE(GEN8_FF_DOP_CLOCK_GATE_DISABLE));

OK

> -
> -	/* WaDisableDopClockGating:chv (pre-production hw) */
> -	I915_WRITE(GEN6_UCGCTL1, I915_READ(GEN6_UCGCTL1) |
> -		   GEN6_EU_TCUNIT_CLOCK_GATE_DISABLE);

OK, I think. This was the weird w/a where it seemed hard to figure out
what it needed. Nothing in BSpec about needing this bit on chv.

>  }
>  
>  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 a8f72e8..368b20a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -788,20 +788,20 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* WaDisablePartialInstShootdown:chv */
> -	WA_SET_BIT_MASKED(GEN8_ROW_CHICKEN,
> -		  PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE);
> -
>  	/* WaDisableThreadStallDopClockGating:chv */
>  	WA_SET_BIT_MASKED(GEN8_ROW_CHICKEN,
> -		  STALL_DOP_GATING_DISABLE);
> -
> -	/* WaDisableDopClockGating:chv (pre-production hw) */
> -	WA_SET_BIT_MASKED(GEN7_ROW_CHICKEN2,
> -		  DOP_CLOCK_GATING_DISABLE);

OK, again the weird w/a but Bspec seems to agree at least.

> +			  PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE |
> +			  STALL_DOP_GATING_DISABLE);

Bspec says bit 5 is MBZ now, and yet the w/a database says it's
forever. And the hardware accepts 1 there so it's not like many other
MBZ bits that you can't set even if you try. Also Bspec has three
different definitions for this bit on gen8, all disagree with each other
and one definiton even manages to disagree with itself. And reading the
hsd stuff I'm not the only that has been confused by this, and yet I see
no conclusion there as to how this bit should be configured.

Oh well, I guess we can leave it set for now and maybe eventually
someone will figure out what we're supposed to do.

>  
> -	/* WaDisableSamplerPowerBypass:chv (pre-production hw) */
> -	WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN3,
> -		  GEN8_SAMPLER_POWER_BYPASS_DIS);

OK

> +	/* Use Force Non-Coherent whenever executing a 3D context. This is a
> +	 * workaround for a possible hang in the unlikely event a TLB
> +	 * invalidation occurs during a PSD flush.
> +	 */
> +	/* WaForceEnableNonCoherent:chv */
> +	/* WaHdcDisableFetchWhenMasked:chv */
> +	WA_SET_BIT_MASKED(HDC_CHICKEN0,
> +			  HDC_FORCE_NON_COHERENT |
> +			  HDC_DONOT_FETCH_MEM_WHEN_MASKED);

OK

Right, so split it up a bit and you can add

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

to the patches.

>  
>  	return 0;
>  }
> -- 
> 2.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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