[Intel-gfx] [igt-dev] [PATCH i-g-t] i915: Exercise sysfs client properties
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Jan 25 10:20:15 UTC 2021
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.
> +
> + 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.
Also, would it revert back to parent pid if parent did one context
create at this point? Could be worth checking if so.
> +
> + 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 (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.
> +}
> +
> +static int find_me(int clients, pid_t pid)
> +{
> + struct dirent *de;
> + char buf[280];
> + int me = -1;
> + DIR *dir;
> +
> + dir = fdopendir(dup(clients));
> + igt_assert(dir);
> + rewinddir(dir);
> +
> + while ((de = readdir(dir))) {
> + int ret;
> +
> + if (!isdigit(de->d_name[0]))
> + continue;
> +
> + snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
> + ret = igt_sysfs_read(clients, buf, buf, sizeof(buf));
> + igt_assert_f(ret > 0, "failed to open '%s/pid'\n", de->d_name);
> + if (atoi(buf) != pid)
> + continue;
> +
> + me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> + break;
> + }
> +
> + closedir(dir);
> + return me;
> +}
> +
> +#define MAX_CLASS 64
> +static int read_runtime(int client, uint64_t *runtime)
> +{
> + int fd = openat(client, "busy", O_DIRECTORY | O_RDONLY);
> + DIR *dir = fdopendir(fd);
> + struct dirent *de;
> + int count = 0;
> +
> + memset(runtime, 0, sizeof(*runtime) * MAX_CLASS);
> + while ((de = readdir(dir))) {
> + int class;
> +
> + if (!isdigit(de->d_name[0]))
> + continue;
> +
> + class = atoi(de->d_name);
> + igt_assert(class < MAX_CLASS);
> + runtime[class] = igt_sysfs_get_u64(fd, de->d_name);
> +
> + count += runtime[class] != 0;
> + }
> + closedir(dir);
> +
> + return count;
> +}
> +
> +static uint64_t measured_usleep(unsigned int usec)
> +{
> + struct timespec tv;
> + unsigned int slept;
> +
> + slept = igt_nsec_elapsed(memset(&tv, 0, sizeof(tv)));
> + igt_assert(slept == 0);
> + do {
> + usleep(usec - slept);
> + slept = igt_nsec_elapsed(&tv) / 1000;
> + } while (slept < usec);
> +
> + return igt_nsec_elapsed(&tv);
> +}
> +
> +static void
> +busy_one(int device, int clients, const struct intel_execution_engine2 *e)
> +{
> + uint64_t active[MAX_CLASS];
> + uint64_t idle[MAX_CLASS];
> + uint64_t old[MAX_CLASS];
> + igt_spin_t *spin;
> + int64_t delay;
> + int i915;
> + int me;
> +
> + /* Create a fresh client with 0 runtime */
> + i915 = gem_reopen_driver(device);
> + gem_context_copy_engines(device, 0, i915, 0);
> +
> + me = find_me(clients, getpid());
> + igt_assert(me != -1);
> + igt_require(faccessat(me, "busy", 0, F_OK) == 0);
> +
> + spin = igt_spin_new(i915,
> + gem_context_clone_with_engines(i915, 0),
> + .engine = e->flags,
> + .flags = IGT_SPIN_POLL_RUN);
> + igt_spin_busywait_until_started(spin);
> +
> + 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.
> + }
> +
> + gem_quiescent_gpu(i915);
> +
> + /* And again now idle */
> + igt_assert_eq(read_runtime(me, idle), 1);
> + igt_info("idle: %'"PRIu64"ns\n", idle[e->class]);
> + igt_assert(idle[e->class] >= active[e->class]);
> +
> + gem_context_destroy(i915, spin->execbuf.rsvd1);
> +
> + /* And finally after the executing context is no more */
> + igt_assert_eq(read_runtime(me, old), 1);
> + igt_info("old: %'"PRIu64"ns\n", old[e->class]);
> + igt_assert_eq_u64(old[e->class], idle[e->class]);
> +
> + /* Once more on the default context for good luck */
> + igt_spin_reset(spin);
> + spin->execbuf.rsvd1 = 0;
> + gem_execbuf(i915, &spin->execbuf);
> + igt_spin_busywait_until_started(spin);
> +
> + 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("active0[%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 */
> + }
> +
> + gem_quiescent_gpu(i915);
> +
> +
> + igt_spin_free(i915, spin);
> + close(i915);
> +}
> +
> +static void busy_all(int device, int clients)
> +{
> + const struct intel_execution_engine2 *e;
> + uint64_t active[MAX_CLASS];
> + uint64_t idle[MAX_CLASS];
> + uint64_t old[MAX_CLASS];
> + uint64_t classes = 0;
> + igt_spin_t *spin;
> + int expect = 0;
> + int64_t delay;
> + int i915;
> + int me;
> +
> + /* Create a fresh client with 0 runtime */
> + i915 = gem_reopen_driver(device);
> + gem_context_copy_engines(device, 0, i915, 0);
> +
> + me = find_me(clients, getpid());
> + igt_assert(me != -1);
> + igt_require(faccessat(me, "busy", 0, F_OK) == 0);
> +
> + spin = igt_spin_new(i915,
> + gem_context_clone_with_engines(i915, 0),
> + .flags = IGT_SPIN_POLL_RUN);
> + __for_each_physical_engine(i915, e) {
> + spin->execbuf.flags &= ~63;
> + spin->execbuf.flags |= e->flags;
> + gem_execbuf(i915, &spin->execbuf);
> +
> + if (!(classes & (1ull << e->class)))
> + expect++;
> + classes |= 1ull << e->class;
> + }
> + igt_spin_busywait_until_started(spin);
> +
> + 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.
> +
> + igt_info("active[%d]: %'"PRIu64"ns\n", i, active[i]);
> + igt_assert(active[i] > old[i]); /* monotonic */
> + igt_assert(active[i] > delay); /* within reason */
> + }
> + }
> +
> + gem_quiescent_gpu(i915);
> +
> + /* And again now idle */
> + igt_assert_eq(read_runtime(me, idle), expect);
> + for (int i = 0; i < ARRAY_SIZE(idle); i++) {
> + if (!idle[i])
> + continue;
> +
> + igt_info("idle[%d]: %'"PRIu64"ns\n", i, idle[i]);
> + igt_assert(idle[i] >= active[i]);
> + }
> +
> + gem_context_destroy(i915, spin->execbuf.rsvd1);
> + igt_spin_free(i915, spin);
> +
> + /* And finally after the executing context is no more */
> + igt_assert_eq(read_runtime(me, old), expect);
> + for (int i = 0; i < ARRAY_SIZE(old); i++) {
> + if (!old[i])
> + continue;
> +
> + igt_info("old[%d]: %'"PRIu64"ns\n", i, old[i]);
> + igt_assert_eq_u64(old[i], idle[i]);
> + }
> +
> + close(i915);
> +}
> +
> +igt_main
> +{
> + const struct intel_execution_engine2 *e;
> + int i915 = -1, clients = -1;
> +
> + igt_fixture {
> + int sys;
> +
> + /* Don't allow [too many] extra clients to be opened */
> + i915 = __drm_open_driver(DRIVER_INTEL);
> + igt_require_gem(i915);
> +
> + sys = igt_sysfs_open(i915);
> + igt_require(sys != -1);
> +
> + clients = openat(sys, "clients", O_RDONLY);
> + igt_require(clients != -1);
> +
> + close(sys);
> + }
> +
> + igt_subtest("pidname")
> + pidname(i915, clients);
> +
> + igt_subtest("create")
> + create(i915, clients);
> +
> + igt_subtest_with_dynamic("busy") {
> + __for_each_physical_engine(i915, e) {
> + igt_dynamic_f("%s", e->name) {
> + igt_fork(child, 1)
> + busy_one(i915, clients, e);
> + igt_waitchildren();
> + }
> + }
> + }
> +
> + igt_subtest("busy-all") {
> + igt_fork(child, 1)
> + busy_all(i915, clients);
> + igt_waitchildren();
> + }
> +
> + igt_fixture {
> + close(clients);
> + close(i915);
> + }
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index ff924ff99..825e01833 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -236,6 +236,7 @@ i915_progs = [
> 'i915_query',
> 'i915_selftest',
> 'i915_suspend',
> + 'sysfs_clients',
> 'sysfs_defaults',
> 'sysfs_heartbeat_interval',
> 'sysfs_preempt_timeout',
>
No issues found, just comments so:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list