Fix validation errors & re-enable VulkanExternalImageTest clear tests

These tests previously encountered errors attempting to transfer images from VK_QUEUE_FAMILY_EXTERNAL back to itself (this is not valid because one of the queues families in a memory barrier must be the family of the queue that executes the barrier). These invalid transfers were made because our ownership tracking started all resources in the "externally owned" state, with the expectation that the first operation on the resource would be a call to glWaitSemaphoreEXT to acquire ownership. It is far from clear that a call to glWaitSemaphoreEXT is always required to gain ownership of a resource. The EXT_external_objects extension inherits Vulkan's semantics, and what the Vulkan spec says is that the first entity to access a resource implicitly assumes ownership (see 11.7.1 "External Resource Sharing"). Binding a resource to memory does not constitute an access to that resource, or affect its ownership. Allocations should not be accesses, either; they happen at a lower level and the entire discussion about determining initial ownership from first access would serve no purpose if the mere allocation of the underlying memory was sufficient to assume ownership. This patch therefore adjusts the initial queue family ownership of resources created in memory objects to be the ANGLE renderer's queue family, just like a locally allocated image would be. Since this ownership state may not be correct (an external API may have already accessed the image, and assumed ownership) we must relax our assertions to allow a call to glWaitSemaphoreEXT while in this state. For images, this is only permitted while the layout is undefined. An alternative would be to set the initial queue family to a sentinel value that indicates that we don't know, but that would require checking for this value before making any accesses, and only then asserting local ownership. There's no real upside to this; the net effect of the first access rule is that we must effectively assume ownership until proven otherwise. Besides appearing to be the spec's intent, this change simplifies some usage scenarios because a queue submission is not required in the source Vulkan instance in order to allocate resources that will be initially accessed from GL. This seems especially important since there's no mechanism to allocate an external memory object from inside GL. The only downside is that the initial ambiguity in ownership prevents us from diagnosing certain errors, but this limitation is temporary; ownership becomes clear as soon as there is at least one access or at least one synchronization operation affecting the resource. Bug: angleproject:4229 Change-Id: Ibca2bfe373810c55352b1d849d07733d5fcfe5f4 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2178946 Commit-Queue: Jamie Madill <jmadill@chromium.org> Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org> Reviewed-by: Jamie Madill <jmadill@chromium.org>

Fix validation errors & re-enable VulkanExternalImageTest clear tests
dae210e6 · Michael Spang · Commit Bot · d08f1d8d · dae210e6 · dae210e6
Commit dae210e6 authored May 04, 2020 by Michael Spang Committed by Commit Bot May 07, 2020
5 changed files
--- a/src/libANGLE/renderer/vulkan/MemoryObjectVk.cpp
+++ b/src/libANGLE/renderer/vulkan/MemoryObjectVk.cpp
@@ -184,10 +184,22 @@ angle::Result MemoryObjectVk::createImage(ContextVk *contextVk,
    uint32_t layerCount;
    gl_vk::GetExtentsAndLayerCount(type, size, &vkExtents, &layerCount);
-    ANGLE_TRY(image->initExternal(
+    // Initialize VkImage with initial layout of VK_IMAGE_LAYOUT_UNDEFINED.
-        contextVk, type, vkExtents, vkFormat, 1, imageUsageFlags,
+    //
-        vk::ImageLayout::ExternalPreInitialized, &externalMemoryImageCreateInfo, 0,
+    // Binding a VkImage with an initial layout of VK_IMAGE_LAYOUT_UNDEFINED to
-        static_cast<uint32_t>(levels) - 1, static_cast<uint32_t>(levels), layerCount));
+    // external memory whose content has already been defined does not make the
+    // content undefined (see 11.7.1. External Resource Sharing).
+    //
+    // If the content is already defined, the ownership rules imply that the
+    // first operation on the texture must be a call to glWaitSemaphoreEXT that
+    // grants ownership of the image and informs us of the true layout.
+    //
+    // If the content is not already defined, the first operation may not be a
+    // glWaitSemaphore, but in this case undefined layout is appropriate.
+    ANGLE_TRY(image->initExternal(contextVk, type, vkExtents, vkFormat, 1, imageUsageFlags,
+                                  vk::ImageLayout::Undefined, &externalMemoryImageCreateInfo, 0,
+                                  static_cast<uint32_t>(levels) - 1, static_cast<uint32_t>(levels),
+                                  layerCount));
    VkMemoryRequirements externalMemoryRequirements;
    image->getImage().getMemoryRequirements(renderer->getDevice(), &externalMemoryRequirements);
@@ -224,7 +236,7 @@ angle::Result MemoryObjectVk::createImage(ContextVk *contextVk,
    VkMemoryPropertyFlags flags = 0;
    ANGLE_TRY(image->initExternalMemory(contextVk, renderer->getMemoryProperties(),
                                        externalMemoryRequirements, importMemoryInfo,
-                                        VK_QUEUE_FAMILY_EXTERNAL, flags));
+                                        renderer->getQueueFamilyIndex(), flags));
    return angle::Result::Continue;
 }

