Re: [PATCH v12 3/4] drm/i915: Add WABB blit for Wa_16018031267 / Wa_16018063123

[Nirmoy Das <nirmoy.das@xxxxxxxxxxxxxxx>]

On 10/18/2023 3:24 PM, Andrzej Hajda wrote:
> On 20.09.2023 23:07, Jonathan Cavitt wrote:
> > Apply WABB blit for Wa_16018031267 / Wa_16018063123.
> > Additionally, update the lrc selftest to exercise the new
> > WABB changes.
> >
> > Co-developed-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx> > ---
> >   drivers/gpu/drm/i915/gt/intel_engine_regs.h |   3 +
> >   drivers/gpu/drm/i915/gt/intel_gt.h          |   4 +
> >   drivers/gpu/drm/i915/gt/intel_gt_types.h    |   2 +
> >   drivers/gpu/drm/i915/gt/intel_lrc.c         | 100 +++++++++++++++++++-
> >   drivers/gpu/drm/i915/gt/selftest_lrc.c      |  65 +++++++++----
> >   5 files changed, 153 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_regs.h 
> > b/drivers/gpu/drm/i915/gt/intel_engine_regs.h
> > index fdd4ddd3a978a..b8618ee3e3041 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_regs.h
> > @@ -118,6 +118,9 @@
> >   #define   CCID_EXTENDED_STATE_RESTORE        BIT(2)
> >   #define   CCID_EXTENDED_STATE_SAVE        BIT(3)
> >   #define RING_BB_PER_CTX_PTR(base)        _MMIO((base) + 0x1c0) /* 
> > gen8+ */
> > +#define   PER_CTX_BB_FORCE            BIT(2)
> > +#define   PER_CTX_BB_VALID            BIT(0)
> > +
> >   #define RING_INDIRECT_CTX(base)            _MMIO((base) + 0x1c4) /* 
> > gen8+ */
> >   #define RING_INDIRECT_CTX_OFFSET(base)        _MMIO((base) + 0x1c8) 
> > /* gen8+ */
> >   #define ECOSKPD(base)                _MMIO((base) + 0x1d0)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h 
> > b/drivers/gpu/drm/i915/gt/intel_gt.h
> > index 239848bcb2a42..40cc0005dd735 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> > @@ -83,6 +83,10 @@ struct drm_printer;
> >             ##__VA_ARGS__);                    \
> >   } while (0)
> >   +#define NEEDS_FASTCOLOR_BLT_WABB(engine) ( \
> > +    IS_GFX_GT_IP_RANGE(engine->gt, IP_VER(12, 55), IP_VER(12, 71)) && \
> > +    engine->class == COPY_ENGINE_CLASS)
> > +
> >   static inline bool gt_is_root(struct intel_gt *gt)
> >   {
> >       return !gt->info.id;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h 
> > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index def7dd0eb6f19..4917633f299dd 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -307,6 +307,8 @@ enum intel_gt_scratch_field {
> >         /* 8 bytes */
> >       INTEL_GT_SCRATCH_FIELD_COHERENTL3_WA = 256,
> > +
> > +    INTEL_GT_SCRATCH_FIELD_DUMMY_BLIT = 384,
> >   };
> >     #define intel_gt_support_legacy_fencing(gt) 
> > ((gt)->ggtt->num_fences > 0)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c 
> > b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index eaf66d9031665..db7088abeef38 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -828,6 +828,18 @@ lrc_ring_indirect_offset_default(const struct 
> > intel_engine_cs *engine)
> >       return 0;
> >   }
> >   +static void
> > +lrc_setup_bb_per_ctx(u32 *regs,
> > +             const struct intel_engine_cs *engine,
> > +             u32 ctx_bb_ggtt_addr)
> > +{
> > +    GEM_BUG_ON(lrc_ring_wa_bb_per_ctx(engine) == -1);
> > +    regs[lrc_ring_wa_bb_per_ctx(engine) + 1] =
> > +        ctx_bb_ggtt_addr |
> > +        PER_CTX_BB_FORCE |
> > +        PER_CTX_BB_VALID;
> > +}
> > +
> >   static void
> >   lrc_setup_indirect_ctx(u32 *regs,
> >                  const struct intel_engine_cs *engine,
> > @@ -1020,7 +1032,13 @@ static u32 context_wa_bb_offset(const struct 
> > intel_context *ce)
> >       return PAGE_SIZE * ce->wa_bb_page;
> >   }
> >   -static u32 *context_indirect_bb(const struct intel_context *ce)
> > +/*
> > + * per_ctx below determines which WABB section is used.
> > + * When true, the function returns the location of the
> > + * PER_CTX_BB.  When false, the function returns the
> > + * location of the INDIRECT_CTX.
> > + */
> > +static u32 *context_wabb(const struct intel_context *ce, bool per_ctx)
> >   {
> >       void *ptr;
> >   @@ -1029,6 +1047,7 @@ static u32 *context_indirect_bb(const struct 
> > intel_context *ce)
> >       ptr = ce->lrc_reg_state;
> >       ptr -= LRC_STATE_OFFSET; /* back to start of context image */
> >       ptr += context_wa_bb_offset(ce);
> > +    ptr += per_ctx ? PAGE_SIZE : 0;
> >         return ptr;
> >   }
> > @@ -1105,7 +1124,8 @@ __lrc_alloc_state(struct intel_context *ce, 
> > struct intel_engine_cs *engine)
> >         if (GRAPHICS_VER(engine->i915) >= 12) {
> >           ce->wa_bb_page = context_size / PAGE_SIZE;
> > -        context_size += PAGE_SIZE;
> > +        /* INDIRECT_CTX and PER_CTX_BB need separate pages. */
> > +        context_size += PAGE_SIZE * 2;
> >       }
> >         if (intel_context_is_parent(ce) && 
> > intel_engine_uses_guc(engine)) {
> > @@ -1407,12 +1427,85 @@ gen12_emit_indirect_ctx_xcs(const struct 
> > intel_context *ce, u32 *cs)
> >       return gen12_emit_aux_table_inv(ce->engine, cs);
> >   }
> >   +static u32 *xehp_emit_fastcolor_blt_wabb(const struct 
> > intel_context *ce, u32 *cs)
> > +{
> > +    struct intel_gt *gt = ce->engine->gt;
> > +    int mocs = gt->mocs.uc_index << 1;
> > +
> > +    /**
> > +     * Wa_16018031267 / Wa_16018063123 requires that SW forces the
> > +     * main copy engine arbitration into round robin mode.  We
> > +     * additionally need to submit the following WABB blt command
> > +     * to produce 4 subblits with each subblit generating 0 byte

It is been a while I looked into the WA. I don't think this can be 0 byte.


> > +     * write requests as WABB:
> > +     *
> > +     * XY_FASTCOLOR_BLT
> > +     *  BG0    -> 5100000E
> > +     *  BG1    -> 0000003F (Dest pitch)
> > +     *  BG2    -> 00000000 (X1, Y1) = (0, 0)
> > +     *  BG3    -> 00040001 (X2, Y2) = (1, 4)
> > +     *  BG4    -> scratch
> > +     *  BG5    -> scratch
> > +     *  BG6-12 -> 00000000
> > +     *  BG13   -> 20004004 (Surf. Width= 2,Surf. Height = 5 )
> > +     *  BG14   -> 00000010 (Qpitch = 4)
> > +     *  BG15   -> 00000000
> > +     */
> > +    *cs++ = XY_FAST_COLOR_BLT_CMD | (16 - 2);
> > +    *cs++ = FIELD_PREP(XY_FAST_COLOR_BLT_MOCS_MASK, mocs) | 0x3f;
> > +    *cs++ = 0;
> > +    *cs++ = 4 << 16 | 1;
> > +    *cs++ = lower_32_bits(ce->vm->rsvd.start);
> > +    *cs++ = upper_32_bits(ce->vm->rsvd.start);


If my above doubts holds then this address should exist so the copy 
engine can do a copy operation. Now sure how this will

work out if this is with NULL_PTE.

Andrzej, could you please validate this once.


Regards,

Nirmoy


> > +    *cs++ = 0;
> > +    *cs++ = 0;
> > +    *cs++ = 0;
> > +    *cs++ = 0;
> > +    *cs++ = 0;
> > +    *cs++ = 0;
> > +    *cs++ = 0;
> > +    *cs++ = 0x20004004;
> > +    *cs++ = 0x10;
> > +    *cs++ = 0;
> > +
> > +    return cs;
> > +}
> > +
> > +static u32 *
> > +xehp_emit_per_ctx_bb(const struct intel_context *ce, u32 *cs)
> > +{
> > +    /* Wa_16018031267, Wa_16018063123 */
> > +    if (NEEDS_FASTCOLOR_BLT_WABB(ce->engine))
> > +        cs = xehp_emit_fastcolor_blt_wabb(ce, cs);
> > +
> > +    return cs;
> > +}
> > +
> > +static void
> > +setup_per_ctx_bb(const struct intel_context *ce,
> > +         const struct intel_engine_cs *engine,
> > +         u32 *(*emit)(const struct intel_context *, u32 *))
> > +{
> > +    /* Place PER_CTX_BB on next page after INDIRECT_CTX */
> > +    u32 * const start = context_wabb(ce, true);
> > +    u32 *cs;
> > +
> > +    cs = emit(ce, start);
> > +
> > +    /* PER_CTX_BB must manually terminate */
> > +    *cs++ = MI_BATCH_BUFFER_END;
> > +
> > +    GEM_BUG_ON(cs - start > I915_GTT_PAGE_SIZE / sizeof(*cs));
> > +    lrc_setup_bb_per_ctx(ce->lrc_reg_state, engine,
> > +                 lrc_indirect_bb(ce) + PAGE_SIZE);
> > +}
> > +
> >   static void
> >   setup_indirect_ctx_bb(const struct intel_context *ce,
> >                 const struct intel_engine_cs *engine,
> >                 u32 *(*emit)(const struct intel_context *, u32 *))
> >   {
> > -    u32 * const start = context_indirect_bb(ce);
> > +    u32 * const start = context_wabb(ce, false);
> >       u32 *cs;
> >         cs = emit(ce, start);
> > @@ -1511,6 +1604,7 @@ u32 lrc_update_regs(const struct intel_context 
> > *ce,
> >           /* Mutually exclusive wrt to global indirect bb */
> >           GEM_BUG_ON(engine->wa_ctx.indirect_ctx.size);
> >           setup_indirect_ctx_bb(ce, engine, fn);
> > +        setup_per_ctx_bb(ce, engine, xehp_emit_per_ctx_bb);
> >       }
> >         return lrc_descriptor(ce) | CTX_DESC_FORCE_RESTORE;
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c 
> > b/drivers/gpu/drm/i915/gt/selftest_lrc.c
>
> Looks OK.
> Shall selftest be in the same commit, ie is spilting breaking sth?
> If not maybe separate commit would be better.
>
> Regards
> Andrzej
>
>
> > index 5f826b6dcf5d6..e17b8777d21dc 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> > @@ -1555,7 +1555,7 @@ static int live_lrc_isolation(void *arg)
> >       return err;
> >   }
> >   -static int indirect_ctx_submit_req(struct intel_context *ce)
> > +static int wabb_ctx_submit_req(struct intel_context *ce)
> >   {
> >       struct i915_request *rq;
> >       int err = 0;
> > @@ -1579,7 +1579,8 @@ static int indirect_ctx_submit_req(struct 
> > intel_context *ce)
> >   #define CTX_BB_CANARY_INDEX  (CTX_BB_CANARY_OFFSET / sizeof(u32))
> >     static u32 *
> > -emit_indirect_ctx_bb_canary(const struct intel_context *ce, u32 *cs)
> > +emit_wabb_ctx_canary(const struct intel_context *ce,
> > +             u32 *cs, bool per_ctx)
> >   {
> >       *cs++ = MI_STORE_REGISTER_MEM_GEN8 |
> >           MI_SRM_LRM_GLOBAL_GTT |
> > @@ -1587,26 +1588,43 @@ emit_indirect_ctx_bb_canary(const struct 
> > intel_context *ce, u32 *cs)
> >       *cs++ = i915_mmio_reg_offset(RING_START(0));
> >       *cs++ = i915_ggtt_offset(ce->state) +
> >           context_wa_bb_offset(ce) +
> > -        CTX_BB_CANARY_OFFSET;
> > +        CTX_BB_CANARY_OFFSET +
> > +        (per_ctx ? PAGE_SIZE : 0);
> >       *cs++ = 0;
> >         return cs;
> >   }
> >   +static u32 *
> > +emit_indirect_ctx_bb_canary(const struct intel_context *ce, u32 *cs)
> > +{
> > +    return emit_wabb_ctx_canary(ce, cs, false);
> > +}
> > +
> > +static u32 *
> > +emit_per_ctx_bb_canary(const struct intel_context *ce, u32 *cs)
> > +{
> > +    return emit_wabb_ctx_canary(ce, cs, true);
> > +}
> > +
> >   static void
> > -indirect_ctx_bb_setup(struct intel_context *ce)
> > +wabb_ctx_setup(struct intel_context *ce, bool per_ctx)
> >   {
> > -    u32 *cs = context_indirect_bb(ce);
> > +    u32 *cs = context_wabb(ce, per_ctx);
> >         cs[CTX_BB_CANARY_INDEX] = 0xdeadf00d;
> >   -    setup_indirect_ctx_bb(ce, ce->engine, 
> > emit_indirect_ctx_bb_canary);
> > +    if (per_ctx)
> > +        setup_per_ctx_bb(ce, ce->engine, emit_per_ctx_bb_canary);
> > +    else
> > +        setup_indirect_ctx_bb(ce, ce->engine, 
> > emit_indirect_ctx_bb_canary);
> >   }
> >   -static bool check_ring_start(struct intel_context *ce)
> > +static bool check_ring_start(struct intel_context *ce, bool per_ctx)
> >   {
> >       const u32 * const ctx_bb = (void *)(ce->lrc_reg_state) -
> > -        LRC_STATE_OFFSET + context_wa_bb_offset(ce);
> > +        LRC_STATE_OFFSET + context_wa_bb_offset(ce) +
> > +        (per_ctx ? PAGE_SIZE : 0);
> >         if (ctx_bb[CTX_BB_CANARY_INDEX] == 
> > ce->lrc_reg_state[CTX_RING_START])
> >           return true;
> > @@ -1618,21 +1636,21 @@ static bool check_ring_start(struct 
> > intel_context *ce)
> >       return false;
> >   }
> >   -static int indirect_ctx_bb_check(struct intel_context *ce)
> > +static int wabb_ctx_check(struct intel_context *ce, bool per_ctx)
> >   {
> >       int err;
> >   -    err = indirect_ctx_submit_req(ce);
> > +    err = wabb_ctx_submit_req(ce);
> >       if (err)
> >           return err;
> >   -    if (!check_ring_start(ce))
> > +    if (!check_ring_start(ce, per_ctx))
> >           return -EINVAL;
> >         return 0;
> >   }
> >   -static int __live_lrc_indirect_ctx_bb(struct intel_engine_cs *engine)
> > +static int __lrc_wabb_ctx(struct intel_engine_cs *engine, bool per_ctx)
> >   {
> >       struct intel_context *a, *b;
> >       int err;
> > @@ -1667,14 +1685,14 @@ static int __live_lrc_indirect_ctx_bb(struct 
> > intel_engine_cs *engine)
> >        * As ring start is restored apriori of starting the indirect 
> > ctx bb and
> >        * as it will be different for each context, it fits to this 
> > purpose.
> >        */
> > -    indirect_ctx_bb_setup(a);
> > -    indirect_ctx_bb_setup(b);
> > +    wabb_ctx_setup(a, per_ctx);
> > +    wabb_ctx_setup(b, per_ctx);
> >   -    err = indirect_ctx_bb_check(a);
> > +    err = wabb_ctx_check(a, per_ctx);
> >       if (err)
> >           goto unpin_b;
> >   -    err = indirect_ctx_bb_check(b);
> > +    err = wabb_ctx_check(b, per_ctx);
> >     unpin_b:
> >       intel_context_unpin(b);
> > @@ -1688,7 +1706,7 @@ static int __live_lrc_indirect_ctx_bb(struct 
> > intel_engine_cs *engine)
> >       return err;
> >   }
> >   -static int live_lrc_indirect_ctx_bb(void *arg)
> > +static int lrc_wabb_ctx(void *arg, bool per_ctx)
> >   {
> >       struct intel_gt *gt = arg;
> >       struct intel_engine_cs *engine;
> > @@ -1697,7 +1715,7 @@ static int live_lrc_indirect_ctx_bb(void *arg)
> >         for_each_engine(engine, gt, id) {
> >           intel_engine_pm_get(engine);
> > -        err = __live_lrc_indirect_ctx_bb(engine);
> > +        err = __lrc_wabb_ctx(engine, per_ctx);
> >           intel_engine_pm_put(engine);
> >             if (igt_flush_test(gt->i915))
> > @@ -1710,6 +1728,16 @@ static int live_lrc_indirect_ctx_bb(void *arg)
> >       return err;
> >   }
> >   +static int live_lrc_indirect_ctx_bb(void *arg)
> > +{
> > +    return lrc_wabb_ctx(arg, false);
> > +}
> > +
> > +static int live_lrc_per_ctx_bb(void *arg)
> > +{
> > +    return lrc_wabb_ctx(arg, true);
> > +}
> > +
> >   static void garbage_reset(struct intel_engine_cs *engine,
> >                 struct i915_request *rq)
> >   {
> > @@ -1947,6 +1975,7 @@ int intel_lrc_live_selftests(struct 
> > drm_i915_private *i915)
> >           SUBTEST(live_lrc_garbage),
> >           SUBTEST(live_pphwsp_runtime),
> >           SUBTEST(live_lrc_indirect_ctx_bb),
> > +        SUBTEST(live_lrc_per_ctx_bb),
> >       };
> >         if (!HAS_LOGICAL_RING_CONTEXTS(i915))