Re: [PATCH i-g-t 2/3] tests/intel/xe_pmu: Add PMU test to validate engine activity stats

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

On Wed, Feb 12, 2025 at 03:28:28PM +0530, Riana Tauro wrote:
>Add a test to validate engine activity by reading PMU counters
>(engine-active-ticks, engine-total-ticks) when running
>workload on every engine
>
>Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>---
> tests/intel/xe_pmu.c | 147 +++++++++++++++++++++++++++++++++++++++++++
> tests/meson.build    |   1 +
> 2 files changed, 148 insertions(+)
> create mode 100644 tests/intel/xe_pmu.c
>
>diff --git a/tests/intel/xe_pmu.c b/tests/intel/xe_pmu.c
>new file mode 100644
>index 000000000..cbb825755
>--- /dev/null
>+++ b/tests/intel/xe_pmu.c
>@@ -0,0 +1,147 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2025 Intel Corporation
>+ */
>+
>+/**
>+ * TEST: Test Xe PMU(Performance Monitoring Unit) functionality
>+ * Category: Metrics
>+ * Functionality: Power/Perf
>+ * Mega feature: Performance Monitoring Unit
>+ * Sub-category: Telemetry
>+ * Test category: Functional tests
>+ */
>+
>+#include "igt.h"
>+#include "igt_perf.h"
>+
>+#include "xe/xe_ioctl.h"
>+#include "xe/xe_spin.h"
>+
>+#define SLEEP_DURATION 2 /* in seconds */
>+const double tolerance = 0.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), \
>+		     "'%s' != '%s' (%f not within +%.1f%%/-%.1f%% tolerance of %f)\n",\
>+		     #x, #ref, (double)(x), \
>+		     (tolerance) * 100.0, (tolerance) * 100.0, \
>+		     (double)(ref))
>+
>+static int open_group(int xe, uint64_t config, int group)
>+{
>+	int fd;
>+
>+	fd = igt_perf_open_group(xe_perf_type_id(xe), config, group);
>+	igt_skip_on(fd < 0 && errno == ENODEV);
>+	igt_assert(fd >= 0);
>+
>+	return fd;
>+}
>+
>+static uint64_t pmu_read_multi(int fd, unsigned int num, uint64_t *val)
>+{
>+	uint64_t buf[2 + num];
>+	unsigned int i;
>+
>+	igt_assert_eq(read(fd, buf, sizeof(buf)), sizeof(buf));
>+
>+	for (i = 0; i < num; i++)
>+		val[i] = buf[2 + i];
>+
>+	return buf[1];
>+}
>+
>+static uint64_t get_event_config(int xe, unsigned int gt, struct drm_xe_engine_class_instance *eci,
>+				 const char *event)
>+{
>+	int ret;
>+	char xe_device[100];
>+	uint64_t pmu_config;
>+	u32 start, end;

end is unused, not sure if the c6 patches use them. If unused, then I 
recommend just dropping it from the helper as well and s/start/shift/.

>+
>+	xe_perf_device(xe, xe_device, sizeof(xe_device));
>+	ret = perf_event_config(xe_device, event, &pmu_config);
>+	igt_assert(ret >= 0);

Well, I commented on Vinay's patch to assert within the helper, so these 
checks can be removed here. If that happens, then please drop the return 
value checks here. If not, then checks needed below as well at (1) and 
(2).

>+	ret = perf_event_format(xe_device, "gt", &start, &end);
>+	igt_assert(ret >= 0);
>+	pmu_config |= (uint64_t)gt << start;
>+
>+	if (eci) {
>+		ret = perf_event_format(xe_device, "engine_class", &start, &end);
(1)
>+		pmu_config |= (uint64_t)eci->engine_class << start;
>+		ret = perf_event_format(xe_device, "engine_instance", &start, &end);
(2)
>+		pmu_config |= (uint64_t)eci->engine_instance << start;
>+	}
>+
>+	return pmu_config;
>+}

Thanks,
Umesh
>+
>+/**
>+ * SUBTEST: engine-activity
>+ * Description: Test to validate engine activity stats by running a workload and
>+ *              reading the active ticks and total ticks PMU counters
>+ */
>+static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci)
>+{
>+	uint64_t config, busy_ticks, total_ticks, before[2], after[2];
>+	struct xe_cork *cork = NULL;
>+	uint32_t vm;
>+	int pmu_fd[2];
>+
>+	config = get_event_config(fd, eci->gt_id, eci, "engine-active-ticks");
>+	pmu_fd[0] = open_group(fd, config, -1);
>+
>+	config = get_event_config(fd, eci->gt_id, eci, "engine-total-ticks");
>+	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);
>+
>+	pmu_read_multi(pmu_fd[0], 2, before);
>+	usleep(SLEEP_DURATION * USEC_PER_SEC);
>+	pmu_read_multi(pmu_fd[0], 2, after);
>+
>+	xe_cork_sync_end(fd, cork);
>+
>+	busy_ticks = after[0] - before[0];
>+	total_ticks = after[1] - before[1];
>+
>+	igt_debug("Engine active ticks:  after %ld, before %ld delta %ld\n", after[0], before[0],
>+		  busy_ticks);
>+	igt_debug("Total ticks: after %ld, before %ld delta %ld\n", after[1], before[1],
>+		  total_ticks);
>+
>+	if (cork)
>+		xe_cork_destroy(fd, cork);
>+
>+	xe_vm_destroy(fd, vm);
>+
>+	close(pmu_fd[0]);
>+	close(pmu_fd[1]);
>+
>+	assert_within_epsilon(busy_ticks, total_ticks, tolerance);
>+}
>+
>+igt_main
>+{
>+	int fd;
>+	struct drm_xe_engine_class_instance *hwe;
>+
>+	igt_fixture {
>+		fd = drm_open_driver(DRIVER_XE);
>+	}
>+
>+	igt_describe("Validate engine activity with workload running by reading pmu counters");
>+	igt_subtest_with_dynamic("engine-activity")
>+		xe_for_each_engine(fd, hwe)
>+			igt_dynamic_f("engine-%s%d", xe_engine_class_string(hwe->engine_class),
>+				      hwe->engine_instance)
>+				engine_activity(fd, hwe);
>+
>+	igt_fixture {
>+		close(fd);
>+	}
>+}
>diff --git a/tests/meson.build b/tests/meson.build
>index 33dffad31..d20f50766 100644
>--- a/tests/meson.build
>+++ b/tests/meson.build
>@@ -309,6 +309,7 @@ intel_xe_progs = [
> 	'xe_pat',
> 	'xe_peer2peer',
> 	'xe_pm',
>+	'xe_pmu',
> 	'xe_pm_residency',
> 	'xe_prime_self_import',
> 	'xe_query',
>-- 
>2.47.1
>