Re: [PATCH v5 3/8] drm/xe/guc: Expose engine activity only for supported GuC version

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

Hi Umesh

On 2/8/2025 3:07 AM, Umesh Nerlige Ramappa wrote:
> On Thu, Feb 06, 2025 at 04:13:52PM +0530, Riana Tauro wrote:
>> Engine activity is supported only on GuC submission version >= 1.14.1
>> Allow enabling/reading engine activity only on supported
>> GuC versions. Warn once if not supported.
>>
>> v2: use guc interface version (John)
>> v3: do not use drm_WARN (Umesh)
>> v4: use variable for supported and use gt logs
>>    use a friendlier log message (Michal)
>>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc_engine_activity.c   | 42 +++++++++++++++++++
>> drivers/gpu/drm/xe/xe_guc_engine_activity.h   |  1 +
>> .../gpu/drm/xe/xe_guc_engine_activity_types.h |  3 ++
>> 3 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity.c b/drivers/ 
>> gpu/drm/xe/xe_guc_engine_activity.c
>> index 9c08af273397..5d67fe38639a 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> @@ -89,6 +89,22 @@ static int allocate_engine_activity_buffers(struct 
>> xe_guc *guc,
>>     return 0;
>> }
>>
>> +static bool engine_activity_supported(struct xe_guc *guc)
>> +{
>> +    struct xe_uc_fw_version *version = &guc- 
>> >fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
>> +    struct xe_gt *gt = guc_to_gt(guc);
>> +
>> +    /* engine activity stats is supported from GuC interface version 
>> (1.14.1) */
>> +    if (GUC_SUBMIT_VER(guc) >= MAKE_GUC_VER(1, 14, 1))
>> +        return true;
>> +
>> +    xe_gt_warn(gt,
>> +           "engine activity stats unsupported in GuC interface v%u. 
>> %u.%u, v%u.%u.%u or newer required\n",
>> +           version->major, version->minor, version->patch, 1, 14, 1);
>> +
>> +    return false;
>> +}
>> +
>> static struct engine_activity *hw_engine_to_engine_activity(struct 
>> xe_hw_engine *hwe)
>> {
>>     struct xe_guc *guc = &hwe->gt->uc.guc;
>> @@ -250,6 +266,9 @@ u64 xe_guc_engine_activity_active_ticks(struct 
>> xe_hw_engine *hwe)
>> {
>>     struct xe_guc *guc =  &hwe->gt->uc.guc;
>>
>> +    if (!xe_guc_engine_activity_supported(guc))
>> +        return 0;
>> +
>>     return get_engine_active_ticks(guc, hwe);
>> }
>>
>> @@ -263,9 +282,27 @@ u64 xe_guc_engine_activity_total_ticks(struct 
>> xe_hw_engine *hwe)
>> {
>>     struct xe_guc *guc =  &hwe->gt->uc.guc;
>>
>> +    if (!xe_guc_engine_activity_supported(guc))
>> +        return 0;
>> +
>>     return get_engine_total_ticks(guc, hwe);
>> }
>>
>> +/**
>> + * xe_guc_engine_activity_supported - Check support for engine 
>> activity stats
>> + * @guc: The GuC object
>> + *
>> + * Engine activity stats is supported from GuC interface version 
>> (1.14.1)
>> + *
>> + * Return: true if engine activity stats supported, false otherwise
>> + */
>> +bool xe_guc_engine_activity_supported(struct xe_guc *guc)
>> +{
>> +    struct xe_guc_engine_activity *engine_activity = &guc- 
>> >engine_activity;
>> +
>> +    return engine_activity->supported;
>> +}
>> +
>> /**
>>  * xe_guc_engine_activity_enable_stats - Enable engine activity stats
>>  * @guc: The GuC object
>> @@ -276,6 +313,9 @@ void xe_guc_engine_activity_enable_stats(struct 
>> xe_guc *guc)
>> {
>>     int ret;
>>
>> +    if (!xe_guc_engine_activity_supported(guc))
>> +        return;
>> +
>>     ret = enable_engine_activity_stats(guc);
>>     if (ret)
>>         xe_gt_err(guc_to_gt(guc), "failed to enable activity 
>> stats%d\n", ret);
>> @@ -302,6 +342,8 @@ int xe_guc_engine_activity_init(struct xe_guc *guc)
>>     struct xe_gt *gt = guc_to_gt(guc);
>>     int ret;
>>
>> +    engine_activity->supported = engine_activity_supported(guc);
>> +
> 
> Is xe_guc_engine_activity_init() called even on a VF? If not, then 
> initializing engine_activity->supported here may not be sufficient.

Will return in xe_guc_engine_activity_init before the initialization
of supported if its VF. So supported will not be set to 1 and will 
return false  for all the other calls

if (IS_SRIOV_VF(xe))
        return 0;

Thanks
Riana


> Thanks,
> Umesh
> 
>>     ret = allocate_engine_activity_group(guc);
>>     if (ret) {
>>         xe_gt_err(gt, "failed to allocate activity group %d\n", ret);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity.h b/drivers/ 
>> gpu/drm/xe/xe_guc_engine_activity.h
>> index c00f3da5513d..9d3ea3f67b6a 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.h
>> @@ -12,6 +12,7 @@ struct xe_hw_engine;
>> struct xe_guc;
>>
>> int xe_guc_engine_activity_init(struct xe_guc *guc);
>> +bool xe_guc_engine_activity_supported(struct xe_guc *guc);
>> void xe_guc_engine_activity_enable_stats(struct xe_guc *guc);
>> u64 xe_guc_engine_activity_active_ticks(struct xe_hw_engine *hwe);
>> u64 xe_guc_engine_activity_total_ticks(struct xe_hw_engine *hwe);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h b/ 
>> drivers/gpu/drm/xe/xe_guc_engine_activity_types.h
>> index a2ab327d3eec..81002c83d65e 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity_types.h
>> @@ -79,6 +79,9 @@ struct xe_guc_engine_activity {
>>     /** @num_activity_group: number of activity groups */
>>     u32 num_activity_group;
>>
>> +    /** @supported: checks if engine activity is supported */
>> +    bool supported;
>> +
>>     /** @eag: holds the device level engine activity data */
>>     struct engine_activity_group *eag;
>>
>> -- 
>> 2.47.1
>>