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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 06.02.2025 11:43, Riana Tauro wrote:
> Add pmu support for per-function engine activity
> stats.

PMU ?
unneeded line wrap ?

> 
> 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

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

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))

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?

likely xe_guc_engine_activity_xxx() will also have checks for
index/function and may use ea->num_functions for that

> +			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,
>  };