Hi Andi,
> -----Original Message-----
> From: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Sent: Wednesday, August 14, 2024 5:05 PM
> To: Gote, Nitin R <nitin.r.gote@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nayana, Venkata Ramana
> <venkata.ramana.nayana@xxxxxxxxx>; Harrison, John C
> <john.c.harrison@xxxxxxxxx>; Wilson, Chris P <chris.p.wilson@xxxxxxxxx>;
> Upadhyay, Tejas <tejas.upadhyay@xxxxxxxxx>
> 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@xxxxxxxxx>
>
> Replace this line with:
>
> Co-developed-by: John Harrison <john.c.harrison@xxxxxxxxx>
> Signed-off-by: John Harrison <john.c.harrison@xxxxxxxxx>
>
> > Signed-off-by: Venkata Ramana Nayana
> <venkata.ramana.nayana@xxxxxxxxx>
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@xxxxxxxxx>
> > Signed-off-by: Nitin Gote <nitin.r.gote@xxxxxxxxx>
>
> ...
>
> > #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[] = {
