Re: [Intel-gfx] [PATCH 3/4] drm/i915/guc: Add support for w/a KLVs

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

On 10/6/2023 17:38, Belgaumkar, Vinay wrote:
> On 9/15/2023 2:55 PM, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > To prevent running out of bits, new w/a enable flags are being added
> > via a KLV system instead of a 32 bit flags word.
> >
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > ---
> >   .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |  1 +
> >   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  3 +
> >   drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 64 ++++++++++++++++++-
> >   drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c     |  6 ++
> >   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  5 +-
> >   5 files changed, 77 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h 
> > b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
> > index dabeaf4f245f3..00d6402333f8e 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
> > @@ -36,6 +36,7 @@ enum intel_guc_load_status {
> >       INTEL_GUC_LOAD_STATUS_INVALID_INIT_DATA_RANGE_START,
> >       INTEL_GUC_LOAD_STATUS_MPU_DATA_INVALID                 = 0x73,
> >       INTEL_GUC_LOAD_STATUS_INIT_MMIO_SAVE_RESTORE_INVALID   = 0x74,
> > +    INTEL_GUC_LOAD_STATUS_KLV_WORKAROUND_INIT_ERROR        = 0x75,
> >       INTEL_GUC_LOAD_STATUS_INVALID_INIT_DATA_RANGE_END,
> >         INTEL_GUC_LOAD_STATUS_READY                            = 0xF0,
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 6c392bad29c19..3b1fc5f96306b 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -186,6 +186,8 @@ struct intel_guc {
> >       struct guc_mmio_reg *ads_regset;
> >       /** @ads_golden_ctxt_size: size of the golden contexts in the 
> > ADS */
> >       u32 ads_golden_ctxt_size;
> > +    /** @ads_waklv_size: size of workaround KLVs */
> > +    u32 ads_waklv_size;
> >       /** @ads_capture_size: size of register lists in the ADS used 
> > for error capture */
> >       u32 ads_capture_size;
> >       /** @ads_engine_usage_size: size of engine usage in the ADS */
> > @@ -295,6 +297,7 @@ struct intel_guc {
> >   #define MAKE_GUC_VER(maj, min, pat)    (((maj) << 16) | ((min) << 
> > 8) | (pat))
> >   #define MAKE_GUC_VER_STRUCT(ver)    MAKE_GUC_VER((ver).major, 
> > (ver).minor, (ver).patch)
> >   #define GUC_SUBMIT_VER(guc) 
> > MAKE_GUC_VER_STRUCT((guc)->submission_version)
> > +#define GUC_FIRMWARE_VER(guc) 
> > MAKE_GUC_VER_STRUCT((guc)->fw.file_selected.ver)
> >     static inline struct intel_guc *log_to_guc(struct intel_guc_log 
> > *log)
> >   {
> > 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 63724e17829a7..792910af3a481 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -46,6 +46,10 @@
> >    *      +---------------------------------------+
> >    *      | padding                               |
> >    *      +---------------------------------------+ <== 4K aligned
> > + *      | w/a KLVs                              |
> > + *      +---------------------------------------+
> > + *      | padding                               |
> > + *      +---------------------------------------+ <== 4K aligned
> >    *      | capture lists                         |
> >    *      +---------------------------------------+
> >    *      | padding                               |
> > @@ -88,6 +92,11 @@ static u32 guc_ads_golden_ctxt_size(struct 
> > intel_guc *guc)
> >       return PAGE_ALIGN(guc->ads_golden_ctxt_size);
> >   }
> >   +static u32 guc_ads_waklv_size(struct intel_guc *guc)
> > +{
> > +    return PAGE_ALIGN(guc->ads_waklv_size);
> > +}
> > +
> >   static u32 guc_ads_capture_size(struct intel_guc *guc)
> >   {
> >       return PAGE_ALIGN(guc->ads_capture_size);
> > @@ -113,7 +122,7 @@ static u32 guc_ads_golden_ctxt_offset(struct 
> > intel_guc *guc)
> >       return PAGE_ALIGN(offset);
> >   }
> >   -static u32 guc_ads_capture_offset(struct intel_guc *guc)
> > +static u32 guc_ads_waklv_offset(struct intel_guc *guc)
> >   {
> >       u32 offset;
> >   @@ -123,6 +132,16 @@ static u32 guc_ads_capture_offset(struct 
> > intel_guc *guc)
> >       return PAGE_ALIGN(offset);
> >   }
> >   +static u32 guc_ads_capture_offset(struct intel_guc *guc)
> > +{
> > +    u32 offset;
> > +
> > +    offset = guc_ads_waklv_offset(guc) +
> > +         guc_ads_waklv_size(guc);
> > +
> > +    return PAGE_ALIGN(offset);
> > +}
> > +
> >   static u32 guc_ads_private_data_offset(struct intel_guc *guc)
> >   {
> >       u32 offset;
> > @@ -791,6 +810,40 @@ guc_capture_prep_lists(struct intel_guc *guc)
> >       return PAGE_ALIGN(total_size);
> >   }
> >   +static void guc_waklv_init(struct intel_guc *guc)
> > +{
> > +    struct intel_gt *gt = guc_to_gt(guc);
> > +    u32 offset, addr_ggtt, remain, size;
> > +
> > +    if (!intel_uc_uses_guc_submission(&gt->uc))
> > +        return;
> > +
> > +    if (GUC_FIRMWARE_VER(guc) < MAKE_GUC_VER(70, 10, 0))
> > +        return;
> should this be <= ?
No. GuC 70.10.0 is when w/a KLVs were introduced. So we want to skip on 
any version that is prior to 70.10.0.

> > +
> > +    GEM_BUG_ON(iosys_map_is_null(&guc->ads_map));
> > +    offset = guc_ads_waklv_offset(guc);
> > +    remain = guc_ads_waklv_size(guc);
> > +
> > +    /* Add workarounds here */
> > +
> extra blank line?
The point is that the comment is a place holder for where the w/a KLVs 
will be added. This is just the prep patch so that whichever w/a 
actually gets implemented first (there are still issues with the current 
options), the w/a patch is only added the w/a itself and not a whole 
bunch of infrastructure as well.

> > +    size = guc_ads_waklv_size(guc) - remain;
>
> Hmm, am I missing something or remain is already set to 
> guc_ads_walkv_size()?
You are missing the actual w/a to be added above :).

