On Tue 09 May 2017, Jason Ekstrand wrote:
> On Tue, May 9, 2017 at 4:49 PM, Chad Versace <chadvers...@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. :-)
Oh... I'm *that* guy :/
> 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.
Ok, I retract my complaint.
> >
> > > +
> > > + 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...
Either way sounds good.
> > 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]
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
