Re: [PATCH v5 7/8] drm/xe/xe_pmu: Add pmu support for per-function engine activity stats

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

Hi Michal

On 2/7/2025 12:45 AM, Michal Wajdeczko wrote:
> 
> 
> On 06.02.2025 11:43, Riana Tauro wrote:
>> Add pmu support for per-function engine activity
>> stats.
> 
> PMU ?
> unneeded line wrap ?
Will fix this
> 
>>
>> per-function engine activity is enabled when sriov_numvfs
>> are set. If sriov_numvfs is set to 2, then the applicable function
>> values are
>>
>> 0 - PF engine activity
>> 1,2 - per-VF engine activity from PF
> 
> 0 - PF engine activity
> 1 - VF1 engine activity
> 2 - VF2 engine activity
Will add this
> 
> but maybe better to show full entries:
> 
> xe_0000_03_00.0/engine...ticks/ - PF activity
> xe_0000_03_00.1/engine...ticks/ - VF1 activity
> xe_0000_03_00.2/engine...ticks/ - VF2 activity

This is not the case since here PF is monitoring the engine activity of 
VF's. Not VF reporting the engine activity stats.
So the function id's are sent to PF to get engine activity stats
> 
> as 'function' term here matches 'PCI function'
> 
>>
>> This can be read from perf tool as shown below
>>
>> ./perf stat -e xe_<bdf>/engine-active-ticks,gt=0,engine_class=0,
>> 			 engine_instance=0,function=1/ -I 1000
>>
>> v2: fix documentation (Umesh)
>>      remove global for functions (Lucas, Michal)
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pmu.c | 38 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> index a758fc517048..66cf2ece97ec 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -13,6 +13,7 @@
>>   #include "xe_hw_engine.h"
>>   #include "xe_pm.h"
>>   #include "xe_pmu.h"
>> +#include "xe_sriov_pf_helpers.h"
>>   
>>   /**
>>    * DOC: Xe PMU (Performance Monitoring Unit)
>> @@ -32,9 +33,10 @@
>>    *	gt[60:63]		Selects gt for the event
>>    *	engine_class[20:27]	Selects engine-class for event
>>    *	engine_instance[12:19]	Selects the engine-instance for the event
>> + *	function[44:59]		Selects the function of the event (SRIOV enabled)
>>    *
>>    * For engine specific events (engine-*), gt, engine_class and engine_instance parameters must be
>> - * set as populated by DRM_XE_DEVICE_QUERY_ENGINES.
>> + * set as populated by DRM_XE_DEVICE_QUERY_ENGINES and function if SRIOV is enabled.
>>    *
>>    * For gt specific events (gt-*) gt parameter must be passed. All other parameters will be 0.
>>    *
>> @@ -49,6 +51,7 @@
>>    */
>>   
>>   #define XE_PMU_EVENT_GT_MASK			GENMASK_ULL(63, 60)
>> +#define XE_PMU_EVENT_FUNCTION_MASK		GENMASK_ULL(59, 44)
>>   #define XE_PMU_EVENT_ENGINE_CLASS_MASK		GENMASK_ULL(27, 20)
>>   #define XE_PMU_EVENT_ENGINE_INSTANCE_MASK	GENMASK_ULL(19, 12)
>>   #define XE_PMU_EVENT_ID_MASK			GENMASK_ULL(11, 0)
>> @@ -58,6 +61,11 @@ static unsigned int config_to_event_id(u64 config)
>>   	return FIELD_GET(XE_PMU_EVENT_ID_MASK, config);
>>   }
>>   
>> +static unsigned int config_to_function_id(u64 config)
>> +{
>> +	return FIELD_GET(XE_PMU_EVENT_FUNCTION_MASK, config);
>> +}
>> +
>>   static unsigned int config_to_engine_class(u64 config)
>>   {
>>   	return FIELD_GET(XE_PMU_EVENT_ENGINE_CLASS_MASK, config);
>> @@ -146,7 +154,7 @@ static bool event_supported(struct xe_pmu *pmu, unsigned int gt,
>>   static bool event_param_valid(struct perf_event *event)
>>   {
>>   	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>> -	unsigned int engine_class, engine_instance;
>> +	unsigned int engine_class, engine_instance, function_id;
>>   	u64 config = event->attr.config;
>>   	struct xe_gt *gt;
>>   
>> @@ -154,18 +162,28 @@ static bool event_param_valid(struct perf_event *event)
>>   	if (!gt)
>>   		return false;
>>   
>> +	function_id = config_to_function_id(config);
>> +	if (function_id && !IS_SRIOV_PF(xe))
> 
> hmm, it rather should be:
> 
> 	if (function_id && IS_SRIOV_VF(xe))
This path is not hit for VF

The perf_pmu_register doesn't initialize for VF's

         if (IS_SRIOV_VF(xe))
                 return 0;

function id is applicable only when SRIOV is enabled and VF engine 
activity is requested from PF. Hence SRIOV_PF here and the check to
see if function_id is valid
> 
> but (see below)
> 
>> +		return false;
>> +
>>   	engine_class = config_to_engine_class(config);
>>   	engine_instance = config_to_engine_instance(config);
>>   
>>   	switch (config_to_event_id(config)) {
>>   	case XE_PMU_EVENT_GT_C6_RESIDENCY:
>> -		if (engine_class || engine_instance)
>> +		if (engine_class || engine_instance || function_id)
>>   			return false;
>>   		break;
>>   	case XE_PMU_EVENT_ENGINE_ACTIVE_TICKS:
>>   	case XE_PMU_EVENT_ENGINE_TOTAL_TICKS:
>>   		if (!event_to_hwe(event))
>>   			return false;
>> +		/*
>> +		 * PF(0) and total vfs when SRIOV is enabled
>> +		 */
>> +		if (function_id > xe_sriov_pf_get_totalvfs(xe))
> 
> shouldn't we rely on checks from one place?
> 
will move it to one place
> likely xe_guc_engine_activity_xxx() will also have checks for
> index/function and may use ea->num_functions for that
event and params support is checked in pmu_event init.
There are additional checks while reading too but the init will return 
not supported if events/functions/parameters are not valid.

Thanks
Riana
> 
>> +			return false;
>> +
>>   		break;
>>   	}
>>   
>> @@ -233,18 +251,22 @@ static u64 read_engine_events(struct perf_event *event, u64 prev)
>>   	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>>   	struct xe_pmu *pmu = &xe->pmu;
>>   	struct xe_hw_engine *hwe;
>> -	u64 val = 0;
>> +	unsigned int function_id;
>> +	u64 config, val = 0;
>>   
>>   	if (!pmu->fw_count)
>>   		return prev;
>>   
>> +	config = event->attr.config;
>> +	function_id = config_to_function_id(config);
>> +
>>   	hwe = event_to_hwe(event);
>>   	if (!hwe)
>>   		drm_warn(&xe->drm, "unknown pmu engine\n");
>> -	else if (config_to_event_id(event->attr.config) == XE_PMU_EVENT_ENGINE_ACTIVE_TICKS)
>> -		val = xe_guc_engine_activity_active_ticks(hwe, 0);
>> +	else if (config_to_event_id(config) == XE_PMU_EVENT_ENGINE_ACTIVE_TICKS)
>> +		val = xe_guc_engine_activity_active_ticks(hwe, function_id);
>>   	else
>> -		val = xe_guc_engine_activity_total_ticks(hwe, 0);
>> +		val = xe_guc_engine_activity_total_ticks(hwe, function_id);
>>   
>>   	return val;
>>   }
>> @@ -347,6 +369,7 @@ static void xe_pmu_event_del(struct perf_event *event, int flags)
>>   }
>>   
>>   PMU_FORMAT_ATTR(gt,			"config:60-63");
>> +PMU_FORMAT_ATTR(function,		"config:44-59");
>>   PMU_FORMAT_ATTR(engine_class,		"config:20-27");
>>   PMU_FORMAT_ATTR(engine_instance,	"config:12-19");
>>   PMU_FORMAT_ATTR(event,			"config:0-11");
>> @@ -355,6 +378,7 @@ static struct attribute *pmu_format_attrs[] = {
>>   	&format_attr_event.attr,
>>   	&format_attr_engine_class.attr,
>>   	&format_attr_engine_instance.attr,
>> +	&format_attr_function.attr,
>>   	&format_attr_gt.attr,
>>   	NULL,
>>   };
>