[Mesa-dev] [PATCH 16/21] anv: Implement VK_KHX_external_semaphore_fd

[Chad Versace]

On Fri 14 Apr 2017, Jason Ekstrand wrote:
> This implementation allocates a 4k BO for each semaphore that can be
> exported using OPAQUE_FD and uses the kernel's already-existing
> synchronization mechanism on BOs.
> ---
>  src/intel/vulkan/anv_batch_chain.c      |  53 ++++++++++--
>  src/intel/vulkan/anv_device.c           |   4 +
>  src/intel/vulkan/anv_entrypoints_gen.py |   1 +
>  src/intel/vulkan/anv_private.h          |  16 +++-
>  src/intel/vulkan/anv_queue.c            | 141 ++++++++++++++++++++++++++++++--
>  5 files changed, 199 insertions(+), 16 deletions(-)

[snip]

> @@ -513,14 +533,33 @@ VkResult anv_CreateSemaphore(
>      VkExternalSemaphoreHandleTypeFlagsKHX handleTypes =
>        export ? export->handleTypes : 0;
>  
> -   /* External semaphores are not yet supported */
> -   assert(handleTypes == 0);
> +   if (handleTypes == 0) {
> +      /* The DRM execbuffer ioctl always execute in-oder so long as you stay
> +       * on the same ring.  Since we don't expose the blit engine as a DMA
> +       * queue, a dummy no-op semaphore is a perfectly valid implementation.
> +       */
> +      semaphore->permanent.type = ANV_SEMAPHORE_TYPE_DUMMY;
> +   } else if (handleTypes & VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT_KHX) {
> +      assert(handleTypes == VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT_KHX);
> +
> +      semaphore->permanent.type = ANV_SEMAPHORE_TYPE_BO;
> +      VkResult result = anv_bo_cache_alloc(device, &device->bo_cache,
> +                                           4096, &semaphore->permanent.bo);
> +      if (result != VK_SUCCESS) {
> +         vk_free2(&device->alloc, pAllocator, semaphore);
> +         return result;
> +      }
> +
> +      /* If we're going to use this as a fence, we need to *not* have the
> +       * EXEC_OBJECT_ASYNC bit set.
> +       */
> +      semaphore->permanent.bo->flags &= ~EXEC_OBJECT_ASYNC;
> +   } else {
> +      assert(!"Unknown handle type");
> +      vk_free2(&device->alloc, pAllocator, semaphore);
> +      return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX);

The Vulkan 1.0.49 spec does not list
VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX as a possible error code for
vkCreateSemaphore. The reason is that
VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX indicates that an external handle
does not match the expected handle type. Since vkCreateSemaphore has no
parameter that's an external handle, it can't return the error.
VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX only makes sense for functions that
import a handle or query the properties of a handle.

This 'else' branch is capturing invalid application usage the API. Since
invalid usages leads to undefined behavior, we could claim that Mesa's
undefined behavior is to return a helpful error code. But...

I much prefer disastrous undefined behavior here instead of helpful
undefined behavior. If we return VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX,
then appsy may accidentally rely on our out-of-spec behavior. It would
be better to just abort() or, like anvil does in most cases, silently
ignore the invalid case and continue blindly ahead.

[snip]

> +static void
> +anv_semaphore_impl_cleanup(struct anv_device *device,
> +                           struct anv_semaphore_impl *impl)
> +{
> +   switch (impl->type) {
> +   case ANV_SEMAPHORE_TYPE_NONE:
> +   case ANV_SEMAPHORE_TYPE_DUMMY:
> +      /* Dummy.  Nothing to do */
> +      break;
> +
> +   case ANV_SEMAPHORE_TYPE_BO:
> +      anv_bo_cache_release(device, &device->bo_cache, impl->bo);
> +      break;
> +
> +   default:
> +      unreachable("Invalid semaphore type");
> +   }

This switch statement is a good candidate for exhaustive switch warnings
(-Wswitch).  But we don't get the warnings unless the default is dropped
and the unreachable is moved to after the switch statement.

> +}

[snip]

>  
> @@ -548,9 +609,73 @@ void anv_GetPhysicalDeviceExternalSemaphorePropertiesKHX(
>      VkExternalSemaphorePropertiesKHX*           pExternalSemaphoreProperties)
>  {
>     switch (pExternalSemaphoreInfo->handleType) {
> +   case VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT_KHX:
> +      pExternalSemaphoreProperties->exportFromImportedHandleTypes = 0;

I expected exportFromImportedHandleTypes to be
VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT_KHX.  Why is it 0?
I admit I don't fully understand the VK_KHX_external_semaphore spec. (To
be fair, I doubt anyone fully understands it).

> +      pExternalSemaphoreProperties->compatibleHandleTypes =
> +         VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT_KHX;
> +      pExternalSemaphoreProperties->externalSemaphoreFeatures =
> +         VK_EXTERNAL_SEMAPHORE_FEATURE_EXPORTABLE_BIT_KHX |
> +         VK_EXTERNAL_SEMAPHORE_FEATURE_IMPORTABLE_BIT_KHX;
> +      break;
> +
>     default:
>        pExternalSemaphoreProperties->exportFromImportedHandleTypes = 0;
>        pExternalSemaphoreProperties->compatibleHandleTypes = 0;
>        pExternalSemaphoreProperties->externalSemaphoreFeatures = 0;
>     }
>  }
> +
> +VkResult anv_ImportSemaphoreFdKHX(
> +    VkDevice                                    _device,
> +    const VkImportSemaphoreFdInfoKHX*           pImportSemaphoreFdInfo)
> +{
> +   ANV_FROM_HANDLE(anv_device, device, _device);
> +   ANV_FROM_HANDLE(anv_semaphore, semaphore, pImportSemaphoreFdInfo->semaphore);
> +
> +   switch (pImportSemaphoreFdInfo->handleType) {
> +   case VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_FD_BIT_KHX: {
> +      struct anv_bo *bo;
> +      VkResult result = anv_bo_cache_import(device, &device->bo_cache,
> +                                            pImportSemaphoreFdInfo->fd, 4096,
> +                                            &bo);
> +      if (result != VK_SUCCESS)
> +         return result;
> +
> +      /* If we're going to use this as a fence, we need to *not* have the
> +       * EXEC_OBJECT_ASYNC bit set.
> +       */
> +      bo->flags &= ~EXEC_OBJECT_ASYNC;
> +
> +      anv_semaphore_impl_cleanup(device, &semaphore->permanent);
> +
> +      semaphore->permanent.type = ANV_SEMAPHORE_TYPE_BO;
> +      semaphore->permanent.bo = bo;
> +
> +      return VK_SUCCESS;
> +   }
> +
> +   default:
> +      return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX);

Yep. VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX is required here.

> +   }
> +}

> +
> +VkResult anv_GetSemaphoreFdKHX(
> +    VkDevice                                    _device,
> +    VkSemaphore                                 _semaphore,
> +    VkExternalSemaphoreHandleTypeFlagBitsKHX    handleType,
> +    int*                                        pFd)
> +{
> +   ANV_FROM_HANDLE(anv_device, device, _device);
> +   ANV_FROM_HANDLE(anv_semaphore, semaphore, _semaphore);
> +
> +   switch (semaphore->permanent.type) {
> +   case ANV_SEMAPHORE_TYPE_BO:
> +      return anv_bo_cache_export(device, &device->bo_cache,
> +                                 semaphore->permanent.bo, pFd);
> +
> +   default:
> +      return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX);

Like vkCreateSemaphore, for the same reasons,
VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX is not a valid error code for
vkGetSemaphoreFdKHX. And like vkCreateSemaphore, I think disastrous
undefined behavior here is better than helpful undefined behavior.

> +   }
> +
> +   return VK_SUCCESS;
> +}