John.

> Thanks,
>
> Vinay.
>
> > +    if (!size)
> > +        return;
> > +
> > +    addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> > +
> > +    ads_blob_write(guc, ads.wa_klv_addr_lo, addr_ggtt);
> > +    ads_blob_write(guc, ads.wa_klv_addr_hi, 0);
> > +    ads_blob_write(guc, ads.wa_klv_size, size);
> > +}
> > +
> > +static int guc_prep_waklv(struct intel_guc *guc)
> > +{
> > +    /* Fudge something chunky for now: */
> > +    return PAGE_SIZE;
> > +}
> > +
> >   static void __guc_ads_init(struct intel_guc *guc)
> >   {
> >       struct intel_gt *gt = guc_to_gt(guc);
> > @@ -838,6 +891,9 @@ static void __guc_ads_init(struct intel_guc *guc)
> >       /* MMIO save/restore list */
> >       guc_mmio_reg_state_init(guc);
> >   +    /* Workaround KLV list */
> > +    guc_waklv_init(guc);
> > +
> >       /* Private Data */
> >       ads_blob_write(guc, ads.private_data, base +
> >                  guc_ads_private_data_offset(guc));
> > @@ -881,6 +937,12 @@ int intel_guc_ads_create(struct intel_guc *guc)
> >           return ret;
> >       guc->ads_capture_size = ret;
> >   +    /* And don't forget the workaround KLVs: */
> > +    ret = guc_prep_waklv(guc);
> > +    if (ret < 0)
> > +        return ret;
> > +    guc->ads_waklv_size = ret;
> > +
> >       /* Now the total size can be determined: */
> >       size = guc_ads_blob_size(guc);
> >   diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > index 0f79cb6585182..a54d58b9243b0 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > @@ -115,6 +115,7 @@ static inline bool guc_load_done(struct 
> > intel_uncore *uncore, u32 *status, bool
> >       case INTEL_GUC_LOAD_STATUS_INIT_DATA_INVALID:
> >       case INTEL_GUC_LOAD_STATUS_MPU_DATA_INVALID:
> >       case INTEL_GUC_LOAD_STATUS_INIT_MMIO_SAVE_RESTORE_INVALID:
> > +    case INTEL_GUC_LOAD_STATUS_KLV_WORKAROUND_INIT_ERROR:
> >           *success = false;
> >           return true;
> >       }
> > @@ -241,6 +242,11 @@ static int guc_wait_ucode(struct intel_guc *guc)
> >               ret = -EPERM;
> >               break;
> >   +        case INTEL_GUC_LOAD_STATUS_KLV_WORKAROUND_INIT_ERROR:
> > +            guc_info(guc, "invalid w/a KLV entry\n");
> > +            ret = -EINVAL;
> > +            break;
> > +
> >           case INTEL_GUC_LOAD_STATUS_HWCONFIG_START:
> >               guc_info(guc, "still extracting hwconfig table.\n");
> >               ret = -ETIMEDOUT;
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > index f97af0168a66b..3266842d925e6 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > @@ -429,7 +429,10 @@ struct guc_ads {
> >       u32 
> > capture_instance[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> >       u32 
> > capture_class[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> >       u32 capture_global[GUC_CAPTURE_LIST_INDEX_MAX];
> > -    u32 reserved[14];
> > +    u32 wa_klv_addr_lo;
> > +    u32 wa_klv_addr_hi;
> > +    u32 wa_klv_size;
> > +    u32 reserved[11];
> >   } __packed;
> >     /* Engine usage stats */