Vulkan: Fix padding out Buffer allocations on AMD.

include/platform/FeaturesVk.h

@@ -208,24 +208,22 @@ struct FeaturesVk : FeatureSetBase
         "RewriteStructSamplers behavior, which produces fewer.",
         &members, "http://anglebug.com/2703"};
 
-    // If the robustBufferAccess feature is enabled, Vulkan considers vertex attribute accesses only
-    // valid up to the last multiple of stride.  If a vertex's attribute range is such that it falls
-    // within the range of the buffer, but beyond the last multiple of stride, the driver is allowed
-    // to either read that range from the buffer anyway, or to return (0, 0, 0, 1).  Most drivers
-    // implement the former, while amdvlk on Linux and AMD's windows driver implement the latter.
-    // For the latter, this workaround limits GL_MAX_VERTEX_ATTRIB_STRIDE to a reasonable value, and
-    // rounds up every buffer allocation size to be a multiple of that.
-    // http://anglebug.com/2514
-    Feature roundUpBuffersToMaxVertexAttribStride = {
-        "round_up_buffers_to_max_vertex_attrib_stride", FeatureCategory::VulkanWorkarounds,
-        "If the robustBufferAccess feature is enabled, Vulkan considers vertex attribute accesses "
-        "only valid up to the last multiple of stride. If a vertex's attribute range is such that "
-        "it falls within the range of the buffer, but beyond the last multiple of stride, the "
-        "driver is allowed to either read that range from the buffer anyway, or to return "
-        "(0, 0, 0, 1). Most drivers implement the former, while some drivers the latter. For the "
-        "latter, this workaround limits GL_MAX_VERTEX_ATTRIB_STRIDE to a reasonable value, and "
-        "rounds up every buffer allocation size to be a multiple of that.",
-        &members, "http://anglebug.com/2848"};
+    // Vulkan considers vertex attribute accesses to count up to the last multiple of the stride.
+    // This additional access supports AMD's robust buffer access implementation.
+    // AMDVLK in particular will return incorrect values when the vertex access extends into the
+    // range that would be the stride padding and the buffer is too small.
+    // This workaround limits GL_MAX_VERTEX_ATTRIB_STRIDE to a reasonable value and pads out
+    // every buffer allocation size to be large enough to support a maximum vertex stride.
+    // http://anglebug.com/4428
+    Feature padBuffersToMaxVertexAttribStride = {
+        "pad_buffers_to_max_vertex_attrib_stride", FeatureCategory::VulkanWorkarounds,
+        "Vulkan considers vertex attribute accesses to count up to the last multiple of the "
+        "stride. This additional access supports AMD's robust buffer access implementation. "
+        "AMDVLK in particular will return incorrect values when the vertex access extends into "
+        "the range that would be the stride padding and the buffer is too small. "
+        "This workaround limits GL_MAX_VERTEX_ATTRIB_STRIDE to a maximum value and "
+        "pads up every buffer allocation size to be a multiple of the maximum stride.",
+        &members, "http://anglebug.com/4428"};
 
     // Whether the VkDevice supports the VK_EXT_swapchain_colorspace extension
     // http://anglebug.com/2514

src/libANGLE/renderer/vulkan/RendererVk.cpp

