diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index 465362b3..245e1521 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -1160,9 +1160,32 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (texelSize == 0 || !ResolveWireRange(record.Desc.BufOffset, record.Desc.BufSize, slice.size, start, size)) return false; size = std::min(size / texelSize, static_cast(m_wireMaxTexelElements)) * texelSize; if (size == 0) return false; - if (start > std::numeric_limits::max() - slice.offset || - (slice.offset + start) % m_wireTexelOffsetAlignment != 0) - WireDescriptorFatal("unaligned-texel-buffer-range@P7"); + if (start > std::numeric_limits::max() - slice.offset) + WireDescriptorFatal("texel-buffer-offset-overflow"); + if ((slice.offset + start) % m_wireTexelOffsetAlignment != 0) { + // P7 A.4, a named decline (rule I (a)). Not reachable through the public API: + // glTexBufferRange already refuses an offset that is not a multiple of + // GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT (GL_Texture.cpp), and that cap is published + // from this same minTexelBufferOffsetAlignment. It stays a decline rather than a + // copy for the reason below. + const Bool writable = storage && state.BoundShaderImages[unit].Access != kMGPipeImageAccessReadOnly; + MGLOG_E_ONCE("ResolveWireTexelBufferDescriptor: binding %u is bound at offset %llu, which is not a " + "multiple of this device's minTexelBufferOffsetAlignment (%llu); the %s view is DECLINED " + "(unaligned-texel-buffer-range)", binding, + static_cast(slice.offset + start), + static_cast(m_wireTexelOffsetAlignment), + writable ? "writable" : "read-only"); + // A read-only fetch can still be served the way an unbound buffer texture is - + // zeros, which is what a missing view gives in monolith - and that keeps the draw. + // A writable imageBuffer must not: the placeholder is shared by every unbound + // binding, so absorbing real stores into it would alias other bindings AND lose + // the application's writes without a word. Copying into an aligned transient slice + // is not the answer either: the transient arena carries no + // {UNIFORM,STORAGE}_TEXEL_BUFFER usage (VkBufferManager::InitializeTransientArenas), + // so no VkBufferView can be made over it, and widening a shared per-frame + // allocation to buy an unreachable path is the wrong trade. See notes/p7/magma-a.md. + return writable ? false : placeholder(); + } VkBufferViewCreateInfo viewInfo{VK_STRUCTURE_TYPE_BUFFER_VIEW_CREATE_INFO}; viewInfo.buffer = slice.buffer; viewInfo.format = format; @@ -1550,10 +1573,43 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (!m_bufferManager->AcquireWireSlice(BufferKind::ShaderStorage, range.Res, slice) || !slice.IsValid()) return false; VkDeviceSize start = 0, size = 0; if (!ResolveWireRange(range.Offset, range.Size, slice.size, start, size)) return false; - if (start > std::numeric_limits::max() - slice.offset || - (slice.offset + start) % m_wireStorageOffsetAlignment != 0) - WireDescriptorFatal(isAtomicCounterBlock ? "unaligned-atomic-buffer-range@P7" - : "unaligned-storage-buffer-range@P7"); + if (start > std::numeric_limits::max() - slice.offset) + WireDescriptorFatal("storage-buffer-offset-overflow"); + if ((slice.offset + start) % m_wireStorageOffsetAlignment != 0) { + // P7 A.4, a named decline (rule I (a)), and the one shape in this cluster an + // application can actually reach. GL has no queryable atomic-counter buffer + // offset alignment, so glBindBufferRange only enforces offset % 4 for + // GL_ATOMIC_COUNTER_BUFFER (GL 4.6 core 6.1.1, GL_Buffer.cpp + // ValidateBufferRangeOffsetAndSize); glslang lowers the counter block onto a + // storage buffer, whose descriptor offset must be a multiple of + // minStorageBufferOffsetAlignment - 16 on lavapipe, 64 on Adreno. Offset 4 is + // therefore a legal bind this backend cannot express. The plain SSBO half is + // unreachable by comparison: GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT is + // published from that same Vulkan limit, so the frontend already refused it + // with GL_INVALID_VALUE. + // + // DECLINED RATHER THAN COPIED, deliberately. Copying the window into an aligned + // transient slice reads correctly and then silently DROPS every shader write, + // and this site cannot tell a writable block from a read-only one: there is no + // readonly bit in the storage-block reflection (ProgramFactory.h carries only + // name and index), an atomic counter is written by definition, and + // MarkWireBufferGpuWritten below already assumes any binding here may be + // written. A correct copy needs a post-dispatch copy-back from the transient + // slice to the unaligned source, ordered with a barrier - and the place to + // record that is the draw/dispatch tail in VulkanRenderer.cpp / WireDraw.inc, + // which belong to wave-2 packages B and C, not to this file. Losing the + // dispatch with a named line in the role log beats returning stale counters + // with no error at all. notes/p7/magma-a.md carries the recipe. + MGLOG_E_ONCE("ResolveStorageBufferDescriptor: block '%s' is bound at offset %llu, which is not a " + "multiple of this device's minStorageBufferOffsetAlignment (%llu); %s ranges cannot be " + "expressed as a descriptor and the shader's writes would be dropped by a read-only " + "copy, so the binding is DECLINED (unaligned-%s-buffer-range)", + blockName.c_str(), static_cast(slice.offset + start), + static_cast(m_wireStorageOffsetAlignment), + isAtomicCounterBlock ? "atomic-counter" : "shader-storage", + isAtomicCounterBlock ? "atomic" : "storage"); + return false; + } if (size > m_wireMaxStorageRange) { // P7 A.3: clamp, do not refuse. The monolith arm binds whatever the GL range // asked for and never consults maxStorageBufferRange at all (see the resident diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 23618ff2..3cdfcb54 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -2706,6 +2706,7 @@ if (MOBILEGL_BUILD_DISAGGREGATED) F1WireScenario.UniformBufferRangeAndRebindPixels F1WireScenario.ShortUniformBufferRangePadsMissingBytes F1WireScenario.SubWordUniformBufferRangePadsMissingBytes + F1WireScenario.UnalignedAtomicCounterRangeDeclinesAndTheSessionLives F1WireScenario.ComputeWrittenVertexAndIndexBuffersDrawAndReadBack F1WireScenario.ColorBlitToDefaultIgnoresViewportAndPreservesOrientation F1WireScenario.ColorBlitToDefaultHonorsScissorAndReversedRect diff --git a/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp index 463cd745..a007c318 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp @@ -1055,6 +1055,65 @@ void main() { color = vec4(0, 1, 0, 1); } glDeleteProgram(graphics); } +TEST_F(F1WireScenario, UnalignedAtomicCounterRangeDeclinesAndTheSessionLives) { + if (!Ready()) return; + // The one shape in the P7 unaligned-range cluster an application can actually reach. + // GL 4.6 core 6.1.1 gives GL_ATOMIC_COUNTER_BUFFER no queryable offset alignment, so + // glBindBufferRange only enforces offset % 4 on it, while glslang lowers the counter + // block onto a storage buffer whose descriptor offset must be a multiple of + // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT (minStorageBufferOffsetAlignment: 16 on + // lavapipe, 64 on Adreno). Offset 4 is therefore a legal bind the backend cannot + // express. It used to take the session with it as a named Fatal; it is now a decline, + // and what this case pins is exactly that - the dispatch is lost, nothing else is. + if (Gl().BackendName() != "DirectVulkan") GTEST_SKIP() << "Magma descriptor-alignment decline"; + GLint storageAlignment = 0; + glGetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &storageAlignment); + if (storageAlignment <= 4) GTEST_SKIP() << "this device can express a 4-byte-aligned counter range"; + const char* compute = R"(#version 430 core +layout(local_size_x=1) in; +layout(binding=0, offset=0) uniform atomic_uint counter; +void main() { atomicCounterIncrement(counter); } +)"; + const GLuint cs = BuildWireProgram({{GL_COMPUTE_SHADER, compute}}); + ASSERT_NE(cs, 0u); + GLuint counters = 0; + glGenBuffers(1, &counters); + const std::array seed{0x11111111u, 0x22222222u, 0x33333333u, 0x44444444u}; + glBindBuffer(GL_ATOMIC_COUNTER_BUFFER, counters); + glBufferData(GL_ATOMIC_COUNTER_BUFFER, GLsizeiptr(seed.size() * sizeof(GLuint)), seed.data(), GL_DYNAMIC_COPY); + // Offset 4: a multiple of 4 and so a legal GL bind, but not a multiple of the storage + // alignment this device reports. The bind itself must NOT raise a GL error. + glBindBufferRange(GL_ATOMIC_COUNTER_BUFFER, 0, counters, GLintptr(4), GLsizeiptr(sizeof(GLuint))); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)) << "offset 4 is a legal GL_ATOMIC_COUNTER_BUFFER range"; + glUseProgram(cs); + glDispatchCompute(1, 1, 1); + glMemoryBarrier(GL_ATOMIC_COUNTER_BARRIER_BIT | GL_BUFFER_UPDATE_BARRIER_BIT); + // The session is still here - this is the whole point - and it still answers GL. + std::array after{}; + glGetBufferSubData(GL_ATOMIC_COUNTER_BUFFER, 0, GLsizeiptr(after.size() * sizeof(GLuint)), after.data()); + ASSERT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)) << "the declined dispatch must not poison the context"; + // The dispatch was declined, so the counter is untouched; the bytes outside the bound + // range must be untouched either way. Both halves of that are worth pinning: a future + // copy-back implementation turns the first EXPECT into seed[1] + 1 and nothing else. + EXPECT_EQ(after[1], seed[1]) << "a declined counter dispatch must not have run"; + EXPECT_EQ(after[0], seed[0]); + EXPECT_EQ(after[2], seed[2]); + EXPECT_EQ(after[3], seed[3]); + // And the context is still usable for ordinary work afterwards. + GLuint probe = 0; + glGenBuffers(1, &probe); + glBindBuffer(GL_ARRAY_BUFFER, probe); + glBufferData(GL_ARRAY_BUFFER, 16, seed.data(), GL_STATIC_DRAW); + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)) << "the session survived the decline"; + glBindBuffer(GL_ARRAY_BUFFER, 0); + glDeleteBuffers(1, &probe); + glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER, 0, 0); + glBindBuffer(GL_ATOMIC_COUNTER_BUFFER, 0); + glUseProgram(0); + glDeleteBuffers(1, &counters); + glDeleteProgram(cs); +} + TEST_F(F1WireScenario, SrgbDrawTracksFramebufferConversion) { if (!Ready()) return; Attach(GL_SRGB8_ALPHA8);