On 2021-03-16 12:10 a.m., Jason Ekstrand wrote:
> On Mon, Mar 15, 2021 at 4:05 PM Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote:
>>
>> Modern userspace APIs like Vulkan are built on an explicit
>> synchronization model. This doesn't always play nicely with the
>> implicit synchronization used in the kernel and assumed by X11 and
>> Wayland. The client -> compositor half of the synchronization isn't too
>> bad, at least on intel, because we can control whether or not i915
>> synchronizes on the buffer and whether or not it's considered written.
>>
>> The harder part is the compositor -> client synchronization when we get
>> the buffer back from the compositor. We're required to be able to
>> provide the client with a VkSemaphore and VkFence representing the point
>> in time where the window system (compositor and/or display) finished
>> using the buffer. With current APIs, it's very hard to do this in such
>> a way that we don't get confused by the Vulkan driver's access of the
>> buffer. In particular, once we tell the kernel that we're rendering to
>> the buffer again, any CPU waits on the buffer or GPU dependencies will
>> wait on some of the client rendering and not just the compositor.
>>
>> This new IOCTL solves this problem by allowing us to get a snapshot of
>> the implicit synchronization state of a given dma-buf in the form of a
>> sync file. It's effectively the same as a poll() or I915_GEM_WAIT only,
>> instead of CPU waiting directly, it encapsulates the wait operation, at
>> the current moment in time, in a sync_file so we can check/wait on it
>> later. As long as the Vulkan driver does the sync_file export from the
>> dma-buf before we re-introduce it for rendering, it will only contain
>> fences from the compositor or display. This allows to accurately turn
>> it into a VkFence or VkSemaphore without any over- synchronization.
>>
>> 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):
>> - Drop the sync_file import as it was all-around sketchy and not nearly
>> as useful as import.
>> - Re-introduce READ/WRITE flag support for export
>> - Rework the commit message
>>
>> Signed-off-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
>> ---
>> drivers/dma-buf/dma-buf.c | 55 ++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/dma-buf.h | 6 ++++
>> 2 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index f264b70c383eb..e7f9dd62c19a9 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
[...]
>> @@ -362,6 +363,57 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
>> return ret;
>> }
>>
>> +static long dma_buf_export_sync_file(struct dma_buf *dmabuf,
>> + void __user *user_data)
>> +{
>> + struct dma_buf_sync_file arg;
>> + struct dma_fence *fence = NULL;
>> + struct sync_file *sync_file;
>> + int fd, ret;
>> +
>> + if (copy_from_user(&arg, user_data, sizeof(arg)))
>> + return -EFAULT;
>> +
>> + if (arg.flags & ~DMA_BUF_SYNC_RW)
>> + return -EINVAL;
>> +
>> + fd = get_unused_fd_flags(O_CLOEXEC);
>> + if (fd < 0)
>> + return fd;
>> +
>> + if (arg.flags & DMA_BUF_SYNC_WRITE) {
>> + ret = dma_resv_get_singleton(dmabuf->resv, NULL, &fence);
>> + if (ret)
>> + goto err_put_fd;
>> + } else if (arg.flags & DMA_BUF_SYNC_READ) {
>> + fence = dma_resv_get_excl(dmabuf->resv);
>> + }
>> +
>> + if (!fence)
>> + fence = dma_fence_get_stub();
>> +
>> + sync_file = sync_file_create(fence);
>> +
>> + dma_fence_put(fence);
>> +
>> + if (!sync_file) {
>> + ret = -EINVAL;
>
> Should this be -EINVAL or -ENOMEM?
The latter makes more sense to me, since sync_file_create returning NULL is not related to invalid ioctl parameters.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
