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

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

Hi Michal

On 1/30/2025 1:48 AM, Michal Wajdeczko wrote:
> 
> 
> On 29.01.2025 11:16, 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)
>> v3: use drm_warn_once to avoid stacktrace (Umesh)
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_engine_activity.c | 32 +++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_engine_activity.h |  1 +
>>   2 files changed, 33 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..4d720afd12ac 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_engine_activity.c
>> @@ -250,6 +250,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 +266,32 @@ 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 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;
> 
> this patch seems to be redundant as patch:
> 
> [PATCH v4 5/8] drm/xe/guc: Bump minimum required GuC version to v70.36.0
> 
> now requires at least submit version 1.17.1
This feature support was added in 1.14.1 hence the version.

[PATCH v4 5/8] drm/xe/guc: Bump minimum required GuC version to v70.36.0
Upstream firmware latest above 70.33 is 36.0
> 
>> +
>> +	drm_warn_once(&guc_to_xe(guc)->drm,
> 
> btw, for GuC related logs we prefer xe_gt_info() logs
will replace in the series with gt versions
> 
> and _once() variant is n/a here as it will hide any other messages from
> drivers running on other devices that might be present on the system
I thought _once would be per-device. Will re-check

But removing _once would log the message after every suspend/resume or 
reset. If that's okay will remove once
> 
> better option seems to be check/log once in the init() and then just set
> a flag that could be used later
This sounds good. Will add this in the next rev
> 
>> +		      "per-engine-class activity not supported for this GuC version\n");
> 
> and maybe we should be more friendly and instead "this" also print
> actual version used to save the user looking for it elsewhere
Sure, will add this

Thanks
Riana
> 
>> +
>> +	return false;
>> +}
>> +
>>   /**
>>    * xe_guc_engine_activity_enable_stats - Enable engine activity stats
>>    * @guc: The GuC object
>> @@ -276,6 +302,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 +331,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 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);
>