From 51883cf1a3df97a3d70a5e7e8d8832ab8e817fbf Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 15:35:02 -0400 Subject: [PATCH] [Fix, Test] (GLState): deliver the GL_MIN_MAP_BUFFER_ALIGNMENT that glGetIntegerv advertises --- MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp | 6 +- .../GLState/BufferState/BufferObject.cpp | 23 +++- .../GLState/BufferState/BufferObject.h | 9 +- .../GLState/BufferState/PipeResource.h | 57 ++++++++- MobileGL/MG_Test/Buffer/BufferTest.cpp | 111 ++++++++++++++++++ 5 files changed, 195 insertions(+), 11 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 8e7a03eb..7f0ac27f 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -1664,7 +1664,11 @@ namespace MobileGL::MG_Impl::GLImpl { *params = MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::Multisample) ? GL_TRUE : GL_FALSE; return; case GL_MIN_MAP_BUFFER_ALIGNMENT: - *params = 64; // TODO + // The same constant the map paths align to (MG_State/GLState/BufferState/ + // PipeResource.h), never a literal: this number is a PROMISE about the pointers + // glMapBuffer and glMapBufferRange return, and the two used to be unrelated - the + // query said 64 while the pointers came out of a std::vector aligned to 16. + *params = static_cast(MG_State::GLState::MIN_MAP_BUFFER_ALIGNMENT); return; case GL_MAX_LABEL_LENGTH: *params = 256; // TODO diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp index b98176f6..1ca335b7 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp @@ -163,7 +163,7 @@ namespace MobileGL::MG_State::GLState { if (!m_resource.IsGpuResident() && !(m_mappingAccess & BufferMappingAccessBit::FlushExplicit)) { // if we didn't flush explicitly if (!(m_mappingAccess & BufferMappingAccessBit::Persistent)) { - Memcpy(m_resource.Bytes() + m_mappedRange.start, m_stagingData.data(), + Memcpy(m_resource.Bytes() + m_mappedRange.start, m_stagingData.data() + m_stagingBias, m_mappedRange.end - m_mappedRange.start); } NotifyFlushMappedRange(m_mappedRange, m_mappingAccess); @@ -175,6 +175,7 @@ namespace MobileGL::MG_State::GLState { m_isMapped = false; m_mappingAccess = BufferMappingAccessBit::Null; m_mappedRange = {0, 0}; + m_stagingBias = 0; m_ownsStagingData = false; } @@ -193,7 +194,7 @@ namespace MobileGL::MG_State::GLState { // FLUSH_EXPLICIT maps are never GPU-resident (only coherent maps are adopted), so // the staged bytes must be copied into the shadow before the backend reads them. if (!(m_mappingAccess & BufferMappingAccessBit::Persistent)) { - Memcpy(m_resource.Bytes() + start, m_stagingData.data() + offset, length); + Memcpy(m_resource.Bytes() + start, m_stagingData.data() + m_stagingBias + offset, length); } NotifyFlushMappedRange({start, end}, m_mappingAccess); } @@ -311,6 +312,9 @@ namespace MobileGL::MG_State::GLState { m_mappedRange = {0, m_size}; if (m_mappingAccess & BufferMappingAccessBit::Write) { + // glMapBuffer maps from offset 0, so no bias: the allocation's own + // GL_MIN_MAP_BUFFER_ALIGNMENT-aligned base is what the application must get. + m_stagingBias = 0; m_stagingData.resize(m_size); m_ownsStagingData = true; @@ -372,14 +376,21 @@ namespace MobileGL::MG_State::GLState { } if (access & BufferMappingAccessBit::Write) { - m_stagingData.resize(range.end - range.start); + // ARB_map_buffer_alignment constrains (returned pointer - offset), not the pointer: + // a map at offset 63 must hand back a pointer 63 bytes past the alignment grid, which + // is exactly what the read path below gets for free from shadowBase + offset. The + // staging store has to be biased by the same phase to match, so it over-allocates by + // it and the mapped bytes start at data() + m_stagingBias. + m_stagingBias = range.start % MIN_MAP_BUFFER_ALIGNMENT; + const SizeT mappedLength = range.end - range.start; + m_stagingData.resize(m_stagingBias + mappedLength); m_ownsStagingData = true; if (!(access & (BufferMappingAccessBit::InvalidateRange | BufferMappingAccessBit::InvalidateBuffer))) { - Memcpy(m_stagingData.data(), m_resource.Bytes() + range.start, m_stagingData.size()); + Memcpy(m_stagingData.data() + m_stagingBias, m_resource.Bytes() + range.start, mappedLength); } - return m_stagingData.data(); + return m_stagingData.data() + m_stagingBias; } else { m_ownsStagingData = false; return m_resource.Bytes() + range.start; @@ -438,7 +449,7 @@ namespace MobileGL::MG_State::GLState { return const_cast(m_resource.Bytes()) + m_mappedRange.start; } if (m_ownsStagingData) { - return const_cast(m_stagingData.data()); + return const_cast(m_stagingData.data()) + m_stagingBias; } return const_cast(m_resource.Bytes()) + m_mappedRange.start; } diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.h b/MobileGL/MG_State/GLState/BufferState/BufferObject.h index cebf4daf..311c5698 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.h +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.h @@ -239,7 +239,14 @@ namespace MobileGL { // Set by MarkGpuWritten, cleared by SyncGpuWrites once the shadow is refreshed. Bool m_gpuWritePending = false; Range1D m_mappedRange; - Vector m_stagingData; + // The write-map staging store. MapAlignedData because the application is handed a + // pointer into it, and biased by m_stagingBias because ARB_map_buffer_alignment + // requires (returned pointer - offset) to be aligned, not the pointer itself: a range + // map at offset 63 must hand back a pointer sitting 63 bytes past the alignment grid. + // The bias is the offset's phase, so the mapped bytes still start at + // m_stagingData.data() + m_stagingBias and the allocation is that much longer. + MapAlignedData m_stagingData; + SizeT m_stagingBias = 0; Bool m_ownsStagingData; }; } // namespace MG_State::GLState diff --git a/MobileGL/MG_State/GLState/BufferState/PipeResource.h b/MobileGL/MG_State/GLState/BufferState/PipeResource.h index 3fcdc0b4..e0d8ca96 100644 --- a/MobileGL/MG_State/GLState/BufferState/PipeResource.h +++ b/MobileGL/MG_State/GLState/BufferState/PipeResource.h @@ -10,8 +10,56 @@ #include #include #include +#include +#include namespace MobileGL::MG_State::GLState { + // GL_MIN_MAP_BUFFER_ALIGNMENT. GL 4.2 / ARB_map_buffer_alignment fix the minimum at 64 and + // MobileGL advertises exactly that (MG_Impl/GLImpl/Getter/GL_Getter.cpp reads this constant), + // so under-reporting is not available - the implementation has to be brought up to the number + // instead. The promise is about POINTERS, not just the query: glMapBuffer must return a + // 64-byte-aligned pointer, and glMapBufferRange must return one whose base - the returned + // pointer minus the offset the caller asked for - is. Every pointer the frontend hands out + // comes from the shadow below or from BufferObject's staging buffer, and std::vector only + // promises alignof(std::max_align_t) (16 on aarch64), so both allocations carry the alignment + // themselves. One constant for the getter and the allocator, because the two may never + // disagree - the same reason the atomic-counter limits are shared through + // MG_Util/ShaderTranspiler/Types.h. + inline constexpr SizeT MIN_MAP_BUFFER_ALIGNMENT = 64; + + // Allocator that gives every allocation MIN_MAP_BUFFER_ALIGNMENT. Deliberately minimal: the + // vectors it backs hold raw bytes and are only ever sized, so allocate/deallocate plus the + // rebinding and equality boilerplate std::vector requires is the whole interface. + template + struct MapAlignedAllocator { + using value_type = T; + + MapAlignedAllocator() noexcept = default; + template + MapAlignedAllocator(const MapAlignedAllocator&) noexcept {} + + T* allocate(SizeT count) { + if (count == 0) return nullptr; + return static_cast( + ::operator new(count * sizeof(T), std::align_val_t{MIN_MAP_BUFFER_ALIGNMENT})); + } + void deallocate(T* pointer, SizeT) noexcept { + ::operator delete(pointer, std::align_val_t{MIN_MAP_BUFFER_ALIGNMENT}); + } + + template + Bool operator==(const MapAlignedAllocator&) const noexcept { + return true; + } + template + Bool operator!=(const MapAlignedAllocator&) const noexcept { + return false; + } + }; + + // Byte store for anything the application may end up holding a mapped pointer into. + using MapAlignedData = std::vector>; + // Opaque, refcounted handle to the backend's GPU storage for one buffer // (the driver-side resource). The active backend derives from it and attaches // its own payload (VkBufferResource / GLESBufferResource). Held by PipeResource. @@ -57,8 +105,8 @@ namespace MobileGL::MG_State::GLState { } // Direct shadow access, used only by the backend's upload-from-shadow path, // which never runs for a GPU-resident (persistent) buffer. - Data& Shadow() { return *m_shadow; } - const Data& Shadow() const { return *m_shadow; } + MapAlignedData& Shadow() { return *m_shadow; } + const MapAlignedData& Shadow() const { return *m_shadow; } // Transition to persistent GPU residency: adopt the backend's coherent // mapped base as the source of truth and drop the CPU shadow. The caller @@ -85,7 +133,10 @@ namespace MobileGL::MG_State::GLState { SharedPtr ReleaseBackend() { return std::move(m_backend); } private: - SharedPtr m_shadow = MakeShared(); + // MapAlignedData, not Data: a read-only glMapBuffer hands the application this very + // pointer, and a range map hands it base + offset, so the base has to be on the + // GL_MIN_MAP_BUFFER_ALIGNMENT grid for either to satisfy ARB_map_buffer_alignment. + SharedPtr m_shadow = MakeShared(); void* m_gpuMapped = nullptr; SharedPtr m_backend; }; diff --git a/MobileGL/MG_Test/Buffer/BufferTest.cpp b/MobileGL/MG_Test/Buffer/BufferTest.cpp index c4f05d56..7a463b9c 100644 --- a/MobileGL/MG_Test/Buffer/BufferTest.cpp +++ b/MobileGL/MG_Test/Buffer/BufferTest.cpp @@ -8,6 +8,7 @@ #include +#include #include #include "Includes.h" @@ -267,6 +268,116 @@ TEST_F(BufferTest, AcquireMemoryRangeWithExplicit) { ASSERT_EQ(actual, expected); } +// GL_MIN_MAP_BUFFER_ALIGNMENT is a promise about POINTERS, and MobileGL used to keep only the +// query half of it: glGetIntegerv answered 64 while every mapped pointer came out of a plain +// std::vector, aligned to alignof(std::max_align_t) - 16 on aarch64. GL 4.2 / +// ARB_map_buffer_alignment fix the minimum at 64, so under-reporting is not available and the +// implementation has to be brought up to the number instead. Note the two different constraints: +// glMapBuffer's pointer must be aligned outright, while glMapBufferRange's must be aligned AFTER +// subtracting the offset the caller asked for - i.e. it sits at the offset's own alignment phase. +// KHR-GLxx.map_buffer_alignment.functional asserts exactly these two, at offset 63, for 24 +// storage-flag combinations across 14 targets, and failed identically on both test devices. +TEST_F(BufferTest, MappedPointersHonourTheAdvertisedMapBufferAlignment) { + GLint advertisedAlignment = 0; + MobileGL::MG_Impl::GLImpl::GetIntegerv(GL_MIN_MAP_BUFFER_ALIGNMENT, &advertisedAlignment); + ASSERT_EQ(advertisedAlignment, static_cast(MobileGL::MG_State::GLState::MIN_MAP_BUFFER_ALIGNMENT)) + << "the query and the allocator must read the same constant"; + ASSERT_GE(advertisedAlignment, 64) << "GL 4.2 fixes the minimum at 64"; + const SizeT alignment = static_cast(advertisedAlignment); + + auto& slot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::Uniform); + Vector bufferNames; + MobileGL::MG_State::pGLContext->GenBufferNames(1, bufferNames); + auto bufObj = MobileGL::MG_State::pGLContext->CreateBufferObject(bufferNames[0]); + slot.Bind(bufObj); + + // The conformance test's own shape: a buffer two alignments long, mapped from the last byte + // inside the first alignment - the offset most likely to expose a base-aligned-only fix. + const SizeT bufferSize = 2 * alignment; + const SizeT offset = alignment - 1; + bufObj->Resize(bufferSize); + Vector initData(bufferSize); + for (SizeT i = 0; i < bufferSize; ++i) initData[i] = static_cast(i); + bufObj->UploadData(DataPtr{.data = initData.data(), .size = bufferSize}, 0); + + const auto addressOf = [](const void* pointer) { return reinterpret_cast(pointer); }; + + // glMapBuffer, read-only: the shadow base itself is handed out. + void* readMapped = bufObj->AcquireMemory(true, true, false); + ASSERT_NE(readMapped, nullptr); + EXPECT_EQ(addressOf(readMapped) % alignment, 0u) << "glMapBuffer(GL_READ_ONLY) returned an unaligned pointer"; + bufObj->ReleaseMemory(); + + // glMapBuffer, write: the staging store is handed out instead. + void* writeMapped = bufObj->AcquireMemory(true, false, true); + ASSERT_NE(writeMapped, nullptr); + EXPECT_EQ(addressOf(writeMapped) % alignment, 0u) << "glMapBuffer(GL_WRITE_ONLY) returned an unaligned pointer"; + EXPECT_EQ(bufObj->GetMappedPointer(), writeMapped) + << "GL_BUFFER_MAP_POINTER must report the pointer the map returned"; + bufObj->ReleaseMemory(); + + // glMapBufferRange, read-only: shadow base + offset, so the phase falls out for free. + const Range1D mapRange{.start = offset, .end = bufferSize}; + void* rangeRead = bufObj->AcquireMemoryRange(mapRange, BufferMappingAccessBit::Read); + ASSERT_NE(rangeRead, nullptr); + EXPECT_EQ((addressOf(rangeRead) - offset) % alignment, 0u) + << "glMapBufferRange(READ) returned a pointer whose base is unaligned"; + bufObj->ReleaseMemory(); + + // glMapBufferRange, write: the staging store has to be biased to the same phase, and the + // write-back has to follow the bias or the bytes land at the wrong place in the shadow. + Uint8* rangeWrite = static_cast(bufObj->AcquireMemoryRange(mapRange, BufferMappingAccessBit::Write)); + ASSERT_NE(rangeWrite, nullptr); + EXPECT_EQ((addressOf(rangeWrite) - offset) % alignment, 0u) + << "glMapBufferRange(WRITE) returned a pointer whose base is unaligned"; + EXPECT_EQ(bufObj->GetMappedPointer(), rangeWrite) + << "GL_BUFFER_MAP_POINTER must report the pointer the map returned"; + // Seeded from the shadow, so the mapped view starts at the offset's byte. + EXPECT_EQ(rangeWrite[0], static_cast(offset)); + rangeWrite[0] = 0xAB; + rangeWrite[bufferSize - offset - 1] = 0xCD; + bufObj->ReleaseMemory(); + + Vector readBack(bufferSize); + bufObj->DownloadSubData(readBack.data(), 0, bufferSize); + EXPECT_EQ(readBack[offset], 0xAB) << "the biased staging write-back landed at the wrong offset"; + EXPECT_EQ(readBack[bufferSize - 1], 0xCD) << "the biased staging write-back landed at the wrong offset"; + EXPECT_EQ(readBack[offset - 1], static_cast(offset - 1)) << "the write-back overran the mapped range"; +} + +// The explicit-flush path reads through the same bias, one flush offset further in: a flush of +// [offset + 4, offset + 8) must copy the bytes the application wrote at rangeWrite[4..8), not the +// ones sitting four bytes into the raw allocation. +TEST_F(BufferTest, ExplicitFlushOfARangeMapFollowsTheAlignmentBias) { + auto& slot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::Uniform); + Vector bufferNames; + MobileGL::MG_State::pGLContext->GenBufferNames(1, bufferNames); + auto bufObj = MobileGL::MG_State::pGLContext->CreateBufferObject(bufferNames[0]); + slot.Bind(bufObj); + + const SizeT alignment = MobileGL::MG_State::GLState::MIN_MAP_BUFFER_ALIGNMENT; + const SizeT bufferSize = 2 * alignment; + const SizeT offset = alignment - 1; + bufObj->Resize(bufferSize); + Vector initData(bufferSize, 0); + bufObj->UploadData(DataPtr{.data = initData.data(), .size = bufferSize}, 0); + + const Range1D mapRange{.start = offset, .end = bufferSize}; + Uint8* mapped = static_cast(bufObj->AcquireMemoryRange( + mapRange, BufferMappingAccessBit::Write | BufferMappingAccessBit::FlushExplicit)); + ASSERT_NE(mapped, nullptr); + mapped[4] = 0x5A; + mapped[5] = 0x5B; + bufObj->FlushMemoryRange(4, 2); + bufObj->ReleaseMemory(); + + Vector readBack(bufferSize); + bufObj->DownloadSubData(readBack.data(), 0, bufferSize); + EXPECT_EQ(readBack[offset + 4], 0x5A); + EXPECT_EQ(readBack[offset + 5], 0x5B); + EXPECT_EQ(readBack[offset + 3], 0x00) << "the explicit flush copied bytes outside the flushed range"; +} + TEST_F(BufferTest, CopyBufferSubData) { auto& srcSlot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::CopyRead); auto& dstSlot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::CopyWrite);