[Intel-gfx] [igt-dev] [PATCH i-g-t] i915: Exercise sysfs client properties
Chris Wilson
chris at chris-wilson.co.uk
Mon Jan 25 10:29:45 UTC 2021
Quoting Tvrtko Ursulin (2021-01-25 10:20:15)
>
> On 22/01/2021 21:49, Chris Wilson wrote:
> > We store every client name, pid and runtime under sysfs. Better check
> > that matches with the actual client.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> > lib/igt_sysfs.c | 36 +++
> > lib/igt_sysfs.h | 3 +
> > tests/Makefile.sources | 3 +
> > tests/i915/sysfs_clients.c | 450 +++++++++++++++++++++++++++++++++++++
> > tests/meson.build | 1 +
> > 5 files changed, 493 insertions(+)
> > create mode 100644 tests/i915/sysfs_clients.c
> >
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index 6aafe5349..e734143ba 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -378,6 +378,42 @@ uint32_t igt_sysfs_get_u32(int dir, const char *attr)
> > return result;
> > }
> >
> > +/**
> > + * igt_sysfs_get_u64:
> > + * @dir: directory for the device from igt_sysfs_open()
> > + * @attr: name of the sysfs node to open
> > + *
> > + * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
> > + *
> > + * Returns:
> > + * The value read.
> > + */
> > +uint64_t igt_sysfs_get_u64(int dir, const char *attr)
> > +{
> > + uint64_t result;
> > +
> > + if (igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1)
> > + return 0;
> > +
> > + return result;
> > +}
> > +
> > +/**
> > + * igt_sysfs_set_u64:
> > + * @dir: directory for the device from igt_sysfs_open()
> > + * @attr: name of the sysfs node to open
> > + * @value: value to set
> > + *
> > + * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> > + *
> > + * Returns:
> > + * True if successfully written
> > + */
> > +bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> > +{
> > + return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
> > +}
> > +
> > /**
> > * igt_sysfs_set_u32:
> > * @dir: directory for the device from igt_sysfs_open()
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> > index 64935a5ca..56741a0a3 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -47,6 +47,9 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> > uint32_t igt_sysfs_get_u32(int dir, const char *attr);
> > bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> >
> > +uint64_t igt_sysfs_get_u64(int dir, const char *attr);
> > +bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> > +
> > bool igt_sysfs_get_boolean(int dir, const char *attr);
> > bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 1c227e750..3f663fe7e 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -114,6 +114,9 @@ TESTS_progs = \
> > TESTS_progs += api_intel_bb
> > api_intel_bb_SOURCES = i915/api_intel_bb.c
> >
> > +TESTS_progs += sysfs_clients
> > +sysfs_clients_SOURCES = i915/sysfs_clients.c
> > +
> > TESTS_progs += sysfs_defaults
> > sysfs_defaults_SOURCES = i915/sysfs_defaults.c
> >
> > diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> > new file mode 100644
> > index 000000000..a77adec6d
> > --- /dev/null
> > +++ b/tests/i915/sysfs_clients.c
> > @@ -0,0 +1,450 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2021 Intel Corporation
> > + */
> > +
> > +#include <ctype.h>
> > +#include <dirent.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include "drmtest.h"
> > +#include "i915/gem.h"
> > +#include "i915/gem_context.h"
> > +#include "i915/gem_engine_topology.h"
> > +#include "igt_aux.h"
> > +#include "igt_dummyload.h"
> > +#include "igt_sysfs.h"
> > +#include "ioctl_wrappers.h"
> > +
> > +static void pidname(int i915, int clients)
> > +{
> > + struct dirent *de;
> > + int sv[2], rv[2];
> > + char buf[280];
> > + int me = -1;
> > + long count;
> > + pid_t pid;
> > + DIR *dir;
> > + int len;
> > +
> > + dir = fdopendir(dup(clients));
> > + igt_assert(dir);
> > + rewinddir(dir);
> > +
> > + count = 0;
> > + while ((de = readdir(dir))) {
> > + if (!isdigit(de->d_name[0]))
> > + continue;
> > +
> > + snprintf(buf, sizeof(buf), "%s/name", de->d_name);
> > + len = igt_sysfs_read(clients, buf, buf, sizeof(buf));
> > + igt_assert_f(len > 0, "failed to open '%s/name'\n", de->d_name);
> > + buf[len - 1] = '\0';
> > + igt_debug("%s: %s\n", de->d_name, buf);
> > +
> > + /* Ignore closed clients created by drm_driver_open() */
> > + if (*buf == '<')
> > + continue;
> > +
> > + close(me);
> > + me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> > + count++;
> > + }
> > + closedir(dir);
> > +
> > + /* We expect there to be only the single client (us) running */
> > + igt_assert_eq(count, 1);
> > + igt_assert(me >= 0);
> > +
> > + len = igt_sysfs_read(me, "name", buf, sizeof(buf));
> > + igt_assert(len > 0);
> > + buf[len - 1] = '\0';
>
> We could add a helper like igt_syfs_str or something since this repeats
> a lot but not important straight away.
-> strterm()
>
> > +
> > + igt_info("My name: %s\n", buf);
> > + igt_assert(strcmp(buf, igt_test_name()) == 0);
> > +
> > + igt_assert(pipe(sv) == 0);
> > + igt_assert(pipe(rv) == 0);
> > +
> > + /* If give our fd to someone else, they take over ownership of client */
> > + igt_fork(child, 1) {
> > + read(sv[0], &pid, sizeof(pid));
> > +
> > + gem_context_destroy(i915, gem_context_create(i915));
> > +
> > + pid = getpid();
> > + write(rv[1], &pid, sizeof(pid));
> > + }
> > + close(sv[0]);
> > + close(rv[1]);
> > +
> > + /* Child exists, but not yet running, we still own the client */
> > + len = igt_sysfs_read(me, "pid", buf, sizeof(buf));
> > + igt_assert(len > 0);
> > + buf[len - 1] = '\0';
> > +
> > + pid = getpid();
> > + igt_info("My pid: %s\n", buf);
> > + igt_assert_eq(atoi(buf), pid);
> > +
> > + /* Release and wait for the child */
> > + igt_assert_eq(write(sv[1], &pid, sizeof(pid)), sizeof(pid));
> > + igt_assert_eq(read(rv[0], &pid, sizeof(pid)), sizeof(pid));
> > +
> > + /* Now child owns the client and pid should be updated to match */
> > + len = igt_sysfs_read(me, "pid", buf, sizeof(buf));
> > + igt_assert(len > 0);
> > + buf[len - 1] = '\0';
> > +
> > + igt_info("New pid: %s\n", buf);
> > + igt_assert_eq(atoi(buf), pid);
> > + igt_waitchildren();
> > +
> > + /* Child has definitely gone, but the client should remain */
> > + len = igt_sysfs_read(me, "pid", buf, sizeof(buf));
> > + igt_assert(len > 0);
> > + buf[len - 1] = '\0';
> > +
> > + igt_info("Old pid: %s\n", buf);
> > + igt_assert_eq(atoi(buf), pid);
>
> "Old pid" as in pid of the child which exited. Do we have a IGT helper
> to cross check what the child sent via pipe matches what fork said? Or
> what waitpid reported as exited.
For the sake of the test, you can just scroll back and check ;)
> Also, would it revert back to parent pid if parent did one context
> create at this point? Could be worth checking if so.
I added such a test.
> > +
> > + close(sv[1]);
> > + close(rv[0]);
> > + close(me);
> > +}
> > +
> > +static long count_clients(int clients)
> > +{
> > + struct dirent *de;
> > + long count = 0;
> > + char buf[280];
> > + DIR *dir;
> > +
> > + dir = fdopendir(dup(clients));
> > + igt_assert(dir);
> > + rewinddir(dir);
> > +
> > + while ((de = readdir(dir))) {
> > + int len;
> > +
> > + if (!isdigit(de->d_name[0]))
> > + continue;
> > +
> > + snprintf(buf, sizeof(buf), "%s/name", de->d_name);
> > + len = igt_sysfs_read(clients, buf, buf, sizeof(buf));
> > + if (len < 0)
> > + continue;
> > +
> > + count += *buf != '<';
> > + }
> > + closedir(dir);
> > +
> > + return count;
> > +}
> > +
> > +static void create(int i915, int clients)
> > +{
> > + int fd[16];
> > +
> > + /* Each new open("/dev/dri/cardN") is a new client */
> > + igt_assert_eq(count_clients(clients), 1);
> > + for (int i = 0; i < ARRAY_SIZE(fd); i++) {
> > + fd[i] = gem_reopen_driver(i915);
> > + igt_assert_eq(count_clients(clients), i + 2);
> > + }
> > +
> > + for (int i = 0; i < ARRAY_SIZE(fd); i++)
> > + close(fd[i]);
> > +
> > + /* Cleanup delayed behind rcu */
> > + igt_until_timeout(30) {
> > + usleep(0);
>
> Intention to yield?
If you think that's more likely to work :)
>
> > + if (count_clients(clients) == 1)
> > + break;
> > + usleep(10000);
> > + }
> > + igt_assert_eq(count_clients(clients), 1);
>
> A variant which closes a single random client and does a cross check in
> every loop iteration could be useful then opens a new client checking
> id's are unique. Can be added later.
One not-recycling test coming up.
> > + delay = -500000; /* 500us slack */
> > + memset(old, 0, sizeof(old));
> > + for (int pass = 0; pass < 5; pass++) {
> > + delay += measured_usleep(1000);
> > + igt_debug("delay: %'"PRIu64"ns\n", delay);
> > +
> > + /* Check that we accumulate the runtime, while active */
> > + igt_assert_eq(read_runtime(me, active), 1);
> > + igt_info("active1[%d]: %'"PRIu64"ns\n", pass, active[e->class]);
> > + igt_assert(active[e->class] > old[e->class]); /* monotonic */
> > + igt_assert(active[e->class] > delay); /* within reason */
>
> Sending greetings to GuC soon. Will need to emit a high prio pulse to
> preempt the spinner, maybe, no, probably won't be enough.
Exactly. The GuC falls far short of what we need atm. A pulse from every
sysfs read() and wait for pulse completion before returning? What
happens for non-preemptible workloads?
> > + delay = -500000; /* 500us slack */
> > + memset(old, 0, sizeof(old));
> > + for (int pass = 0; pass < 5; pass++) {
> > + delay += measured_usleep(1000);
> > + igt_debug("delay: %'"PRIu64"ns\n", delay);
> > +
> > + /* Check that we accumulate the runtime, while active */
> > + igt_assert_eq(read_runtime(me, active), expect);
> > + for (int i = 0; i < ARRAY_SIZE(active); i++) {
> > + if (!active[i])
> > + continue;
>
> Don't you want do skip based on the bitmap in classes here? Although the
> assert on expect will catch failures to account some class already so
> optional I guess.
Bitmap came afterwards, and indeed it should have replaced the !active[i].
-Chris
More information about the Intel-gfx
mailing list