--- a/src/libANGLE/renderer/vulkan/SemaphoreVk.cpp
+++ b/src/libANGLE/renderer/vulkan/SemaphoreVk.cpp
@@ -121,7 +121,8 @@ angle::Result SemaphoreVk::wait(gl::Context *context,
            ANGLE_TRY(contextVk->endRenderPassAndGetCommandBuffer(&commandBuffer));
            // Queue ownership transfer.
-            bufferHelper.changeQueue(rendererQueueFamilyIndex, commandBuffer);
+            bufferHelper.acquireFromExternal(contextVk, VK_QUEUE_FAMILY_EXTERNAL,
+                                             rendererQueueFamilyIndex, commandBuffer);
        }
    }
@@ -136,15 +137,12 @@ angle::Result SemaphoreVk::wait(gl::Context *context,
            vk::ImageHelper &image = textureVk->getImage();
            vk::ImageLayout layout = GetVulkanImageLayout(textureAndLayout.layout);
-            // Inform the image that the layout has been externally changed.
-            image.onExternalLayoutChange(layout);
            vk::CommandBuffer *commandBuffer;
            ANGLE_TRY(contextVk->endRenderPassAndGetCommandBuffer(&commandBuffer));
-            // Queue ownership transfer.
+            // Queue ownership transfer and layout transition.
-            image.changeLayoutAndQueue(image.getAspectFlags(), layout, rendererQueueFamilyIndex,
+            image.acquireFromExternal(contextVk, VK_QUEUE_FAMILY_EXTERNAL, rendererQueueFamilyIndex,
-                                       commandBuffer);
+                                      layout, commandBuffer);
        }
    }
