[PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE

[PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE

[Erik Kurzinger]

These new ioctls perform a task similar to
DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
timeline point to import or export the fence to or from on a timeline
syncobj.

This eliminates the need to use a temporary binary syncobj along with
DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
technique userspace has had to employ up to this point. While that does
work, it is rather awkward from the programmer's perspective.  Since DRM
syncobjs have been proposed as the basis for display server explicit
synchronization protocols, e.g. [1] and [2], providing a more
streamlined interface now seems worthwhile.

[1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
[2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967

Accompanying userspace patches...
IGT: https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/e6f5c81bf893010411f1b471d68bb493ed36af67
libdrm: https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/22180768f85f1cce36ff34bbef34956b8803d6aa

V1 -> V2:
fixed conflict with DRM_IOCTL_MODE_GETFB2
re-ordered arguments of drm_syncobj_import_sync_file_fence

V2 -> V3:
add missing comma (whoops)

Signed-off-by: Erik Kurzinger <ekurzinger@nvidia.com>
Reviewed-by: Simon Ser <contact@emersion.fr>
---
 drivers/gpu/drm/drm_internal.h |  4 +++
 drivers/gpu/drm/drm_ioctl.c    |  4 +++
 drivers/gpu/drm/drm_syncobj.c  | 62 ++++++++++++++++++++++++++++++----
 include/uapi/drm/drm.h         |  8 +++++
 4 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index ba12acd55139..903731937595 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -255,6 +255,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_private);
 int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 			    struct drm_file *file_private);
+int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
+				       struct drm_file *file_private);
+int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
+				       struct drm_file *file_private);
 
 /* drm_framebuffer.c */
 void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f03ffbacfe9b..92d6da811afd 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -711,6 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
 		      DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE, drm_syncobj_import_sync_file_ioctl,
+		      DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE, drm_syncobj_export_sync_file_ioctl,
+		      DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index be3e8787d207..ee87707e7587 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -185,6 +185,13 @@
  * Note that if you want to transfer a struct &dma_fence_chain from a given
  * point on a timeline syncobj from/into a binary syncobj, you can use the
  * point 0 to mean take/replace the fence in the syncobj.
+ *
+ * &DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and &DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
+ * let the client import or export the struct &dma_fence_chain of a syncobj
+ * at a particular timeline point from or to a sync file.
+ * These behave similarly to &DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
+ * and &DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above, except
+ * that they accommodate timeline syncobjs in addition to binary syncobjs.
  */
 
 #include <linux/anon_inodes.h>
@@ -736,10 +743,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 }
 
 static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
-					      int fd, int handle)
+					      int fd, int handle, u64 point)
 {
 	struct dma_fence *fence = sync_file_get_fence(fd);
 	struct drm_syncobj *syncobj;
+	int ret = 0;
 
 	if (!fence)
 		return -EINVAL;
@@ -750,14 +758,23 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
 		return -ENOENT;
 	}
 
-	drm_syncobj_replace_fence(syncobj, fence);
+	if (point == 0) {
+		drm_syncobj_replace_fence(syncobj, fence);
+	} else {
+		struct dma_fence_chain *chain = dma_fence_chain_alloc();
+		if (chain) {
+			drm_syncobj_add_point(syncobj, chain, fence, point);
+		} else {
+			ret = -ENOMEM;
+		}
+	}
 	dma_fence_put(fence);
 	drm_syncobj_put(syncobj);
-	return 0;
+	return ret;
 }
 
 static int drm_syncobj_export_sync_file(struct drm_file *file_private,
-					int handle, int *p_fd)
+					int handle, u64 point, int *p_fd)
 {
 	int ret;
 	struct dma_fence *fence;
@@ -767,7 +784,7 @@ static int drm_syncobj_export_sync_file(struct drm_file *file_private,
 	if (fd < 0)
 		return fd;
 
-	ret = drm_syncobj_find_fence(file_private, handle, 0, 0, &fence);
+	ret = drm_syncobj_find_fence(file_private, handle, point, 0, &fence);
 	if (ret)
 		goto err_put_fd;
 
@@ -877,7 +894,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 
 	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
 		return drm_syncobj_export_sync_file(file_private, args->handle,
-						    &args->fd);
+						    0 /* binary */, &args->fd);
 
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
 					&args->fd);
@@ -902,7 +919,8 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
 		return drm_syncobj_import_sync_file_fence(file_private,
 							  args->fd,
-							  args->handle);
+							  args->handle,
+							  0 /* binary */);
 
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
 					&args->handle);
@@ -1651,3 +1669,33 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 
 	return ret;
 }
