diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 325875ab..726a05ae 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -1490,9 +1490,13 @@ namespace MobileGL::MG_Backend::DirectGLES { hostBaseFrom, hostBaseTo, start, end); } #endif + // Read per range, like the pull arm: a ring copy staged for an earlier range of + // this same drain puts every later partial range of the store behind it + // (InvalidateFlushAccessFor's queued-copy rule, dev f973008c). + const Bool ringCopyInFlight = resource.ringCopyRetireSerial > DirectGLES::CompletedFrameSerial(); const GLbitfield access = mapUsable ? InvalidateFlushAccessFor(start, end, coveredFrom, coveredTo, limit, - resource.storageSize) + resource.storageSize, ringCopyInFlight) : 0u; if (access != 0) { BindBufferId(TempBufferTarget, resource.id); @@ -1512,6 +1516,8 @@ namespace MobileGL::MG_Backend::DirectGLES { BindBufferId(GL_COPY_WRITE_BUFFER, resource.id); g_GLESFuncs.glCopyBufferSubData(GL_COPY_READ_BUFFER, GL_COPY_WRITE_BUFFER, (GLintptr)ringOffset, (GLintptr)start, (GLsizeiptr)size); + // Commands recorded now retire with the fence the next Present inserts. + resource.ringCopyRetireSerial = DirectGLES::CurrentFrameSerial() + 1; } else { UploadRangeFrom(resource, hostBase, start, end); } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 90d55482..463829f5 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -1005,10 +1005,12 @@ namespace MobileGL::MG_Backend::DirectGLES { // matches, exactly like the per-VAO memo stamps. Uint64 drawCleanEpoch = 0; // Frame serial whose completion retires the last glCopyBufferSubData the upload - // ring staged INTO this store (FlushPendingRangesNow's ring tier); 0 = none. Until - // it retires, FlushPendingRangesNow keeps every partial range off the - // range-invalidating map: on Adreno 830 that map, on a store with a queued copy, - // loses the copy (see FlushPendingRangesNow). Draw thread only, like the flush. + // ring staged INTO this store (the flush ladder's ring tier: FlushPendingRangesNow + // on the pull arm, FlushPendingRangesFrom on the push arm); 0 = none. Until it + // retires, both keep every partial range off the range-invalidating map: on + // Adreno 830 that map, on a store with a queued copy, loses the copy (see + // FlushPendingRangesNow and InvalidateFlushAccessFor). Draw thread only, like the + // flush. Uint64 ringCopyRetireSerial = 0; // Zero-copy coherent persistent map (EXT_buffer_storage): the GL store is // immutable, persistently+coherently mapped, and persistentPtr is what the app @@ -1192,10 +1194,14 @@ namespace MobileGL::MG_Backend::DirectGLES { // ("do not take tier 1"), which drops the range onto the staging ring and costs a copy // rather than a corruption. // + // `ringCopyInFlight`: the store still has an upload-ring glCopyBufferSubData queued that the + // GPU may not have run (GLESBufferResource::ringCopyRetireSerial not yet retired). + // // Returns the glMapBufferRange access bits, or 0 when tier 1 must not be taken. inline constexpr SizeT kEsprytInvalidateRangeMinBytes = 128u * 1024u; constexpr GLbitfield InvalidateFlushAccessFor(SizeT queuedStart, SizeT queuedEnd, SizeT mapStart, - SizeT mapEnd, SizeT limit, SizeT storageSize) { + SizeT mapEnd, SizeT limit, SizeT storageSize, + Bool ringCopyInFlight) { if (mapEnd <= mapStart) return 0u; // THE WIDENING REFUSAL. Not >=, not "covers": exactly, in both directions. A map // narrower than the queued range leaves bytes unwritten inside a range it has just @@ -1207,6 +1213,13 @@ namespace MobileGL::MG_Backend::DirectGLES { // page-substitution fast path needs a page-coverable range to engage, and below it // the driver falls back to waiting out the WAR hazard on the CPU. if (!wholeBuffer && size < kEsprytInvalidateRangeMinBytes) return 0u; + // A partial range-invalidating map must not overtake a ring copy into this store the GPU + // has not run yet. Adreno 830 answers such a map on a store with a copy still queued by + // losing the copy (dev f973008c, the pull arm's FlushPendingRangesNow: create-instancing + // drew whole Embeddium terrain sections empty, SSIM 0.870). The range goes to the ring + // instead, GPU-ordered behind the copies. The whole-buffer orphan-map is exempt: it + // rewrites every byte from the shadow the queued copies were taken from. + if (!wholeBuffer && ringCopyInFlight) return 0u; return GL_MAP_WRITE_BIT | (wholeBuffer ? GL_MAP_INVALIDATE_BUFFER_BIT : GL_MAP_INVALIDATE_RANGE_BIT); } diff --git a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp index a8fb9009..6a1eb1b0 100644 --- a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp +++ b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp @@ -819,19 +819,19 @@ namespace { } // namespace TEST(EsprytFlushLadder, AWholeBufferRangeOrphansTheStore) { - EXPECT_EQ(InvalidateFlushAccessFor(0, kStore, 0, kStore, kStore, kStore), + EXPECT_EQ(InvalidateFlushAccessFor(0, kStore, 0, kStore, kStore, kStore, false), static_cast(GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_BUFFER_BIT)); } TEST(EsprytFlushLadder, ALargePartialRangeInvalidatesExactlyThatRange) { const SizeT start = 4096; const SizeT end = start + kEsprytInvalidateRangeMinBytes; - EXPECT_EQ(InvalidateFlushAccessFor(start, end, start, end, kStore, kStore), + EXPECT_EQ(InvalidateFlushAccessFor(start, end, start, end, kStore, kStore, false), static_cast(GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT)); } TEST(EsprytFlushLadder, ASmallPartialRangeFallsThroughToTheStagingRing) { - EXPECT_EQ(InvalidateFlushAccessFor(4096, 4096 + 64, 4096, 4096 + 64, kStore, kStore), 0u) + EXPECT_EQ(InvalidateFlushAccessFor(4096, 4096 + 64, 4096, 4096 + 64, kStore, kStore, false), 0u) << "below the threshold the map WAITS out the WAR hazard on the CPU instead of " "substituting pages, which is the whole reason tier 2 exists"; } @@ -846,19 +846,33 @@ TEST(EsprytFlushLadder, AMapWiderThanTheQueuedRangeRefusesTierOne) { const SizeT queuedStart = 4096; const SizeT queuedEnd = queuedStart + kEsprytInvalidateRangeMinBytes; // Page-aligned outward, the exact widening the in-tree note records. - EXPECT_EQ(InvalidateFlushAccessFor(queuedStart, queuedEnd, 0, queuedEnd + 4096, kStore, kStore), 0u) + EXPECT_EQ(InvalidateFlushAccessFor(queuedStart, queuedEnd, 0, queuedEnd + 4096, kStore, kStore, false), 0u) << "a widened INVALIDATE_RANGE declares bytes dead that the shadow is not about to " "rewrite, and overwrites whatever the GPU put there"; // And narrower, which is the same corruption read the other way round: bytes left // unwritten inside a range that has just been declared dead. - EXPECT_EQ(InvalidateFlushAccessFor(queuedStart, queuedEnd, queuedStart, queuedEnd - 8, kStore, kStore), 0u); + EXPECT_EQ(InvalidateFlushAccessFor(queuedStart, queuedEnd, queuedStart, queuedEnd - 8, kStore, kStore, false), + 0u); } TEST(EsprytFlushLadder, AnEmptyRangeIsNeverTierOne) { - EXPECT_EQ(InvalidateFlushAccessFor(4096, 4096, 4096, 4096, kStore, kStore), 0u); + EXPECT_EQ(InvalidateFlushAccessFor(4096, 4096, 4096, 4096, kStore, kStore, false), 0u); +} + +// dev f973008c's rule on the push arm (P8-MF): Adreno 830 loses an upload-ring copy still queued +// on a store when a partial range-invalidating map of that store overtakes it, so a partial range +// waits behind the copy on the ring. The whole-buffer orphan-map is exempt. +TEST(EsprytFlushLadder, APartialRangeDoesNotOvertakeAQueuedRingCopy) { + const SizeT start = 4096; + const SizeT end = start + kEsprytInvalidateRangeMinBytes; + EXPECT_EQ(InvalidateFlushAccessFor(start, end, start, end, kStore, kStore, true), 0u) + << "a range-invalidating map of a store with a queued ring copy loses the copy on Adreno 830"; + EXPECT_EQ(InvalidateFlushAccessFor(0, kStore, 0, kStore, kStore, kStore, true), + static_cast(GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_BUFFER_BIT)) + << "the whole-buffer orphan-map rewrites every byte from the shadow the copies were taken from"; } #else -// The same five names in a pull build, for the G2/G14 reason above: the ladder's push arm +// The same six names in a pull build, for the G2/G14 reason above: the ladder's push arm // (FlushPendingRangesFrom) is the only one that carries this decision as a function - the pull // arm's FlushPendingRangesNow is byte-frozen against 5cb826b0 (ID-15) and may not grow one. #define MGL_PUSH_ONLY_OR_SKIP() \ @@ -869,4 +883,5 @@ TEST(EsprytFlushLadder, ALargePartialRangeInvalidatesExactlyThatRange) { MGL_PUS TEST(EsprytFlushLadder, ASmallPartialRangeFallsThroughToTheStagingRing) { MGL_PUSH_ONLY_OR_SKIP(); } TEST(EsprytFlushLadder, AMapWiderThanTheQueuedRangeRefusesTierOne) { MGL_PUSH_ONLY_OR_SKIP(); } TEST(EsprytFlushLadder, AnEmptyRangeIsNeverTierOne) { MGL_PUSH_ONLY_OR_SKIP(); } +TEST(EsprytFlushLadder, APartialRangeDoesNotOvertakeAQueuedRingCopy) { MGL_PUSH_ONLY_OR_SKIP(); } #endif // MOBILEGL_PIPE_PUSH