Vulkan: Fix buffer storage reuse bug when robustAccess is enabled

There is an optimization in vulkan backend that when the bufferData is
called and current storage size is big enough for new bufferData call,
we just reuse the storage. Mean while, when hasRobustAccess() is true,
we must use the VkBuffer with the exact user size that glBufferData call
provides so that driver can set proper access boundary. In order to
satisfy both requirement, if robust resource access is enabled, we
create a separate VkBuffer with the exact user provided size but bind to
the same memory. There is a bug here that if robustAccess is true, this
buffer of user provided size is not been recreated when storage is
reused but with different user size (both has same allocation size).
This causes we keep using the smaller VkBuffer and subsequently causes
missing triangles. This CL clears mBufferWithUserSize when size changes
and storage is reused.

The other bug here is that previously we are checking
isRobustResourceInitEnabled, which is incorrect. We should check
hasRobustAccess. This appears works for chrome possibly due to both are
enabled. This CL switches it to check hasRobustAccess.

This CL also renames mBufferForVertexArray to mBufferWithUserSize to
reflect what its true meaning.

Bug: chromium:1476475
Change-Id: I843cc3a705f8a582a97bc0307f03aa1eb9fad3ff
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4864003
Commit-Queue: Charlie Lao <cclao@google.com>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>

src/libANGLE/renderer/vulkan/BufferVk.cpp

@@ -435,6 +435,10 @@ angle::Result BufferVk::setDataWithMemoryType(const gl::Context *context,
         ANGLE_TRY(GetMemoryTypeIndex(contextVk, size, memoryPropertyFlags, &mMemoryTypeIndex));
         ANGLE_TRY(acquireBufferHelper(contextVk, size, mUsageType));
     }
