[PATCH i-g-t] tests/intel/xe_pmu: Add function engine activity test
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Fri Mar 7 22:32:39 UTC 2025
Hi Riana,
I am numbering the comments so that I can let you know if any are optional.
On Tue, Mar 04, 2025 at 03:40:19PM +0530, Riana Tauro wrote:
>Add a test to validate per-function engine activity by
>running cork on all functions simulatenously
>
>Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>---
> tests/intel/xe_pmu.c | 89 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 85 insertions(+), 4 deletions(-)
>
>diff --git a/tests/intel/xe_pmu.c b/tests/intel/xe_pmu.c
>index 4584fc7d3..5a93d581a 100644
>--- a/tests/intel/xe_pmu.c
>+++ b/tests/intel/xe_pmu.c
>@@ -14,6 +14,7 @@
>
> #include "igt.h"
> #include "igt_perf.h"
>+#include "igt_sriov_device.h"
> #include "igt_sysfs.h"
>
> #include "xe/xe_gt.h"
>@@ -106,7 +107,7 @@ static uint64_t add_format_config(const char *xe_device, const char *format, uin
> }
>
> static uint64_t get_event_config(int xe, unsigned int gt, struct drm_xe_engine_class_instance *eci,
>- const char *event)
>+ int function, const char *event)
> {
> char xe_device[100];
> uint64_t pmu_config = 0;
>@@ -122,6 +123,9 @@ static uint64_t get_event_config(int xe, unsigned int gt, struct drm_xe_engine_c
> pmu_config |= add_format_config(xe_device, "engine_instance", eci->engine_instance);
> }
>
>+ if (function != -1)
>+ pmu_config |= add_format_config(xe_device, "function", function);
(1)
How about just have a new helper that takes in the function parameter:
- Make char xe_device[100] global to this file and initialize it in fixture
- define new helper, something like:
static uint64_t get_event_config_fn(int xe, unsigned int gt,
struct drm_xe_engine_class_instance *eci,
uint32_t function,
const char *event)
{
return get_event_config(xe, gt, eci, event) |
add_format_config(xe_device, "function", function);
}
For any counters that are not function specific (rc6 etc.), they can use the old helper.
>+
> return pmu_config;
> }
>
>@@ -143,10 +147,10 @@ static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci, un
> uint32_t vm;
> int pmu_fd[2];
>
>- config = get_event_config(fd, eci->gt_id, eci, "engine-active-ticks");
>+ config = get_event_config(fd, eci->gt_id, eci, -1, "engine-active-ticks");
> pmu_fd[0] = open_group(fd, config, -1);
>
>- config = get_event_config(fd, eci->gt_id, eci, "engine-total-ticks");
>+ config = get_event_config(fd, eci->gt_id, eci, -1, "engine-total-ticks");
> pmu_fd[1] = open_group(fd, config, pmu_fd[0]);
>
> vm = xe_vm_create(fd, 0, 0);
>@@ -187,6 +191,75 @@ static void engine_activity(int fd, struct drm_xe_engine_class_instance *eci, un
> igt_assert(!engine_active_ticks);
> }
>
>+/**
>+ * SUBTEST: fn-engine-activity-load
>+ * Description: Test to validate engine activity by running load on all functions simultaneously
>+ */
>+static void engine_activity_fn(int fd, struct drm_xe_engine_class_instance *eci, int num_fns)
>+{
>+ uint64_t config, engine_active_ticks, engine_total_ticks;
>+ int num = num_fns + 1;
(2)
s/num_fns/num_vfs/ everywhere since it truly is number of vfs.
OR
don't rename, but add 1 to num_fns when you call igt_sriov_get_enabled_vfs().
>+ uint64_t after[2 * num], before[2 * num], pmu_fd[2 * num];
>+ struct pmu_function {
>+ struct xe_cork *cork;
>+ uint32_t vm;
>+ int fd;
(3)
why not just include after[2], before[2], pmu_fd[2] inside this struct?
>+ } fn[num];
>+ int i, idx = 0;
If you include everything within the struct, then you can drop idx. Just use 0 and 1 index.
>+
>+ pmu_fd[0] = -1;
>+ for (i = 0; i < num; i++) {
>+ config = get_event_config(fd, eci->gt_id, eci, i, "engine-active-ticks");
>+ pmu_fd[idx] = open_group(fd, config, pmu_fd[0]);
>+
>+ config = get_event_config(fd, eci->gt_id, eci, i, "engine-total-ticks");
>+ pmu_fd[idx + 1] = open_group(fd, config, pmu_fd[0]);
>+
>+ if (i > 0)
>+ fn[i].fd = igt_sriov_open_vf_drm_device(fd, i);
>+ else
>+ fn[i].fd = fd;
>+
>+ fn[i].vm = xe_vm_create(fn[i].fd, 0, 0);
>+ fn[i].cork = xe_cork_create_opts(fn[i].fd, eci, fn[i].vm, 1, 1);
>+ xe_cork_sync_start(fn[i].fd, fn[i].cork);
>+
>+ idx += 2;
>+ }
>+
>+ pmu_read_multi(pmu_fd[0], 2 * num, before);
>+ usleep(SLEEP_DURATION * USEC_PER_SEC);
>+ pmu_read_multi(pmu_fd[0], 2 * num, after);
ok, I see why you haven't included before/after/pmu_fd in the struct. Maybe you
can still include pmu_fd within the struct since you are only passing
pmu_fd[0] to all the calls (which would be fn[0].pmu_fd[0]. That should still
help get rid of idx in the above loop.
>+ idx = 0;
>+ for (i = 0; i < num; i++) {
For this loop you can use a local idx = num * 2;
>+ xe_cork_sync_end(fn[i].fd, fn[i].cork);
>+
>+ engine_active_ticks = after[idx] - before[idx];
>+ engine_total_ticks = after[idx + 1] - before[idx + 1];
>+
>+ igt_debug("Engine active ticks: after %ld, before %ld delta %ld\n", after[idx],
>+ before[idx], engine_active_ticks);
>+ igt_debug("Engine total ticks: after %ld, before %ld delta %ld\n", after[idx + 1],
>+ before[idx + 1], engine_total_ticks);
(4) Maybe also print the function num.
>+
>+ if (fn[i].cork)
>+ xe_cork_destroy(fn[i].fd, fn[i].cork);
>+
>+ xe_vm_destroy(fn[i].fd, fn[i].vm);
>+
>+ close(pmu_fd[idx]);
>+ close(pmu_fd[idx + 1]);
>+
>+ if (i > 0)
>+ close(fn[i].fd);
>+
>+ assert_within_epsilon(engine_active_ticks, engine_total_ticks, tolerance);
>+
>+ idx += 2;
>+ }
>+}
>+
> /**
> * SUBTEST: gt-c6-idle
> * Description: Basic residency test to validate idle residency
>@@ -201,7 +274,7 @@ static void test_gt_c6_idle(int xe, unsigned int gt)
> uint64_t val;
>
> /* Get the PMU config for the gt-c6 event */
>- pmu_config = get_event_config(xe, gt, NULL, "gt-c6-residency");
>+ pmu_config = get_event_config(xe, gt, NULL, -1, "gt-c6-residency");
>
> pmu_fd = open_pmu(xe, pmu_config);
>
>@@ -253,6 +326,14 @@ igt_main
> test_each_engine("engine-activity-load", fd, eci)
> engine_activity(fd, eci, TEST_LOAD);
>
>+ igt_describe("Validate per-function engine activity");
>+ test_each_engine("fn-engine-activity-load", fd, eci) {
>+ int num_fns = igt_sriov_get_enabled_vfs(fd);
>+
>+ igt_require(igt_sriov_is_pf(fd) && num_fns);
>+ engine_activity_fn(fd, eci, num_fns);
>+ }
>+
> igt_fixture {
> close(fd);
> }
From my end, (3) is optional.
Are you able to configure sched_if_idle? If the results are for a specific sched
policy, then I would add a igt_require in the test.
Thanks,
Umesh
>--
>2.47.1
>
More information about the igt-dev
mailing list