diff --git a/src/libANGLE/renderer/vulkan/BufferVk.cpp b/src/libANGLE/renderer/vulkan/BufferVk.cpp index 0abfc8b508..50a478c796 100644 --- a/src/libANGLE/renderer/vulkan/BufferVk.cpp +++ b/src/libANGLE/renderer/vulkan/BufferVk.cpp @@ -1337,6 +1337,9 @@ angle::Result BufferVk::acquireBufferHelper(ContextVk *contextVk, size_t size = roundUpPow2(sizeInBytes, kBufferSizeGranularity); size_t alignment = renderer->getDefaultBufferAlignment(); + if (getenv("POC_TRACE")) + fprintf(stderr, "[POC] acquireBufferHelper(mBuffer.valid=%d, &mBuffer=%p)\n", + mBuffer.valid(), static_cast(&mBuffer)); if (mBuffer.valid()) { ANGLE_TRY(contextVk->releaseBufferAllocation(&mBuffer)); diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp index 1429619383..cc92ad7755 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp +++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp @@ -4634,6 +4634,8 @@ BufferHelper &BufferHelper::operator=(BufferHelper &&other) mCurrentReadEvents = std::move(other.mCurrentReadEvents); } mXFBOrComputeWriteHeuristicBits = std::move(other.mXFBOrComputeWriteHeuristicBits); + mPOCGroundTruthReadAccess = other.mPOCGroundTruthReadAccess; + other.mPOCGroundTruthReadAccess.fill(0); mSerial = other.mSerial; // BufferHelper is usually std::move()'ed when the GL buffer backing is internally recreated as @@ -4870,6 +4872,20 @@ void BufferHelper::initializeBarrierTracker(ErrorContext *context) mCurrentReadAccess = 0; mCurrentWriteStages = 0; mCurrentReadStages = 0; + // POC instrumentation: ground-truth tracker is properly reset on reinit. + if (getenv("POC_TRACE")) + { + VkAccessFlags stale = 0; + for (size_t i = 0; i < mPOCGroundTruthReadAccess.size(); ++i) + stale |= mPOCGroundTruthReadAccess.data()[i]; + fprintf(stderr, + "[POC] initializeBarrierTracker(this=%p) xfbBits=0x%x " + "groundTruthSum=0x%x\n", + static_cast(this), + static_cast(mXFBOrComputeWriteHeuristicBits.bits()), + static_cast(stale)); + } + mPOCGroundTruthReadAccess.fill(0); } angle::Result BufferHelper::initializeNonZeroMemory(ErrorContext *context, @@ -5164,11 +5180,57 @@ void BufferHelper::recordReadBarrier(Context *context, // If the type of read already tracked by mCurrentReadEvents, it means we must already inserted // the barrier when mCurrentReadEvents is set. No new barrier is needed. EventStage eventStage = GetBufferMemoryBarrierData(stageIndex).eventStage; + if (getenv("POC_TRACE")) + fprintf(stderr, + "[POC] recordReadBarrier(this=%p) stage=%d access=0x%x hasEventAndAccess=%d\n", + static_cast(this), static_cast(eventStage), + static_cast(readAccessType), + mCurrentReadEvents.hasEventAndAccess(eventStage, readAccessType)); if (mCurrentReadEvents.hasEventAndAccess(eventStage, readAccessType)) { ASSERT((context->getRenderer()->getPipelineStageMask(eventStage) & readPipelineStageFlags) == readPipelineStageFlags); ASSERT((mCurrentReadEvents.getAccessFlags(eventStage) & readAccessType) == readAccessType); + // POC instrumentation: hasEventAndAccess() returned true, claiming a barrier with + // dstAccessMask >= readAccessType was already emitted for this incarnation. Verify + // against the ground-truth tracker that is properly reset in initializeBarrierTracker(). + if ((mPOCGroundTruthReadAccess[eventStage] & readAccessType) != readAccessType) + { + fprintf(stderr, + "\n" + "===================================================================\n" + "*** BUG TRIGGERED: BufferHelper::recordReadBarrier() barrier skip ***\n" + "===================================================================\n" + "hasEventAndAccess(eventStage=%d, readAccess=0x%x) == true, so the\n" + "early-return is taken and NO barrier is emitted for this read.\n" + "\n" + "But the ground-truth tracker (reset on reinit) shows that since the\n" + "last initializeBarrierTracker()/write, the only dstAccessMask bits\n" + "actually emitted for this EventStage were 0x%x.\n" + "\n" + "mCurrentReadEvents.mAccessFlags[stage] = 0x%x <-- contains STALE bits\n" + "mPOCGroundTruthReadAccess[stage] = 0x%x <-- correct value\n" + "stale bits surviving release() = 0x%x\n" + "\n" + "Root cause: RefCountedEventArrayWithAccessFlags inherits non-virtual\n" + "release() from RefCountedEventArray which clears only mBitMask, leaving\n" + "mAccessFlags[] stale across BufferHelper::initializeBarrierTracker().\n" + "The next replaceEventAtStage() OR-accumulates into the stale value, so\n" + "hasEventAndAccess() reports a barrier that was never emitted.\n" + "\n" + "Vulkan consequence: the prior write is NOT made visible to access type\n" + "0x%x at this stage -> SYNC-HAZARD-READ-AFTER-WRITE.\n" + "===================================================================\n", + static_cast(eventStage), static_cast(readAccessType), + static_cast(mPOCGroundTruthReadAccess[eventStage]), + static_cast(mCurrentReadEvents.getAccessFlags(eventStage)), + static_cast(mPOCGroundTruthReadAccess[eventStage]), + static_cast(mCurrentReadEvents.getAccessFlags(eventStage) & + ~mPOCGroundTruthReadAccess[eventStage]), + static_cast(readAccessType)); + fflush(stderr); + abort(); + } return; } @@ -5223,12 +5285,31 @@ void BufferHelper::recordReadEvent(Context *context, // waitEvent later. mCurrentReadEvents.replaceEventAtStage( context, eventStage, refCountedEventArray->getEvent(eventStage), readAccessType); + // POC instrumentation: mirror replaceEventAtStage's mAccessFlags |= readAccessType. + mPOCGroundTruthReadAccess[eventStage] |= readAccessType; + if (getenv("POC_TRACE")) + fprintf(stderr, + "[POC] recordReadEvent(this=%p) VkEvent path: stage=%d access=0x%x " + "-> mAccessFlags=0x%x groundTruth=0x%x\n", + static_cast(this), static_cast(eventStage), + static_cast(readAccessType), + static_cast(mCurrentReadEvents.getAccessFlags(eventStage)), + static_cast(mPOCGroundTruthReadAccess[eventStage])); } else { // Accumulate new read usage to be used in pipelineBarrier. mCurrentReadAccess |= readAccessType; mCurrentReadStages |= readPipelineStageFlags; + if (getenv("POC_TRACE")) + fprintf(stderr, + "[POC] recordReadEvent(this=%p) non-VkEvent path: stage=%d access=0x%x " + "(feature=%d eventStageValid=%d xfbBits=0x%x)\n", + static_cast(this), static_cast(eventStage), + static_cast(readAccessType), + context->getFeatures().useVkEventForBufferBarrier.enabled, + eventStage != EventStage::InvalidEnum, + static_cast(mXFBOrComputeWriteHeuristicBits.bits())); } if (getResourceUse() >= queueSerial) @@ -5258,6 +5339,12 @@ void BufferHelper::recordWriteBarrier(Context *context, Renderer *renderer = context->getRenderer(); updatePipelineStageWriteHistory(context, stageIndex); + if (getenv("POC_TRACE")) + fprintf(stderr, + "[POC] recordWriteBarrier(this=%p) writeStage=%d access=0x%x xfbBits(after)=0x%x\n", + static_cast(this), static_cast(stageIndex), + static_cast(writeAccessType), + static_cast(mXFBOrComputeWriteHeuristicBits.bits())); // Barrier against prior read VkEvents. if (!mCurrentReadEvents.empty()) @@ -5285,6 +5372,8 @@ void BufferHelper::recordWriteBarrier(Context *context, } // Garbage collect the event, which tracks GPU completion automatically. mCurrentReadEvents.releaseToEventCollector(eventCollector); + // POC instrumentation: mirror releaseToEventCollector's mAccessFlags[*] = 0. + mPOCGroundTruthReadAccess.fill(0); } // Barrier against prior write VkEvent. diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.h b/src/libANGLE/renderer/vulkan/vk_helpers.h index 21b481a819..c54b04936c 100644 --- a/src/libANGLE/renderer/vulkan/vk_helpers.h +++ b/src/libANGLE/renderer/vulkan/vk_helpers.h @@ -1098,6 +1098,11 @@ class BufferHelper : public ReadWriteResource static constexpr uint32_t kXFBOrComputeWriteHeuristicWindowSize = 16; angle::BitSet16 mXFBOrComputeWriteHeuristicBits; + // POC instrumentation: ground-truth dstAccessMask actually emitted per EventStage + // for the current incarnation. Properly reset in initializeBarrierTracker(), unlike + // RefCountedEventArrayWithAccessFlags::mAccessFlags which survives base release(). + angle::PackedEnumMap mPOCGroundTruthReadAccess; + BufferSerial mSerial; // Manages the descriptorSet cache that created with this BufferHelper object. DescriptorSetCacheManager mDescriptorSetCacheManager; diff --git a/src/tests/gl_tests/TransformFeedbackTest.cpp b/src/tests/gl_tests/TransformFeedbackTest.cpp index bc13f6456e..9588d89b2d 100644 --- a/src/tests/gl_tests/TransformFeedbackTest.cpp +++ b/src/tests/gl_tests/TransformFeedbackTest.cpp @@ -5273,6 +5273,197 @@ TEST_P(TransformFeedbackTestVkEvent, BufferVkEventUAF) glFinish(); } +// Reproduces a Vulkan barrier-tracking bug in RefCountedEventArrayWithAccessFlags: +// the inherited (non-virtual) RefCountedEventArray::release() clears only mBitMask, +// leaving mAccessFlags[] stale across BufferHelper::initializeBarrierTracker(). +// After reinit, the next replaceEventAtStage() OR-accumulates into the stale flags, +// so recordReadBarrier()'s hasEventAndAccess() early-out fires for an access type +// for which NO barrier was emitted in the current incarnation -> required Vulkan +// memory dependency is omitted (SYNC-HAZARD-READ-AFTER-WRITE). +// +// On production Android (Mali/Adreno/PowerVR), the VkEvent path is reached via native +// VK_EXT_transform_feedback writes. On SwiftShader (no native XFB), it is reached via +// the compute-shader heuristic gated by Feature::IsVertexSyncDeferred (Mali default). +// Both UseVkEventForBufferBarrier and IsVertexSyncDeferred are on by default in +// production on Mali GPUs. +// +// This test relies on POC instrumentation in vk_helpers.cpp that aborts when the +// early-out is taken with stale mAccessFlags bits. +TEST_P(TransformFeedbackTestVkEvent, StaleAccessFlagsSkipsBarrier) +{ + // Transform-feedback writer: outputs 0.0 so buffer bytes reinterpret as uint32 index 0. + constexpr char kXfbVS[] = R"(#version 300 es +out float xfbOut; +void main(){ xfbOut = 0.0; gl_Position = vec4(0,0,0,1); gl_PointSize = 1.0; })"; + constexpr char kXfbFS[] = R"(#version 300 es +precision mediump float; out vec4 c; void main(){ c = vec4(1); })"; + std::vector tfVaryings = {"xfbOut"}; + mProgram = CompileProgramWithTransformFeedback(kXfbVS, kXfbFS, tfVaryings, + GL_INTERLEAVED_ATTRIBS); + ASSERT_NE(0u, mProgram); + + // Compute writer: writes 0u to B[0] via SSBO (PipelineStage::ComputeShader). + GLuint csProg = 0; + if (getClientMajorVersion() * 10 + getClientMinorVersion() >= 31) + { + constexpr char kCS[] = R"(#version 310 es +layout(local_size_x=1) in; +layout(std430, binding=0) buffer Out { uint d[]; } o; +void main(){ o.d[0] = 0u; })"; + csProg = CompileComputeProgram(kCS); + ASSERT_NE(0u, csProg); + } + + // Draw program: reads attribute 0 as position. + constexpr char kDrawVS[] = R"(#version 300 es +layout(location=0) in vec4 a; +void main(){ gl_Position = a; gl_PointSize = 1.0; })"; + constexpr char kDrawFS[] = R"(#version 300 es +precision mediump float; out vec4 c; void main(){ c = vec4(0,1,0,1); })"; + ANGLE_GL_PROGRAM(drawProg, kDrawVS, kDrawFS); + + // Buffer B: the buffer whose BufferHelper is reinit'd in-place. + constexpr GLsizeiptr kBufSize = 256; + std::vector zeros(kBufSize, 0); + GLBuffer bufB; + glBindBuffer(GL_ARRAY_BUFFER, bufB); + glBufferData(GL_ARRAY_BUFFER, kBufSize, zeros.data(), GL_STREAM_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + // Position buffer P: provides one vertex for indexed draws. + GLBuffer bufP; + constexpr GLfloat kPos[4] = {0.0f, 0.0f, 0.0f, 1.0f}; + glBindBuffer(GL_ARRAY_BUFFER, bufP); + glBufferData(GL_ARRAY_BUFFER, sizeof(kPos), kPos, GL_STATIC_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + + // VAO 1: B as ELEMENT_ARRAY_BUFFER, P as vertex attrib 0. + GLVertexArray vaoIdx; + glBindVertexArray(vaoIdx); + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, bufB); + glBindBuffer(GL_ARRAY_BUFFER, bufP); + glEnableVertexAttribArray(0); + glVertexAttribPointer(0, 4, GL_FLOAT, GL_FALSE, 0, nullptr); + glBindVertexArray(0); + + // VAO 2: B as vertex attrib 0. + GLVertexArray vaoAttr; + glBindVertexArray(vaoAttr); + glBindBuffer(GL_ARRAY_BUFFER, bufB); + glEnableVertexAttribArray(0); + glVertexAttribPointer(0, 1, GL_FLOAT, GL_FALSE, 0, nullptr); + glBindVertexArray(0); + + // Render target. + GLTexture colorTex; + glBindTexture(GL_TEXTURE_2D, colorTex); + glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGBA8, 4, 4); + GLFramebuffer fbo; + glBindFramebuffer(GL_FRAMEBUFFER, fbo); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, colorTex, 0); + glViewport(0, 0, 4, 4); + ASSERT_GL_NO_ERROR(); + + // Writes B from the GPU. Uses native XFB if available (PipelineStage::TransformFeedback, + // production path on Android TBR), otherwise compute SSBO write (PipelineStage::ComputeShader, + // sets heuristic via IsVertexSyncDeferred). Both are production-default on Mali. + auto gpuWriteB = [&]() { + if (csProg != 0) + { + glUseProgram(csProg); + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, bufB); + glDispatchCompute(1, 1, 1); + glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, 0); + glMemoryBarrier(GL_VERTEX_ATTRIB_ARRAY_BARRIER_BIT | GL_ELEMENT_ARRAY_BARRIER_BIT); + } + else + { + glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, mTransformFeedback); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, bufB); + glUseProgram(mProgram); + glEnable(GL_RASTERIZER_DISCARD); + glBeginTransformFeedback(GL_POINTS); + glDrawArrays(GL_POINTS, 0, 1); + glEndTransformFeedback(); + glDisable(GL_RASTERIZER_DISCARD); + glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, 0); + glBindTransformFeedback(GL_TRANSFORM_FEEDBACK, 0); + } + }; + + // ---- STEP 0: prime mXFBOrComputeWriteHeuristicBits. For the compute path, the heuristic + // requires (mCurrentReadStages & VERTEX_INPUT_BIT) != 0 at write time, so read B at + // VertexInput first (non-VkEvent path since heuristic==0). ---- + glUseProgram(drawProg); + glBindVertexArray(vaoAttr); + glDrawArrays(GL_POINTS, 0, 1); + glBindVertexArray(0); + ASSERT_GL_NO_ERROR(); + + // ---- STEP 1: GPU-write B. updatePipelineStageWriteHistory sets the heuristic bit so + // subsequent reads take the VkEvent path. recordWriteEvent sets mCurrentWriteEvent. ---- + gpuWriteB(); + ASSERT_GL_NO_ERROR(); + + // ---- STEP 2: read B as INDEX_READ at EventStage::VertexInput. + // recordReadBarrier emits a barrier; recordReadEvent (VkEvent path) does + // replaceEventAtStage(VertexInput, E1, VK_ACCESS_INDEX_READ_BIT). ---- + glUseProgram(drawProg); + glBindVertexArray(vaoIdx); + glDrawElements(GL_POINTS, 1, GL_UNSIGNED_INT, nullptr); + glBindVertexArray(0); + ASSERT_GL_NO_ERROR(); + + // ---- STEP 3: orphan B while in-use. glBufferData(NULL, sameSize, sameUsage) on an + // in-use buffer => calculateBufferUpdateTypeOnFullUpdate => StorageRedefined => + // acquireBufferHelper => releaseBufferAllocation => BufferHelper::release(Context*) + // [base RefCountedEventArray::release() clears mBitMask only, mAccessFlags[VertexInput] + // STALE = INDEX_READ_BIT] => initBufferAllocation => initializeBarrierTracker + // (resets mCurrentWrite*/mCurrentRead* but NOT mAccessFlags or heuristic bits). ---- + glBindBuffer(GL_ARRAY_BUFFER, bufB); + glBufferData(GL_ARRAY_BUFFER, kBufSize, nullptr, GL_STREAM_DRAW); + glBindBuffer(GL_ARRAY_BUFFER, 0); + ASSERT_GL_NO_ERROR(); + + // ---- STEP 4: GPU-write the new B incarnation. mCurrentReadEvents.empty() so + // releaseToEventCollector is NOT called -> stale mAccessFlags survives. recordWriteEvent + // sets mCurrentWriteEvent (heuristic bit still set; not reset on reinit). ---- + gpuWriteB(); + ASSERT_GL_NO_ERROR(); + + // ---- STEP 5: read B as VERTEX_ATTRIBUTE_READ at EventStage::VertexInput. + // mBitMask[VertexInput]==false so recordReadBarrier emits a barrier with + // dstAccessMask = VK_ACCESS_VERTEX_ATTRIBUTE_READ_BIT. recordReadEvent then + // replaceEventAtStage OR-accumulates into the STALE mAccessFlags: + // mAccessFlags[VertexInput] = INDEX_READ | VERTEX_ATTRIBUTE_READ + // mBitMask[VertexInput] = true ---- + glUseProgram(drawProg); + glBindVertexArray(vaoAttr); + glDrawArrays(GL_POINTS, 0, 1); + glBindVertexArray(0); + ASSERT_GL_NO_ERROR(); + + // ---- STEP 6: read B as INDEX_READ at EventStage::VertexInput. + // recordReadBarrier: hasEventAndAccess(VertexInput, INDEX_READ_BIT) + // = mBitMask[VertexInput] && ((INDEX_READ|VERTEX_ATTRIBUTE_READ) & INDEX_READ)==INDEX_READ + // = true -> early return, NO barrier emitted. + // But the only barrier between STEP 4's write and this read had + // dstAccessMask = VERTEX_ATTRIBUTE_READ_BIT, which per the Vulkan spec does NOT make + // the write visible to VK_ACCESS_INDEX_READ_BIT. + // The POC instrumentation in vk_helpers.cpp aborts here with a diagnostic. ---- + glUseProgram(drawProg); + glBindVertexArray(vaoIdx); + glDrawElements(GL_POINTS, 1, GL_UNSIGNED_INT, nullptr); + glBindVertexArray(0); + + glFinish(); + if (csProg != 0) + { + glDeleteProgram(csProg); + } + ASSERT_GL_NO_ERROR(); +} + GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(TransformFeedbackTest); ANGLE_INSTANTIATE_TEST_ES3_AND(TransformFeedbackTest, ES3_VULKAN().disable(Feature::SupportsTransformFeedbackExtension), @@ -5321,5 +5512,10 @@ ANGLE_INSTANTIATE_TEST_ES3_AND(WebGLTransformFeedbackTest, GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(TransformFeedbackTestVkEvent); ANGLE_INSTANTIATE_TEST_ES3_AND( TransformFeedbackTestVkEvent, - ES3_VULKAN().enable(Feature::UseVkEventForBufferBarrier).disable(Feature::RecycleVkEvent)); + ES3_VULKAN().enable(Feature::UseVkEventForBufferBarrier).disable(Feature::RecycleVkEvent), + // POC: SwiftShader has no native VK_EXT_transform_feedback, so the VkEvent heuristic is + // reached via the compute path with IsVertexSyncDeferred (Mali production default). + ES31_VULKAN_SWIFTSHADER() + .enable(Feature::UseVkEventForBufferBarrier) + .enable(Feature::IsVertexSyncDeferred)); } // anonymous namespace