<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 9, 2017 at 4:49 PM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <div><div class="h5">> --- > 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 */ </div></div>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. <div><div class="h5"> > + .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); </div></div>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 </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + > + 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); > + } </div></div>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. </blockquote><div> </div><div>See above comment. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + 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. </blockquote><div> </div><div>So did I at first. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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. </blockquote><div> </div><div>Probably a good idea. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + 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. </blockquote><div> </div><div>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... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Out of caution, though, please make vkGetPhysicalDeviceExternalSemaphorePropertiesKHX report that FENCE_FD is unsupported on Android with an `#ifndef ANDROID`. </blockquote><div> </div><div>I can do that if you'd like. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > +#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(<a href="http://args.name" rel="noreferrer" target="_blank">args.name</a>) >= sizeof(name); </blockquote><div> </div><div>Not a bad idea. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + memcpy(<a href="http://args.name" rel="noreferrer" target="_blank">args.name</a>, name, sizeof(name)); > + > + int ret = anv_ioctl(fd1, SYNC_IOC_MERGE, &args); > + if (ret == -1) > + return -1; > + > + return args.fence; > +} [snip] </blockquote></div> </div></div>