Vulkan: Bug fix in atomic counter buffer count calculation

When binding a new atomic counter buffer to a slot it is
necessary to take into account the previous binding of that
slot. This rectifies a bug introduced in f7b607c

Bug: angleproject:3566
Test: angle_unittests.exe --gtest_filter=AtomicCounterBufferTest31.AtomicCounterBuffer*
Change-Id: If78954d27c3345e8620294a84e839058d7dd7b9a
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2388431
Commit-Queue: Mohan Maiya <[email protected]>
Reviewed-by: Tim Van Patten <[email protected]>
Reviewed-by: Shahbaz Youssefi <[email protected]>

Author: Mohan Maiya

src/libANGLE/State.cpp

@@ -345,6 +345,7 @@ State::State(const State *shareContextState,
       mVertexArray(nullptr),
       mActiveSampler(0),
       mTexturesIncompatibleWithSamplers(0),
+      mValidAtomicCounterBufferCount(0),
       mPrimitiveRestart(false),
       mDebug(debug),
       mMultiSampling(false),
@@ -443,7 +444,6 @@ void State::initialize(Context *context)
             caps.maxCombinedTextureImageUnits);
 
         mAtomicCounterBuffers.resize(caps.maxAtomicCounterBufferBindings);
-        mValidAtomicCounterBufferCount = 0;
         mShaderStorageBuffers.resize(caps.maxShaderStorageBufferBindings);
         mImageUnits.resize(caps.maxImageUnits);
     }
@@ -1973,12 +1973,21 @@ angle::Result State::setIndexedBufferBinding(const Context *context,
                                        size);
             break;
         case BufferBinding::AtomicCounter:
-            UpdateIndexedBufferBinding(context, &mAtomicCounterBuffers[index], buffer, target,
-                                       offset, size);
-            if (buffer)
+            if (!mAtomicCounterBuffers[index].get() && buffer)
             {
+                // going from an invalid binding to a valid one, increment the count
                 mValidAtomicCounterBufferCount++;
+                ASSERT(mValidAtomicCounterBufferCount <=
+                       static_cast<uint32_t>(getCaps().maxAtomicCounterBufferBindings));
+            }
+            else if (mAtomicCounterBuffers[index].get() && !buffer)
+            {
+                // going from a valid binding to an invalid one, decrement the count
+                mValidAtomicCounterBufferCount--;
+                ASSERT(mValidAtomicCounterBufferCount >= 0);
             }
+            UpdateIndexedBufferBinding(context, &mAtomicCounterBuffers[index], buffer, target,
+                                       offset, size);
             break;
         case BufferBinding::ShaderStorage:
             UpdateIndexedBufferBinding(context, &mShaderStorageBuffers[index], buffer, target,
@@ -2051,6 +2060,7 @@ angle::Result State::detachBuffer(Context *context, const Buffer *buffer)
         {
             UpdateIndexedBufferBinding(context, &buf, nullptr, BufferBinding::AtomicCounter, 0, 0);
             mValidAtomicCounterBufferCount--;
+            ASSERT(mValidAtomicCounterBufferCount >= 0);
         }
     }
 

src/libANGLE/State.h

@@ -1034,7 +1034,7 @@ class State : angle::NonCopyable
 
     BufferVector mUniformBuffers;
     BufferVector mAtomicCounterBuffers;
-    size_t mValidAtomicCounterBufferCount;
+    uint32_t mValidAtomicCounterBufferCount;
     BufferVector mShaderStorageBuffers;
 
     BindingPointer<TransformFeedback> mTransformFeedback;

src/tests/gl_tests/AtomicCounterBufferTest.cpp

@@ -185,6 +185,137 @@ TEST_P(AtomicCounterBufferTest31, AtomicCounterRead)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::white);
 }
 
