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

["Siluvery, Arun" <arun.siluvery@linux.intel.com>]

On 28/10/2014 17:06, Ville Syrjälä wrote:
> 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.
>

I thought of doing that but then combined them as these are early 
pre-production ones and we may not need them in future but I agree 
splitting them helps in reverting them if required.

>>
>> 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.
>
I am using w/a database as reference and it says forever, spec seems to 
disagree but probably not yet updated.

regards
Arun

>>
>> -	/* 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
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx