[PATCH i-g-t 4/8] tests/intel/xe_drm_fdinfo: Add single engine tests
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Mon Jul 1 18:26:27 UTC 2024
On Mon, Jul 01, 2024 at 12:35:24PM -0500, Lucas De Marchi wrote:
>On Sat, Jun 22, 2024 at 07:00:58AM GMT, Umesh Nerlige Ramappa wrote:
>>Add simple tests that submit work to one engine and measure utilization
>>per class.
>>
>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>>---
>>tests/intel/xe_drm_fdinfo.c | 109 ++++++++++++++++++++++++++++++++++++
>>1 file changed, 109 insertions(+)
>>
>>diff --git a/tests/intel/xe_drm_fdinfo.c b/tests/intel/xe_drm_fdinfo.c
>>index 27459b7f1..852931d71 100644
>>--- a/tests/intel/xe_drm_fdinfo.c
>>+++ b/tests/intel/xe_drm_fdinfo.c
>>@@ -25,6 +25,15 @@
>> * SUBTEST: basic
>> * Description: Check if basic fdinfo content is present
>> *
>>+ * SUBTEST: drm-idle
>>+ * Description: Check that engines show no load when idle
>>+ *
>>+ * SUBTEST: drm-busy-idle
>>+ * Description: Check that engines show load when idle after busy
>>+ *
>>+ * SUBTEST: drm-busy-idle-isolation
>>+ * Description: Check that engine load does not spill over to other drm clients
>>+ *
>
>can we split the mem-related basic check from the utilization-related
>basic checks?
will add a separate basic subtest for utilization.
>
>> * SUBTEST: drm-total-resident
>> * Description: Create and compare total and resident memory consumption by client
>> *
>>@@ -39,11 +48,18 @@ IGT_TEST_DESCRIPTION("Read and verify drm client memory consumption and engine u
>>
>>#define BO_SIZE (65536)
>>
>>+/* flag masks */
>>+#define TEST_BUSY (1 << 0)
>>+#define TEST_TRAILING_IDLE (1 << 1)
>>+#define TEST_ISOLATION (1 << 2)
>
>shoud we document the mapping to the subtests?
If you mean something like what the xe_exec_basic (below) is doing, then
I can try that.
{ "basic", 0 },
{ "basic-defer-mmap", DEFER_ALLOC },
{ "basic-defer-bind", DEFER_ALLOC | DEFER_BIND },
>
>>+
>>struct pceu_cycles {
>> uint64_t cycles;
>> uint64_t total_cycles;
>>};
>>
>>+const unsigned long batch_duration_ns = 500e6;
>
>please use * NSEC_PER_SEC
>
>>+
>>static const char *engine_map[] = {
>> "rcs",
>> "bcs",
>>@@ -76,6 +92,27 @@ static void read_engine_cycles(int xe, struct pceu_cycles *pceu)
>> pceu[class].total_cycles = info.total_cycles[class];
>> }
>>}
>>+
>>+/*
>>+ * Helper for cases where we assert on time spent sleeping (directly or
>>+ * indirectly), so make it more robust by ensuring the system sleep time
>>+ * is within test tolerance to start with.
>>+ */
>>+static unsigned int measured_usleep(unsigned int usec)
>>+{
>>+ struct timespec ts = { };
>>+ unsigned int slept;
>>+
>>+ slept = igt_nsec_elapsed(&ts);
>>+ igt_assert(slept == 0);
>>+ do {
>>+ usleep(usec - slept);
>
>mismatch between usec and nsec here in the first sleep. I guess it's
>easier to convert the arg to nsec and then always have the same unit
>
>>+ slept = igt_nsec_elapsed(&ts) / 1000;
>
>NSEC_PER_USEC
>
>>+ } while (slept < usec);
>>+
>>+ return igt_nsec_elapsed(&ts);
>
>wrong unit? you receive usec as arg and are supposed to return usec, but
>rather return nsec.
Hmm, there is a lot of unit mismatch in this helper. I think I will just
convert everything here to nsec.
>
>Also, I'm not sure why exactly this is needed. usleep() from the C
>library already guarantees it's going to sleep for *at least* the
>specified amount.
>
>usleep(3):
>...
>DESCRIPTION
> The usleep() function suspends execution of the calling thread
> for (at least) usec microseconds. The sleep may be lengthened
> slightly by any system activity or by the time spent processing
> the call or by the granularity of system timers.
>
>RETURN VALUE
> The usleep() function returns 0 on success. On error, -1 is
> returned, with errno set to indicate the error.
>
I think the requirement is to know how long the delay actually was
because the "sleep may be lengthened". All utilization/counter
comparisons will be against the actual delay.
>
>>+}
>>+
>>/* Subtests */
>>static void test_active(int fd, struct drm_xe_engine *engine)
>>{
>>@@ -451,6 +488,66 @@ xe_spin_ctx_destroy(int fd, struct xe_spin_ctx *ctx)
>> free(ctx);
>>}
>>
>>+static void
>>+check_results(struct pceu_cycles *s1, struct pceu_cycles *s2,
>>+ int class, int width, unsigned int flags)
>>+{
>>+ double percent;
>>+
>>+ igt_debug("%s: sample 1: cycles %lu, total_cycles %lu\n",
>>+ engine_map[class], s1[class].cycles, s1[class].total_cycles);
>>+ igt_debug("%s: sample 2: cycles %lu, total_cycles %lu\n",
>>+ engine_map[class], s2[class].cycles, s2[class].total_cycles);
>>+
>>+ percent = ((s2[class].cycles - s1[class].cycles) * 100) /
>>+ ((s2[class].total_cycles + 1) - s1[class].total_cycles);
>>+
>>+ /* for parallel engines scale the busyness with width */
>
>s/parallel engines/parallel submission/ ?
will change
>
>>+ percent = percent / width;
>>+
>>+ igt_debug("%s: percent: %f\n", engine_map[class], percent);
>>+
>>+ if (flags & TEST_BUSY)
>>+ igt_assert(percent >= 95 && percent <= 105);
>
>percent should never be > 100.
Yikes... that's a miss. I did see a couple >=100 though!! and modified
this to check how far it goes above 100, but completely forgot to debug
the issue itself. Will debug.
>Also, for >= 95 to be true this needs to
>be the only client. Should we check for number of clients and skip if
>we find others?
Didn't think of that. Wondering if there is a way to enforce a single
client somehow in the test. If not, then I agree to skip if more clients
are present. Any idea how to check if there are other clients?
>
>>+ else
>>+ igt_assert(!percent);
>>+}
>>+
>>+static void
>>+single(int fd, struct drm_xe_engine_class_instance *hwe, int width, int count,
>>+ unsigned int flags)
>>+{
>>+ struct pceu_cycles pceu1[DRM_XE_ENGINE_CLASS_COMPUTE + 1];
>>+ struct pceu_cycles pceu2[DRM_XE_ENGINE_CLASS_COMPUTE + 1];
>>+ struct xe_spin_ctx *ctx = NULL;
>>+ int local_fd = fd;
>>+ uint32_t vm;
>>+
>>+ if (flags & TEST_ISOLATION)
>>+ local_fd = drm_reopen_driver(fd);
>
>so you have 2 clients in the same process... Maybe add this info to the
>subtest description above.
sure
>
>>+
>>+ vm = xe_vm_create(local_fd, 0, 0);
>>+ if (flags & TEST_BUSY) {
>>+ ctx = xe_spin_ctx_init(local_fd, hwe, vm, width, count);
>>+ xe_spin_sync_start(local_fd, ctx);
>>+ }
>>+
>>+ read_engine_cycles(local_fd, pceu1);
>>+ measured_usleep(batch_duration_ns / 1000);
>>+ if (flags & TEST_TRAILING_IDLE)
>>+ xe_spin_sync_end(local_fd, ctx);
>>+ read_engine_cycles(local_fd, pceu2);
>>+
>>+ check_results(pceu1, pceu2, hwe->engine_class, width, flags);
>>+
>>+ xe_spin_sync_end(local_fd, ctx);
>>+ xe_spin_ctx_destroy(local_fd, ctx);
>>+ xe_vm_destroy(local_fd, vm);
>>+
>>+ if (flags & TEST_ISOLATION)
>>+ close(local_fd);
>
>humn... it doesn't seem this is actually testing any isolation? It's
>just reopening the fd and testing the same thing on the duplicated fd.
>
>what I'd expect of an isolation test is, e.g.:
>
>/proc/X/fdinfo/Y
> drm-client: A
> drm-rcs-cycles: 0
>
>when executing on another fd:
>
>/proc/X/fdinfo/Z
> drm-client: B
> drm-rcs-cyles: 10
>
>
>so, check that read_engine_cycles() returns !0 for fd where you
>submitted the spin and 0 for where you didn't.
will do
Thanks,
Umesh
>
>Lucas De Marchi
>
>>+}
>>+
>>igt_main
>>{
>> struct drm_xe_engine_class_instance *hwe;
>>@@ -485,6 +582,18 @@ igt_main
>> igt_subtest("basic")
>> basic(xe, num_classes);
>>
>>+ igt_subtest("drm-idle")
>>+ xe_for_each_engine(xe, hwe)
>>+ single(xe, hwe, 1, 1, 0);
>>+
>>+ igt_subtest("drm-busy-idle")
>>+ xe_for_each_engine(xe, hwe)
>>+ single(xe, hwe, 1, 1, TEST_BUSY | TEST_TRAILING_IDLE);
>>+
>>+ igt_subtest("drm-busy-idle-isolation")
>>+ xe_for_each_engine(xe, hwe)
>>+ single(xe, hwe, 1, 1, TEST_BUSY | TEST_TRAILING_IDLE | TEST_ISOLATION);
>>+
>> igt_describe("Create and compare total and resident memory consumption by client");
>> igt_subtest("drm-total-resident")
>> test_total_resident(xe);
>>--
>>2.34.1
>>
More information about the igt-dev
mailing list