[igt-dev] [PATCH i-g-t 07/23] i915/perf: Add support for 64-bit counters
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Tue Sep 6 20:58:48 UTC 2022
On Tue, Sep 06, 2022 at 11:02:49PM +0300, Lionel Landwerlin wrote:
>On 06/09/2022 22:37, Umesh Nerlige Ramappa wrote:
>>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 at 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
>
>
>I'm not familiar with checkpatch but I'm struggling to see why it
>would insert random changes like that.
>
>If you ran it on previous patches it would have produced the same
>isolated changes no?
>
>It would be best to have single commit doing coding style changes at
>the beginning or end of the series.
I am going to drop the coding style changes here and follow that up as a
separate series since it's not relevant here.
Thanks,
Umesh
>
>
>Thanks,
>
>
>-Lionel
>
>
>>
>>>
>>>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);
>>>
>>>
>
More information about the igt-dev
mailing list