On Tue, May 2, 2017 at 5:15 PM, Chad Versace <chadvers...@chromium.org>
wrote:
> 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.
>
They do get disastrous behavior in debug builds because of the assert. In
release builds, however, I'm not sure what I think. I don't know that I
like the idea of silently continuing because it would most likely give them
a perfectly "valid" dummy semaphore and they would get an error much later
when they try to export from it. We could abort(). I don't know. I
really don't think apps will rely on that error.
> [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.
>
Sure, I can do that.
> > +}
>
> [snip]
>
> >
> > @@ -548,9 +609,73 @@ void anv_GetPhysicalDeviceExternalSemap
> horePropertiesKHX(
> > 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).
>
I didn't figure there was any reason to allow re-export but there's no harm
in it for OPAQUE_FD.
> > + 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.
>
Hrm... Maybe? We could abort(). What I don't want to do is just get rid of
the case and silently return a random return value (which may be VK_SUCCES!)
As a side-note, I'm beginning to come to the conclusion that we may want to
make the undefined behavior in the driver a bit more contained. While our
driver shouldn't be robust against API misuse, I would like it to be
deterministic and try to crash early rather than just corrupting something
and maybe crashing later in a weird way.
--Jason
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
