[Intel-gfx] [PATCH v4 2/2] drm/i915: Implement WaProgramMgsrForL3BankSpecificMmioReads

[Tvrtko Ursulin]

On 26/03/2018 17:12, Yunwei Zhang wrote:
> L3Bank could be fused off in hardware for debug purpose, and it
> is possible that subslice is enabled while its corresponding L3Bank pairs
> are disabled. In such case, if MCR packet control register(0xFDC) is
> programed to point to a disabled bank pair, a MMIO read into L3Bank range
> will return 0 instead of correct values.
> 
> However, this is not going to be the case in any production silicon.
> Therefore, we only check at initialization and issue a warning should
> this really happen.
> 
> v2:
>   - use fls instead of find_last_bit (Chris)
>   - use is_power_of_2() instead of counting bit set (Chris)
> v3:
>   - rebase on latest tip
> v4:
>   - Added referecens (Mika)
> 
> References: HSDES#1405586840
> 
> Signed-off-by: Yunwei Zhang <yunwei.zhang at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h        |  4 ++++
>   drivers/gpu/drm/i915/intel_engine_cs.c | 18 ++++++++++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1bca695f..4f2f5e1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2691,6 +2691,10 @@ enum i915_power_well_id {
>   #define   GEN10_F2_SS_DIS_SHIFT		18
>   #define   GEN10_F2_SS_DIS_MASK		(0xf << GEN10_F2_SS_DIS_SHIFT)
>   
> +#define	GEN10_MIRROR_FUSE3		_MMIO(0x9118)
> +#define GEN10_L3BANK_PAIR_COUNT     4
> +#define GEN10_L3BANK_MASK   0x0F
> +
>   #define GEN8_EU_DISABLE0		_MMIO(0x9134)
>   #define   GEN8_EU_DIS0_S0_MASK		0xffffff
>   #define   GEN8_EU_DIS0_S1_SHIFT		24
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index cc19e0a..3eb763c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -811,7 +811,25 @@ static u32 calculate_mcr(u32 mcr, struct drm_i915_private *dev_priv)
>   static void wa_init_mcr(struct drm_i915_private *dev_priv)
>   {
>   	u32 mcr;
> +	u32 fuse3;
> +	const struct sseu_dev_info *sseu = &(INTEL_SSEU(dev_priv));
> +	u32 slice;

fuse3 and slice can be moved into the is_power_of_2 block below.

>   
> +	/* If more than one slice are enabled, L3Banks should be all enabled */
> +	if (is_power_of_2(sseu->slice_mask)) {
> +		/*
> +		 * WaProgramMgsrForL3BankSpecificMmioReads:
> +		 * read FUSE3 for enabled L3 Bank IDs, if L3 Bank matches
> +		 * enabled subslice, no need to redirect MCR packet
> +		 */
> +		slice = fls(sseu->slice_mask);
> +		fuse3 = I915_READ(GEN10_MIRROR_FUSE3);
> +		if (WARN_ON(!((fuse3 & GEN10_L3BANK_MASK)
> +			       & ((sseu->subslice_mask[slice]
> +			       | sseu->subslice_mask[slice]>>GEN10_L3BANK_PAIR_COUNT)

Spaces around operators to satisfy checkpatch.

> +			       & GEN10_L3BANK_MASK))))

Whole conditional is a bit hard to read (maybe it is just me!) so maybe 
store sseu->subslice_mask[slice] to a local? Not sure if that would make 
it better.

> +			DRM_WARN("Production silicon should have matched L3Bank and subslice enabled\n");

Aren't both WARN_ON and DRM_WARN an overkill? One should be enough I hope.

> +	}
>   	mcr = I915_READ(GEN8_MCR_SELECTOR);
>   	mcr = calculate_mcr(mcr, dev_priv);
>   	I915_WRITE(GEN8_MCR_SELECTOR, mcr);
> 

Regards,

Tvrtko