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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Arun Siluvery <arun.siluvery@linux.intel.com> writes:

> 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.
>
> v2: behaviour of disable gather at set shader bit can be specified by
> two different registers, use a better option (Ben).
>

For me it looks like there are 2 orthogonal goals for this patch.

I think the actual workaround should be one patch, the resetting
of the set shader bit and the patch named accordingly.

Then the set shader initialization in a different patch,
if there is justification for it (that I have not managed yet
to find).

But lets concentrate on the workaround itself...

> 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         |  5 +++++
>  drivers/gpu/drm/i915/intel_lrc.c        |  8 ++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++
>  3 files changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8991cd5..8719a5a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1721,6 +1721,10 @@ enum skl_disp_power_wells {
>  #define   MEM_DISPLAY_TRICKLE_FEED_DISABLE (1<<2) /* 85x only */
>  #define FW_BLC		0x020d8
>  #define FW_BLC2		0x020dc
> +#define GEN7_RS_CHICKEN  0x20DC
> +#define   GEN9_RS_CHICKEN_DISABLE_GATHER_AT_SHADER (1<<2)
> +#define GEN7_FF_SLICE_CHICKEN1    0x20E0
> +#define   GEN9_PER_CTX_DISABLE_GATHER_CONTROL  (1<<15)
>  #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 +5840,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 */

I am thinking how we could also alert the reader that this workaround
needs to be revisited when it has been given a name.
By adding WaNoName:skl,bxt along with the comment above?

> +	if (IS_SKYLAKE(dev)) {

As Ben noted, documentation is rather sparse. But the reference
to previous problems with this bit being save/restored in wrong order,
we can conclude that this should be for BXT also.

> +		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));
> +	}
> +

The actual value of 'disable set shader' is orthogonal and beyond scope
of this Workaround so the rest should be strip out to different patch.

-Mika

>  	/* 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..5e8e5f9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -985,6 +985,17 @@ 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(GEN7_FF_SLICE_CHICKEN1,
> +				  GEN9_PER_CTX_DISABLE_GATHER_CONTROL);
> +		WA_SET_BIT_MASKED(GEN7_RS_CHICKEN,
> +				  GEN9_RS_CHICKEN_DISABLE_GATHER_AT_SHADER);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1068,6 +1079,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);
>  }
>  
> -- 
> 1.9.1
>
> _______________________________________________
> 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