From 9145433890784488d6605bc0e3a843de1d0fe683 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 29 Sep 2026 12:16:55 -0400 Subject: [PATCH] [Fix] (P11 M2, Espryt, Magma): the texture emitter carries a level's exact dirty footprint as its regions (dev 9524c688) - a single partial box as one region, only a whole-level write as none - so both servers upload exactly the texels written over GPU content; the applier keeps it exact up to 4096 regions, and Espryt's handle arm applies the storage's box-or-rects cut-offs itself while its texture holds no GPU writes --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 24 +++++++-- MobileGL/MG_Impl/Pipe/TextureEmit.h | 54 +++++++++++++++---- MobileGL/MG_Pipe/PipeApply.cpp | 15 ++++-- MobileGL/MG_Pipe/PipeApply.h | 18 ++++--- MobileGL/MG_Test/Pipe/TextureEmitTest.cpp | 58 ++++++++++++++++----- 5 files changed, 133 insertions(+), 36 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 0378c5e0..dd118478 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -9038,9 +9038,27 @@ namespace MobileGL::MG_Backend::DirectGLES { // whole story" (D-D3), which is the same statement the // frontend's GetStorageDirtyRects makes by returning 0 - so // both arms reach the box branch below by the same route. - dirtyRectCount = - std::min(pendingUpload->Regions.size(), - MG_State::GLState::MipmapStorage::kMaxDirtyRects); + // + // P11 M2: the record's list is the level's EXACT footprint now + // (TextureEmit.h), which the storage's own rect list only + // covered, and the storage withheld that list - "0 rects" - when + // it held more rects than it keeps or when they added up to 3/4 + // of the union box (one driver call on nearly the same bytes). + // Both cut-offs are this side's to apply now, so the box-or-rects + // choice on a texture the GPU has not written is what it was. + // (Once the GPU may have written it, the footprint below uploads + // every region exactly, whatever this chose.) Never a partial + // list: a truncated list is dropped writes. + SizeT summedTexels = 0; + for (const auto& region : pendingUpload->Regions) { + summedTexels += static_cast(region.W) * static_cast(region.H) * + static_cast(std::max(region.D, 1)); + } + const Bool listWithheld = + pendingUpload->Regions.size() < 2 || + pendingUpload->Regions.size() > MG_State::GLState::MipmapStorage::kMaxDirtyRects || + summedTexels * 4 >= dirtyRegion.TexelCount() * 3; + dirtyRectCount = listWithheld ? 0 : pendingUpload->Regions.size(); for (SizeT r = 0; r < dirtyRectCount; ++r) { const auto& region = pendingUpload->Regions[r]; dirtyRects[r] = MG_State::GLState::MipmapDirtyRegion{ diff --git a/MobileGL/MG_Impl/Pipe/TextureEmit.h b/MobileGL/MG_Impl/Pipe/TextureEmit.h index 9ed12cc6..4858d698 100644 --- a/MobileGL/MG_Impl/Pipe/TextureEmit.h +++ b/MobileGL/MG_Impl/Pipe/TextureEmit.h @@ -1469,21 +1469,49 @@ namespace MobileGL::MG_Pipe { const MGPBox unionBox = MGPipeBoxOfDirtyRegion(mipmap->GetStorageDirtyRegion(uploadTarget, level)); if (unionBox.W == 0 || unionBox.H == 0 || unionBox.D == 0) return false; - // THE REGION LIST BEHIND THE BOX. 0 is legal and means "the union box is the whole - // story" - it covers a single rect (identical to the box by construction), more - // rects than the cap, and a summed area so close to the box's that one big upload - // beats many small ones. The invariant that makes the server's choice safe is that - // the two describe the SAME texels: every rect lies inside the box, and their union - // is the box. + // THE REGION LIST BEHIND THE BOX IS THE LEVEL'S EXACT DIRTY FOOTPRINT (P11 M2, dev + // 9524c688): disjoint boxes holding every texel written since the level was last + // clean and no other. The storage's rect list this used to carry only COVERS the + // writes - it merges touching rects into their bounding box, folds when full and is + // withheld (0) past 3/4 of the union box - and once the far side's image may hold GPU + // writes, whatever it uploads beyond the writes goes up over them. Both servers read + // the list as the texels to upload: Magma always, Espryt once its driver texture may + // hold GPU writes (and otherwise it re-applies the storage's box-or-rects choice + // itself, so the Mali job count is the server's call as before). + // + // 0 IS NOW THE WHOLE-LEVEL SPELLING ONLY: a footprint that is the whole level. Every + // other write carries its boxes, a single one included, so that the applier's + // accumulation (boxes union, lists concatenate, "0 regions" makes the entry box only) + // never widens two exact contributions into their union box. A footprint larger than + // one record may carry falls back to the storage's rect list - a COVER, stated once. // // READ AS BOXES, ONCE, because both shapes below want them in that form: the // whole-level record converts each one straight back through MGPipeBuildSubRegion, // and a piece clips each one to its own slab. - MG_State::GLState::MipmapDirtyRegion rects[MG_State::GLState::MipmapStorage::kMaxDirtyRects]; - const SizeT rectCount = mipmap->GetStorageDirtyRects( - uploadTarget, level, rects, MG_State::GLState::MipmapStorage::kMaxDirtyRects); - MGPBox rectBoxes[MG_State::GLState::MipmapStorage::kMaxDirtyRects]; - for (SizeT i = 0; i < rectCount; ++i) rectBoxes[i] = MGPipeBoxOfDirtyRegion(rects[i]); + mipmap->GetStorageDirtyFootprint(uploadTarget, level, m_footprint); + m_rectBoxes.clear(); + const Bool wholeLevelWrite = + m_footprint.size() == 1 && m_footprint.front().lo.x() <= 0 && m_footprint.front().lo.y() <= 0 && + m_footprint.front().lo.z() <= 0 && m_footprint.front().hi.x() >= levelSize.x() && + m_footprint.front().hi.y() >= levelSize.y() && + m_footprint.front().hi.z() >= std::max(levelSize.z(), 1); + if (wholeLevelWrite) { + // The box is the story. + } else if (m_footprint.size() <= static_cast(kMGPipeMaxPendingUploadRegions)) { + for (const auto& box : m_footprint) m_rectBoxes.push_back(MGPipeBoxOfDirtyRegion(box)); + } else { + MGLOG_W_ONCE("MGPipe: texture {slot=%u, gen=%u} level %u has %zu disjoint dirty boxes, more than one " + "record carries (%u); it goes out as the storage's rect list, which covers the writes " + "but not exactly", + pending.Handle.Slot, pending.Handle.Gen, static_cast(level), m_footprint.size(), + kMGPipeMaxPendingUploadRegions); + MG_State::GLState::MipmapDirtyRegion rects[MG_State::GLState::MipmapStorage::kMaxDirtyRects]; + const SizeT coverCount = mipmap->GetStorageDirtyRects( + uploadTarget, level, rects, MG_State::GLState::MipmapStorage::kMaxDirtyRects); + for (SizeT i = 0; i < coverCount; ++i) m_rectBoxes.push_back(MGPipeBoxOfDirtyRegion(rects[i])); + } + const SizeT rectCount = m_rectBoxes.size(); + const MGPBox* rectBoxes = m_rectBoxes.data(); Bool dispatched = false; Bool accepted = true; @@ -1623,6 +1651,10 @@ namespace MobileGL::MG_Pipe { Vector m_renderbuffers; Vector m_drain; Vector m_regions; + // EmitOneLevel's scratch, kept for its capacity: the level's exact footprint and the + // boxes the record's regions are built from. + Vector m_footprint; + Vector m_rectBoxes; Bool m_draining = false; MGPResourceDesc m_lastDesc{}; diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index f5ff8566..54c0d06c 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -901,9 +901,18 @@ namespace MobileGL::MG_Pipe { for (MGPipeResourceRecord::PendingUpload& entry : stored.PendingUploads) { if (entry.UploadTarget != record.Target || entry.Level != record.Level) continue; entry.UnionBox = UnionOfBoxes(entry.UnionBox, record.UnionBox); - if (record.RegionCount == 0 || entry.Regions.empty() || - static_cast(entry.Regions.size()) + record.RegionCount > - kMGPipeMaxPendingUploadRegions) { + if (record.RegionCount == 0 || entry.Regions.empty()) { + // A whole-level contribution (the emitter's only box-only spelling since P11 + // M2) owns every texel of the level, so the entry is that box, exactly. + entry.Regions.clear(); + return true; + } + if (static_cast(entry.Regions.size()) + record.RegionCount > kMGPipeMaxPendingUploadRegions) { + // The one lossy answer left: the box covers every texel the list named, and + // also the gaps between them (see kMGPipeMaxPendingUploadRegions). + MGLOG_W_ONCE("MGPipe: a texture level's pending upload outgrew %u regions and becomes its union " + "box, which spans the gaps between the writes", + kMGPipeMaxPendingUploadRegions); entry.Regions.clear(); return true; } diff --git a/MobileGL/MG_Pipe/PipeApply.h b/MobileGL/MG_Pipe/PipeApply.h index 68fbdc68..abade410 100644 --- a/MobileGL/MG_Pipe/PipeApply.h +++ b/MobileGL/MG_Pipe/PipeApply.h @@ -229,12 +229,18 @@ namespace MobileGL::MG_Pipe { // unbounded growth a corrupt Uint16 would otherwise buy. inline constexpr Uint32 kMGPipeMaxPendingUploads = 256; - // The rect list behind one pending entry. The frontend keeps at most MipmapStorage's - // kMaxDirtyRects = 96 per level and answers "0 rects" for everything it cannot describe - // that way, which is the model this mirrors: an accumulation that would exceed this - // collapses to BOX ONLY - the same answer, with the same meaning, and never a dropped - // region. 256 is that bound with room for several emissions accumulating behind a bail. - inline constexpr Uint32 kMGPipeMaxPendingUploadRegions = 256; + // The rect list behind one pending entry, and the most regions one record may carry. The + // list is the level's EXACT dirty footprint since P11 M2 (TextureEmit.h, dev 9524c688): one + // box per disjoint piece of the writes, however many, where it used to be MipmapStorage's + // bounded cover (kMaxDirtyRects = 96, "0 rects" past it). An accumulation that would exceed + // this still collapses to BOX ONLY - never a dropped region, since the box covers every + // texel the list named - but a box is no longer the same answer: it spans the gaps between + // the writes, and a server whose image may hold GPU writes there uploads the stale shadow + // over them. So the bound is set far above what a frame's writes to one level come to (a + // hundred atlas sprites is ~100), and the collapse is the rare case it was before rather + // than the ordinary one. 4096 regions of 40 bytes is a 160 KiB tail, a small fraction of + // the half of SEG_CMD one record may take. + inline constexpr Uint32 kMGPipeMaxPendingUploadRegions = 4096; // The default uniform block's image, the one allocation P4a adds per program. The size // comes from the program's own MGPProgramDesc::GlobalUboSize, so it is checked ONCE at diff --git a/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp b/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp index a1ade06b..fc020d87 100644 --- a/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/TextureEmitTest.cpp @@ -213,7 +213,8 @@ TEST(TextureEmit, TheEmitterIsOneNeverDestroyedProcessSingleton) { X(TextureEmit, TheUnionBoxAndTheRegionListDescribeTheSameTexels) \ X(TextureEmit, AScatteredUploadCarriesTheLevelShadowsStridesAndNotZero) \ X(TextureEmit, AWholeLevelUploadCarriesZeroStrides) \ - X(TextureEmit, MoreThanKMaxDirtyRectsCollapsesToTheBoxWithRegionCountZero) \ + X(TextureEmit, MoreWritesThanTheRectListKeepsCrossAsTheExactFootprint) \ + X(TextureEmit, ASinglePartialWriteCarriesItsBoxAsOneRegion) \ X(TextureEmit, ALevelTooLargeForTheStageChunkIsCutIntoSlabs) \ X(TextureEmit, AnUploadThroughAViewKeysOnTheStorageOwner) \ X(TextureEmit, EveryTexturesParamsNameItsBuiltinSamplerCso) \ @@ -575,27 +576,58 @@ TEST(TextureEmit, AWholeLevelUploadCarriesZeroStrides) { EXPECT_EQ(whole.SrcOffset, 0u); } -TEST(TextureEmit, MoreThanKMaxDirtyRectsCollapsesToTheBoxWithRegionCountZero) { +// P11 M2 (dev 9524c688): THE REGIONS ARE THE EXACT FOOTPRINT, NOT THE STORAGE'S RECT LIST. This +// case used to pin the list - "more writes than kMaxDirtyRects collapse to the box, RegionCount 0" +// - and that collapse is the shape that put the stale shadow over GPU-written texels on the far +// side. Far more overlapping writes than the list keeps: the storage's footprint is the oracle, +// read BEFORE the drain clears the level, and every box of it must cross, in order, with nothing +// invented and nothing truncated. +TEST(TextureEmit, MoreWritesThanTheRectListKeepsCrossAsTheExactFootprint) { TextureScope scope; const auto texture = MakeTexture2D(7, 128); - // Far more writes than the rect cap. The storage MERGES rather than truncates - a dropped - // rect is a dropped write - and degrades toward the union box; what the emitter must never - // do is invent or truncate. So the storage's own answer is the oracle, read BEFORE the drain - // clears the level, and 0 means "the union box is the whole story". for (Int i = 0; i < 4 * static_cast(MipmapStorage::kMaxDirtyRects); ++i) { const Int x = (i * 7) % 120; const Int y = (i * 11) % 120; texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 0, IntVec3{x, y, 0}, IntVec3{2, 2, 1}); } - MipmapDirtyRegion oracle[MipmapStorage::kMaxDirtyRects]; - const SizeT oracleCount = texture->GetStorageDirtyRects(TextureUploadTarget::Texture2D, 0, oracle, - MipmapStorage::kMaxDirtyRects); - EXPECT_LE(oracleCount, MipmapStorage::kMaxDirtyRects); + Vector oracle; + texture->GetStorageDirtyFootprint(TextureUploadTarget::Texture2D, 0, oracle); + ASSERT_GT(oracle.size(), MipmapStorage::kMaxDirtyRects) + << "the writes must outgrow the rect list, or this case pins nothing the list did not"; + ASSERT_LE(oracle.size(), static_cast(kMGPipeMaxPendingUploadRegions)); Textures().DrainTextureSubData(Ctx()); - EXPECT_EQ(Textures().LastSubData().RegionCount, static_cast(oracleCount)) - << "the emitted region count is not the storage's own answer"; - EXPECT_EQ(Textures().LastRegions().size(), oracleCount); + ASSERT_EQ(Textures().LastSubData().RegionCount, static_cast(oracle.size())) + << "the emitted region count is not the storage's footprint"; + const Vector regions = Textures().LastRegions(); + ASSERT_EQ(regions.size(), oracle.size()); + for (SizeT i = 0; i < oracle.size(); ++i) { + const MGPBox box = MGPipeBoxOfDirtyRegion(oracle[i]); + EXPECT_TRUE(regions[i].X == box.X && regions[i].Y == box.Y && regions[i].Z == box.Z && + regions[i].W == box.W && regions[i].H == box.H && regions[i].D == box.D) + << "region " << i << " is not footprint box " << i; + } +} + +// P11 M2: A SINGLE PARTIAL BOX CARRIES ITSELF AS ONE REGION. "0 regions" is the whole-level +// spelling only, so the applier's accumulation - which makes an entry box-only the moment one +// contribution carries no list - cannot widen two exact contributions into their union box. +TEST(TextureEmit, ASinglePartialWriteCarriesItsBoxAsOneRegion) { + TextureScope scope; + const auto texture = MakeTexture2D(90, 64); + texture->MarkStorageDirtyRegion(TextureUploadTarget::Texture2D, 0, IntVec3{3, 5, 0}, IntVec3{4, 2, 1}); + Textures().DrainTextureSubData(Ctx()); + const MGPSubData record = Textures().LastSubData(); + ASSERT_EQ(record.RegionCount, 1u); + const Vector regions = Textures().LastRegions(); + ASSERT_EQ(regions.size(), 1u); + EXPECT_EQ(regions[0].X, 3); + EXPECT_EQ(regions[0].Y, 5); + EXPECT_EQ(regions[0].W, 4u); + EXPECT_EQ(regions[0].H, 2u); + EXPECT_EQ(record.UnionBox.X, 3); + EXPECT_EQ(record.UnionBox.W, 4u); + EXPECT_EQ(regions[0].SrcRowStride, 64u * 4u) << "a partial box carries the level's pitch"; } // ==================== fix A2: the stage-chunk slab split ====================