On 6/11/21 04:09, Jason Ekstrand wrote: > This patch is analogous to the previous sync file export patch in that > it allows you to import a sync_file into a dma-buf. Unlike the previous > patch, however, this does add genuinely new functionality to dma-buf. > Without this, the only way to attach a sync_file to a dma-buf is to > submit a batch to your driver of choice which waits on the sync_file and > claims to write to the dma-buf. Even if said batch is a no-op, a submit > is typically way more overhead than just attaching a fence. A submit > may also imply extra synchronization with other work because it happens > on a hardware queue. > > In the Vulkan world, this is useful for dealing with the out-fence from > vkQueuePresent. Current Linux window-systems (X11, Wayland, etc.) all > rely on dma-buf implicit sync. Since Vulkan is an explicit sync API, we > get a set of fences (VkSemaphores) in vkQueuePresent and have to stash > those as an exclusive (write) fence on the dma-buf. We handle it in > Mesa today with the above mentioned dummy submit trick. This ioctl > would allow us to set it directly without the dummy submit. > > This may also open up possibilities for GPU drivers to move away from > implicit sync for their kernel driver uAPI and instead provide sync > files and rely on dma-buf import/export for communicating with other > implicit sync clients. > > We make the explicit choice here to only allow setting RW fences which > translates to an exclusive fence on the dma_resv. There's no use for > read-only fences for communicating with other implicit sync userspace > and any such attempts are likely to be racy at best. When we got to > insert the RW fence, the actual fence we set as the new exclusive fence > is a combination of the sync_file provided by the user and all the other > fences on the dma_resv. This ensures that the newly added exclusive > fence will never signal before the old one would have and ensures that > we don't break any dma_resv contracts. We require userspace to specify > RW in the flags for symmetry with the export ioctl and in case we ever > want to support read fences in the future. > > There is one downside here that's worth documenting: If two clients > writing to the same dma-buf using this API race with each other, their > actions on the dma-buf may happen in parallel or in an undefined order. > Both with and without this API, the pattern is the same: Collect all > the fences on dma-buf, submit work which depends on said fences, and > then set a new exclusive (write) fence on the dma-buf which depends on > said work. The difference is that, when it's all handled by the GPU > driver's submit ioctl, the three operations happen atomically under the > dma_resv lock. If two userspace submits race, one will happen before > the other. You aren't guaranteed which but you are guaranteed that > they're strictly ordered. If userspace manages the fences itself, then > these three operations happen separately and the two render operations > may happen genuinely in parallel or get interleaved. However, this is a > case of userspace racing with itself. As long as we ensure userspace > can't back the kernel into a corner, it should be fine. > > v2 (Jason Ekstrand): > - Use a wrapper dma_fence_array of all fences including the new one > when importing an exclusive fence. > > v3 (Jason Ekstrand): > - Lock around setting shared fences as well as exclusive > - Mark SIGNAL_SYNC_FILE as a read-write ioctl. > - Initialize ret to 0 in dma_buf_wait_sync_file > > v4 (Jason Ekstrand): > - Use the new dma_resv_get_singleton helper > > v5 (Jason Ekstrand): > - Rename the IOCTLs to import/export rather than wait/signal > - Drop the WRITE flag and always get/set the exclusive fence > > v6 (Jason Ekstrand): > - Split import and export into separate patches > - New commit message > > v7 (Daniel Vetter): > - Fix the uapi header to use the right struct in the ioctl > - Use a separate dma_buf_import_sync_file struct > - Add kerneldoc for dma_buf_import_sync_file > > Signed-off-by: Jason Ekstrand > Cc: Christian König > Cc: Daniel Vetter > Cc: Sumit Semwal > Cc: Maarten Lankhorst > --- > drivers/dma-buf/dma-buf.c | 36 ++++++++++++++++++++++++++++++++++++ > include/uapi/linux/dma-buf.h | 22 ++++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 831828d71b646..88afd723015a2 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -422,6 +422,40 @@ static long dma_buf_export_sync_file(struct dma_buf *dmabuf, > put_unused_fd(fd); > return ret; > } > + > +static long dma_buf_import_sync_file(struct dma_buf *dmabuf, > + const void __user *user_data) > +{ > + struct dma_buf_import_sync_file arg; > + struct dma_fence *fence, *singleton = NULL; > + int ret = 0; > + > + if (copy_from_user(&arg, user_data, sizeof(arg))) > + return -EFAULT; > + > + if (arg.flags != DMA_BUF_SYNC_RW) > + return -EINVAL; > + > + fence = sync_file_get_fence(arg.fd); > + if (!fence) > + return -EINVAL; > + > + dma_resv_lock(dmabuf->resv, NULL); > + > + singleton = dma_resv_get_singleton(dmabuf->resv, fence); > + if (IS_ERR(singleton)) { > + ret = PTR_ERR(singleton); > + } else if (singleton) { > + dma_resv_add_excl_fence(dmabuf->resv, singleton); > + dma_resv_add_shared_fence(dmabuf->resv, singleton); Shouldn't there be a dma_fence_put(singleton) here? > + } > + > + dma_resv_unlock(dmabuf->resv); > + > + dma_fence_put(fence); > + > + return ret; > +} > #endif > > static long dma_buf_ioctl(struct file *file, > @@ -470,6 +504,8 @@ static long dma_buf_ioctl(struct file *file, > #if IS_ENABLED(CONFIG_SYNC_FILE) > case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: > return dma_buf_export_sync_file(dmabuf, (void __user *)arg); > + case DMA_BUF_IOCTL_IMPORT_SYNC_FILE: > + return dma_buf_import_sync_file(dmabuf, (const void __user *)arg); > #endif > > default: > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index 82f12a4640403..7382fd67351ba 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -115,6 +115,27 @@ struct dma_buf_export_sync_file { > __s32 fd; > }; > > +/** > + * struct dma_buf_import_sync_file - Insert a sync_file into a dma-buf > + * > + * Userspace can perform a DMA_BUF_IOCTL_IMPORT_SYNC_FILE to insert a > + * sync_file into a dma-buf for the purposes of implicit synchronization > + * with other dma-buf consumers. This allows clients using explicitly > + * synchronized APIs such as Vulkan to inter-op with dma-buf consumers > + * which expect implicit synchronization such as OpenGL or most media > + * drivers/video. > + */ > +struct dma_buf_import_sync_file { > + /** > + * @flags: Read/write flags > + * > + * Must be DMA_BUF_SYNC_RW. > + */ > + __u32 flags; > + /** @fd: Sync file descriptor */ > + __s32 fd; > +}; > + > #define DMA_BUF_BASE 'b' > #define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) > > @@ -125,5 +146,6 @@ struct dma_buf_export_sync_file { > #define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32) > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64) > #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, struct dma_buf_export_sync_file) > +#define DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct dma_buf_import_sync_file) > > #endif