[igt-dev] [PATCH] tests/syncobj_sync_file: new test

[Simon Ser]

Overall looks pretty good to me, a few comments below.

On Tuesday, July 25th, 2023 at 00:33, Erik Kurzinger <ekurzinger at nvidia.com> wrote:

> diff --git a/lib/igt_syncobj.h b/lib/igt_syncobj.h
> index e6725671d..01f5f062b 100644
> --- a/lib/igt_syncobj.h
> +++ b/lib/igt_syncobj.h
> @@ -34,7 +34,9 @@ int __syncobj_handle_to_fd(int fd, struct drm_syncobj_handle *args);
>  int __syncobj_fd_to_handle(int fd, struct drm_syncobj_handle *args);
>  int syncobj_handle_to_fd(int fd, uint32_t handle, uint32_t flags);
>  uint32_t syncobj_fd_to_handle(int fd, int syncobj_fd, uint32_t flags);
> +int __syncobj_import_sync_file(int fd, struct drm_syncobj_sync_file *args);
>  void syncobj_import_sync_file(int fd, uint32_t handle, int sync_file);
> +int __syncobj_export_sync_file(int fd, struct drm_syncobj_sync_file *args);

Hm, this is a bit confusing because the old transfer logic and the new IOCTL
both have the same name modulo the "__" prefix. Usually IGT uses a pattern
where the function without the "__" prefix just calls the one with the prefix
and asserts that it succeeds. Maybe we should rename the old functions in a
separate patch? Or find a different name for the new functions?

Optional nit: maybe it would be more ergonomic if the new function took
arguments instead of a struct drm_syncobj_sync_file. Callers wouldn't have to
init the struct.

> +const char *test_binary_import_export_desc =
> +	"Verifies importing and exporting a sync file with a binary syncobj.";
> +static void
> +test_binary_import_export(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +	int timeline = sw_sync_timeline_create();
> +
> +	args.handle = syncobj_create(fd, 0);
> +	args.fd = sw_sync_timeline_create_fence(timeline, 1);
> +	args.point = 0;
> +	igt_assert_eq(__syncobj_import_sync_file(fd, &args), 0);
> +	close(args.fd);
> +	args.fd = -1;
> +	igt_assert_eq(__syncobj_export_sync_file(fd, &args), 0);
> +
> +	igt_assert(!syncobj_wait(fd, &args.handle, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(args.fd), 0);
> +
> +	sw_sync_timeline_inc(timeline, 1);
> +	igt_assert(syncobj_wait(fd, &args.handle, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(args.fd), 1);
> +
> +	close(args.fd);
> +	close(timeline);
> +	syncobj_destroy(fd, args.handle);
> +}
> +
> +const char *test_timeline_import_export_desc =
> +	"Verifies importing and exporting sync files with a timeline syncobj.";
> +static void
> +test_timeline_import_export(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +	int timeline = sw_sync_timeline_create();
> +	int fence1, fence2;
> +	uint64_t point1 = 1, point2 = 2;
> +
> +	args.handle = syncobj_create(fd, 0);
> +	args.fd = sw_sync_timeline_create_fence(timeline, 1);
> +	args.point = 1;
> +	igt_assert_eq(__syncobj_import_sync_file(fd, &args), 0);
> +	close(args.fd);
> +	args.fd = -1;
> +	igt_assert_eq(__syncobj_export_sync_file(fd, &args), 0);
> +	fence1 = args.fd;
> +
> +	args.fd = sw_sync_timeline_create_fence(timeline, 2);
> +	args.point = 2;
> +	igt_assert_eq(__syncobj_import_sync_file(fd, &args), 0);
> +	close(args.fd);
> +	args.fd = -1;
> +	igt_assert_eq(__syncobj_export_sync_file(fd, &args), 0);
> +	fence2 = args.fd;
> +
> +	igt_assert(!syncobj_timeline_wait(fd, &args.handle, &point1, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(fence1), 0);
> +	igt_assert(!syncobj_timeline_wait(fd, &args.handle, &point2, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(fence2), 0);
> +
> +	sw_sync_timeline_inc(timeline, 1);
> +	igt_assert(syncobj_timeline_wait(fd, &args.handle, &point1, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(fence1), 1);
> +	igt_assert(!syncobj_timeline_wait(fd, &args.handle, &point2, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(fence2), 0);
> +
> +	sw_sync_timeline_inc(timeline, 1);
> +	igt_assert(syncobj_timeline_wait(fd, &args.handle, &point1, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(fence1), 1);
> +	igt_assert(syncobj_timeline_wait(fd, &args.handle, &point2, 1, 0, 0, NULL));
> +	igt_assert_eq(sync_fence_status(fence2), 1);
> +
> +	close(fence1);
> +	close(fence2);
> +	close(timeline);
> +	syncobj_destroy(fd, args.handle);
> +}
> +
> +const char *test_invalid_handle_import_desc =
> +	"Verifies that importing a sync file to an invalid syncobj fails.";
> +static void
> +test_invalid_handle_import(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +	int timeline = sw_sync_timeline_create();
> +
> +	args.handle = 0;
> +	args.point = 0;
> +	args.fd = sw_sync_timeline_create_fence(timeline, 1);
> +	igt_assert_eq(__syncobj_import_sync_file(fd, &args), -EINVAL);

Shouldn't this be ENOENT?

drm_syncobj_import_sync_file_fence() returns ENOENT when drm_syncobj_find()
fails.

> +
> +	close(args.fd);
> +	close(timeline);
> +}
> +
> +const char *test_invalid_handle_export_desc =
> +	"Verifies that exporting a sync file from an invalid syncobj fails.";
> +static void
> +test_invalid_handle_export(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +
> +	args.handle = 0;
> +	args.point = 0;
> +	args.fd = -1;
> +	igt_assert_eq(__syncobj_export_sync_file(fd, &args), -EINVAL);

Ditto

> +}
> +
> +const char *test_invalid_fd_import_desc =
> +	"Verifies that importing an invalid sync file fails.";
> +static void
> +test_invalid_fd_import(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +
> +	args.handle = syncobj_create(fd, 0);
> +	args.point = 0;
> +	args.fd = -1;
> +	igt_assert_eq(__syncobj_import_sync_file(fd, &args), -EINVAL);
> +
> +	syncobj_destroy(fd, args.handle);
> +}
> +
> +const char *test_unsubmitted_export_desc =
> +	"Verifies that exporting a sync file for an unsubmitted point fails.";
> +static void
> +test_unsubmitted_export(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +
> +	args.handle = syncobj_create(fd, 0);
> +	args.point = 0;
> +	args.fd = -1;
> +	igt_assert_eq(__syncobj_export_sync_file(fd, &args), -EINVAL);
> +
> +	syncobj_destroy(fd, args.handle);
> +}
> +
> +static bool has_syncobj_timeline(int fd)
> +{
> +	uint64_t value;
> +	return drmGetCap(fd, DRM_CAP_SYNCOBJ_TIMELINE,
> +			 &value) == 0 && value;
> +}
> +
> +static bool has_syncobj_sync_file_import_export(int fd)
> +{
> +	struct drm_syncobj_sync_file args = { 0 };
> +	args.handle = 0xffffffff;

0 is guaranteed to always be an invalid drm_syncobj handle, while in theory
0xffffffff could be a valid one. So it's a bit safer to just leave the handle
to zero here.

> +	/* if sync file import/export is supported this will fail with ENOENT,
> +	 * otherwise it will fail with EINVAL */
> +	return __syncobj_export_sync_file(fd, &args) == -ENOENT;
> +}