Overall looks pretty good to me, a few comments below.
On Friday, July 21st, 2023 at 20:50, Erik Kurzinger <ekurzinger@xxxxxxxxxx> 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;
> +}
