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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Sep 06, 2022 at 04:05:59PM +0300, Lionel Landwerlin wrote:
>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@intel.com>
>
>With the unnecessary changes removed :

The noise is from minor checkpatch corrections - spaces to tabs in the 
beginning of the line. I agree, it's not relevant to this patch.

Do you still want me to remove them? I am okay either ways.

Thanks,
Umesh

>
>Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@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);
>
>