<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 9, 2017 at 10:28 AM, Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. <div><div class="h5"> > --- > 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, &current); > + 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); </div></div>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); </blockquote><div> </div><div>How do I do the equivalent of pthread_join() on said timer? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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? </blockquote><div> </div><div>1 appears to work reliably. I just wanted to make sure. I'll drop it to one. It should only produce warnings at worst. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Does this provide any more value than the coverage (of execbuf + wait/signaling) in gem_exec_fence? </blockquote><div> </div><div>They're different wait mechanisms... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > +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? </blockquote><div> </div><div>Less work at trigger_syncobj time but that's all. I can create and destroy it here if you'd rather. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + 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 </blockquote><div> ? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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) </blockquote><div> </div><div>What's the difference between wait and sync? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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. <div><div class="h5"></div></div></blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + > +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); </div></div>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. </blockquote></div> </div><div class="gmail_extra">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. </div></div>