[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