Re: [PATCH 3/3] dma-buf: Add an API for exporting sync files (v6)

["Michel Dänzer" <michel@daenzer.net>]

On 2021-03-16 12:10 a.m., Jason Ekstrand wrote:
> On Mon, Mar 15, 2021 at 4:05 PM Jason Ekstrand <jason@jlekstrand.net> 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@jlekstrand.net>
>> ---
>>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel