[Intel-gfx] [PATCH i-g-t] syncobj: Add some wait and reset tests
Chris Wilson
chris at chris-wilson.co.uk
Wed Aug 9 18:16:55 UTC 2017
Quoting Jason Ekstrand (2017-08-09 18:46:49)
> On Wed, Aug 9, 2017 at 10:28 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> Quoting Jason Ekstrand (2017-08-09 18:04:42)
> > This adds both trivial error-checking tests as well as more complex
> > tests which actually test whether or not waits do what they're supposed
> > to do. They only currently work on i915 but it should be simple to hook
> > them up for other drivers by simply implementing the little function
> > pointer hook provided at the top for triggering a syncobj.
>
> Note that this requires a libdrm version more recent than is requested.
>
> > ---
> > tests/Makefile.sources | 1 +
> > tests/syncobj_wait.c | 624 ++++++++++++++++++++++++++++++
> +++++++++++++++++++
> > 2 files changed, 625 insertions(+)
> > create mode 100644 tests/syncobj_wait.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index bb013c7..430b637 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -230,6 +230,7 @@ TESTS_progs = \
> > prime_vgem \
> > sw_sync \
> > syncobj_basic \
> > + syncobj_wait \
> > template \
> > tools_test \
> > vgem_basic \
> > diff --git a/tests/syncobj_wait.c b/tests/syncobj_wait.c
> > new file mode 100644
> > index 0000000..6689d34
> > --- /dev/null
> > +++ b/tests/syncobj_wait.c
> > @@ -0,0 +1,624 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining
> a
> > + * copy of this software and associated documentation files (the
> "Software"),
> > + * to deal in the Software without restriction, including without
> limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> next
> > + * paragraph) shall be included in all copies or substantial portions of
> the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +#include <unistd.h>
> > +#include <time.h>
> > +#include <sys/ioctl.h>
> > +#include "drm.h"
> > +
> > +IGT_TEST_DESCRIPTION("Tests for the drm sync object wait API");
> > +
> > +/* One one tenth of a second */
> > +#define SHORT_TIME_NSEC 100000000ull
> > +
> > +/** A per-platform function which triggers a set of sync objects
> > + *
> > + * If wait is set, the function should wait for the work to complete so
> > + * that an immediate call to SYNCOBJ_WAIT will return success. If wait
> is
> > + * not set, then the function should try to submit enough work that an
> > + * immediate call to SYNCOBJ_WAIT with a timeout of 0 will time out.
> > + */
> > +void (*trigger_syncobj)(int fd, uint32_t *syncobjs, int count, bool
> wait);
> > +
> > +#define NSECS_PER_SEC 1000000000ull
> > +
> > +static uint64_t
> > +gettime_ns(void)
> > +{
> > + struct timespec current;
> > + clock_gettime(CLOCK_MONOTONIC, ¤t);
> > + return (uint64_t)current.tv_sec * NSECS_PER_SEC + current.tv_nsec;
> > +}
> > +
> > +static uint64_t
> > +short_timeout(void)
> > +{
> > + return gettime_ns() + SHORT_TIME_NSEC;
> > +}
> > +
> > +static uint32_t
> > +syncobj_create(int fd)
> > +{
> > + struct drm_syncobj_create create = { 0 };
> > + int ret;
> > +
> > + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_CREATE, &create);
> > + igt_assert(ret == 0);
> > + igt_assert(create.handle > 0);
> > +
> > + return create.handle;
> > +}
> > +
> > +static void
> > +syncobj_destroy(int fd, uint32_t handle)
> > +{
> > + struct drm_syncobj_destroy destroy = { 0 };
> > + int ret;
> > +
> > + destroy.handle = handle;
> > + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_DESTROY, &destroy);
> > + igt_assert(ret == 0);
> > +}
> > +
> > +struct delayed_trigger {
> > + int fd;
> > + uint32_t *syncobjs;
> > + int count;
> > + uint64_t nsec;
> > +};
> > +
> > +static void *
> > +trigger_syncobj_delayed_func(void *data)
> > +{
> > + struct delayed_trigger *trigger = data;
> > + struct timespec time;
> > +
> > + time.tv_sec = trigger->nsec / NSECS_PER_SEC;
> > + time.tv_nsec = trigger->nsec % NSECS_PER_SEC;
> > +
> > + nanosleep(&time, NULL);
> > + trigger_syncobj(trigger->fd, trigger->syncobjs, trigger->count,
> true);
> > + free(data);
> > +
> > + return NULL;
> > +}
> > +
> > +static pthread_t
> > +trigger_syncobj_delayed(int fd, uint32_t *syncobjs, int count, uint64_t
> nsec)
> > +{
> > + struct delayed_trigger *trigger;
> > + pthread_t thread;
> > + int ret;
> > +
> > + trigger = malloc(sizeof(*trigger));
> > + trigger->fd = fd;
> > + trigger->syncobjs = syncobjs;
> > + trigger->count = count;
> > + trigger->nsec = nsec;
> > +
> > + ret = pthread_create(&thread, NULL,
> > + trigger_syncobj_delayed_func, trigger);
>
> This is just a timer:
>
> timer_t timer;
> struct sigevent sev;
> struct itimerspec its;
>
> memset(&sev, 0, sizeof(sev));
> sev.sigev_notify = SIGEV_THREAD;
> sev.sigev_value.sival_ptr = tigger;
> sev.sigev_notify_function = trigger_syncobj_delayed_func;
> igt_assert(timer_create(CLOCK_MONOTONIC, &sev, &timer) == 0);
>
> memset(&its, 0, sizeof(its));
> its.it_value.tv_sec = nsec / NSEC_PER_SEC;
> its.it_value.tv_nsec = nsec % NSEC_PER_SEC;
> igt_assert(timer_settime(timer, 0, &its, NULL) == 0);
>
>
> How do I do the equivalent of pthread_join() on said timer?
You aren't doing anything more complicated than deleting the thread, so
timer_delete().
> > + igt_assert(ret == 0);
> > +
> > + return thread;
> > +}
>
> [snip]
>
> Key tests missing here are signal (SIGINT) handling, especially how the
> timeout parameters is handled on repeats, see igt_interruptible(), though
> you must use igt_ioctl(). Polling multiple handles is not
> checked, especially combinations of signaled/unsignaled syncojbs.
>
> > +
> > +/******** i915 specific bits ******/
> > +struct {
> > + uint32_t batch;
> > +} i915;
> > +
> > +#define I915_BATCH_COUNT 128
>
> Is overrunning the ring/guc-wq and blocking a concern?
>
> Do 128 provide any more significance than 2, one for wait, one for signal?
> Do 2 provide any more than 1?
>
>
> 1 appears to work reliably. I just wanted to make sure. I'll drop it to one.
> It should only produce warnings at worst.
>
>
> Does this provide any more value than the coverage (of execbuf +
> wait/signaling) in gem_exec_fence?
>
>
> They're different wait mechanisms...
I know you are testing the syncobj wait-ioctl, I am questioning whether
this pattern of i915<->syncobj is interesting at all as it should already
be covered elsewhere. If we only need to use i915 as a plain and simple
trigger mechanism (i.e. not the split submit/signal), we can just
replace i915 with vgem. I want to try and avoid having i915/gem
dependencies in what should be a driver agnostic test.
> > +static void
> > +i915_trigger_syncobj(int fd, uint32_t *syncobjs, int count, bool wait)
> > +{
> > + struct drm_i915_gem_exec_object2 exec_obj = { 0 };
> > + struct drm_i915_gem_exec_fence *fence_array;
> > + struct drm_i915_gem_execbuffer2 execbuf = { 0 };
> > + struct drm_i915_gem_wait gem_wait;
> > + int i, ret;
> > +
> > + fence_array = calloc(count, sizeof(*fence_array));
> > + for (i = 0; i < count; i++) {
> > + fence_array[i].handle = syncobjs[i];
> > + fence_array[i].flags = I915_EXEC_FENCE_SIGNAL;
> > + }
> > +
> > + exec_obj.handle = i915.batch;
>
> Is there any advantage in keeping the handle around in a global?
>
>
> Less work at trigger_syncobj time but that's all. I can create and destroy it
> here if you'd rather.
Keep it all local, definitely. The cost of allocating, clflushing, and binding
1 page for each trigger should be immaterial in the test runtime. If you
were concerned you would not have set .batch_len=8 and just left it as 0
as per normal.
> > +
> > + execbuf.buffers_ptr = to_user_pointer(&exec_obj);
> > + execbuf.buffer_count = 1;
> > + execbuf.batch_start_offset = 0;
> > + execbuf.batch_len = 8;
> > + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_FENCE_ARRAY;
> > +
> > + for (i = 0; i < I915_BATCH_COUNT; i++) {
> > + if (i == I915_BATCH_COUNT - 1) {
> > + execbuf.cliprects_ptr = to_user_pointer
> (fence_array);
> > + execbuf.num_cliprects = count;
> > + }
> > + ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &
> execbuf);
>
> gem_execbuf
>
> ?
This (igt_assert(drmIoctl(EXECBUFFER2)) is open-coding gem_execbuf().
Anytime you use a raw ioctl/drmIoctl, it is a sign that we are missing a
wrapper.
> > + igt_assert(ret == 0);
> > + }
> > +
> > + free(fence_array);
> > +
> > + if (wait) {
>
> This is just gem_sync() (actually just gem_wait, but you since you just
> want a sync, you can use gem_sync)
>
>
> What's the difference between wait and sync?
gem_sync() is a gem_wait(fd, bo, NULL [== infinity])
>
>
> > + gem_wait.bo_handle = i915.batch;
> > + gem_wait.flags = 0;
> > + gem_wait.timeout_ns = INT64_MAX;
> > + ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_WAIT, &gem_wait);
> > + igt_assert(ret == 0);
> > + }
> > +}
> > +
> > +static void
> > +i915_init(int fd)
> > +{
> > + uint32_t batch_data[2] = {0, MI_BATCH_BUFFER_END};
> > +
> > + i915.batch = gem_create(fd, 4096);
> > + gem_write(fd, i915.batch, 0, batch_data, sizeof(batch_data));
> > + trigger_syncobj = i915_trigger_syncobj;
> > +}
> > +/******** end of i915 bits ******/
>
> If this used either vgem or sw_sync, you wouldn't need i915 specific
> details.
>
>
> Those paths might be worth implementing as well but I wanted to make sure it
> worked for syncobjs triggered by the actual driver. See also our discussion
> about default waits. Also, that requires adding syncobj support to vgem.
> Maybe a worth endeavor but I don't really see the point atm.
>
>
> > +
> > +static bool
> > +has_syncobj_wait(int fd)
> > +{
> > + struct drm_syncobj_wait wait = { 0 };
> > + uint64_t value;
> > + int ret;
> > +
> > + if (drmGetCap(fd, DRM_CAP_SYNCOBJ, &value))
> > + return false;
> > + if (!value)
> > + return false;
> > +
> > + /* Try waiting for zero sync objects should fail with EINVAL */
> > + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_WAIT, &wait);
> > + return ret == -1 && errno == EINVAL;
> > +}
> > +
> > +igt_main
> > +{
> > + int fd;
> > +
> > + igt_fixture {
> > + fd = drm_open_driver_render(DRIVER_INTEL);
> > + igt_require_gem(fd);
> > + igt_require(has_syncobj_wait(fd));
> > +
> > + if (is_i915_device(fd))
> > + i915_init(fd);
>
> You asked for i915-only anyway, see DRIVER_INTEL. You meant DRIVER_ANY,
> but maybe just using DRIVER_VGEM and avoiding driver details would be
> better.
>
>
> I was hoping that Dave would come along and add amdgpu bits and then we would
> need it. But maybe the extra check here is overkill.
We shouldn't need driver specific tests of the syncobj wait-ioctl
though. Given driver coverage of fence signaling (required elsewhere)
and syncobj interaction, that should then give us coverage of all edges
that lead to the wait-ioctl, and so we only need a single central tester
for the wait-ioctl ABI.
-Chris
More information about the Intel-gfx
mailing list