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

Mon Aug 19 11:18:48 UTC 2024

Hi Andi,

> -----Original Message-----
> From: Andi Shyti <andi.shyti at linux.intel.com>
> Sent: Wednesday, August 14, 2024 5:05 PM
> To: Gote, Nitin R <nitin.r.gote at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Nayana, Venkata Ramana
> <venkata.ramana.nayana at intel.com>; Harrison, John C
> <john.c.harrison at intel.com>; Wilson, Chris P <chris.p.wilson at intel.com>;
> Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Subject: Re: [PATCH v10] drm/i915: WA context support for L3flush
> 
> Hi Nitin,
> 
> I had a quick read through and just few comments for now.

Thank you for the review.
I just came to know from @Ewins, Jon in VLK-35222, that this WA is no longer required.
So, I will not spend time on this patch anymore as it is no longer required.

Br,
Nitin

> 
> 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[] = {