+
+int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
+				       struct drm_file *file_private)
+{
+	struct drm_syncobj_sync_file *args = data;
+
+	if (!drm_core_check_feature(dev, args->point == 0 ?
+				    DRIVER_SYNCOBJ :
+				    DRIVER_SYNCOBJ_TIMELINE))
+		return -EOPNOTSUPP;
+
+	return drm_syncobj_import_sync_file_fence(file_private,
+						  args->fd,
+						  args->handle,
+						  args->point);
+}
+
+int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
+				       struct drm_file *file_private)
+{
+	struct drm_syncobj_sync_file *args = data;
+
+	if (!drm_core_check_feature(dev, args->point == 0 ?
+				    DRIVER_SYNCOBJ :
+				    DRIVER_SYNCOBJ_TIMELINE))
+		return -EOPNOTSUPP;
+
+	return drm_syncobj_export_sync_file(file_private, args->handle,
+					    args->point, &args->fd);
+}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 863e47200911..3a00eaa7cc33 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -884,6 +884,12 @@ struct drm_syncobj_transfer {
 	__u32 pad;
 };
 
+struct drm_syncobj_sync_file {
+	__u32 handle;
+	__u32 fd;
+	__u64 point;
+};
+
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time point to become available */
@@ -1191,6 +1197,8 @@ extern "C" {
 #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
 
 #define DRM_IOCTL_SYNCOBJ_EVENTFD	DRM_IOWR(0xCF, struct drm_syncobj_eventfd)
+#define DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE	DRM_IOWR(0xD0, struct drm_syncobj_sync_file)
+#define DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE	DRM_IOWR(0xD1, struct drm_syncobj_sync_file)
 
 /*
  * Device specific ioctls should only be in their respective headers
-- 
2.41.0

Re: [PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE

[Erik Kurzinger]

The Xwayland merge request linked below has been updated to the new interface (via pending libdrm wrappers).
I have also posted a revised IGT test here https://lists.freedesktop.org/archives/igt-dev/2023-July/058449.html

On 7/21/23 11:50, Erik Kurzinger wrote:
> These new ioctls perform a task similar to
> DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
> IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
> timeline point to import or export the fence to or from on a timeline
> syncobj.
> 
> This eliminates the need to use a temporary binary syncobj along with
> DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
> technique userspace has had to employ up to this point. While that does
> work, it is rather awkward from the programmer's perspective.  Since DRM
> syncobjs have been proposed as the basis for display server explicit
> synchronization protocols, e.g. [1] and [2], providing a more
> streamlined interface now seems worthwhile.
> 
> [1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
> [2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
> 
> Accompanying userspace patches...
> IGT: https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/8cbee0c1782d6232de129e78cece3b94113992a5
> libdrm: https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/8e1ac8d831e2f7b202314c849a61a8e623657c0b
> 
> V1 -> V2:
> fixed conflict with DRM_IOCTL_MODE_GETFB2
> re-ordered arguments of drm_syncobj_import_sync_file_fence
> 
> V2 -> V3:
> add missing comma (whoops)
> 
> Signed-off-by: Erik Kurzinger <ekurzinger@nvidia.com>
> Reviewed-by: Simon Ser <contact@emersion.fr>
> ---
>  drivers/gpu/drm/drm_internal.h |  4 +++
>  drivers/gpu/drm/drm_ioctl.c    |  4 +++
>  drivers/gpu/drm/drm_syncobj.c  | 62 ++++++++++++++++++++++++++++++----
>  include/uapi/drm/drm.h         |  8 +++++
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index ba12acd55139..903731937595 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -255,6 +255,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
>  				      struct drm_file *file_private);
>  int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>  			    struct drm_file *file_private);
> +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
> +				       struct drm_file *file_private);
> +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
> +				       struct drm_file *file_private);
>  
>  /* drm_framebuffer.c */
>  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index f03ffbacfe9b..92d6da811afd 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -711,6 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  		      DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
>  		      DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE, drm_syncobj_import_sync_file_ioctl,
> +		      DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE, drm_syncobj_export_sync_file_ioctl,
> +		      DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER),
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index be3e8787d207..ee87707e7587 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -185,6 +185,13 @@
>   * Note that if you want to transfer a struct &dma_fence_chain from a given
>   * point on a timeline syncobj from/into a binary syncobj, you can use the
>   * point 0 to mean take/replace the fence in the syncobj.
> + *
> + * &DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and &DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
> + * let the client import or export the struct &dma_fence_chain of a syncobj
> + * at a particular timeline point from or to a sync file.
> + * These behave similarly to &DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
> + * and &DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above, except
> + * that they accommodate timeline syncobjs in addition to binary syncobjs.
>   */
>  
>  #include <linux/anon_inodes.h>
> @@ -736,10 +743,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>  }
>  
>  static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> -					      int fd, int handle)
> +					      int fd, int handle, u64 point)
>  {
>  	struct dma_fence *fence = sync_file_get_fence(fd);
>  	struct drm_syncobj *syncobj;
> +	int ret = 0;
>  
>  	if (!fence)
>  		return -EINVAL;
> @@ -750,14 +758,23 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>  		return -ENOENT;
>  	}
>  
> -	drm_syncobj_replace_fence(syncobj, fence);
> +	if (point == 0) {
> +		drm_syncobj_replace_fence(syncobj, fence);
> +	} else {
> +		struct dma_fence_chain *chain = dma_fence_chain_alloc();
> +		if (chain) {
> +			drm_syncobj_add_point(syncobj, chain, fence, point);
> +		} else {
> +			ret = -ENOMEM;
> +		}
> +	}
>  	dma_fence_put(fence);
>  	drm_syncobj_put(syncobj);
> -	return 0;
> +	return ret;
>  }
>  
>  static int drm_syncobj_export_sync_file(struct drm_file *file_private,
> -					int handle, int *p_fd)
> +					int handle, u64 point, int *p_fd)
>  {
>  	int ret;
>  	struct dma_fence *fence;
> @@ -767,7 +784,7 @@ static int drm_syncobj_export_sync_file(struct drm_file *file_private,
>  	if (fd < 0)
>  		return fd;
>  
> -	ret = drm_syncobj_find_fence(file_private, handle, 0, 0, &fence);
> +	ret = drm_syncobj_find_fence(file_private, handle, point, 0, &fence);
>  	if (ret)
>  		goto err_put_fd;
>  
> @@ -877,7 +894,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>  
>  	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
>  		return drm_syncobj_export_sync_file(file_private, args->handle,
> -						    &args->fd);
> +						    0 /* binary */, &args->fd);
>  
>  	return drm_syncobj_handle_to_fd(file_private, args->handle,
>  					&args->fd);
> @@ -902,7 +919,8 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>  	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
>  		return drm_syncobj_import_sync_file_fence(file_private,
>  							  args->fd,
> -							  args->handle);
> +							  args->handle,
> +							  0 /* binary */);
>  
>  	return drm_syncobj_fd_to_handle(file_private, args->fd,
>  					&args->handle);
> @@ -1651,3 +1669,33 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>  
>  	return ret;
>  }
> +
> +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
> +				       struct drm_file *file_private)
> +{
> +	struct drm_syncobj_sync_file *args = data;
> +
> +	if (!drm_core_check_feature(dev, args->point == 0 ?
> +				    DRIVER_SYNCOBJ :
> +				    DRIVER_SYNCOBJ_TIMELINE))
> +		return -EOPNOTSUPP;
> +
> +	return drm_syncobj_import_sync_file_fence(file_private,
> +						  args->fd,
> +						  args->handle,
> +						  args->point);
> +}
> +
> +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
> +				       struct drm_file *file_private)
> +{
> +	struct drm_syncobj_sync_file *args = data;
> +
> +	if (!drm_core_check_feature(dev, args->point == 0 ?
> +				    DRIVER_SYNCOBJ :
> +				    DRIVER_SYNCOBJ_TIMELINE))
> +		return -EOPNOTSUPP;
> +
> +	return drm_syncobj_export_sync_file(file_private, args->handle,
> +					    args->point, &args->fd);
> +}
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 863e47200911..3a00eaa7cc33 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -884,6 +884,12 @@ struct drm_syncobj_transfer {
>  	__u32 pad;
>  };
>  
> +struct drm_syncobj_sync_file {
> +	__u32 handle;
> +	__u32 fd;
> +	__u64 point;
> +};
> +
>  #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
>  #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
>  #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time point to become available */
> @@ -1191,6 +1197,8 @@ extern "C" {
>  #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
>  
>  #define DRM_IOCTL_SYNCOBJ_EVENTFD	DRM_IOWR(0xCF, struct drm_syncobj_eventfd)
> +#define DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE	DRM_IOWR(0xD0, struct drm_syncobj_sync_file)
> +#define DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE	DRM_IOWR(0xD1, struct drm_syncobj_sync_file)
>  
>  /*
>   * Device specific ioctls should only be in their respective headers

Re: [PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE

[Simon Ser]

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

On Friday, July 21st, 2023 at 20:50, Erik Kurzinger <ekurzinger@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;
> +}

Re: [PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE

[Simon Ser]

On Wednesday, July 26th, 2023 at 11:41, Simon Ser <contact@emersion.fr> wrote:

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

Ooops, that reply was meant for IGT… Re-sending it there.