On Tue, Apr 04, 2017 at 02:27:30PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
