[PATCH 2/4] sync_file: add replace and export some functionality

[Daniel Vetter]

On Tue, Mar 14, 2017 at 10:50:52AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> Using sync_file to back vulkan semaphores means need to replace
> the fence underlying the sync file. This replace function removes
> the callback, swaps the fence, and returns the old one. This
> also exports the alloc and fdget functionality for the semaphore
> wrapper code.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  drivers/dma-buf/sync_file.c | 46 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/sync_file.h   |  5 ++++-
>  2 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 105f48c..df7bb37 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,7 +28,14 @@
>  
>  static const struct file_operations sync_file_fops;
>  
> -static struct sync_file *sync_file_alloc(void)
> +/**
> + * sync_file_alloc() - allocate an unfenced sync file
> + *
> + * Creates a sync_file.
> + * The 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_alloc(void)
>  {
>  	struct sync_file *sync_file;
>  
> @@ -54,6 +61,7 @@ static struct sync_file *sync_file_alloc(void)
>  	kfree(sync_file);
>  	return NULL;
>  }
> +EXPORT_SYMBOL(sync_file_alloc);
>  
>  static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
>  {
> @@ -92,7 +100,7 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
>  }
>  EXPORT_SYMBOL(sync_file_create);
>  
> -static struct sync_file *sync_file_fdget(int fd)
> +struct sync_file *sync_file_fdget(int fd)
>  {
>  	struct file *file = fget(fd);
>  
> @@ -108,6 +116,7 @@ static struct sync_file *sync_file_fdget(int fd)
>  	fput(file);
>  	return NULL;
>  }
> +EXPORT_SYMBOL(sync_file_fdget);
>  
>  /**
>   * sync_file_get_fence - get the fence related to the sync_file fd
> @@ -125,13 +134,40 @@ struct dma_fence *sync_file_get_fence(int fd)
>  	if (!sync_file)
>  		return NULL;
>  
> +	mutex_lock(&sync_file->lock);
>  	fence = dma_fence_get(sync_file->fence);
> +	mutex_unlock(&sync_file->lock);
>  	fput(sync_file->file);
>  
>  	return fence;
>  }
>  EXPORT_SYMBOL(sync_file_get_fence);
>  
> +/**
> + * 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).
> + * 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;
> +	mutex_lock(&sync_file->lock);
> +	if (sync_file->fence) {
> +		if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> +			dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> +		ret_fence = sync_file->fence;
> +	}

uabi semantics question: Should we wake up everyone when the fence gets
replaced? What's the khr semaphore expectation here?

Spec quote in the kernel-doc (if available) would be best ...

> +	if (fence)
> +		sync_file->fence = dma_fence_get(fence);
> +	else
> +		sync_file->fence = NULL;
> +	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)
>  {
> @@ -292,8 +328,10 @@ static void sync_file_free(struct kref *kref)
>  	struct sync_file *sync_file = container_of(kref, struct sync_file,
>  						     kref);
>  
> -	if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> -		dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> +	if (sync_file->fence) {
> +		if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> +			dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> +	}
>  	dma_fence_put(sync_file->fence);
>  	kfree(sync_file);
>  }

I think you've missed _poll, won't that oops now?
-Daniel

> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 5aef17f..9511a54 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -50,7 +50,10 @@ struct sync_file {
>  
>  #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
>  
> +struct sync_file *sync_file_alloc(void);
>  struct sync_file *sync_file_create(struct dma_fence *fence);
>  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 */
> -- 
> 2.7.4
> 
> _______________________________________________
> 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