Re: [PATCH v1 2/2] drm/i915:gen9: Add disable gather at set shader w/a

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

On 04/08/2015 00:21, Ben Widawsky wrote:
> On Mon, Aug 03, 2015 at 08:24:57PM +0100, Arun Siluvery wrote:
>> This WA is implemented in init_context as well as WA batch init.
>> There are also some dependent bits need to be set in other registers
>> for this to be complete.
>>
>> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>>   drivers/gpu/drm/i915/intel_lrc.c        |  8 ++++++++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 16 ++++++++++++++++
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 8991cd5..24b8bb9 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1720,7 +1720,9 @@ enum skl_disp_power_wells {
>>   #define   MEM_DISPLAY_A_TRICKLE_FEED_DISABLE (1<<2) /* 830/845 only */
>>   #define   MEM_DISPLAY_TRICKLE_FEED_DISABLE (1<<2) /* 85x only */
>>   #define FW_BLC		0x020d8
>> +#define   GEN9_DISABLE_GATHER_AT_SET_SHADER    (1<<7)
>>   #define FW_BLC2		0x020dc
>> +#define   GEN9_RS_CHICKEN_DISABLE_GATHER_AT_SHADER (1<<2)
>
> Neither of these belong here. BLC is for backlight. Create a new define if we
> don't have one.
>
> #define RS_CHICKEN		0x20dc
I thought of reusing existing define but created a new one as you suggested.

>
>>   #define FW_BLC_SELF	0x020e0 /* 915+ only */
>>   #define   FW_BLC_SELF_EN_MASK      (1<<31)
>>   #define   FW_BLC_SELF_FIFO_MASK    (1<<16) /* 945 only */
>> @@ -5836,6 +5838,7 @@ enum skl_disp_power_wells {
>>   # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC	((1<<10) | (1<<26))
>>   # define GEN9_RHWO_OPTIMIZATION_DISABLE		(1<<14)
>>   #define COMMON_SLICE_CHICKEN2			0x7014
>> +#define  GEN9_DISABLE_GATHER_SET_SHADER_SLICE   (1<<12)
>>   # define GEN8_CSC2_SBE_VUE_CACHE_CONSERVATIVE	(1<<0)
>>
>>   #define HIZ_CHICKEN					0x7018
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 9faad82..d3a03f3 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1292,6 +1292,14 @@ static int gen9_init_perctx_bb(struct intel_engine_cs *ring,
>>   	struct drm_device *dev = ring->dev;
>>   	uint32_t index = wa_ctx_start(wa_ctx, *offset, CACHELINE_DWORDS);
>>
>> +	/* WA to reset "disable gather at set shader slice" bit */
>> +	if (IS_SKYLAKE(dev)) {
>> +		wa_ctx_emit(batch, index, MI_LOAD_REGISTER_IMM(1));
>> +		wa_ctx_emit(batch, index, COMMON_SLICE_CHICKEN2);
>> +		wa_ctx_emit(batch, index,
>> +			    _MASKED_BIT_DISABLE(GEN9_DISABLE_GATHER_SET_SHADER_SLICE));
>> +	}
>> +
>
> Shouldn't this be for BXT as well? Also, why bother with the revid check below
> and not here?

spec says only SKL+

>
>>   	/* WaSetDisablePixMaskCammingAndRhwoInCommonSliceChicken:skl,bxt */
>>   	if ((IS_SKYLAKE(dev) && (INTEL_REVID(dev) <= SKL_REVID_B0)) ||
>>   	    (IS_BROXTON(dev) && (INTEL_REVID(dev) == BXT_REVID_A0))) {
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index dcd1b8f..4fc4b5e 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -985,6 +985,15 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>>   		tmp |= HDC_FORCE_CSR_NON_COHERENT_OVR_DISABLE;
>>   	WA_SET_BIT_MASKED(HDC_CHICKEN0, tmp);
>>
>> +	/* WA to gather at set shader - skl,bxt
>> +	 * These are dependent bits need to be set for the WA.
>> +	 */
>> +	if (IS_SKYLAKE(dev) && (INTEL_REVID(dev) > SKL_REVID_D0) ||
>> +	    (IS_BROXTON(dev) && INTEL_REVID(dev) > BXT_REVID_A0)) {
>> +		WA_SET_BIT_MASKED(FW_BLC, GEN9_DISABLE_GATHER_AT_SET_SHADER);
>> +		WA_SET_BIT_MASKED(FW_BLC2, GEN9_RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
>> +	}
>> +
>>   	return 0;
>>   }
>>
>> @@ -1068,6 +1077,13 @@ static int skl_init_workarounds(struct intel_engine_cs *ring)
>>   				  HDC_FENCE_DEST_SLM_DISABLE |
>>   				  HDC_BARRIER_PERFORMANCE_DISABLE);
>>
>> +	/* WA to Disable gather at set shader - skl
>> +	 * This bit needs to be reset in Per ctx WA batch and it is also
>> +	 * dependent on other bits in different register, all of them need
>> +	 * be set for the WA to be complete.
>> +	 */
>> +	WA_SET_BIT_MASKED(COMMON_SLICE_CHICKEN2, GEN9_DISABLE_GATHER_SET_SHADER_SLICE);
>> +
>>   	return skl_tune_iz_hashing(ring);
>>   }
>>
>
> I wouldn't set both 20dc, and 20d8, I am not sure what implication it has.
> Instead, set or read bit 15 of 0x20e0 and then just set one. To me, it seems
> like the best way to do this is to set 1<<15 of 0x20e0, and then use bit 2 of
> 0x20dc for the workaround. We don't need per context controls of something we
> have to disable always anyway.
>
changed it to use 0x20e0

regards
Arun


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