@@ -158,6 +156,8 @@ angle::Result SemaphoreVk::signal(gl::Context *context,
 {
    ContextVk *contextVk = vk::GetImpl(context);
+    uint32_t rendererQueueFamilyIndex = contextVk->getRenderer()->getQueueFamilyIndex();
    if (!bufferBarriers.empty())
    {
        // Perform a queue ownership transfer for each buffer.
@@ -170,7 +170,8 @@ angle::Result SemaphoreVk::signal(gl::Context *context,
            ANGLE_TRY(contextVk->endRenderPassAndGetCommandBuffer(&commandBuffer));
            // Queue ownership transfer.
-            bufferHelper.changeQueue(VK_QUEUE_FAMILY_EXTERNAL, commandBuffer);
+            bufferHelper.releaseToExternal(contextVk, rendererQueueFamilyIndex,
+                                           VK_QUEUE_FAMILY_EXTERNAL, commandBuffer);
        }
    }
@@ -196,8 +197,8 @@ angle::Result SemaphoreVk::signal(gl::Context *context,
            ANGLE_TRY(contextVk->endRenderPassAndGetCommandBuffer(&commandBuffer));
            // Queue ownership transfer and layout transition.
-            image.changeLayoutAndQueue(image.getAspectFlags(), layout, VK_QUEUE_FAMILY_EXTERNAL,
+            image.releaseToExternal(contextVk, rendererQueueFamilyIndex, VK_QUEUE_FAMILY_EXTERNAL,
-                                       commandBuffer);
+                                    layout, commandBuffer);
        }
    }

--- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp
+++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp
@@ -2252,6 +2252,26 @@ void BufferHelper::changeQueue(uint32_t newQueueFamilyIndex, CommandBuffer *comm
    mCurrentQueueFamilyIndex = newQueueFamilyIndex;
 }
+void BufferHelper::acquireFromExternal(ContextVk *contextVk,
+                                       uint32_t externalQueueFamilyIndex,
+                                       uint32_t rendererQueueFamilyIndex,
+                                       CommandBuffer *commandBuffer)
+{
+    mCurrentQueueFamilyIndex = externalQueueFamilyIndex;
+    changeQueue(rendererQueueFamilyIndex, commandBuffer);
+}
+void BufferHelper::releaseToExternal(ContextVk *contextVk,
+                                     uint32_t rendererQueueFamilyIndex,
+                                     uint32_t externalQueueFamilyIndex,
+                                     CommandBuffer *commandBuffer)
+{
+    ASSERT(mCurrentQueueFamilyIndex == rendererQueueFamilyIndex);
+    changeQueue(externalQueueFamilyIndex, commandBuffer);
+}
 bool BufferHelper::canAccumulateRead(ContextVk *contextVk, VkAccessFlags readAccessType)
 {
    // We only need to start a new command buffer when we need a new barrier.
@@ -2725,13 +2745,33 @@ void ImageHelper::changeLayoutAndQueue(VkImageAspectFlags aspectMask,
    forceChangeLayoutAndQueue(aspectMask, newLayout, newQueueFamilyIndex, commandBuffer);
 }
-void ImageHelper::onExternalLayoutChange(ImageLayout newLayout)
+void ImageHelper::acquireFromExternal(ContextVk *contextVk,
+                                      uint32_t externalQueueFamilyIndex,
+                                      uint32_t rendererQueueFamilyIndex,
+                                      ImageLayout currentLayout,
+                                      CommandBuffer *commandBuffer)
+{
+    // The image must be newly allocated or have been released to the external
+    // queue. If this is not the case, it's an application bug, so ASSERT might
+    // eventually need to change to a warning.
+    ASSERT(mCurrentLayout == ImageLayout::Undefined ||
+           mCurrentQueueFamilyIndex == externalQueueFamilyIndex);
+    mCurrentLayout           = currentLayout;
+    mCurrentQueueFamilyIndex = externalQueueFamilyIndex;
+    changeLayoutAndQueue(getAspectFlags(), mCurrentLayout, rendererQueueFamilyIndex, commandBuffer);
+}
+void ImageHelper::releaseToExternal(ContextVk *contextVk,
+                                    uint32_t rendererQueueFamilyIndex,
+                                    uint32_t externalQueueFamilyIndex,
+                                    ImageLayout desiredLayout,
+                                    CommandBuffer *commandBuffer)
 {
-    mCurrentLayout = newLayout;
+    ASSERT(mCurrentQueueFamilyIndex == rendererQueueFamilyIndex);
-    // The image must have already been owned by EXTERNAL.  If this is not the case, it's an
+    changeLayoutAndQueue(getAspectFlags(), desiredLayout, externalQueueFamilyIndex, commandBuffer);
-    // application bug, so ASSERT might eventually need to change to a warning.
-    ASSERT(mCurrentQueueFamilyIndex == VK_QUEUE_FAMILY_EXTERNAL);
 }
 uint32_t ImageHelper::getBaseLevel()

--- a/src/libANGLE/renderer/vulkan/vk_helpers.h
+++ b/src/libANGLE/renderer/vulkan/vk_helpers.h
@@ -637,6 +637,18 @@ class BufferHelper final : public Resource
    void changeQueue(uint32_t newQueueFamilyIndex, CommandBuffer *commandBuffer);
+    // Performs an ownership transfer from an external instance or API.
+    void acquireFromExternal(ContextVk *contextVk,
+                             uint32_t externalQueueFamilyIndex,
+                             uint32_t rendererQueueFamilyIndex,
+                             CommandBuffer *commandBuffer);
+    // Performs an ownership transfer to an external instance or API.
+    void releaseToExternal(ContextVk *contextVk,
+                           uint32_t rendererQueueFamilyIndex,
+                           uint32_t externalQueueFamilyIndex,
+                           CommandBuffer *commandBuffer);
    // Currently always returns false. Should be smarter about accumulation.
    bool canAccumulateRead(ContextVk *contextVk, VkAccessFlags readAccessType);
    bool canAccumulateWrite(ContextVk *contextVk, VkAccessFlags writeAccessType);
@@ -1156,6 +1168,20 @@ class ImageHelper final : public Resource, public angle::Subject
                              uint32_t newQueueFamilyIndex,
                              CommandBuffer *commandBuffer);
+    // Performs an ownership transfer from an external instance or API.
+    void acquireFromExternal(ContextVk *contextVk,
+                             uint32_t externalQueueFamilyIndex,
+                             uint32_t rendererQueueFamilyIndex,
+                             ImageLayout currentLayout,
+                             CommandBuffer *commandBuffer);
+    // Performs an ownership transfer to an external instance or API.
+    void releaseToExternal(ContextVk *contextVk,
+                           uint32_t rendererQueueFamilyIndex,
+                           uint32_t externalQueueFamilyIndex,
+                           ImageLayout desiredLayout,
+                           CommandBuffer *commandBuffer);
    // If the image is used externally to GL, its layout could be different from ANGLE's internal
    // state.  This function is used to inform ImageHelper of an external layout change.
    void onExternalLayoutChange(ImageLayout newLayout);

--- a/src/tests/gl_tests/VulkanExternalImageTest.cpp
+++ b/src/tests/gl_tests/VulkanExternalImageTest.cpp
@@ -156,8 +156,6 @@ TEST_P(VulkanExternalImageTest, ShouldImportSemaphoreOpaqueFd)
 // Test creating and clearing a simple RGBA8 texture in a opaque fd.
 TEST_P(VulkanExternalImageTest, ShouldClearOpaqueFdRGBA8)
 {
-    // http://anglebug.com/4229
-    ANGLE_SKIP_TEST_IF(IsVulkan());
    ANGLE_SKIP_TEST_IF(!EnsureGLExtensionEnabled("GL_EXT_memory_object_fd"));
    VulkanExternalHelper helper;
@@ -208,8 +206,6 @@ TEST_P(VulkanExternalImageTest, ShouldClearOpaqueFdRGBA8)
 // Test creating and clearing a simple RGBA8 texture in a zircon vmo.
 TEST_P(VulkanExternalImageTest, ShouldClearZirconVmoRGBA8)
 {
-    // http://anglebug.com/4229
-    ANGLE_SKIP_TEST_IF(IsVulkan());
    ANGLE_SKIP_TEST_IF(!EnsureGLExtensionEnabled("GL_ANGLE_memory_object_fuchsia"));
    VulkanExternalHelper helper;