Re: [PATCH v3 3/3] drm/i915/guc: Enable Wa_14019159160

[John Harrison <john.c.harrison@xxxxxxxxx>]

On 2/26/2024 05:25, Nilawar, Badal wrote:
> Hi John,
>
> On 04-01-2024 23:35, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > Use the new w/a KLV support to enable a MTL w/a. Note, this w/a is a
> > super-set of Wa_16019325821, so requires turning that one as well as
> > setting the new flag for Wa_14019159160 itself.
> >
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/gen8_engine_cs.c      |  3 ++
> >   drivers/gpu/drm/i915/gt/intel_engine_types.h  |  1 +
> >   drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h |  7 ++++
> >   drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  1 +
> >   drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 34 ++++++++++++++-----
> >   .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  1 +
> >   6 files changed, 38 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c 
> > b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > index 9cccd60a5c41d..359b21fb02ab2 100644
> > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > @@ -744,6 +744,7 @@ static u32 *gen12_emit_preempt_busywait(struct 
> > i915_request *rq, u32 *cs)
> >     /* Wa_14014475959:dg2 */
> >   /* Wa_16019325821 */
> > +/* Wa_14019159160 */
> >   #define HOLD_SWITCHOUT_SEMAPHORE_PPHWSP_OFFSET    0x540
> >   static u32 hold_switchout_semaphore_offset(struct i915_request *rq)
> >   {
> > @@ -753,6 +754,7 @@ static u32 hold_switchout_semaphore_offset(struct 
> > i915_request *rq)
> >     /* Wa_14014475959:dg2 */
> >   /* Wa_16019325821 */
> > +/* Wa_14019159160 */
> >   static u32 *hold_switchout_emit_wa_busywait(struct i915_request 
> > *rq, u32 *cs)
> >   {
> >       int i;
> > @@ -793,6 +795,7 @@ gen12_emit_fini_breadcrumb_tail(struct 
> > i915_request *rq, u32 *cs)
> >         /* Wa_14014475959:dg2 */
> >       /* Wa_16019325821 */
> > +    /* Wa_14019159160 */
> >       if (intel_engine_uses_wa_hold_switchout(rq->engine))
> >           cs = hold_switchout_emit_wa_busywait(rq, cs);
> >   diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
> > b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index b519812ba120d..ba55c059063db 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -697,6 +697,7 @@ intel_engine_has_relative_mmio(const struct 
> > intel_engine_cs * const engine)
> >     /* Wa_14014475959:dg2 */
> >   /* Wa_16019325821 */
> > +/* Wa_14019159160 */
> >   static inline bool
> >   intel_engine_uses_wa_hold_switchout(struct intel_engine_cs *engine)
> >   {
> > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h 
> > b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
> > index 58012edd4eb0e..bebf28e3c4794 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
> > @@ -101,4 +101,11 @@ enum {
> >       GUC_CONTEXT_POLICIES_KLV_NUM_IDS = 5,
> >   };
> >   +/*
> > + * Workaround keys:
> > + */
> > +enum {
> > +    GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE                = 0x9001,
> > +};
> > +
> >   #endif /* _ABI_GUC_KLVS_ABI_H */
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > index d5c856be31491..db3cb628f40dc 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > @@ -295,6 +295,7 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc)
> >           flags |= GUC_WA_HOLD_CCS_SWITCHOUT;
> >         /* Wa_16019325821 */
> > +    /* Wa_14019159160 */
> >       if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71)))
> >           flags |= GUC_WA_RCS_CCS_SWITCHOUT;
> >   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 6af3fa8b92e34..68d9e277eca8b 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -815,6 +815,25 @@ guc_capture_prep_lists(struct intel_guc *guc)
> >       return PAGE_ALIGN(total_size);
> >   }
> >   +/* Wa_14019159160 */
> > +static u32 guc_waklv_ra_mode(struct intel_guc *guc, u32 offset, u32 
> > remain)
> > +{
> How about making this function generic by passing KLV id as arg?
At this point, there is only one KLV supported. So there is no advantage 
to making the code more complex.

The next patch in the series (not yet posted because this one was not 
supposed to be taking so long to get through CI and merged!) adds 
support for another KLV which is similarly zero length. At that point, 
the helper function is updated to become more generic.

John.

> > +    u32 size;
> > +    u32 klv_entry[] = {
> > +        /* 16:16 key/length */
> > +        FIELD_PREP(GUC_KLV_0_KEY, 
> > GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE) |
> > +        FIELD_PREP(GUC_KLV_0_LEN, 0),
> > +        /* 0 dwords data */
> > +    };
> > +
> > +    size = sizeof(klv_entry);
> > +    GEM_BUG_ON(remain < size);
> > +
> > +    iosys_map_memcpy_to(&guc->ads_map, offset, klv_entry, size);
> Otherwise preparing and adding klv entry can be wrapped in generic 
> function.
>
> Regards,
> Badal
> > +
> > +    return size;
> > +}
> > +
> >   static void guc_waklv_init(struct intel_guc *guc)
> >   {
> >       struct intel_gt *gt = guc_to_gt(guc);
> > @@ -830,15 +849,12 @@ static void guc_waklv_init(struct intel_guc *guc)
> >       offset = guc_ads_waklv_offset(guc);
> >       remain = guc_ads_waklv_size(guc);
> >   -    /*
> > -     * Add workarounds here:
> > -     *
> > -     * if (want_wa_<name>) {
> > -     *    size = guc_waklv_<name>(guc, offset, remain);
> > -     *    offset += size;
> > -     *    remain -= size;
> > -     * }
> > -     */
> > +    /* Wa_14019159160 */
> > +    if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) {
> > +        size = guc_waklv_ra_mode(guc, offset, remain);
> > +        offset += size;
> > +        remain -= size;
> > +    }
> >         size = guc_ads_waklv_size(guc) - remain;
> >       if (!size)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index b09b97c9cd120..80da3573706fa 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -4385,6 +4385,7 @@ static void guc_default_vfuncs(struct 
> > intel_engine_cs *engine)
> >               engine->flags |= I915_ENGINE_USES_WA_HOLD_SWITCHOUT;
> >         /* Wa_16019325821 */
> > +    /* Wa_14019159160 */
> >       if ((engine->class == COMPUTE_CLASS || engine->class == 
> > RENDER_CLASS) &&
> >           IS_GFX_GT_IP_RANGE(engine->gt, IP_VER(12, 70), IP_VER(12, 
> > 71)))
> >           engine->flags |= I915_ENGINE_USES_WA_HOLD_SWITCHOUT;