Re: [Intel-gfx] [PATCH 6/6] drm/i915/pmu: Export counters from all tiles

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/05/2023 01:58, Umesh Nerlige Ramappa wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Start exporting frequency and RC6 counters from all tiles.
> 
> Existing counters keep their names and config values and new one use the
> namespace added in the previous patch, with the "-gtN" added to their
> names.
> 
> Interrupts counter is an odd one off. Because it is the global device
> counters (not only GT) we choose not to add per tile versions for now.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 87 ++++++++++++++++++++++-----------
>   1 file changed, 59 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 12b2f3169abf..284e5c5b97bb 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -546,8 +546,9 @@ config_status(struct drm_i915_private *i915, u64 config)
>   	struct intel_gt *gt = to_gt(i915);
>   
>   	unsigned int gt_id = config_gt_id(config);
> +	unsigned int max_gt_id = HAS_EXTRA_GT_LIST(i915) ? 1 : 0;
>   
> -	if (gt_id)
> +	if (gt_id > max_gt_id)

Would this be clearer as:

unsigned in num_gts = 1 + (unsigned int)HAS_EXTRA_GT_LIST(i915);

if (gt_id >= num_gts)

?

Just thinking out loud, no real opinion either way.

>   		return -ENOENT;
>   
>   	switch (config_counter(config)) {
> @@ -561,6 +562,8 @@ config_status(struct drm_i915_private *i915, u64 config)
>   			return -ENODEV;
>   		break;
>   	case I915_PMU_INTERRUPTS:
> +		if (gt_id)
> +			return -ENOENT;
>   		break;
>   	case I915_PMU_RC6_RESIDENCY:
>   		if (!gt->rc6.supported)
> @@ -930,11 +933,20 @@ static const struct attribute_group i915_pmu_cpumask_attr_group = {
>   	.attrs = i915_cpumask_attrs,
>   };
>   
> -#define __event(__config, __name, __unit) \
> +#define __event(__counter, __name, __unit) \
>   { \
> -	.config = (__config), \
> +	.counter = (__counter), \
>   	.name = (__name), \
>   	.unit = (__unit), \
> +	.global = false, \
> +}
> +
> +#define __global_event(__counter, __name, __unit) \
> +{ \
> +	.counter = (__counter), \
> +	.name = (__name), \
> +	.unit = (__unit), \
> +	.global = true, \
>   }
>   
>   #define __engine_event(__sample, __name) \
> @@ -973,15 +985,16 @@ create_event_attributes(struct i915_pmu *pmu)
>   {
>   	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
>   	static const struct {
> -		u64 config;
> +		unsigned int counter;
>   		const char *name;
>   		const char *unit;
> +		bool global;
>   	} events[] = {
> -		__event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "M"),
> -		__event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency", "M"),
> -		__event(I915_PMU_INTERRUPTS, "interrupts", NULL),
> -		__event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
> -		__event(I915_PMU_SOFTWARE_GT_AWAKE_TIME, "software-gt-awake-time", "ns"),
> +		__event(0, "actual-frequency", "M"),
> +		__event(1, "requested-frequency", "M"),
> +		__global_event(2, "interrupts", NULL),
> +		__event(3, "rc6-residency", "ns"),
> +		__event(4, "software-gt-awake-time", "ns"),
>   	};
>   	static const struct {
>   		enum drm_i915_pmu_engine_sample sample;
> @@ -996,12 +1009,17 @@ create_event_attributes(struct i915_pmu *pmu)
>   	struct i915_ext_attribute *i915_attr = NULL, *i915_iter;
>   	struct attribute **attr = NULL, **attr_iter;
>   	struct intel_engine_cs *engine;
> -	unsigned int i;
> +	struct intel_gt *gt;
> +	unsigned int i, j;
>   
>   	/* Count how many counters we will be exposing. */
> -	for (i = 0; i < ARRAY_SIZE(events); i++) {
> -		if (!config_status(i915, events[i].config))
> -			count++;
> +	for_each_gt(gt, i915, j) {
> +		for (i = 0; i < ARRAY_SIZE(events); i++) {
> +			u64 config = ___I915_PMU_OTHER(j, events[i].counter);
> +
> +			if (!config_status(i915, config))
> +				count++;
> +		}
>   	}
>   
>   	for_each_uabi_engine(engine, i915) {
> @@ -1031,26 +1049,39 @@ create_event_attributes(struct i915_pmu *pmu)
>   	attr_iter = attr;
>   
>   	/* Initialize supported non-engine counters. */
> -	for (i = 0; i < ARRAY_SIZE(events); i++) {
> -		char *str;
> -
> -		if (config_status(i915, events[i].config))
> -			continue;
> -
> -		str = kstrdup(events[i].name, GFP_KERNEL);
> -		if (!str)
> -			goto err;
> +	for_each_gt(gt, i915, j) {
> +		for (i = 0; i < ARRAY_SIZE(events); i++) {
> +			u64 config = ___I915_PMU_OTHER(j, events[i].counter);
> +			char *str;
>   
> -		*attr_iter++ = &i915_iter->attr.attr;
> -		i915_iter = add_i915_attr(i915_iter, str, events[i].config);
> +			if (config_status(i915, config))
> +				continue;
>   
> -		if (events[i].unit) {
> -			str = kasprintf(GFP_KERNEL, "%s.unit", events[i].name);
> +			if (events[i].global || !HAS_EXTRA_GT_LIST(i915))
> +				str = kstrdup(events[i].name, GFP_KERNEL);
> +			else
> +				str = kasprintf(GFP_KERNEL, "%s-gt%u",
> +						events[i].name, j);
>   			if (!str)
>   				goto err;
>   
> -			*attr_iter++ = &pmu_iter->attr.attr;
> -			pmu_iter = add_pmu_attr(pmu_iter, str, events[i].unit);
> +			*attr_iter++ = &i915_iter->attr.attr;
> +			i915_iter = add_i915_attr(i915_iter, str, config);
> +
> +			if (events[i].unit) {
> +				if (events[i].global || !HAS_EXTRA_GT_LIST(i915))

Maybe worth moving the condition to for_each_gt?

   bool use_gt_suffix = HAS_EXTRA_GT_LIST(i915) && !events[i].global;

Again, more questionable bike shedding.

Regards,

Tvrtko

> +					str = kasprintf(GFP_KERNEL, "%s.unit",
> +							events[i].name);
> +				else
> +					str = kasprintf(GFP_KERNEL, "%s-gt%u.unit",
> +							events[i].name, j);
> +				if (!str)
> +					goto err;
> +
> +				*attr_iter++ = &pmu_iter->attr.attr;
> +				pmu_iter = add_pmu_attr(pmu_iter, str,
> +							events[i].unit);
> +			}
>   		}
>   	}
>