+    else if (size != static_cast<size_t>(mState.getSize()))
+    {
+        mBuffer.onBufferUserSizeChange(renderer);
+    }
 
     if (data != nullptr)
     {

src/libANGLE/renderer/vulkan/ContextVk.h

@@ -675,6 +675,7 @@ class ContextVk : public ContextImpl, public vk::Context, public MultisampleText
     bool shouldConvertUint8VkIndexType(gl::DrawElementsType glIndexType) const;
 
     bool isRobustResourceInitEnabled() const;
+    bool hasRobustAccess() const { return mState.hasRobustAccess(); }
 
     // Queries that begin and end automatically with render pass start and end
     angle::Result beginRenderPassQuery(QueryVk *queryVk);

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -4734,8 +4734,8 @@ BufferHelper &BufferHelper::operator=(BufferHelper &&other)
 {
     ReadWriteResource::operator=(std::move(other));
 
-    mSuballocation        = std::move(other.mSuballocation);
-    mBufferForVertexArray = std::move(other.mBufferForVertexArray);
+    mSuballocation      = std::move(other.mSuballocation);
+    mBufferWithUserSize = std::move(other.mBufferWithUserSize);
 
     mCurrentQueueFamilyIndex = other.mCurrentQueueFamilyIndex;
     mCurrentWriteAccess      = other.mCurrentWriteAccess;
@@ -5041,13 +5041,14 @@ const Buffer &BufferHelper::getBufferForVertexArray(ContextVk *contextVk,
     ASSERT(mSuballocation.valid());
     ASSERT(actualDataSize <= mSuballocation.getSize());
 
-    if (!contextVk->isRobustResourceInitEnabled() || !mSuballocation.isSuballocated())
+    if (!contextVk->hasRobustAccess() || !mSuballocation.isSuballocated() ||
+        actualDataSize == mSuballocation.getSize())
     {
         *offsetOut = mSuballocation.getOffset();
         return mSuballocation.getBuffer();
     }
 
-    if (!mBufferForVertexArray.valid())
+    if (!mBufferWithUserSize.valid())
     {
         // Allocate buffer that is backed by sub-range of the memory for vertex array usage. This is
         // only needed when robust resource init is enabled so that vulkan driver will know the
@@ -5060,27 +5061,41 @@ const Buffer &BufferHelper::getBufferForVertexArray(ContextVk *contextVk,
         createInfo.sharingMode           = VK_SHARING_MODE_EXCLUSIVE;
         createInfo.queueFamilyIndexCount = 0;
         createInfo.pQueueFamilyIndices   = nullptr;
-        mBufferForVertexArray.init(contextVk->getDevice(), createInfo);
+        mBufferWithUserSize.init(contextVk->getDevice(), createInfo);
 
         VkMemoryRequirements memoryRequirements;
-        mBufferForVertexArray.getMemoryRequirements(contextVk->getDevice(), &memoryRequirements);
+        mBufferWithUserSize.getMemoryRequirements(contextVk->getDevice(), &memoryRequirements);
         ASSERT(contextVk->getRenderer()->isMockICDEnabled() ||
                mSuballocation.getSize() >= memoryRequirements.size);
         ASSERT(!contextVk->getRenderer()->isMockICDEnabled() ||
                mSuballocation.getOffset() % memoryRequirements.alignment == 0);
 
-        mBufferForVertexArray.bindMemory(contextVk->getDevice(), mSuballocation.getDeviceMemory(),
-                                         mSuballocation.getOffset());
+        mBufferWithUserSize.bindMemory(contextVk->getDevice(), mSuballocation.getDeviceMemory(),
+                                       mSuballocation.getOffset());
     }
     *offsetOut = 0;
-    return mBufferForVertexArray;
+    return mBufferWithUserSize;
+}
+
+void BufferHelper::onBufferUserSizeChange(RendererVk *renderer)
+{
+    // Buffer's user size and allocation size may be different due to alignment requirement. In
+    // normal usage we just use the actual allocation size and it is good enough. But when
+    // robustResourceInit is enabled, mBufferWithUserSize is created to mjatch the exact user
+    // size. Thus when user size changes, we must clear and recreate this mBufferWithUserSize.
+    if (mBufferWithUserSize.valid())
+    {
+        BufferSuballocation unusedSuballocation;
+        renderer->collectSuballocationGarbage(mUse, std::move(unusedSuballocation),
+                                              std::move(mBufferWithUserSize));
+    }
 }
 
 void BufferHelper::destroy(RendererVk *renderer)
 {
     mDescriptorSetCacheManager.destroyKeys(renderer);
     unmap(renderer);
-    mBufferForVertexArray.destroy(renderer->getDevice());
+    mBufferWithUserSize.destroy(renderer->getDevice());
     mSuballocation.destroy(renderer);
 }
 
@@ -5098,11 +5113,11 @@ void BufferHelper::release(RendererVk *renderer)
             mSuballocation.getBufferBlock()->releaseAllCachedDescriptorSetCacheKeys(renderer);
         }
         renderer->collectSuballocationGarbage(mUse, std::move(mSuballocation),
-                                              std::move(mBufferForVertexArray));
+                                              std::move(mBufferWithUserSize));
     }
     mUse.reset();
     mWriteUse.reset();
-    ASSERT(!mBufferForVertexArray.valid());
+    ASSERT(!mBufferWithUserSize.valid());
 }
 
 void BufferHelper::releaseBufferAndDescriptorSetCache(RendererVk *renderer)

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -867,6 +867,12 @@ class BufferHelper : public ReadWriteResource
                                           VkBufferUsageFlags usage,
                                           VkDeviceSize size);
 
+    // Buffer's user size and allocation size may be different due to alignment requirement. In
+    // normal usage we just use the actual allocation size and it is good enough. But when
+    // robustResourceInit is enabled, mBufferWithUserSize is created to match the exact user
+    // size. Thus when user size changes, we must clear and recreate this mBufferWithUserSize.
+    void onBufferUserSizeChange(RendererVk *renderer);
+
   private:
     void initializeBarrierTracker(Context *context);
 
