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;
> +}
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
