Re: [PATCH i-g-t] tests/intel/oa: Fix oa-exponents test

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

On Thu, Sep 26, 2024 at 01:51:20PM -0700, Ashutosh Dixit wrote:
>test_oa_exponents is assuming that reports is an array of report size
>elements, whereas reports is an array of u32's. The same bug exists in both
>i915 and xe IGT's.
>
>Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2723
>Fixes: a5565306b277 ("tests/intel/xe_oa: Add oa exponent tests")
>Fixes: b3c80c93b544 ("i915/perf: Move OA format array from stack to heap")
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

Yikes!! Thanks for finding and fixing this.

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Umesh
>---
> tests/intel/perf.c  | 19 ++++++++++++-------
> tests/intel/xe_oa.c | 19 ++++++++++++-------
> 2 files changed, 24 insertions(+), 14 deletions(-)
>
>diff --git a/tests/intel/perf.c b/tests/intel/perf.c
>index c5a103c945..a38d1b4919 100644
>--- a/tests/intel/perf.c
>+++ b/tests/intel/perf.c
>@@ -2108,6 +2108,7 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
> #define NUM_TIMER_REPORTS 30
> 		uint32_t *reports = malloc(NUM_TIMER_REPORTS * format_size);
> 		uint32_t *timer_reports = reports;
>+		void *this, *prev;
>
> 		igt_debug("testing OA exponent %d,"
> 			  " expected ts delta = %"PRIu64" (%"PRIu64"ns/%.2fus/%.2fms)\n",
>@@ -2161,21 +2162,25 @@ test_oa_exponents(const struct intel_execution_engine2 *e)
>
> 		__perf_close(stream_fd);
>
>+		this = reports + format_size / 4;
>+		prev = reports;
>+
> 		igt_debug("report%04i ts=%"PRIx64" hw_id=0x%08x\n", 0,
>-			  oa_timestamp(&reports[0], fmt),
>-			  oa_report_get_ctx_id(&reports[0]));
>+			  oa_timestamp(prev, fmt),
>+			  oa_report_get_ctx_id(prev));
> 		for (int i = 1; i < n_timer_reports; i++) {
>-			uint64_t delta = oa_timestamp_delta(&reports[i],
>-							    &reports[i - 1],
>-							    fmt);
>+			uint64_t delta = oa_timestamp_delta(this, prev, fmt);
>
> 			igt_debug("report%04i ts=%"PRIx64" hw_id=0x%08x delta=%"PRIu64" %s\n", i,
>-				  oa_timestamp(&reports[i], fmt),
>-				  oa_report_get_ctx_id(&reports[i]),
>+				  oa_timestamp(this, fmt),
>+				  oa_report_get_ctx_id(this),
> 				  delta, expected_report_timing_delta(delta,
> 								      expected_timestamp_delta) ? "" : "******");
>
> 			matches += expected_report_timing_delta(delta,expected_timestamp_delta);
>+
>+			this += format_size;
>+			prev += format_size;
> 		}
>
> 		igt_debug("matches=%u/%u\n", matches, n_timer_reports - 1);
>diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>index aae9be2c40..5d7a40090b 100644
>--- a/tests/intel/xe_oa.c
>+++ b/tests/intel/xe_oa.c
>@@ -1692,6 +1692,7 @@ static void test_oa_exponents(const struct drm_xe_engine_class_instance *hwe)
> #define NUM_TIMER_REPORTS 30
> 		uint32_t *reports = malloc(NUM_TIMER_REPORTS * format_size);
> 		uint32_t *timer_reports = reports;
>+		void *this, *prev;
>
> 		igt_debug("testing OA exponent %d,"
> 			  " expected ts delta = %"PRIu64" (%"PRIu64"ns/%.2fus/%.2fms)\n",
>@@ -1741,21 +1742,25 @@ static void test_oa_exponents(const struct drm_xe_engine_class_instance *hwe)
>
> 		__perf_close(stream_fd);
>
>+		this = reports + format_size / 4;
>+		prev = reports;
>+
> 		igt_debug("report%04i ts=%"PRIx64" hw_id=0x%08x\n", 0,
>-			  oa_timestamp(&reports[0], fmt),
>-			  oa_report_get_ctx_id(&reports[0]));
>+			  oa_timestamp(prev, fmt),
>+			  oa_report_get_ctx_id(prev));
> 		for (int i = 1; i < n_timer_reports; i++) {
>-			uint64_t delta = oa_timestamp_delta(&reports[i],
>-							    &reports[i - 1],
>-							    fmt);
>+			uint64_t delta = oa_timestamp_delta(this, prev, fmt);
>
> 			igt_debug("report%04i ts=%"PRIx64" hw_id=0x%08x delta=%"PRIu64" %s\n", i,
>-				  oa_timestamp(&reports[i], fmt),
>-				  oa_report_get_ctx_id(&reports[i]),
>+				  oa_timestamp(this, fmt),
>+				  oa_report_get_ctx_id(this),
> 				  delta, expected_report_timing_delta(delta,
> 								      expected_timestamp_delta) ? "" : "******");
>
> 			matches += expected_report_timing_delta(delta,expected_timestamp_delta);
>+
>+			this += format_size;
>+			prev += format_size;
> 		}
>
> 		igt_debug("matches=%u/%u\n", matches, n_timer_reports - 1);
>-- 
>2.41.0
>