@@ -883,7 +889,7 @@ class BufferHelper : public ReadWriteResource
     // when robust resource init is enabled, we may want to create a dedicated VkBuffer for the
     // suballocation so that vulkan driver will ensure no access beyond this sub-range. In that
     // case, this VkBuffer will be created lazily as needed.
-    Buffer mBufferForVertexArray;
+    Buffer mBufferWithUserSize;
 
     // For memory barriers.
     uint32_t mCurrentQueueFamilyIndex;

src/tests/gl_tests/RobustBufferAccessBehaviorTest.cpp

@@ -802,6 +802,48 @@ TEST_P(RobustBufferAccessBehaviorTest, OutOfBoundsArrayBuffers)
     DrawAndVerifyOutOfBoundsArrays(/*first*/ (numberOfQuads - 1) * 6, /*count*/ 6);
 }
 
+// Regression test for glBufferData with slightly increased size. Implementation may decided to
+// reuse the buffer storage if underline storage is big enough (due to alignment, implementation may
+// allocate more storage than data size.) This tests ensure it works correctly when this reuse
+// happens.
+TEST_P(RobustBufferAccessBehaviorTest, BufferDataWithIncreasedSize)
+{
+    ANGLE_SKIP_TEST_IF(!initExtension());
+
+    ANGLE_GL_PROGRAM(drawGreen, essl1_shaders::vs::Passthrough(), essl1_shaders::fs::Green());
+
+    // Clear to red and draw one triangle on the bottom left with green. The right top half should
+    // be red.
+    glClearColor(1.0f, 0.0f, 0.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+    std::array<float, 2 * 3> quadVertices = {-1, 1, -1, -1, 1, -1};
+    constexpr size_t kBufferSize          = sizeof(quadVertices[0]) * quadVertices.size();
+    GLBuffer vertexBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, vertexBuffer);
+    glBufferData(GL_ARRAY_BUFFER, kBufferSize, quadVertices.data(), GL_STATIC_DRAW);
+    glUseProgram(drawGreen);
+    const GLint positionLocation = glGetAttribLocation(drawGreen, essl1_shaders::PositionAttrib());
+    ASSERT_NE(-1, positionLocation);
+    glVertexAttribPointer(positionLocation, 2, GL_FLOAT, GL_FALSE, 0, 0);
+    glEnableVertexAttribArray(positionLocation);
+    glDrawArrays(GL_TRIANGLES, 0, 3);
+    EXPECT_PIXEL_COLOR_EQ(1, 1, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(getWindowWidth() - 1, getWindowHeight() - 1, GLColor::red);
+    EXPECT_GL_NO_ERROR();
+
+    // Clear to blue and call glBufferData with two triangles and draw the entire window with green.
+    // Both bottom left and top right should be green.
+    glClearColor(0.0f, 0.0f, 1.0f, 1.0f);
+    glClear(GL_COLOR_BUFFER_BIT);
+    std::array<float, 2 * 3 * 2> twoQuadVertices = {-1, 1, -1, -1, 1, -1, -1, 1, 1, -1, 1, 1};
+    glBufferData(GL_ARRAY_BUFFER, kBufferSize * 2, twoQuadVertices.data(), GL_STATIC_DRAW);
+    glUseProgram(drawGreen);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    EXPECT_PIXEL_COLOR_EQ(1, 1, GLColor::green);
+    EXPECT_PIXEL_COLOR_EQ(getWindowWidth() - 1, getWindowHeight() - 1, GLColor::green);
+    EXPECT_GL_NO_ERROR();
+}
+
 // Prepare an element array buffer that indexes out-of-bounds beginning with the start index passed
 // in. Ensure that drawElements flags either no error or INVALID_OPERATION. In the case of
 // INVALID_OPERATION, no canvas pixels can be touched.  In the case of NO_ERROR, all written values