<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 2, 2017 at 5:15 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">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] <div><div class="h5"> > @@ -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-><a href="http://permanent.bo" rel="noreferrer" target="_blank">permanent.bo</a>); > + 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); </div></div>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. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> [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. </blockquote><div> </div><div>Sure, I can do that. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > +} [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). <div><div class="h5"></div></div></blockquote><div> </div><div>I didn't figure there was any reason to allow re-export but there's no harm in it for OPAQUE_FD. </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"> > + 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-><a href="http://permanent.bo" rel="noreferrer" target="_blank">permanent.bo</a> = bo; > + > + return VK_SUCCESS; > + } > + > + default: > + return vk_error(VK_ERROR_INVALID_EXTERNAL_HANDLE_KHX); </div></div>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-><a href="http://permanent.bo" rel="noreferrer" target="_blank">permanent.bo</a>, 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. </blockquote><div> </div><div>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!) </div><div>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. </div><div> </div><div>--Jason </div></div></div></div>