[PATCH v10] drm/i915: WA context support for L3flush

[Andi Shyti]

Hi Nitin,

I had a quick read through and just few comments for now.

On Tue, Aug 13, 2024 at 11:46:57AM +0530, Nitin Gote wrote:

...

> Another key requirement is to have this context dummy mapped so that the
> HW doesn't generate any PFs. This H/W issue may cause L3 cached GPUVAs
> which belong to previous context to get associated with the l3flush
> context. So, w/o dummy mapping, HW is expected to PF whenever these stale
> addresses are referenced.
> 
> This patch has been co-developed with John Harrison.

Please, remove this line.

> v2: CONTEXT_WA_L3FLUSH bit set calling function change (Chris)
>     Handled ce and ppgtt leaks (Chris)
> v3: s/setup_dummy_context/setup_wa_l3flush_context (JohnH)
>     Replace firmware wording with IFWI (JohnH)
>     Inplace of REG_BIT(31) use meaningfull BIT define (JohnH)
>     Replace few GUC #def with enum (JohnH)
> v4: Replace 'dummy/wa_l3flush' (JohnH)
> v5 (Tejas): For old IFWI, print warning and let driver proceed (Matt)
>             Adjust IS_DG2 check as G12 does not need this WA (Matt)
>             Use correct WA# (Matt)
>             Rename APIs to dg2 specific (Matt)
> v6 (Tejas): Drop ppgtt->vm ref right after context create (Chris)
>             Change variable to destroy context (Chris)
>             Use MI_LRI for multiple reg ops (Chris)
> v7 (Tejas): MTL does not have BCS0, handled it
> v8 (Tejas): Resolve recursive merge error
> v9 (Tejas): Use s/engine->uncore/engine->i915->uncore (Chris)
>             Modify no.of regs 4->2 for MI_LRI (Chris)
>             Unref ppgtt->vm only for err
>             Modify GEM_BUG_ON for dg2_10/11
>             Handle return value for l3flush context setup
> v10 (Nitin): Rebase this patch.

In which list have been all these versions sent?

> Cc: John Harrison <john.c.harrison at intel.com>

Replace this line with:

Co-developed-by: John Harrison <john.c.harrison at intel.com>
Signed-off-by: John Harrison <john.c.harrison at intel.com>

> Signed-off-by: Venkata Ramana Nayana <venkata.ramana.nayana at intel.com>
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> Signed-off-by: Nitin Gote <nitin.r.gote at intel.com>

...

>  #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
>  #define I915_GEM_HWS_GGTT_BIND		0x46
>  #define I915_GEM_HWS_GGTT_BIND_ADDR	(I915_GEM_HWS_GGTT_BIND * sizeof(u32))
> +#define I915_GEM_HWS_WA_L3FLUSH         0x48
> +#define I915_GEM_HWS_WA_L3FLUSH_ADDR	(I915_GEM_HWS_WA_L3FLUSH * sizeof(u32))

please, use tabs here.

>  #define I915_GEM_HWS_PXP		0x60
>  #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
>  #define I915_GEM_HWS_GSC		0x62

...

> +	/* BIT(31) unlockbit manage by IFWI */
> +	if (misccpctl & GEN12_DOP_CLOCK_GATE_LOCK) {
> +		drm_warn(&engine->i915->drm, "MISCCPCTL lockbit set, update IFWI\n");
> +		ret = -EPERM;
> +		return ret;

just return -EPERM.

> +	}
> +
> +	ppgtt = i915_ppgtt_create(engine->gt, 0);
> +	if (IS_ERR(ppgtt))
> +		return PTR_ERR(ppgtt);
> +
> +	ce = intel_engine_create_pinned_context(engine,
> +						&ppgtt->vm, SZ_4K,
> +						I915_GEM_HWS_WA_L3FLUSH_ADDR,
> +						&wa_l3flush, "wa_l3flush_context");
> +	if (IS_ERR(ce)) {
> +		/* Keep this vm isolated! */
> +		i915_vm_put(&ppgtt->vm);

Please, add this in the goto error path because...

> +		return PTR_ERR(ce);
> +	}
> +
> +	/* Ensure this context does not get registered for use as a real context */
> +	__set_bit(CONTEXT_WA_L3FLUSH, &ce->flags);
> +
> +	ret = intel_guc_assign_wa_guc_id(&engine->gt->uc.guc, ce);
> +	if (ret < 0)
> +		goto err;

... you are missing it here

> +	engine->wa_l3flush_context = ce;
> +	i915_vm_put(ce->vm);
> +	return 0;
> +
> +err:
> +	intel_engine_destroy_pinned_context(ce);
> +	return ret;
> +}

...

>  #define   GEN8_DOP_CLOCK_GATE_CFCLK_ENABLE	(1 << 2)
>  #define   GEN8_DOP_CLOCK_GATE_GUC_ENABLE	(1 << 4)
>  #define   GEN8_DOP_CLOCK_GATE_MEDIA_ENABLE	(1 << 6)
> +#define   GEN12_DOP_CLOCK_GATE_LOCK             REG_BIT(31) /* bits[0, 31] RO */

use tabs

>  
>  #define GEN8_UCGCTL6				_MMIO(0x9430)
>  #define   GEN8_GAPSUNIT_CLOCK_GATE_DISABLE	(1 << 24)

...

> +/*
> + * Global workaround keys.
> + */
> +enum  {
> +	GUC_WORKAROUND_KLV_ID_COPY_ENGINE_SECURITY_WA = 0x301,
> +};

are we expecting more keys? Why a single element enum?

> +
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index 7995f059f30d..b981be11a59c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -834,10 +834,40 @@ static void guc_waklv_enable_simple(struct intel_guc *guc,
>  	*remain -= size;
>  }
>  
> +/* Wa_14015997824: DG2 */

does it make sense to put this on a different patch?

Andi

> +static void guc_waklv_init_bcs(struct intel_guc *guc, struct intel_context *dummy_ce)
> +{
> +	u32 offset, addr_ggtt, alloc_size, real_size;
> +	u32 klv_entry[] = {