Re: [PATCH v4 6/8] drm/xe/guc: Expose engine busyness only for supported GuC version

[Riana Tauro <riana.tauro@intel.com>]

On 1/18/2024 11:43 AM, Nilawar, Badal wrote:
> Hi Riana,
> 
> On 22-12-2023 13:16, Riana Tauro wrote:
>> Guc version numbers are 8 bits only so convert to 32 bit 8.8.8
>> to allow version comparisions. use compatibility version
>> for the same.
>>
>> Engine busyness is supported only on GuC versions >= 70.11.1.
>> Allow enabling/reading engine busyness only on supported
>> GuC versions. Warn once if not supported.
>>
>> v2: rebase
>>      fix guc comparison error (Matthew Brost)
>>      add a macro for guc version comparison
>>
>> v3: do not show pmu counters if guc engine busyness
>>      is not supported
>>
>> v4: add version check comment only in the check function
>>      remove it otherwise (Umesh)
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt.c                  | 11 ++++++
>>   drivers/gpu/drm/xe/xe_gt.h                  |  1 +
>>   drivers/gpu/drm/xe/xe_guc_engine_busyness.c | 37 +++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_engine_busyness.h |  2 +-
>>   drivers/gpu/drm/xe/xe_pmu.c                 | 12 +++++--
>>   5 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 5825471a3422..a48cceaa7750 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -800,3 +800,14 @@ u64 xe_gt_total_active_ticks(struct xe_gt *gt)
>>   {
>>       return xe_guc_engine_busyness_active_ticks(&gt->uc.guc);
>>   }
>> +
>> +/**
>> + * xe_gt_engine_busyness_supported - Checks support for engine busyness
>> + * @gt: GT structure
>> + *
>> + * Returns true if engine busyness is supported, false otherwise.
>> + */
>> +bool xe_gt_engine_busyness_supported(struct xe_gt *gt)
>> +{
>> +    return xe_guc_engine_busyness_supported(&gt->uc.guc);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>> index 9bac85cdf609..bef99eb2fed2 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.h
>> +++ b/drivers/gpu/drm/xe/xe_gt.h
>> @@ -42,6 +42,7 @@ int xe_gt_resume(struct xe_gt *gt);
>>   void xe_gt_reset_async(struct xe_gt *gt);
>>   void xe_gt_sanitize(struct xe_gt *gt);
>> +bool xe_gt_engine_busyness_supported(struct xe_gt *gt);
>>   u64 xe_gt_engine_busy_ticks(struct xe_gt *gt, struct xe_hw_engine 
>> *hwe);
>>   u64 xe_gt_total_active_ticks(struct xe_gt *gt);
>>   /**
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_busyness.c 
>> b/drivers/gpu/drm/xe/xe_guc_engine_busyness.c
>> index 24e72555647a..2dd06563d0ad 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_busyness.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_busyness.c
>> @@ -32,6 +32,9 @@
>>    * engine busyness % = (ticks_engine / ticks_gt) * 100
>>    */
>> +/* GuC version number components are only 8-bit, so converting to a 
>> 32bit 8.8.8 */
>> +#define GUC_VER(maj, min, pat)    (((maj) << 16) | ((min) << 8) | (pat))
>> +
>>   static void guc_engine_busyness_usage_map(struct xe_guc *guc,
>>                         struct xe_hw_engine *hwe,
>>                         struct iosys_map *engine_map,
>> @@ -110,6 +113,9 @@ static void 
>> guc_engine_busyness_enable_stats(struct xe_guc *guc)
>>       struct xe_device *xe = guc_to_xe(guc);
>>       int ret;
>> +    if (!xe_guc_engine_busyness_supported(guc))
>> +        return;
>> +
>>       ret = xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action), 0, 0);
>>       if (ret)
>>           drm_err(&xe->drm, "Failed to enable usage stats %pe", 
>> ERR_PTR(ret));
>> @@ -122,6 +128,28 @@ static void guc_engine_busyness_fini(struct 
>> drm_device *drm, void *arg)
>>       xe_bo_unpin_map_no_vm(guc->busy.bo);
>>   }
>> +/*
>> + * xe_guc_engine_busynes_supported- check if engine busyness is 
>> supported
>> + * @guc: The GuC object
>> + *
>> + * Engine busyness is supported only above guc 70.11.1
>> + *
>> + * Returns true if supported, false otherwise
>> + */
>> +bool xe_guc_engine_busyness_supported(struct xe_guc *guc)
>> +{
>> +    struct xe_uc_fw *uc_fw = &guc->fw;
>> +    struct xe_uc_fw_version *version = 
>> &uc_fw->versions.found[XE_UC_FW_VER_COMPATIBILITY];
> Why not XE_UC_FW_VER_RELEASE here? Or should we check firmware type 
> (compatibility or release) first and then derive version from it.
Hi Badal

Even Release can be used. Used submission version (VF compatibility 
version).
Why do we have to check firmware type ?

However, This patch is currently on hold.

Thanks
Riana
> 
> Regards,
> Badal
>> +
>> +    if (GUC_VER(version->major, version->minor, version->patch) >= 
>> GUC_VER(1, 3, 1))
>> +        return true;
>> +
>> +    drm_WARN_ON_ONCE(&guc_to_xe(guc)->drm,
>> +             "Engine busyness supported from 70.11.1 GuC version\n");
>> +
>> +    return false;
>> +}
>> +
>>   /*
>>    * xe_guc_engine_busyness_active_ticks - Gets the total active ticks
>>    * @guc: The GuC object
>> @@ -133,6 +161,9 @@ u64 xe_guc_engine_busyness_active_ticks(struct 
>> xe_guc *guc)
>>   {
>>       u64 ticks_gt;
>> +    if (!xe_guc_engine_busyness_supported(guc))
>> +        return 0;
>> +
>>       guc_engine_busyness_get_usage(guc, NULL, NULL, &ticks_gt);
>>       return ticks_gt;
>> @@ -150,6 +181,9 @@ u64 xe_guc_engine_busyness_ticks(struct xe_guc 
>> *guc, struct xe_hw_engine *hwe)
>>   {
>>       u64 ticks_engine;
>> +    if (!xe_guc_engine_busyness_supported(guc))
>> +        return 0;
>> +
>>       guc_engine_busyness_get_usage(guc, hwe, &ticks_engine, NULL);
>>       return ticks_engine;
>> @@ -173,6 +207,9 @@ int xe_guc_engine_busyness_init(struct xe_guc *guc)
>>       u32 size;
>>       int err;
>> +    if (!xe_guc_engine_busyness_supported(guc))
>> +        return 0;
>> +
>>       /* Initialization already done */
>>       if (guc->busy.bo)
>>           return 0;
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_busyness.h 
>> b/drivers/gpu/drm/xe/xe_guc_engine_busyness.h
>> index 57325910ebc4..e3c74e0236af 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_busyness.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_busyness.h
>> @@ -14,5 +14,5 @@ struct xe_guc;
>>   int xe_guc_engine_busyness_init(struct xe_guc *guc);
>>   u64 xe_guc_engine_busyness_active_ticks(struct xe_guc *guc);
>>   u64 xe_guc_engine_busyness_ticks(struct xe_guc *guc, struct 
>> xe_hw_engine *hwe);
>> -
>> +bool xe_guc_engine_busyness_supported(struct xe_guc *guc);
>>   #endif
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> index c2be157a6f5d..f91652886b67 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -55,14 +55,16 @@ static int
>>   config_status(struct xe_device *xe, u64 config)
>>   {
>>       unsigned int gt_id = config_gt_id(config);
>> +    struct xe_gt *gt = xe_device_get_gt(xe, gt_id);
>>       if (gt_id >= XE_PMU_MAX_GT)
>>           return -ENOENT;
>> -    if (config_counter(config) == DRM_XE_PMU_TOTAL_ACTIVE_TICKS(0))
>> -        return 0;
>> +    if (config_counter(config) == DRM_XE_PMU_TOTAL_ACTIVE_TICKS(0) &&
>> +        !xe_gt_engine_busyness_supported(gt))
>> +        return -ENOENT;
>> -    return -ENOENT;
>> +    return 0;
>>   }
>>   static int engine_event_status(struct xe_hw_engine *hwe,
>> @@ -71,6 +73,10 @@ static int engine_event_status(struct xe_hw_engine 
>> *hwe,
>>       if (!hwe)
>>           return -ENODEV;
>> +    if (sample == DRM_XE_PMU_SAMPLE_BUSY_TICKS &&
>> +        !xe_gt_engine_busyness_supported(hwe->gt))
>> +        return -ENOENT;
>> +
>>       /* Other engine events will be added, XE_ENGINE_SAMPLE_COUNT 
>> will be changed */
>>       return (sample >= DRM_XE_PMU_SAMPLE_BUSY_TICKS && sample < 
>> XE_ENGINE_SAMPLE_COUNT)
>>           ? 0 : -ENOENT;