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

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

Hi Michal

On 2/7/2025 12:09 AM, Michal Wajdeczko wrote:
> 
> 
> On 06.02.2025 11:43, 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)
> 
> maybe rename it a little to distinguish from other function that just
> look at engine_activity->supported flag?

Hmm, this is static and internally used. Not sure what to rename it to

Is this okay ? is_engine_activity_supported
> 
>> +{
>> +	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;
> 
> it also doesn't work on VFs
> so maybe it should be different logic:
Will add this
> 
> 	if (IS_SRIOV_VF) {
> 		xt_gt_into("EA not supported on VFs\n");
> 		return false;
> 	}
> 
> 	if (GUC_SUBMIT_VER < 1.14.1) {
> 		xt_gt_into("EA not supported on vA, need vB+\n");
> 		return false;
> 	}
> 
>> +
>> +	xe_gt_warn(gt,
> 
> does it really need to be 'warn' level?
yeah CI flags it as a failure. Will change it to debug
> 
>> +		   "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)
> 
> what about VFs?
Will add the check to return false in case of VF's as suggested above
> 
>> + *
>> + * 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);
> 
> should we continue if not supported ?
Thanks for catching this. Shouldn't continue.
Will fix it
> 
>> +
>>   	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 */
> 
> indicates?
will fix this

Thanks
Riana
> 
>> +	bool supported;
>> +
>>   	/** @eag: holds the device level engine activity data */
>>   	struct engine_activity_group *eag;
>>   
>