[igt-dev] [PATCH i-g-t 07/23] i915/perf: Add support for 64-bit counters

[Lionel Landwerlin]

On 23/08/2022 21:30, Umesh Nerlige Ramappa wrote:
> Add support for 64-bit counters.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>

With the unnecessary changes removed :

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>

> ---
>   tests/i915/perf.c | 70 ++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
> index 5fd143e9..19c33324 100644
> --- a/tests/i915/perf.c
> +++ b/tests/i915/perf.c
> @@ -101,6 +101,8 @@ struct oa_format {
>   	int a40_high_off; /* bytes */
>   	int a40_low_off;
>   	int n_a40;
> +	int a64_off;
> +	int n_a64;
>   	int a_off;
>   	int n_a;
>   	int first_a;
> @@ -455,7 +457,7 @@ max_oa_exponent_for_freq_gt(uint64_t frequency)
>   static uint64_t
>   oa_exponent_to_ns(int exponent)
>   {
> -       return 1000000000ULL * (2ULL << exponent) / intel_perf->devinfo.timestamp_frequency;
> +	return 1000000000ULL * (2ULL << exponent) / intel_perf->devinfo.timestamp_frequency;
Can't see any change on the line above.
>   }
>   
>   static bool
> @@ -658,6 +660,15 @@ gen8_read_40bit_a_counter(const uint32_t *report,
>   	return a40_low[a_id] | high;
>   }
>   
> +static uint64_t
> +gen12_read_64bit_a_counter(const uint32_t *report, enum drm_i915_oa_format fmt, int a_id)
> +{
> +	struct oa_format format = get_oa_format(fmt);
> +	uint64_t *a64 = (uint64_t *)(((uint8_t *)report) + format.a64_off);
> +
> +	return a64[a_id];
> +}
> +
>   static uint64_t
>   gen8_40bit_a_delta(uint64_t value0, uint64_t value1)
>   {
> @@ -670,8 +681,8 @@ gen8_40bit_a_delta(uint64_t value0, uint64_t value1)
>   static void
>   accumulate_uint32(size_t offset,
>   		  uint32_t *report0,
> -                  uint32_t *report1,
> -                  uint64_t *delta)
> +		  uint32_t *report1,
> +		  uint64_t *delta)
This hunk appears to just be noise.
>   {
>   	uint32_t value0 = *(uint32_t *)(((uint8_t *)report0) + offset);
>   	uint32_t value1 = *(uint32_t *)(((uint8_t *)report1) + offset);
> @@ -681,10 +692,10 @@ accumulate_uint32(size_t offset,
>   
>   static void
>   accumulate_uint40(int a_index,
> -                  uint32_t *report0,
> -                  uint32_t *report1,
> +		  uint32_t *report0,
> +		  uint32_t *report1,
>   		  enum drm_i915_oa_format format,
> -                  uint64_t *delta)
> +		  uint64_t *delta)

Here too

>   {
>   	uint64_t value0 = gen8_read_40bit_a_counter(report0, format, a_index),
>   		 value1 = gen8_read_40bit_a_counter(report1, format, a_index);
> @@ -692,6 +703,19 @@ accumulate_uint40(int a_index,
>   	*delta += gen8_40bit_a_delta(value0, value1);
>   }
>   
> +static void
> +accumulate_uint64(int a_index,
> +		  const uint32_t *report0,
> +		  const uint32_t *report1,
> +		  enum drm_i915_oa_format format,
> +		  uint64_t *delta)
> +{
> +	uint64_t value0 = gen12_read_64bit_a_counter(report0, format, a_index),
> +		 value1 = gen12_read_64bit_a_counter(report1, format, a_index);
> +
> +	*delta += (value1 - value0);
> +}
> +
>   static void
>   accumulate_reports(struct accumulator *accumulator,
>   		   uint32_t *start,
> @@ -717,6 +741,11 @@ accumulate_reports(struct accumulator *accumulator,
>   				  deltas + idx++);
>   	}
>   
> +	for (int i = 0; i < format.n_a64; i++) {
> +		accumulate_uint64(i, start, end, accumulator->format,
> +				  deltas + idx++);
> +	}
> +
>   	for (int i = 0; i < format.n_a; i++) {
>   		accumulate_uint32(format.a_off + 4 * i,
>   				  start, end, deltas + idx++);
> @@ -747,6 +776,9 @@ accumulator_print(struct accumulator *accumulator, const char *title)
>   
>   		for (int i = 0; i < format.n_a40; i++)
>   			igt_debug("\tA%u = %"PRIu64"\n", i, deltas[idx++]);
> +
> +		for (int i = 0; i < format.n_a64; i++)
> +			igt_debug("\tA64_%u = %"PRIu64"\n", i, deltas[idx++]);
>   	} else {
>   		igt_debug("\ttime delta = %"PRIu64"\n", deltas[idx++]);
>   	}
> @@ -803,7 +835,19 @@ gen8_sanity_check_test_oa_reports(const uint32_t *oa_report0,
>   		if (undefined_a_counters[j])
>   			continue;
>   
> -		igt_debug("A%d: delta = %"PRIu64"\n", j, delta);
> +		igt_debug("A40_%d: delta = %"PRIu64"\n", j, delta);
> +		igt_assert(delta <= max_delta);
> +	}
> +
> +	for (int j = 0; j < format.n_a64; j++) {
> +		uint64_t delta = 0;
> +
> +		accumulate_uint64(j, oa_report0, oa_report1, fmt, &delta);
> +
> +		if (undefined_a_counters[j])
> +			continue;
> +
> +		igt_debug("A64_%d: delta = %"PRIu64"\n", format.first_a + j, delta);
>   		igt_assert(delta <= max_delta);
>   	}
>   
> @@ -1368,6 +1412,18 @@ print_reports(uint32_t *oa_report0, uint32_t *oa_report1, int fmt)
>   			  j, value0, value1, delta);
>   	}
>   
> +	for (int j = 0; j < format.n_a64; j++) {
> +		uint64_t value0 = gen12_read_64bit_a_counter(oa_report0, fmt, j);
> +		uint64_t value1 = gen12_read_64bit_a_counter(oa_report1, fmt, j);
> +		uint64_t delta = value1 - value0;
> +
> +		if (undefined_a_counters[j])
> +			continue;
> +
> +		igt_debug("A_64%d: 1st = %"PRIu64", 2nd = %"PRIu64", delta = %"PRIu64"\n",
> +			  format.first_a + j, value0, value1, delta);
> +	}
> +
>   	for (int j = 0; j < format.n_a; j++) {
>   		uint32_t *a0 = (uint32_t *)(((uint8_t *)oa_report0) +
>   					    format.a_off);