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
> +
> + 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.
> +
> + 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.
> + 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.
> +
> + 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.
Out of caution, though, please make
vkGetPhysicalDeviceExternalSemaphorePropertiesKHX report that FENCE_FD
is unsupported on Android with an `#ifndef ANDROID`.
> +
> +#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);
> + 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
