From f858841c5fbe6fdde77ff41bf25f15b0db00b2bb Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 11 Sep 2026 15:48:28 -0400 Subject: [PATCH] [Fix] (MG_Remote, GLImpl): gate the transform-feedback reconciliation on the transport like every other new site, and give the unwritten writeback wait a named Fatal instead of an empty body --- .../MG_Impl/GLImpl/Drawing/GL_Drawing.cpp | 8 ++++- MobileGL/MG_Remote/Client/GpuWritePending.cpp | 32 +++++++++++++++---- MobileGL/MG_Remote/Client/GpuWritePending.h | 7 ++++ .../MG_Remote/Client/PersistentMapTracker.cpp | 19 +++++++++-- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp index c8ab458d..4b289545 100644 --- a/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp +++ b/MobileGL/MG_Impl/GLImpl/Drawing/GL_Drawing.cpp @@ -1324,7 +1324,13 @@ namespace MobileGL::MG_Impl::GLImpl { // fence introduces. Under split it pays the reconciliation itself, which is the // same cost the fence used to charge every caller - here charged only to the // capture shapes that actually need reordering. - buffer->SyncGpuWrites(); + // + // TRANSPORT-GATED LIKE EVERY OTHER NEW SITE (D-J). Without the test this fires in + // a build-split lane running MOBILEGL_TRANSPORT=monolith on Magma - whose + // BeginXfbCaptureForDraw does mark the capture targets - where the fence at the + // caller still runs, so the readback it emits is pure new work on the monolith + // path and integration-gpu cannot see it. + if (MG_Config::Transport != MG_Config::TransportMode::Monolith) buffer->SyncGpuWrites(); #endif const Range1D range = bindingPoint.GetRange(); const Uint8* mapped = buffer->MappedData(); diff --git a/MobileGL/MG_Remote/Client/GpuWritePending.cpp b/MobileGL/MG_Remote/Client/GpuWritePending.cpp index dd9b55d7..112c5713 100644 --- a/MobileGL/MG_Remote/Client/GpuWritePending.cpp +++ b/MobileGL/MG_Remote/Client/GpuWritePending.cpp @@ -15,6 +15,9 @@ #include #include #include +#include + +#include namespace MobileGL::MG_Remote::Client { @@ -167,16 +170,31 @@ namespace MobileGL::MG_Remote::Client { } void AwaitBufferWriteback(BufferObject& buffer) { - (void)buffer; // THE WAIT IS THE BARRIER'S WAIT (R-3). The reply-slot id IS the record's seq, so // "appliedSeq reached my readback" and "my answer is back" are one condition, and - // ClientSession::EmitAndWait has already paid for it by the time the emitter returns. - // With no session - a build-split lane running monolith, and every unit case - the - // emission WAS the application, synchronously, so the writeback has already landed - // and there is nothing to wait for. Spelling that as "return" rather than as a loop - // is deliberate: a loop here would be a hang in exactly that configuration, which is - // the configuration every gate lane runs. + // ClientSession::EmitAndWait is what pays for it. With no session - a build-split lane + // running monolith, and every unit case - the emission WAS the application, + // synchronously, so the writeback has already landed and there is nothing to wait for. + // Spelling that as "return" rather than as a loop is deliberate: a loop here would be + // a hang in exactly that configuration, which is the configuration every gate lane + // runs. if (ClientSession::Active() == nullptr) return; + + // AND THE OTHER ARM IS A NAMED FATAL, NOT AN EMPTY BODY. A session exists, so the + // apply side is no longer synchronous, and if the flag is still set the shadow this + // caller is about to read is STALE - which is the whole failure the third state was + // introduced to stop. An empty body here would make that failure silent and would let + // s1/c1 land a session without noticing that nobody ever wrote the wait; a stub that + // aborts by name is the house shape for exactly this (EmitTables.cpp's + // UnmigratedVerbFatal), and it is what gives the hole a red spelling before the + // transport arrives. + if (!buffer.HasOutstandingGpuWrite()) return; + MGLOG_F("MGPipe: Fatal{UnimplementedWritebackWait} - a ClientSession is active and buffer %u " + "still has an outstanding GPU write after its readback was emitted. The wait is " + "ClientSession::EmitAndWait's (R-3: the reply slot id IS the record seq); P5 package " + "b1 landed the third state and s1/c1 own the wait itself.", + buffer.GetExternalIndex()); + std::abort(); } } // namespace MobileGL::MG_Remote::Client diff --git a/MobileGL/MG_Remote/Client/GpuWritePending.h b/MobileGL/MG_Remote/Client/GpuWritePending.h index 73f4cbf5..873bcf18 100644 --- a/MobileGL/MG_Remote/Client/GpuWritePending.h +++ b/MobileGL/MG_Remote/Client/GpuWritePending.h @@ -41,6 +41,13 @@ // ~0ull) and marks the capture targets, for the same reason: the wait exists only so that // a later MapBuffer sees real results, which is precisely what the flag is for. // +// WHAT NO ROW COVERS, WRITTEN DOWN SO THE NEXT READER DOES NOT HAVE TO ASK. The set is built +// from the APPLICATION's bindings, so it says nothing about a backend's own scratch buffers - +// Espryt's converted-vertex-stream and primitive-restart substitution buffers, Magma's UBO +// ring. None of those has a MarkGpuWritten today either, so the client set is no NARROWER than +// monolith's and this is not a regression; it is a standing hole in both, and it stays one +// until the phase that migrates the backend's own allocations. +// // GATED ON THE TRANSPORT, like everything else in this package: on the monolith path the six // backend sites still run and a second marker would be new behaviour (D-J), and rows 4 and 5 // would remove a stall monolith is entitled to keep. diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp index 90c48f1b..d3cda550 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp @@ -123,9 +123,22 @@ namespace MobileGL::MG_Remote::Client { // parses them today is that the negative control needs a spelling before the thing // it controls exists. Falling back would make `MOBILEGL_IPC_ADOPT_TIER=0` look like // a working T0 run and silently produce pmap bytes it must not produce. - MGLOG_F("MGPipe: MOBILEGL_IPC_ADOPT_TIER=%u names an adoption tier P11 implements and P5 " - "does not; P5 runs at T2 (emulate) only.", - static_cast(tier)); + // 0 and 1 are the two CONTRACT ยง5 promises - a real cross-process shared mapping and a + // server-side staging map - and they name P11. Anything else is not a tier at all, and + // saying "P11 implements it" of a 7 would be a lie the operator then repeats. Both die + // here rather than at parse, which is late: the abort lands at the first + // map_persistent, so a mis-set run gets through EGL bring-up and a frame of setup + // first. Moving it to the parse means a knob-validity rule in ConfigLoader, which is + // c0's file; filed for the integrator rather than taken here. + if (tier <= 1) { + MGLOG_F("MGPipe: MOBILEGL_IPC_ADOPT_TIER=%u names adoption tier T%u, which P11 implements " + "and P5 does not; P5 runs at T2 (emulate) only.", + static_cast(tier), static_cast(tier)); + } else { + MGLOG_F("MGPipe: MOBILEGL_IPC_ADOPT_TIER=%u is not an adoption tier; the only values are 0 " + "and 1 (P11) and 2 (emulate, the P5 default).", + static_cast(tier)); + } std::abort(); }