[PATCH 5/8] sync_file: add support for a semaphore object

[Daniel Vetter]

On Tue, Apr 04, 2017 at 02:27:30PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> This object can be used to implement the Vulkan semaphores.
> 
> The object behaviour differs from fence, in that you can
> replace the underlying fence, and you cannot merge semaphores.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>

Since you're going to all the trouble of having distinct types of
sync_file:
- I think we should reject non-TYPE_FENCE sync_file in
  sync_file_get_fence. This is to make sure no one can pull existing
  userspace over the table. Fence vs. sema also have different semantics,
  so I expect we'll have separate cs flags anyway.

- Already mentioned in the drm_syncobj patch, but I'd reorder that one
  after this one here and restrict drm_syncobj to only TYPE_SEMA. At least
  I can't see any reason why you'd want to make a plain fence persistent
  using an idr, they're kinda ephemeral.

A few more minor nits below. Patch itself looks good, but I want to figure
the type confusion semantics out first.

Cheers, Daniel

> ---
>  drivers/dma-buf/sync_file.c    | 36 +++++++++++++++++++++++++++++++++++-
>  include/linux/sync_file.h      |  2 ++
>  include/uapi/linux/sync_file.h | 14 ++++++++++++++
>  3 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 6376f6f..a82f6d8 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -44,7 +44,7 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
>  {
>  	if (flags)
>  		return -EINVAL;
> -	if (type != SYNC_FILE_TYPE_FENCE)
> +	if (type != SYNC_FILE_TYPE_FENCE && type != SYNC_FILE_TYPE_SEMAPHORE)
>  		return -EINVAL;
>  	return 0;
>  }
> @@ -200,6 +200,38 @@ sync_file_get_fence_locked(struct sync_file *sync_file)
>  					 sync_file_held(sync_file));
>  }
>  
> +/**
> + * sync_file_replace_fence - replace the fence related to the sync_file
> + * @sync_file:	 sync file to replace fence in
> + * @fence: fence to replace with (or NULL for no fence).

Maybe mention here that this is only valid for TYPE_SEMA?

> + * Returns previous fence.
> + */
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> +					  struct dma_fence *fence)
> +{
> +	struct dma_fence *ret_fence = NULL;
> +
> +	if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)

I think this could hide bugs, should we wrap it into a if(WARN_ON())?

> +		return NULL;
> +
> +	if (fence)
> +		dma_fence_get(fence);
> +
> +	mutex_lock(&sync_file->lock);
> +
> +	ret_fence = sync_file_get_fence_locked(sync_file);
> +	if (ret_fence) {
> +		if (test_bit(POLL_ENABLED, &ret_fence->flags))
> +			dma_fence_remove_callback(ret_fence, &sync_file->cb);
> +	}
> +
> +	RCU_INIT_POINTER(sync_file->fence, fence);
> +
> +	mutex_unlock(&sync_file->lock);
> +	return ret_fence;
> +}
> +EXPORT_SYMBOL(sync_file_replace_fence);
> +
>  static int sync_file_set_fence(struct sync_file *sync_file,
>  			       struct dma_fence **fences, int num_fences)
>  {
> @@ -278,6 +310,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>  
>  	if (a->type != b->type)
>  		return NULL;
> +	if (a->type != SYNC_FILE_TYPE_FENCE)
> +		return NULL;
>  
>  	if (!rcu_access_pointer(a->fence) ||
>  	    !rcu_access_pointer(b->fence))
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 4bf661b..245c7da 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -62,4 +62,6 @@ struct sync_file *sync_file_alloc(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);
>  struct sync_file *sync_file_fdget(int fd);
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> +					  struct dma_fence *fence);
>  #endif /* _LINUX_SYNC_H */
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index f439cda..5f266e0 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -80,6 +80,20 @@ struct sync_file_info {
>  #define SYNC_FILE_TYPE_FENCE 0
>  
>  /**
> + * DOC: SYNC_FILE_TYPE_SEMAPHORE - semaphore sync file object

Iirc you don't need the DOC: - kerneldoc should pick up #defines as-is.
You can iirc reference them from within kernel-doc using %DEFINED_THING.

> + *
> + * This is a sync file that operates like a Vulkan semaphore.
> + * The object should just be imported/exported but not use the
> + * sync file ioctls (except info).
> + * This object can have it's backing fence replaced multiple times.
> + * Each signal operation assigns a backing fence.
> + * Each wait operation waits on the current fence, and removes it.
> + * These operations should happen via driver command submission interfaces.
> + * This is useful for shared vulkan semaphores.
> + */
> +#define SYNC_FILE_TYPE_SEMAPHORE 1
> +
> +/**
>   * struct sync_file_type - data returned from sync file type ioctl
>   * @type:	sync_file type
>   * @flags:	sync_file creation flags
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch