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

[Erik Kurzinger <ekurzinger@nvidia.com>]

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