@@ -1620,11 +1620,7 @@ void RendererVk::initFeatures(DisplayVk *displayVk, const ExtensionNameList &dev
     // Disabled on AMD/windows due to buggy behavior.
     ANGLE_FEATURE_CONDITION((&mFeatures), disallowSeamfulCubeMapEmulation, IsWindows() && isAMD);
 
-    // Round up buffer sizes in the presence of robustBufferAccess to emulate GLES behavior when
-    // vertex attributes are fetched from beyond the last multiple of the stride.  Currently, only
-    // AMD is known to refuse reading these attributes.
-    ANGLE_FEATURE_CONDITION((&mFeatures), roundUpBuffersToMaxVertexAttribStride,
-                            isAMD && mPhysicalDeviceFeatures.robustBufferAccess);
+    ANGLE_FEATURE_CONDITION((&mFeatures), padBuffersToMaxVertexAttribStride, isAMD);
     mMaxVertexAttribStride = std::min(static_cast<uint32_t>(gl::limits::kMaxVertexAttribStride),
                                       mPhysicalDeviceProperties.limits.maxVertexInputBindingStride);
 

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -1466,13 +1466,13 @@ angle::Result BufferHelper::init(ContextVk *contextVk,
     VkBufferCreateInfo modifiedCreateInfo;
     const VkBufferCreateInfo *createInfo = &requestedCreateInfo;
 
-    if (renderer->getFeatures().roundUpBuffersToMaxVertexAttribStride.enabled)
+    if (renderer->getFeatures().padBuffersToMaxVertexAttribStride.enabled)
     {
         const VkDeviceSize maxVertexAttribStride = renderer->getMaxVertexAttribStride();
         ASSERT(maxVertexAttribStride);
-        modifiedCreateInfo      = requestedCreateInfo;
-        modifiedCreateInfo.size = roundUp(modifiedCreateInfo.size, maxVertexAttribStride);
-        createInfo              = &modifiedCreateInfo;
+        modifiedCreateInfo = requestedCreateInfo;
+        modifiedCreateInfo.size += maxVertexAttribStride;
+        createInfo = &modifiedCreateInfo;
     }
 
     ANGLE_VK_TRY(contextVk, mBuffer.init(contextVk->getDevice(), *createInfo));

src/tests/gl_tests/VertexAttributeTest.cpp

@@ -8,6 +8,7 @@
 #include "platform/FeaturesVk.h"
 #include "test_utils/ANGLETest.h"
 #include "test_utils/gl_raii.h"
+#include "util/random_utils.h"
 
 using namespace angle;
 
@@ -2503,6 +2504,101 @@ void main()
     checkPixels();
 }
 
+// Tests that large strides that read past the end of the buffer work correctly.
+// Requires ES 3.1 to query MAX_VERTEX_ATTRIB_STRIDE.
+TEST_P(VertexAttributeTestES31, LargeStride)
+{
+    struct Vertex
+    {
+        Vector4 position;
+        Vector2 color;
+    };
+
+    constexpr uint32_t kColorOffset = offsetof(Vertex, color);
+
+    // Get MAX_VERTEX_ATTRIB_STRIDE.
+    GLint maxStride;
+    glGetIntegerv(GL_MAX_VERTEX_ATTRIB_STRIDE, &maxStride);
+
+    uint32_t bufferSize  = static_cast<uint32_t>(maxStride);
+    uint32_t stride      = sizeof(Vertex);
+    uint32_t numVertices = bufferSize / stride;
+
+    // The last vertex fits in the buffer size. The last vertex stride extends past it.
+    ASSERT_LT(numVertices * stride, bufferSize);
+    ASSERT_GT(numVertices * stride + kColorOffset, bufferSize);
+
+    RNG rng(0);
+
+    std::vector<Vertex> vertexData;
+    std::vector<GLColor> expectedColors;
+    for (uint32_t vertexIndex = 0; vertexIndex < numVertices; ++vertexIndex)
+    {
+        int x = vertexIndex % getWindowWidth();
+        int y = vertexIndex / getWindowWidth();
+
+        // Generate and clamp a 2 component vector.
+        Vector4 randomVec4 = RandomVec4(rng.randomInt(), 0.0f, 1.0f);
+        GLColor randomColor(randomVec4);
+        randomColor[2]     = 0;
+        randomColor[3]     = 255;
+        Vector4 clampedVec = randomColor.toNormalizedVector();
+
+        vertexData.push_back({Vector4(x, y, 0.0f, 1.0f), Vector2(clampedVec[0], clampedVec[1])});
+        expectedColors.push_back(randomColor);
+    }
+
+    GLBuffer buffer;
+    glBindBuffer(GL_ARRAY_BUFFER, buffer);
+    glBufferData(GL_ARRAY_BUFFER, bufferSize, vertexData.data(), GL_STATIC_DRAW);
+
+    constexpr char kVS[] = R"(#version 310 es
+in vec4 pos;
+in vec2 color;
+out vec2 vcolor;
+void main()
+{
+    vcolor = color;
+    gl_Position = vec4(((pos.x + 0.5) / 64.0) - 1.0, ((pos.y + 0.5) / 64.0) - 1.0, 0, 1);
+    gl_PointSize = 1.0;
+})";
+
+    constexpr char kFS[] = R"(#version 310 es
+precision mediump float;
+in vec2 vcolor;
+out vec4 fcolor;
+void main()
+{
+    fcolor = vec4(vcolor, 0.0, 1.0);
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    glUseProgram(program);
+
+    GLint posLoc = glGetAttribLocation(program, "pos");
+    ASSERT_NE(-1, posLoc);
+    GLint colorLoc = glGetAttribLocation(program, "color");
+    ASSERT_NE(-1, colorLoc);
+
+    glVertexAttribPointer(posLoc, 4, GL_FLOAT, GL_FALSE, stride, nullptr);
+    glEnableVertexAttribArray(posLoc);
+    glVertexAttribPointer(colorLoc, 2, GL_FLOAT, GL_FALSE, stride,
+                          reinterpret_cast<GLvoid *>(kColorOffset));
+    glEnableVertexAttribArray(colorLoc);
+
+    glDrawArrays(GL_POINTS, 0, numVertices);
+
+    // Validate pixels.
+    std::vector<GLColor> actualColors(getWindowWidth() * getWindowHeight());
+    glReadPixels(0, 0, getWindowWidth(), getWindowHeight(), GL_RGBA, GL_UNSIGNED_BYTE,
+                 actualColors.data());
+
+    actualColors.resize(numVertices);
+
+    ASSERT_GL_NO_ERROR();
+    EXPECT_EQ(expectedColors, actualColors);
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these
 // tests should be run against.
 // D3D11 Feature Level 9_3 uses different D3D formats for vertex attribs compared to Feature Levels