+// Updating atomic counter buffer's offsets was optimized based on a count of valid bindings.
+// This test will fail if there are bugs in how we count valid bindings.
+TEST_P(AtomicCounterBufferTest31, AtomicCounterBufferRangeRead)
+{
+    // Skipping due to a bug on the Qualcomm driver.
+    // http://anglebug.com/3726
+    ANGLE_SKIP_TEST_IF(IsNexus5X());
+
+    // Skipping due to a bug on the Qualcomm Vulkan Android driver.
+    // http://anglebug.com/3726
+    ANGLE_SKIP_TEST_IF(IsAndroid() && IsVulkan());
+
+    // Skipping test while we work on enabling atomic counter buffer support in th D3D renderer.
+    // http://anglebug.com/1729
+    ANGLE_SKIP_TEST_IF(IsD3D11());
+
+    constexpr char kFS[] =
+        "#version 310 es\n"
+        "precision highp float;\n"
+        "layout(binding = 0, offset = 4) uniform atomic_uint ac;\n"
+        "out highp vec4 my_color;\n"
+        "void main()\n"
+        "{\n"
+        "    my_color = vec4(0.0);\n"
+        "    uint a1 = atomicCounter(ac);\n"
+        "    if (a1 == 3u) my_color = vec4(1.0);\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, essl31_shaders::vs::Simple(), kFS);
+
+    glUseProgram(program.get());
+
+    // The initial value of counter 'ac' is 3u.
+    unsigned int bufferData[]     = {0u, 0u, 0u, 0u, 0u, 11u, 3u, 1u};
+    constexpr GLintptr kOffset    = 20;
+    GLint maxAtomicCounterBuffers = 0;
+    GLBuffer atomicCounterBuffer;
+
+    glGetIntegerv(GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS, &maxAtomicCounterBuffers);
+    // Repeatedly bind the same buffer (GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS + 1) times
+    // A bug in counting valid atomic counter buffers will cause a crash when we
+    // exceed GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS
+    for (int32_t i = 0; i < maxAtomicCounterBuffers + 1; i++)
+    {
+        glBindBuffer(GL_ATOMIC_COUNTER_BUFFER, atomicCounterBuffer);
+        glBufferData(GL_ATOMIC_COUNTER_BUFFER, sizeof(bufferData), bufferData, GL_STATIC_DRAW);
+        glBindBufferRange(GL_ATOMIC_COUNTER_BUFFER, 0, atomicCounterBuffer, kOffset,
+                          sizeof(bufferData) - kOffset);
+    }
+
+    drawQuad(program.get(), essl31_shaders::PositionAttrib(), 0.0f);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::white);
+}
+
+// Updating atomic counter buffer's offsets was optimized based on a count of valid bindings.
+// Repeatedly bind/unbind buffers across available binding points. The test will fail if
+// there are bugs in how we count valid bindings.
+TEST_P(AtomicCounterBufferTest31, AtomicCounterBufferRepeatedBindUnbind)
+{
+    // Skipping due to a bug on the Qualcomm Vulkan Android driver.
+    // http://anglebug.com/3726
+    ANGLE_SKIP_TEST_IF(IsAndroid() && IsVulkan());
+
+    // Skipping test while we work on enabling atomic counter buffer support in th D3D renderer.
+    // http://anglebug.com/1729
+    ANGLE_SKIP_TEST_IF(IsD3D11());
+
+    constexpr char kFS[] =
+        "#version 310 es\n"
+        "precision highp float;\n"
+        "layout(binding = 0, offset = 4) uniform atomic_uint ac;\n"
+        "out highp vec4 my_color;\n"
+        "void main()\n"
+        "{\n"
+        "    my_color = vec4(0.0);\n"
+        "    uint a1 = atomicCounter(ac);\n"
+        "    if (a1 == 3u) my_color = vec4(1.0);\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, essl31_shaders::vs::Simple(), kFS);
+
+    glUseProgram(program.get());
+
+    constexpr int32_t kBufferCount = 16;
+    // The initial value of counter 'ac' is 3u.
+    unsigned int bufferData[3] = {11u, 3u, 1u};
+    GLBuffer atomicCounterBuffer[kBufferCount];
+    // Populate atomicCounterBuffer[0] with valid data and the rest with nullptr
+    for (int32_t bufferIndex = 0; bufferIndex < kBufferCount; bufferIndex++)
+    {
+        glBindBuffer(GL_ATOMIC_COUNTER_BUFFER, atomicCounterBuffer[bufferIndex]);
+        glBufferData(GL_ATOMIC_COUNTER_BUFFER, sizeof(bufferData),
+                     (bufferIndex == 0) ? bufferData : nullptr, GL_STATIC_DRAW);
+    }
+
+    GLint maxAtomicCounterBuffers = 0;
+    glGetIntegerv(GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS, &maxAtomicCounterBuffers);
+
+    // Cycle through multiple buffers
+    for (int32_t i = 0; i < kBufferCount; i++)
+    {
+        constexpr int32_t kBufferIndices[kBufferCount] = {7, 12, 15, 5, 13, 14, 1, 2,
+                                                          0, 6,  4,  9, 8,  11, 3, 10};
+        int32_t bufferIndex                            = kBufferIndices[i];
+
+        // Randomly bind/unbind buffers to/from different binding points,
+        // capped by GL_MAX_COMPUTE_ATOMIC_COUNTER_BUFFERS
+        for (int32_t bufferCount = 0; bufferCount < maxAtomicCounterBuffers; bufferCount++)
+        {
+            constexpr uint32_t kBindingSlotsSize                = kBufferCount;
+            constexpr uint32_t kBindingSlots[kBindingSlotsSize] = {1,  3,  4, 14, 15, 9, 0, 6,
+                                                                   12, 11, 8, 5,  10, 2, 7, 13};
+
+            uint32_t bindingSlotIndex = bufferCount % kBindingSlotsSize;
+            uint32_t bindingSlot      = kBindingSlots[bindingSlotIndex];
+            uint32_t bindingPoint     = bindingSlot % maxAtomicCounterBuffers;
+            bool even                 = (bufferCount % 2 == 0);
+            int32_t bufferId          = (even) ? 0 : atomicCounterBuffer[bufferIndex];
+
+            glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, bindingPoint, bufferId);
+        }
+    }
+
+    // Bind atomicCounterBuffer[0] to slot 0 and verify result
+    glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, 0, atomicCounterBuffer[0]);
+    drawQuad(program.get(), essl31_shaders::PositionAttrib(), 0.0f);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::white);
+}
+
 // Test atomic counter increment and decrement.
 TEST_P(AtomicCounterBufferTest31, AtomicCounterIncrementAndDecrement)
 {