Re: [PATCH 1/8] sync_file: add type/flags to sync file object creation.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 04, 2017 at 02:27:26PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This allows us to create sync files with different semantics,
> and clearly define the interoperation between them it also
> provides flags to allow for tweaks on those semantics.
> 
> This provides a validation interface for drivers that accept
> types from userspace so they can return EINVAL instead of ENOMEM.
> 
> This provides an ioctl for userspace to retrieve the type/flags
> of an object it may recieve from somewhere else.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Since you've bothered to type the docs for the ioctl structs too, please
squash in the below (and double-check that kernel-doc is still happy):


diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
index 31671b469627..375848c590ce 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -163,3 +163,8 @@ DMA Fence uABI/Sync File
 .. kernel-doc:: include/linux/sync_file.h
    :internal:
 
+Sync File IOCTL Definitions
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. kernel-doc:: include/uapi/linux/sync_file.h
+   :internal:


With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

One open question: Do we expect the current sync_file_get_fence to only
ever work for a FENCE type sync_file? Might be good to clarify that in the
kernel-doc for sync_file_get_fence().
-Daniel



> ---
>  drivers/dma-buf/sw_sync.c      |  2 +-
>  drivers/dma-buf/sync_file.c    | 62 +++++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/drm_atomic.c   |  2 +-
>  include/linux/sync_file.h      |  9 +++++-
>  include/uapi/linux/sync_file.h | 27 ++++++++++++++++++
>  5 files changed, 95 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 69c5ff3..1c47de6 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -315,7 +315,7 @@ static long sw_sync_ioctl_create_fence(struct sync_timeline *obj,
>  		goto err;
>  	}
>  
> -	sync_file = sync_file_create(&pt->base);
> +	sync_file = sync_file_create(&pt->base, SYNC_FILE_TYPE_FENCE, 0);
>  	dma_fence_put(&pt->base);
>  	if (!sync_file) {
>  		err = -ENOMEM;
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2321035..b33af9d 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,9 +28,32 @@
>  
>  static const struct file_operations sync_file_fops;
>  
> -static struct sync_file *sync_file_alloc(void)
> +/**
> + * sync_file_validate_type_flags - validate type/flags for support
> + * @type: type of sync file object
> + * @flags: flags to sync object.
> + *
> + * Validates the flags are correct so userspace can get a more
> + * detailed error type.
> + */
> +int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
> +{
> +	if (flags)
> +		return -EINVAL;
> +	if (type != SYNC_FILE_TYPE_FENCE)
> +		return -EINVAL;
> +	return 0;
> +}
> +EXPORT_SYMBOL(sync_file_validate_type_flags);
> +
> +static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
>  {
>  	struct sync_file *sync_file;
> +	int ret;
> +
> +	ret = sync_file_validate_type_flags(type, flags);
> +	if (ret)
> +		return NULL;
>  
>  	sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
>  	if (!sync_file)
> @@ -47,6 +70,8 @@ static struct sync_file *sync_file_alloc(void)
>  
>  	INIT_LIST_HEAD(&sync_file->cb.node);
>  
> +	sync_file->type = type;
> +	sync_file->flags = flags;
>  	return sync_file;
>  
>  err:
> @@ -72,11 +97,13 @@ static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
>   * sync_file can be released with fput(sync_file->file). Returns the
>   * sync_file or NULL in case of error.
>   */
> -struct sync_file *sync_file_create(struct dma_fence *fence)
> +struct sync_file *sync_file_create(struct dma_fence *fence,
> +				   uint32_t type,
> +				   uint32_t flags)
>  {
>  	struct sync_file *sync_file;
>  
> -	sync_file = sync_file_alloc();
> +	sync_file = sync_file_alloc(type, flags);
>  	if (!sync_file)
>  		return NULL;
>  
> @@ -200,7 +227,10 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>  	struct dma_fence **fences, **nfences, **a_fences, **b_fences;
>  	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
>  
> -	sync_file = sync_file_alloc();
> +	if (a->type != b->type)
> +		return NULL;
> +
> +	sync_file = sync_file_alloc(a->type, a->flags);
>  	if (!sync_file)
>  		return NULL;
>  
> @@ -437,6 +467,27 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>  	return ret;
>  }
>  
> +static long sync_file_ioctl_type(struct sync_file *sync_file,
> +				 unsigned long arg)
> +{
> +	struct sync_file_type type;
> +	int ret;
> +	if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
> +		return -EFAULT;
> +
> +	if (type.flags || type.type)
> +		return -EINVAL;
> +
> +	type.type = sync_file->type;
> +	type.flags = sync_file->flags;
> +
> +	if (copy_to_user((void __user *)arg, &type, sizeof(type)))
> +		ret = -EFAULT;
> +	else
> +		ret = 0;
> +	return ret;
> +}
> +
>  static long sync_file_ioctl(struct file *file, unsigned int cmd,
>  			    unsigned long arg)
>  {
> @@ -449,6 +500,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
>  	case SYNC_IOC_FILE_INFO:
>  		return sync_file_ioctl_fence_info(sync_file, arg);
>  
> +	case SYNC_IOC_TYPE:
> +		return sync_file_ioctl_type(sync_file, arg);
> +
>  	default:
>  		return -ENOTTY;
>  	}
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..bb5a740 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1917,7 +1917,7 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
>  	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
>  		return -EFAULT;
>  
> -	fence_state->sync_file = sync_file_create(fence);
> +	fence_state->sync_file = sync_file_create(fence, SYNC_FILE_TYPE_FENCE, 0);
>  	if (!fence_state->sync_file)
>  		return -ENOMEM;
>  
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 3e3ab84..ede4182 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -20,6 +20,8 @@
>  #include <linux/spinlock.h>
>  #include <linux/dma-fence.h>
>  #include <linux/dma-fence-array.h>
> +#include <uapi/linux/sync_file.h>
> +
>  
>  /**
>   * struct sync_file - sync file to export to the userspace
> @@ -30,6 +32,8 @@
>   * @wq:			wait queue for fence signaling
>   * @fence:		fence with the fences in the sync_file
>   * @cb:			fence callback information
> + * @type:               sync file type
> + * @flags:              flags used to create sync file
>   */
>  struct sync_file {
>  	struct file		*file;
> @@ -43,11 +47,14 @@ struct sync_file {
>  
>  	struct dma_fence	*fence;
>  	struct dma_fence_cb cb;
> +	uint32_t type;
> +	uint32_t flags;
>  };
>  
>  #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
>  
> -struct sync_file *sync_file_create(struct dma_fence *fence);
> +int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
> +struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
>  struct dma_fence *sync_file_get_fence(int fd);
>  
>  #endif /* _LINUX_SYNC_H */
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index 5b287d6..f439cda 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -69,6 +69,26 @@ struct sync_file_info {
>  #define SYNC_IOC_MAGIC		'>'
>  
>  /**
> + * DOC: SYNC_FILE_TYPE_FENCE - fence sync file object
> + *
> + * This sync file is a wrapper around a dma fence or a dma fence array.
> + * It can be merged with another fence sync file object to create a new
> + * merged object.
> + * The fence backing this object cannot be replaced.
> + * This is useful for shared fences.
> + */
> +#define SYNC_FILE_TYPE_FENCE 0
> +
> +/**
> + * struct sync_file_type - data returned from sync file type ioctl
> + * @type:	sync_file type
> + * @flags:	sync_file creation flags
> + */
> +struct sync_file_type {
> +	__u32	type;
> +	__u32	flags;
> +};
> +/**
>   * Opcodes  0, 1 and 2 were burned during a API change to avoid users of the
>   * old API to get weird errors when trying to handling sync_files. The API
>   * change happened during the de-stage of the Sync Framework when there was
> @@ -94,4 +114,11 @@ struct sync_file_info {
>   */
>  #define SYNC_IOC_FILE_INFO	_IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>  
> +/**
> + * DOC: SYNC_IOC_TYPE - get creation type and flags of sync_file.
> + *
> + * Takes a struct sync_file_type. Returns the created values of type and flags.
> + */
> +#define SYNC_IOC_TYPE		_IOWR(SYNC_IOC_MAGIC, 5, struct sync_file_type)
> +
>  #endif /* _UAPI_LINUX_SYNC_H */
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel