Re: [PATCH i-g-t v2 3/3] tests/intel/xe_pmu: Add idle engine activity tests

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

On Mon, Feb 17, 2025 at 07:50:18PM +0530, Riana Tauro wrote:
>Add two tests to validate engine activity counters
>(engine-active-ticks, engine-total-ticks) when idle and
>trailing idle (workload is ended before reading counters)
>
>v2: add trailing idle test
>    rename busy to load (Umesh)
>
>Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>---
> tests/intel/xe_pmu.c | 40 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 34 insertions(+), 6 deletions(-)
>
>diff --git a/tests/intel/xe_pmu.c b/tests/intel/xe_pmu.c
>index aa7d53523..5d2178b62 100644
>--- a/tests/intel/xe_pmu.c
>+++ b/tests/intel/xe_pmu.c
>@@ -11,6 +11,12 @@
>  * Sub-category: Telemetry
>  * Test category: Functional tests
>  *
>+ * SUBTEST: engine-activity-idle
>+ * Description: Test to validate engine activity shows no load when idle
>+ *
>+ * SUBTEST: engine-activity-load-idle
>+ * Description: Test to validate engine activity with full load followed by trailing idle
>+ *
>  * SUBTEST: engine-activity-load
>  * Description: Test to validate engine activity stats by running a workload and
>  *              reading engine active ticks and engine total ticks PMU counters
>@@ -24,6 +30,10 @@
> #define SLEEP_DURATION 2 /* in seconds */
> const double tolerance = 0.1;
>
>+/* flag masks */
>+#define TEST_LOAD		BIT(0)
>+#define TEST_TRAILING_IDLE	BIT(1)
>+
> #define assert_within_epsilon(x, ref, tolerance) \
> 	igt_assert_f((double)(x) <= (1.0 + (tolerance)) * (double)(ref) && \
> 		     (double)(x) >= (1.0 - (tolerance)) * (double)(ref), \
>@@ -95,7 +105,7 @@ static uint64_t get_event_config(int xe, unsigned int gt, struct drm_xe_engine_c
> 	return pmu_config;
> }
>
>-static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci)
>+static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci, unsigned int flags)
> {
> 	uint64_t config, engine_active_ticks, engine_total_ticks, before[2], after[2];
> 	struct xe_cork *cork = NULL;
>@@ -109,14 +119,20 @@ static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci)
> 	pmu_fd[1] = open_group(fd, config, pmu_fd[0]);
>
> 	vm = xe_vm_create(fd, 0, 0);
>-	cork = xe_cork_create_opts(fd, eci, vm, 1, 1);
>-	xe_cork_sync_start(fd, cork);
>+
>+	if (flags & TEST_LOAD) {
>+		cork = xe_cork_create_opts(fd, eci, vm, 1, 1);
>+		xe_cork_sync_start(fd, cork);
>+	}
>
> 	pmu_read_multi(pmu_fd[0], 2, before);
> 	usleep(SLEEP_DURATION * USEC_PER_SEC);
>+	if (flags & TEST_TRAILING_IDLE)
>+		xe_cork_sync_end(fd, cork);
> 	pmu_read_multi(pmu_fd[0], 2, after);
>
>-	xe_cork_sync_end(fd, cork);
>+	if ((flags & TEST_LOAD) && !cork->ended)

Looks like !cork->ended is already inside xe_cork_sync_end(), but comes 
with a Warning!!. I would move xe_cork_sync_end() into a helper within 
this file and check for that. That way we can continue using the helper 
in future tests rather that adding !cork->ended in checks similar to 
above. But, this is is fine for now and we can look at it when more 
tests come in.

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

Thanks,
Umesh


>+		xe_cork_sync_end(fd, cork);
>
> 	engine_active_ticks = after[0] - before[0];
> 	engine_total_ticks = after[1] - before[1];
>@@ -134,7 +150,10 @@ static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci)
> 	close(pmu_fd[0]);
> 	close(pmu_fd[1]);
>
>-	assert_within_epsilon(engine_active_ticks, engine_total_ticks, tolerance);
>+	if (flags & TEST_LOAD)
>+		assert_within_epsilon(engine_active_ticks, engine_total_ticks, tolerance);
>+	else
>+		igt_assert(!engine_active_ticks);
> }
>
> igt_main
>@@ -146,9 +165,18 @@ igt_main
> 		fd = drm_open_driver(DRIVER_XE);
> 	}
>
>+	igt_describe("Validate there is no engine activity when idle");
>+	test_each_engine("engine-activity-idle", fd, eci)
>+		engine_activity(fd, eci, 0);
>+
>+	igt_describe("Validate engine activity with load and trailing idle");
>+	test_each_engine("engine-activity-load-idle", fd, eci)
>+		engine_activity(fd, eci, TEST_LOAD | TEST_TRAILING_IDLE);
>+
> 	igt_describe("Validate engine activity with workload");
> 	test_each_engine("engine-activity-load", fd, eci)
>-		engine_activity(fd, eci);
>+		engine_activity(fd, eci, TEST_LOAD);
>+
>
> 	igt_fixture {
> 		close(fd);
>-- 
>2.47.1
>