Re: [PATCH i-g-t] syncobj: Add some wait and reset tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Jason Ekstrand (2017-08-09 18:46:49)
> On Wed, Aug 9, 2017 at 10:28 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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, &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);
> 
>     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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux