[PATCH i-g-t,v3] tests/intel/xe_sysfs_preempt_timeout: Porting sysfs preempt test in xe
Ch, Sai Gowtham
sai.gowtham.ch at intel.com
Wed Jul 31 10:34:28 UTC 2024
>-----Original Message-----
>From: Goyal, Nakshtra <nakshtra.goyal at intel.com>
>Sent: Tuesday, July 30, 2024 7:02 PM
>To: igt-dev at lists.freedesktop.org; Gandi, Ramadevi <ramadevi.gandi at intel.com>
>Cc: Kumar, Janga Rahul <janga.rahul.kumar at intel.com>; Ch, Sai Gowtham
><sai.gowtham.ch at intel.com>
>Subject: [PATCH i-g-t,v3] tests/intel/xe_sysfs_preempt_timeout: Porting sysfs
>preempt test in xe
>
>From: Nakshtra Goyal <nakshtra.goyal at intel.com>
>
>Test solution which should allow igt runner to properly run
>xe_sysfs_preempt_timeout, so adding one into xe BAT runs
>
>Signed-off-by: Nakshtra Goyal <nakshtra.goyal at intel.com>
>---
> tests/intel-ci/xe-fast-feedback.testlist | 2 +
> tests/intel/xe_sysfs_preempt_timeout.c | 243 +++++++++++++++++++++++
> tests/meson.build | 1 +
> 3 files changed, 246 insertions(+)
> create mode 100644 tests/intel/xe_sysfs_preempt_timeout.c
>
>diff --git a/tests/intel-ci/xe-fast-feedback.testlist b/tests/intel-ci/xe-fast-
>feedback.testlist
>index 01b01dcf9..4ba90e2dd 100644
>--- a/tests/intel-ci/xe-fast-feedback.testlist
>+++ b/tests/intel-ci/xe-fast-feedback.testlist
>@@ -1,6 +1,8 @@
> # Should be the first test
> igt at xe_module_load@load
>
>+igt at xe_sysfs_preempt_timeout@preempt_timeout_us-timeout
>+
It's better to create a separate patch for this.
> igt at fbdev@eof
> igt at fbdev@info
> igt at fbdev@nullptr
>diff --git a/tests/intel/xe_sysfs_preempt_timeout.c
>b/tests/intel/xe_sysfs_preempt_timeout.c
>new file mode 100644
>index 000000000..8e0612c47
>--- /dev/null
>+++ b/tests/intel/xe_sysfs_preempt_timeout.c
>@@ -0,0 +1,243 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+/**
>+ * TEST: sysfs preempt timeout
>+ * Category: Core
>+ * Mega feature: SysMan
>+ * Sub-category: SysMan tests
>+ * Functionality: sysfs preempt timeout
>+ * Feature: SMI, context
>+ * Test category: SysMan
>+ *
>+ * SUBTEST: %s-timeout
>+ * Description: Test to measure the delay from requestion the preemption to its
>+ * completion. Send down some non-preemptable workloads and then
>+ * request a switch to a higher priority context. The HW will not
>+ * be able to respond, so the kernel will be forced to reset the hog.
>+ * Test category: functionality test
>+ *
>+ * arg[1]:
>+ *
>+ * @preempt_timeout_us: preempt timeout us
>+ */
>+
>+#include <dirent.h>
>+#include <errno.h>
>+#include <fcntl.h>
>+#include <inttypes.h>
>+#include <sched.h>
>+#include <sys/types.h>
>+#include <unistd.h>
>+#include "xe/xe_spin.h"
>+#include "igt_syncobj.h"
>+#include "lib/intel_reg.h"
>+#include "xe_drm.h"
>+#include "xe/xe_ioctl.h"
>+#include "xe/xe_query.h"
>+
>+#include "igt.h"
>+#include "igt_params.h"
>+#include "drmtest.h"
>+#include "igt_debugfs.h"
>+#include "igt_dummyload.h"
>+#include "igt_sysfs.h"
>+#include "intel_allocator.h"
>+#include "sw_sync.h"
>+
Do we need all these headers ? Can you cross check that and remove the unwanted ones ?
>+#define ATTR "preempt_timeout_us"
>+
>+static void set_preempt_timeout(int engine, unsigned int value) {
>+ unsigned int delay;
>+
>+ igt_assert_lte(0, igt_sysfs_printf(engine, ATTR, "%u", value));
>+ igt_sysfs_scanf(engine, ATTR, "%u", &delay);
>+ igt_assert_eq(delay, value);
>+}
>+
>+static uint64_t __test_timeout(int xe, int engine, unsigned int
>+timeout) {
>+ struct drm_xe_sync sync = {
>+ .handle = syncobj_create(xe, 0),
>+ .type = DRM_XE_SYNC_TYPE_SYNCOBJ,
>+ .flags = DRM_XE_SYNC_FLAG_SIGNAL,
>+ };
>+
>+ struct drm_xe_exec exec1 = {
>+ .num_batch_buffer = 1,
>+ .num_syncs = 1,
>+ .syncs = to_user_pointer(&sync),
>+ };
>+ struct drm_xe_exec exec2 = {
>+ .num_batch_buffer = 1,
>+ };
>+/* high priority property */
>+ struct drm_xe_ext_set_property ext = {
>+ .base.next_extension = 0,
>+ .base.name =
>DRM_XE_EXEC_QUEUE_EXTENSION_SET_PROPERTY,
>+ .property = DRM_XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY,
>+ .value = 2, /* High priority */
>+ };
>+ struct drm_xe_engine_class_instance *hwe;
>+ uint64_t ahnd = 0;
>+ uint32_t exec_queues[2];
>+ uint32_t vm[2];
>+ uint32_t bo[2];
Declare the variables of same data types in a single line.
>+ size_t bo_size;
>+ struct xe_spin *spin[2];
>+ struct timespec ts = {};
>+ double elapsed;
>+ uint64_t addr1 = 0x1a0000;
>+ uint64_t addr2 = 0x100000;
>+ int i, n_engines;
>+
>+ n_engines = 0;
>+ /* select an random engine */
>+ i = rand() % xe_number_engines(xe);
>+ xe_for_each_engine(xe, hwe) {
>+ if (i == n_engines++)
>+ break;
>+ }
>+
>+ /* set preempt timeout*/
>+ set_preempt_timeout(engine, timeout);
>+ vm[0] = xe_vm_create(xe, 0, 0);
>+ vm[1] = xe_vm_create(xe, 0, 0);
>+ exec_queues[0] = xe_exec_queue_create(xe, vm[0], hwe, 0);
>+ exec_queues[1] = xe_exec_queue_create(xe, vm[1], hwe,
>to_user_pointer(&ext));
>+ ahnd = intel_allocator_open(xe, 0, INTEL_ALLOCATOR_RELOC);
>+ bo_size = xe_bb_size(xe, sizeof(*spin));
>+ bo[0] = xe_bo_create(xe, vm[0], bo_size, vram_if_possible(xe, 0), 0);
>+ spin[0] = xe_bo_map(xe, bo[0], bo_size);
>+ xe_vm_bind_async(xe, vm[0], 0, bo[0], 0, addr1, bo_size, &sync, 1);
>+ xe_spin_init_opts(spin[0], .addr = addr1,
>+ .preempt = false);
>+ exec1.address = addr1;
>+ exec1.exec_queue_id = exec_queues[0];
>+ xe_exec(xe, &exec1);
>+ xe_spin_wait_started(spin[0]);
>+
>+ igt_nsec_elapsed(&ts);
>+ bo[1] = xe_bo_create(xe, vm[1], bo_size, vram_if_possible(xe, 0), 0);
>+ spin[1] = xe_bo_map(xe, bo[1], bo_size);
>+ xe_vm_bind_sync(xe, vm[1], bo[1], 0, addr2, bo_size);
Any specific reason to use vm_bind_async for first spinner and vm_bind_sync for second spinner ?
>+ xe_spin_init_opts(spin[1], .addr = addr2);
>+ exec2.address = addr2;
>+ exec2.exec_queue_id = exec_queues[1];
No need to initialise two exec's like exec1 and exec2, submitting xe_exec with single exec struct for both the spinners should be enough.
It's redundant.
>+ xe_exec(xe, &exec2);
>+ xe_spin_wait_started(spin[1]);
>+ elapsed = igt_nsec_elapsed(&ts);
>+ xe_spin_end(spin[1]);
>+
>+ xe_vm_unbind_async(xe, vm[0], 0, 0, addr1, bo_size, &sync, 1);
>+ igt_assert(syncobj_wait(xe, &sync.handle, 1, INT64_MAX, 0, NULL));
>+
>+ xe_spin_end(spin[0]);
>+ xe_vm_unbind_sync(xe, vm[1], 0, addr2, bo_size);
>+ syncobj_destroy(xe, sync.handle);
>+
>+ xe_exec_queue_destroy(xe, exec_queues[0]);
>+ xe_vm_destroy(xe, vm[0]);
>+ xe_exec_queue_destroy(xe, exec_queues[1]);
>+ xe_vm_destroy(xe, vm[1]);
>+
>+ put_ahnd(ahnd);
>+ return elapsed;
>+}
>+
>+static void test_timeout(int xe, int engine, const char **property) {
>+ int delays[] = { 1000, 50000, 100000, 500000 };
>+ unsigned int saved;
>+ uint64_t elapsed;
>+ int epsilon;
>+
>+ /*
>+ * Send down some non-preemptable workloads and then request a
>+ * switch to a higher priority context. The HW will not be able to
>+ * respond, so the kernel will be forced to reset the hog. This
>+ * timeout should match our specification, and so we can measure
>+ * the delay from requesting the preemption to its completion.
>+ */
>+
>+ igt_assert(igt_sysfs_scanf(engine, ATTR, "%u", &saved) == 1);
>+ igt_debug("Initial %s:%u\n", ATTR, saved);
>+
>+ elapsed = __test_timeout(xe, engine, 1000);
>+ epsilon = 2 * elapsed / 1000;
>+ if (epsilon < 50000)
>+ epsilon = 50000;
>+ igt_info("Minimum timeout measured as %.3fus; setting error threshold to
>%dus\n",
>+ elapsed * 1e-3, epsilon);
>+ igt_require(epsilon < 10000000);
>+
>+ for (int i = 0; i < ARRAY_SIZE(delays); i++) {
>+ elapsed = __test_timeout(xe, engine, delays[i]);
>+ igt_info("%s:%d, elapsed=%.3fus\n",
>+ ATTR, delays[i], elapsed * 1e-3);
>+
>+ /*
>+ * We need to give a couple of jiffies slack for the scheduler
>+ * timeouts and then a little more slack fr the overhead in
>+ * submitting and measuring.
>+ */
>+ igt_assert_f(elapsed / 1000 / 1000 < delays[i] + epsilon,
>+ "Forced preemption timeout exceeded
>request!\n");
>+ }
>+
>+ set_preempt_timeout(engine, saved);
>+}
>+
>+igt_main
>+{
>+ static const struct {
>+ const char *name;
>+ void (*fn)(int, int, const char **);
>+ } tests[] = {
>+ { "timeout", test_timeout },
>+ { }
>+ };
>+
>+ const char *property[][3] = { {"preempt_timeout_us",
>"preempt_timeout_min", "preempt_timeout_max"},
Please check the alignment here.
>+ };
>+ int count = sizeof(property) / sizeof(property[0]);
>+ int xe = -1;
>+ int sys_fd;
>+ int gt;
Use single line to declare the variable.
>+
>+ igt_fixture {
>+ xe = drm_open_driver(DRIVER_XE);
>+ xe_device_get(xe);
>+
>+ sys_fd = igt_sysfs_open(xe);
>+ igt_require(sys_fd != -1);
>+ close(sys_fd);
>+ }
>+
>+ for (int i = 0; i < count; i++) {
>+ for (typeof(*tests) *t = tests; t->name; t++) {
>+ igt_subtest_with_dynamic_f("%s-%s", property[i][0], t-
>>name) {
>+ xe_for_each_gt(xe, gt) {
>+ int engines_fd = -1;
>+ int gt_fd = -1;
Declare them globally, this will make code redundant for each loop, bring this out of the loop.
Idea of this test looks good however few cleanups need to be done as mentioned in the above comments.
----
Sai Gowtham CH
>+
>+ gt_fd = xe_sysfs_gt_open(xe, gt);
>+ igt_require(gt_fd != -1);
>+ engines_fd = openat(gt_fd, "engines",
>O_RDONLY);
>+ igt_require(engines_fd != -1);
>+
>+ igt_sysfs_engines(xe, engines_fd,
>property[i], t->fn);
>+ close(engines_fd);
>+ close(gt_fd);
>+ }
>+ }
>+ }
>+ }
>+ igt_fixture {
>+ xe_device_put(xe);
>+ close(xe);
>+ }
>+}
>diff --git a/tests/meson.build b/tests/meson.build index e649466be..335c8b837
>100644
>--- a/tests/meson.build
>+++ b/tests/meson.build
>@@ -315,6 +315,7 @@ intel_xe_progs = [
> 'xe_spin_batch',
> 'xe_sysfs_defaults',
> 'xe_sysfs_scheduler',
>+ 'xe_sysfs_preempt_timeout',
> ]
>
> chamelium_progs = [
>--
>2.34.1
More information about the igt-dev
mailing list