[Mesa-dev] [PATCH 18/21] anv: Implement support for exporting semaphores as FENCE_FD
Jason Ekstrand
jason at jlekstrand.net
Wed May 10 00:00:51 UTC 2017
On Tue, May 9, 2017 at 4:49 PM, Chad Versace <chadversary at chromium.org>
wrote:
> I learned a lot by reviewing this patch. Before reviewing it, some parts
> of the external_semaphore spec still confused me. But reviewing this
> forced me to really learn the behavior of external semaphores.
>
> Other than some small security nits, the only real issue I found was
> the error behavior of vkQueueSubmit. Maybe I'm wrong and your error
> behavior is fine, though. Let's discuss.
>
> A little complaint against the current spec... The patch was hard to
> review because the 1.0.49 has incorrect language regarding external
> semaphores. I had to go review the unreleased spec to get to the truth.
> Anyway, on with review...
>
> On Fri 14 Apr 2017, Jason Ekstrand wrote:
> > ---
> > src/intel/vulkan/anv_batch_chain.c | 96 ++++++++++++++++++++++++++++++
> ++++++--
> > src/intel/vulkan/anv_device.c | 25 ++++++++++
> > src/intel/vulkan/anv_gem.c | 36 ++++++++++++++
> > src/intel/vulkan/anv_private.h | 24 +++++++---
> > src/intel/vulkan/anv_queue.c | 73 +++++++++++++++++++++++++++--
> > 5 files changed, 240 insertions(+), 14 deletions(-)
>
> > diff --git a/src/intel/vulkan/anv_batch_chain.c
> b/src/intel/vulkan/anv_batch_chain.c
> > index 0529f22..ec37c81 100644
> > --- a/src/intel/vulkan/anv_batch_chain.c
> > +++ b/src/intel/vulkan/anv_batch_chain.c
> > @@ -1387,6 +1387,23 @@ setup_execbuf_for_cmd_buffer(struct anv_execbuf
> *execbuf,
> > return VK_SUCCESS;
> > }
> >
> > +static void
> > +setup_empty_execbuf(struct anv_execbuf *execbuf, struct anv_device
> *device)
> > +{
> > + anv_execbuf_add_bo(execbuf, &device->trivial_batch_bo, NULL, 0,
> > + &device->alloc);
> > +
> > + execbuf->execbuf = (struct drm_i915_gem_execbuffer2) {
> > + .buffers_ptr = (uintptr_t) execbuf->objects,
> > + .buffer_count = execbuf->bo_count,
> > + .batch_start_offset = 0,
> > + .batch_len = 8, /* GEN8_MI_BATCH_BUFFER_END and NOOP */
>
> Instead of hard-coding a magic number here, I think
> .batch_len = device->trivial_batch_bo.next - device->trivial_batch_bo.
> start,
> is better. With that, the comment never becomes stale.
>
> > + .flags = I915_EXEC_HANDLE_LUT | I915_EXEC_RENDER,
> > + .rsvd1 = device->context_id,
> > + .rsvd2 = 0,
> > + };
> > +}
> > +
> > VkResult
> > anv_cmd_buffer_execbuf(struct anv_device *device,
> > struct anv_cmd_buffer *cmd_buffer,
> > @@ -1398,11 +1415,13 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
> > struct anv_execbuf execbuf;
> > anv_execbuf_init(&execbuf);
> >
> > + int in_fence = -1;
> > VkResult result = VK_SUCCESS;
> > for (uint32_t i = 0; i < num_in_semaphores; i++) {
> > ANV_FROM_HANDLE(anv_semaphore, semaphore, in_semaphores[i]);
> > - assert(semaphore->temporary.type == ANV_SEMAPHORE_TYPE_NONE);
> > - struct anv_semaphore_impl *impl = &semaphore->permanent;
> > + struct anv_semaphore_impl *impl =
> > + semaphore->temporary.type != ANV_SEMAPHORE_TYPE_NONE ?
> > + &semaphore->temporary : &semaphore->permanent;
> >
> > switch (impl->type) {
> > case ANV_SEMAPHORE_TYPE_BO:
> > @@ -1411,13 +1430,42 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
> > if (result != VK_SUCCESS)
> > return result;
> > break;
> > +
> > + case ANV_SEMAPHORE_TYPE_SYNC_FILE:
> > + if (in_fence == -1) {
> > + in_fence = impl->fd;
> > + } else {
> > + int merge = anv_gem_sync_file_merge(device, in_fence,
> impl->fd);
> > + if (merge == -1)
> > + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX);
>
> Because we immediately close the semaphore's fd, the spec does not allow
> us to return VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX here. We must either
> defer closing the fd until immediately before VK_SUCCESS, or return
> VK_ERROR_DEVICE_LOST.
>
> From the 1.0.49 spec:
>
> If vkQueueSubmit fails, it may return VK_ERROR_OUT_OF_HOST_MEMORY or
> VK_ERROR_OUT_OF_DEVICE_MEMORY. If it does, the implementation must
> ensure that the state and contents of any resources or
> synchronization primitives referenced by the submitted command
> buffers and any semaphores referenced by pSubmits is unaffected by
> the call or its failure. If vkQueueSubmit fails in such a way that
> the implementation can not make that guarantee, the implementation
> must return VK_ERROR_DEVICE_LOST
>
Yes, I wrote that text. :-) anv_QueueSubmit already turns all errors into
VK_DEVICE_LOST so we can return as descriptive an error as we want here.
Really, we should probably use the sync_file_info ioctl on import to see if
it's a sync file and fail then.
>
> > +
> > + close(impl->fd);
> > + close(in_fence);
> > + in_fence = merge;
> > + }
> > +
> > + impl->fd = -1;
> > + break;
> > +
> > default:
> > break;
> > }
> > +
> > + /* Waiting on a semaphore with temporary state implicitly resets
> it back
> > + * to the permanent state.
> > + */
> > + if (semaphore->temporary.type != ANV_SEMAPHORE_TYPE_NONE) {
> > + assert(semaphore->temporary.type ==
> ANV_SEMAPHORE_TYPE_SYNC_FILE);
> > + semaphore->temporary.type = ANV_SEMAPHORE_TYPE_NONE;
> > + }
> > }
> >
> > + bool need_out_fence = false;
> > for (uint32_t i = 0; i < num_out_semaphores; i++) {
> > ANV_FROM_HANDLE(anv_semaphore, semaphore, out_semaphores[i]);
> > + /* Out fences can't have temporary state because that would imply
> > + * that we imported a sync file and are trying to signal it.
> > + */
> > assert(semaphore->temporary.type == ANV_SEMAPHORE_TYPE_NONE);
> > struct anv_semaphore_impl *impl = &semaphore->permanent;
> >
> > @@ -1428,17 +1476,55 @@ anv_cmd_buffer_execbuf(struct anv_device *device,
> > if (result != VK_SUCCESS)
> > return result;
> > break;
> > +
> > + case ANV_SEMAPHORE_TYPE_SYNC_FILE:
> > + need_out_fence = true;
> > + break;
> > +
> > default:
> > break;
> > }
> > }
> >
> > - result = setup_execbuf_for_cmd_buffer(&execbuf, cmd_buffer);
> > - if (result != VK_SUCCESS)
> > - return result;
> > + if (cmd_buffer) {
> > + result = setup_execbuf_for_cmd_buffer(&execbuf, cmd_buffer);
> > + if (result != VK_SUCCESS)
> > + return result;
> > + } else {
> > + setup_empty_execbuf(&execbuf, device);
> > + }
>
> In the if-tree above, the patch has the same problem with error behavior
> as above. We need to defer updating the semaphores until VK_SUCCESS or
> return VK_ERROR_DEVICE_LOST.
>
See above comment.
> > +
> > + if (in_fence != -1) {
> > + execbuf.execbuf.flags |= I915_EXEC_FENCE_IN;
> > + execbuf.execbuf.rsvd2 |= (uint32_t)in_fence;
> > + }
> > +
> > + if (need_out_fence)
> > + execbuf.execbuf.flags |= I915_EXEC_FENCE_OUT;
> >
> > result = anv_device_execbuf(device, &execbuf.execbuf, execbuf.bos);
> >
> > + /* Execbuf does not consume the in_fence. It's our job to close it.
> */
>
> Weird. I didn't know that. I expected execbuf to transfer sync_file
> ownership.
>
So did I at first.
> > + close(in_fence);
> > +
> > + if (result == VK_SUCCESS && need_out_fence) {
> > + int out_fence = execbuf.execbuf.rsvd2 >> 32;
> > + for (uint32_t i = 0; i < num_out_semaphores; i++) {
> > + ANV_FROM_HANDLE(anv_semaphore, semaphore, out_semaphores[i]);
> > + /* Out fences can't have temporary state because that would
> imply
> > + * that we imported a sync file and are trying to signal it.
> > + */
> > + assert(semaphore->temporary.type == ANV_SEMAPHORE_TYPE_NONE);
> > + struct anv_semaphore_impl *impl = &semaphore->permanent;
> > +
> > + if (impl->type == ANV_SEMAPHORE_TYPE_SYNC_FILE) {
> > + assert(impl->fd == -1);
> > + impl->fd = dup(out_fence);
> > + }
> > + }
>
> This loop looks right to me.
>
> > + close(out_fence);
> > + }
> > +
> > anv_execbuf_finish(&execbuf, &device->alloc);
> >
> > return result;
> > diff --git a/src/intel/vulkan/anv_device.c
> b/src/intel/vulkan/anv_device.c
> > index f6e77ab..2885bb6 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
>
> [snip]
>
> > +static void
> > +anv_device_init_trivial_batch(struct anv_device *device)
> > +{
> > + anv_bo_init_new(&device->trivial_batch_bo, device, 4096);
> > + void *map = anv_gem_mmap(device, device->trivial_batch_bo.gem_
> handle,
> > + 0, 4096, 0);
> > +
> > + struct anv_batch batch;
> > + batch.start = batch.next = map;
> > + batch.end = map + 4096;
>
> On a release build, the above leaves much of the batch initialized to
> undefined values; that includes the batch->alloc callback. It should
> instead use a designated initializer list.
>
Probably a good idea.
> > +
> > + anv_batch_emit(&batch, GEN7_MI_BATCH_BUFFER_END, bbe);
> > + anv_batch_emit(&batch, GEN7_MI_NOOP, noop);
> > +
> > + if (!device->info.has_llc)
> > + anv_clflush_range(map, batch.next - map);
> > +
> > + anv_gem_munmap(map, device->trivial_batch_bo.size);
> > +}
>
> Other than that, anv_device_init_trivial_batch looks good.
>
> [snip]
>
> > diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c
> > index 1392bf4..ffdc5a1 100644
> > --- a/src/intel/vulkan/anv_gem.c
> > +++ b/src/intel/vulkan/anv_gem.c
>
> [snip]
>
> > @@ -400,3 +401,38 @@ anv_gem_fd_to_handle(struct anv_device *device, int
> fd)
> >
> > return args.handle;
> > }
> > +
> > +#ifndef SYNC_IOC_MAGIC
> > +/* duplicated from linux/sync_file.h to avoid build-time depnedency
> > + * on new (v4.7) kernel headers. Once distro's are mostly using
> > + * something newer than v4.7 drop this and #include <linux/sync_file.h>
> > + * instead.
> > + */
> > +struct sync_merge_data {
> > + char name[32];
> > + __s32 fd2;
> > + __s32 fence;
> > + __u32 flags;
> > + __u32 pad;
> > +};
>
> This will break Android because it defines struct sync_merge_data
> differently. I don't want the patch blocked on that, though. I'll handle
> fixing Android later, after this patch lands.
>
Now that we've pulled in the i915 headers, I can probably just pull in the
sync_file header and drop this hunk entirely. Still doesn't solve the
android problem though...
> Out of caution, though, please make
> vkGetPhysicalDeviceExternalSemaphorePropertiesKHX report that FENCE_FD
> is unsupported on Android with an `#ifndef ANDROID`.
>
I can do that if you'd like.
> > +
> > +#define SYNC_IOC_MAGIC '>'
> > +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data)
> > +#endif
> > +
> > +int
> > +anv_gem_sync_file_merge(struct anv_device *device, int fd1, int fd2)
> > +{
> > + const char name[] = "anv merge fence";
> > + struct sync_merge_data args = {
> > + .fd2 = fd2,
> > + .fence = -1,
> > + };
>
> For security future-proofing, I'd like to see here:
>
> STATIC_ASSERT(sizeof(args.name) >= sizeof(name);
>
Not a bad idea.
> > + memcpy(args.name, name, sizeof(name));
> > +
> > + int ret = anv_ioctl(fd1, SYNC_IOC_MERGE, &args);
> > + if (ret == -1)
> > + return -1;
> > +
> > + return args.fence;
> > +}
>
> [snip]
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170509/34989b9b/attachment-0001.html>
More information about the mesa-dev
mailing list