Re: [PATCH v3 08/10] drm/xe/guc: Expose engine activity only for supported GuC version

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

Hi Umesh

On 1/17/2025 4:40 AM, Umesh Nerlige Ramappa wrote:
> On Mon, Jan 06, 2025 at 01:25:57PM +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 submission version (John)
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc_engine_activity.c | 31 +++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_guc_engine_activity.h |  1 +
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_engine_activity.c b/drivers/ 
>> gpu/drm/xe/xe_guc_engine_activity.c
>> index 14950b42d1b1..5aea42f0cded 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> @@ -255,6 +255,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);
>> }
>>
>> @@ -268,9 +271,31 @@ 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_enable_stats - Enable engine activity stats
>> + * @guc: The GuC object
>> + *
>> + * Engine activity stats is supported from GuC submission version
>> + * (1.14.1)
>> + *
>> + * Return: true if engine activity stats supported, false otherwise
>> + */
>> +bool xe_guc_engine_activity_supported(struct xe_guc *guc)
>> +{
>> +    if (GUC_SUBMIT_VER(guc) >= MAKE_GUC_VER(1, 14, 1))
>> +        return true;
>> +
>> +    drm_WARN_ON_ONCE(&guc_to_xe(guc)->drm,
>> +             "Engine busyness not supported in this GuC version\n");
> 
> A warn ON may be heavy. Can we live with just a debug print here?
i can replace it with a drm_warn_once which doesn't print the 
stacktrace. I thought warn_on would be helpful to flag machines on CI 
which doesn't have the required guc versions.

Can replace it with debug/info if not necessary

Thanks
Riana
> 
>> +    return false;
>> +}
>> +
>> /**
>>  * xe_guc_engine_activity_enable_stats - Enable engine activity stats
>>  * @guc: The GuC object
>> @@ -281,6 +306,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);
>> @@ -307,6 +335,9 @@ int xe_guc_engine_activity_init(struct xe_guc *guc)
>>     struct xe_gt *gt = guc_to_gt(guc);
>>     int ret;
>>
>> +    if (!xe_guc_engine_activity_supported(guc))
>> +        return 0;
>> +
>>     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 6cd435b81bbe..e2c2f1bb2b59 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);
>> -- 
>> 2.47.1
>>