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

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

 



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@xxxxxxxxxxxxxx>
> ---
>  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, &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);

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
_______________________________________________
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