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