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