[Intel-gfx] [PATCH i-g-t] syncobj: Add some wait and reset tests (v3)
Chris Wilson
chris at chris-wilson.co.uk
Wed Aug 9 20:45:24 UTC 2017
Quoting Jason Ekstrand (2017-08-09 19:49:34)
> 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.
>
> v2:
> - Actually add the reset tests.
> v3:
> - Only do one execbuf for trigger
> - Use do_ioctl and do_ioctl_err
> - Better check for syncobj support
> - Add local_/LOCAL_ defines of things
> - Use a timer instead of a pthread
>
> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> ---
> tests/Makefile.sources | 1 +
> tests/syncobj_wait.c | 691 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 692 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..d584b96
> --- /dev/null
> +++ b/tests/syncobj_wait.c
> @@ -0,0 +1,691 @@
> +/*
> + * 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 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 LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
> +#define LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
> +struct local_syncobj_wait {
> + __u64 handles;
> + /* absolute timeout */
> + __s64 timeout_nsec;
> + __u32 count_handles;
> + __u32 flags;
> + __u32 first_signaled; /* only valid when not waiting all */
> + __u32 pad;
> +};
> +
> +struct local_syncobj_reset {
> + __u32 handle;
> + __u32 flags;
> +};
> +
> +#define LOCAL_IOCTL_SYNCOBJ_WAIT DRM_IOWR(0xC3, struct local_syncobj_wait)
> +#define LOCAL_IOCTL_SYNCOBJ_RESET DRM_IOWR(0xC4, struct local_syncobj_reset)
> +
> +#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);
I recommend a pattern like:
int err;
static int __syncobj_create(int fd, uint32_t *syncobj)
{
struct local_syncobj_create create = {};
int err;
if (igt_ioctl(fd, LOCAL_IOCTL_SYNCOBJ_CREATE, &create))
return -errno;
*syncobj = create.handle;
return 0;
}
static uint32_t syncobj_create(int fd)
{
uint32_t syncobj;
igt_assert_eq(__syncobj_create(fd, &syncobj), 0);
igt_assert(syncobj);
return syncobj;
}
igt_assert_eq() prints the comparison and its values, makes debugging
less guess work, and __syncobj_create() is much easier to understand
than the macro expansion inside the assert output.
You may want to pass the ioctl_arg or individual members as convenience
dictates.
> + 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(union sigval sigval)
> +{
> + struct delayed_trigger *trigger = sigval.sival_ptr;
> + struct timespec time;
> +
> + trigger_syncobj(trigger->fd, trigger->syncobjs, trigger->count, true);
> + free(trigger);
> +}
> +
> +static timer_t
> +trigger_syncobj_delayed(int fd, uint32_t *syncobjs, int count, uint64_t nsec)
> +{
> + struct delayed_trigger *trigger;
> + timer_t timer;
> + struct sigevent sev;
> + struct itimerspec its;
> +
> + trigger = malloc(sizeof(*trigger));
> + trigger->fd = fd;
> + trigger->syncobjs = syncobjs;
> + trigger->count = count;
> + trigger->nsec = nsec;
> +
> + memset(&sev, 0, sizeof(sev));
> + sev.sigev_notify = SIGEV_THREAD;
> + sev.sigev_value.sival_ptr = trigger;
> + 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);
> +
> + return timer;
> +}
> +
> +static void
> +test_wait_bad_flags(int fd)
> +{
> + struct local_syncobj_wait wait = { 0 };
> + wait.flags = 0xdeadbeef;
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, EINVAL);
> +}
> +
> +static void
> +test_wait_zero_handles(int fd)
> +{
> + struct local_syncobj_wait wait = { 0 };
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, EINVAL);
> +}
> +
> +static void
> +test_wait_illegal_handle(int fd)
> +{
> + struct local_syncobj_wait wait = { 0 };
> + uint32_t handle = 0;
> +
> + wait.count_handles = 1;
> + wait.handles = to_user_pointer(&handle);
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, ENOENT);
Better check a valid + an invalid handle also generates an error.
EFAULT testing is missing, i.e. wait.handles = -1; handles is allowed to
be in read-only memory, so another good test would be:
wait.handles = -1;
igt_assert_eq(__syncobj_wait(fd, &eait), -EFAULT);
ptr = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1);
igt_assert(ptr != MAP_FAILED);
wait.handles = ptr;
igt_assert_eq(__syncobj_wait(fd, &eait), -ENOENT);
do_or_die(mprotect(ptr, 4096, PROT_READ));
igt_assert_eq(__syncobj_wait(fd, &eait), -ENOENT);
do_or_die(mprotect(ptr, 4096, PROT_NONE));
igt_assert_eq(__syncobj_wait(fd, &eait), -EFAULT);
munmap(ptr, 4096);
> +}
> +
> +static void
> +test_reset_bad_flags(int fd)
> +{
> + struct local_syncobj_reset reset = { 0 };
> + reset.flags = 0xdeadbeef;
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_RESET, &reset, EINVAL);
> +}
> +
> +static void
> +test_reset_illegal_handle(int fd)
> +{
> + struct local_syncobj_reset reset = { 0 };
> + reset.handle = 0;
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_RESET, &reset, ENOENT);
reset.handle = syncobj_create(fd);
do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_RESET, &reset, 0);
syncobj_destroy(fd, reset.handle);
do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_RESET, &reset, ENOENT);
Also good for test_wait_illegal_handle.
> +}
> +
> +static void
> +test_wait_unsignaled(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct local_syncobj_wait wait = { 0 };
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
> + wait.timeout_nsec = short_timeout();
> +
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, EINVAL);
Does polling an unsubmitted syncobj also raise an error?
i.e.
wait.timeout_nsec = 0,
wait.timeout_nsec = short,
wait.timeout_nsec = infinity
all generate EINVAL?
> +
> + syncobj_destroy(fd, syncobj);
> +}
> +
> +static void
> +test_wait_for_submit_unsignaled(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct local_syncobj_wait wait = { 0 };
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
> + wait.flags = LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT;
> + wait.timeout_nsec = short_timeout();
> +
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, ETIME);
Same question about wait.timeout_nsec = 0.
> +
> + syncobj_destroy(fd, syncobj);
> +}
> +
> +static void
> +test_wait_signaled(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct local_syncobj_wait wait = { 0 };
> + int ret;
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
assert unsigned here.
> +
> + trigger_syncobj(fd, &syncobj, 1, false);
> +
> + wait.timeout_nsec = 0;
> + ret = drmIoctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait);
> + igt_warn_on(ret != -1 || errno != ETIME);
It had better well be signaled at this point, i.e.
wait.timeout_nsec = 0;
while (drmIoctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait))
;
should terminate (imo, consider it like glQuerySync). Hmm, this of course
rules out just using dma_fence_is_signaled() as the only condition due to
the lax rules on
dma_fence.
> + wait.timeout_nsec = short_timeout();
> + do_ioctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait);
> +
> + syncobj_destroy(fd, syncobj);
> +}
> +
> +static void
> +test_wait_for_submit_signaled(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct local_syncobj_wait wait = { 0 };
> + int ret;
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
> + wait.flags = LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT;
> +
> + trigger_syncobj(fd, &syncobj, 1, false);
> +
> + wait.timeout_nsec = 0;
> + ret = drmIoctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait);
> + igt_warn_on(ret != -1 || errno != ETIME);
Same arguments as above.
> +
> + wait.timeout_nsec = short_timeout();
> + do_ioctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait);
> +
> + syncobj_destroy(fd, syncobj);
> +}
> +
> +static void
> +test_wait_for_submit_delayed_signal(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct local_syncobj_wait wait = { 0 };
> + timer_t timer;
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
> + wait.flags = LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT;
> +
> + timer = trigger_syncobj_delayed(fd, &syncobj, 1, SHORT_TIME_NSEC);
> +
> + wait.timeout_nsec = 0;
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, ETIME);
> +
> + wait.timeout_nsec = gettime_ns() + SHORT_TIME_NSEC * 2;
> + do_ioctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait);
> +
> + timer_delete(timer);
> +
> + syncobj_destroy(fd, syncobj);
> +static void
> +test_wait_all_some_signaled(int fd)
> +{
> + struct local_syncobj_wait wait = { 0 };
> + uint32_t syncobjs[2];
> +
> + syncobjs[0] = syncobj_create(fd);
> + syncobjs[1] = syncobj_create(fd);
> +
> + trigger_syncobj(fd, syncobjs, 1, true);
> +
> + wait.handles = to_user_pointer(&syncobjs);
> + wait.count_handles = 2;
> + wait.timeout_nsec = short_timeout();
> + wait.flags = LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_ALL;
> +
> + do_ioctl_err(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait, EINVAL);
And in reverse order. Also include invalid in the subtest name, I was
expected to see a busy/idle pair, not signaled/unsubmitted.
test_invalid_wait_all_some_unsubmitted? Maybe covered below?
At some point, you just give in and start compiling the tests using
combinatorics. And pine for automated fuzz exploration.
> +static void
> +test_wait_for_submit_any_some_signaled(int fd)
> +{
> + struct local_syncobj_wait wait = { 0 };
> + uint32_t tmp, syncobjs[2];
> +
> + syncobjs[0] = syncobj_create(fd);
> + syncobjs[1] = syncobj_create(fd);
> +
> + trigger_syncobj(fd, syncobjs, 1, true);
> +
> + wait.handles = to_user_pointer(&syncobjs);
> + wait.count_handles = 2;
> + wait.timeout_nsec = short_timeout();
> + wait.flags = LOCAL_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT;
> +
> + do_ioctl(fd, LOCAL_IOCTL_SYNCOBJ_WAIT, &wait);
> + igt_assert(wait.first_signaled == 0);
Whenever possible try to use igt_assert_eq(wait.first_signaled, 0);
Quite often we wait for an error before going overboard in adding
igt_assert_f(), but igt_assert_eq() are easy to add.
Looking good though.
-Chris
More information about the Intel-gfx
mailing list