diff --git a/MobileGL/MG_Impl/Pipe/ResourceTracker.h b/MobileGL/MG_Impl/Pipe/ResourceTracker.h index c1bf990f..daf3cfa1 100644 --- a/MobileGL/MG_Impl/Pipe/ResourceTracker.h +++ b/MobileGL/MG_Impl/Pipe/ResourceTracker.h @@ -579,7 +579,11 @@ namespace MobileGL::MG_Pipe { // transport this is rule B in the reverse direction and is // Fatal{ProtocolCorruption, "OnBufferWriteback.Seg"}; // anything else -> the same Fatal. - inline void MGPipeClientOnBufferWriteback(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes) { + // + // `producedAtSeq` is the event's ProducedAtSeq (the record the server read the bytes back + // at); 0 for the monolith callback, whose answer is synchronous with its request. + inline void MGPipeClientOnBufferWritebackAt(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes, + Uint64 producedAtSeq) { auto* buffer = MGPipeResourceTrackerInstance().Resolve(res); if (buffer == nullptr) { MGLOG_E_ONCE("MGPipe: OnBufferWriteback for a handle {%u,%u} that resolves to no buffer", @@ -641,7 +645,12 @@ namespace MobileGL::MG_Pipe { } #endif buffer->WritebackFromBackend(DataPtr{bytePtr, static_cast(bytes.Size)}, - static_cast(offset)); + static_cast(offset), producedAtSeq); + } + + // The monolith callback's shape (MGPipeCallbacks::OnBufferWriteback). + inline void MGPipeClientOnBufferWriteback(MGPipeHandle res, Uint64 offset, MGPBlobRef bytes) { + MGPipeClientOnBufferWritebackAt(res, offset, bytes, 0); } // A draw or dispatch wrote these ranges. ARCHITECTURE.md 7.1 calls this a NARROWING @@ -670,7 +679,7 @@ namespace MobileGL::MG_Pipe { res.Gen); return; } - buffer->MarkGpuWritten(); + buffer->MarkGpuWrittenInChannelOrder(); } // MONOLITH ONLY (CONTRACT-P5C §4.1): with an active transport the three reverse entries diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 59b47ccb..a441007d 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -508,9 +508,9 @@ namespace MobileGL::MG_Remote::Client { blob.Seg = Wire::kSegEvent; blob.Offset = events.OffsetInSegment(bytes); blob.Size = head->Size; - MG_Pipe::MGPipeClientOnBufferWriteback( + MG_Pipe::MGPipeClientOnBufferWritebackAt( MG_Pipe::MGPipeHandle{head->Resource.Slot, head->Resource.Gen}, - head->Offset, blob); + head->Offset, blob, head->ProducedAtSeq); ++delivered; break; } diff --git a/MobileGL/MG_Remote/Client/GpuWritePending.cpp b/MobileGL/MG_Remote/Client/GpuWritePending.cpp index 52b1fe8c..01d17290 100644 --- a/MobileGL/MG_Remote/Client/GpuWritePending.cpp +++ b/MobileGL/MG_Remote/Client/GpuWritePending.cpp @@ -31,7 +31,7 @@ namespace MobileGL::MG_Remote::Client { ClientSession* session = ClientSession::Active(); if (session == nullptr) return 0; // A quarter of the ring, not the MaxRecordBytes half: the record carries its own - // header and the 24-byte EventBufferWritebackHead beside the payload, and the ring + // header and the 32-byte EventBufferWritebackHead beside the payload, and the ring // may still hold a few small events (gpu-written, gl-error) posted earlier in the // same verb's apply. Each slice round-trips with its own barrier + drain, so the // ring never holds more than one slice's bytes. @@ -72,6 +72,11 @@ namespace MobileGL::MG_Remote::Client { return MGPipeStageChunkBytesFor(session->StageCapacityBytes()); } + Uint64 GpuWriteOrderSeq() { + ClientSession* session = ClientSession::Active(); + return session == nullptr ? 0 : session->LastPublishedSeq(); + } + void AwaitBufferWriteback(BufferObject& 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 diff --git a/MobileGL/MG_Remote/Client/GpuWritePending.h b/MobileGL/MG_Remote/Client/GpuWritePending.h index da14ca18..9d480edc 100644 --- a/MobileGL/MG_Remote/Client/GpuWritePending.h +++ b/MobileGL/MG_Remote/Client/GpuWritePending.h @@ -33,6 +33,12 @@ namespace MobileGL::MG_Remote::Client { // caller writes, because the loop would be a hang in exactly that configuration. void AwaitBufferWriteback(MG_State::GLState::BufferObject& buffer); + // The order a GPU-write mark is stamped with: the last command seq this session + // published, so the work the mark stands for is in a LATER record and only a writeback + // the server produced after that record can retire it (BufferObject::WritebackFromBackend). + // 0 with no session, where every writeback is synchronous and needs no ordering. + Uint64 GpuWriteOrderSeq(); + // The slice a whole-buffer readback is cut into so one writeback event always fits // SEG_EVENT. The writeback's bytes travel INLINE in the event record (P5c ev, CONTRACT-P5C diff --git a/MobileGL/MG_Remote/Server/ServerSession.cpp b/MobileGL/MG_Remote/Server/ServerSession.cpp index a110711e..5e57972d 100644 --- a/MobileGL/MG_Remote/Server/ServerSession.cpp +++ b/MobileGL/MG_Remote/Server/ServerSession.cpp @@ -457,6 +457,7 @@ namespace MobileGL::MG_Remote::Server { head.Resource = Transport::EventHandle{res.Slot, res.Gen}; head.Offset = offset; head.Size = bytes.Size; + head.ProducedAtSeq = session.Consumer().ProducingSeq(); std::memcpy(slot, &head, sizeof(head)); if (bytes.Size != 0) { std::memcpy(static_cast(slot) + sizeof(head), diff --git a/MobileGL/MG_Remote/Transport/EventRing.h b/MobileGL/MG_Remote/Transport/EventRing.h index ef8dfb43..9d771cdd 100644 --- a/MobileGL/MG_Remote/Transport/EventRing.h +++ b/MobileGL/MG_Remote/Transport/EventRing.h @@ -73,12 +73,20 @@ namespace MobileGL::MG_Remote::Transport { // makes "the destination is the client's shadow" (contract table 1 row 22) // reachable without a second segment. `Size` is therefore the record's own // tail length and is cross-checked against it. + // + // `ProducedAtSeq` is the SEG_CMD seq of the record whose apply posted the + // writeback (the record count the server had applied, when it was posted + // between records). Writebacks a client did not ask for - a transform + // feedback capture closing - drain whenever the client next waits, which + // can be after it has marked the same buffer written by a later record; the + // seq is what lets it tell an old writeback from the answer to its own. struct EventBufferWritebackHead { EventHandle Resource; - std::uint64_t Offset; // destination offset inside the resource - std::uint64_t Size; // inline byte count that follows + std::uint64_t Offset; // destination offset inside the resource + std::uint64_t Size; // inline byte count that follows + std::uint64_t ProducedAtSeq; // SEG_CMD seq the bytes are current as of }; - static_assert(sizeof(EventBufferWritebackHead) == 24, "wire shape"); + static_assert(sizeof(EventBufferWritebackHead) == 32, "wire shape"); // OnGpuWritten(res, rangeCount, ranges). EventRange[RangeCount] follows. struct EventGpuWrittenHead { diff --git a/MobileGL/MG_Remote/Transport/SessionRings.h b/MobileGL/MG_Remote/Transport/SessionRings.h index 142aefca..f57e91b8 100644 --- a/MobileGL/MG_Remote/Transport/SessionRings.h +++ b/MobileGL/MG_Remote/Transport/SessionRings.h @@ -466,10 +466,20 @@ namespace MobileGL::MG_Remote::Transport { // A terminal rejection after Pop must not acknowledge an unapplied record. // A bool callback returning false ends this consumer's session; it cannot retry // the popped record. Existing void callbacks retain their original semantics. - if constexpr (std::is_same_v, bool>) { - if (!apply(view)) return false; - } else { - apply(view); + // ProducingSeq() names THIS record while its apply runs, so a reverse + // event the apply posts carries the seq of the record that produced it. + struct ApplyingScope { + bool& flag; + explicit ApplyingScope(bool& f) : flag(f) { flag = true; } + ~ApplyingScope() { flag = false; } + }; + { + const ApplyingScope applying(m_applying); + if constexpr (std::is_same_v, bool>) { + if (!apply(view)) return false; + } else { + apply(view); + } } ++m_appliedSeq; Watermark::AdvanceApplied(*m_control, m_appliedSeq); @@ -535,6 +545,11 @@ namespace MobileGL::MG_Remote::Transport { void NotifyClient(); std::uint64_t AppliedSeq() const { return m_appliedSeq; } + // The seq of the record whose apply is running, or - between records - + // the last one applied. Every byte the server reads back at this point is + // current as of exactly this record, which is what a writeback event + // stamps so the client can order it against its own later records. + std::uint64_t ProducingSeq() const { return m_applying ? m_appliedSeq + 1 : m_appliedSeq; } // How many kRecBorrowSlot records this consumer has seen. Non-zero in P5 // is a finding, not a statistic. std::uint64_t BorrowedRecordsSeen() const { return m_borrowedSeen; } @@ -556,6 +571,7 @@ namespace MobileGL::MG_Remote::Transport { std::uint64_t m_retirableCursor = 0; std::uint64_t m_borrowedSeen = 0; bool m_borrowHeld = false; + bool m_applying = false; // Out of line so ApplyOne, which is a template in a header this layer // keeps free of MobileGL/Includes.h, can still log. diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp index aa39b61f..ef6d0367 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp @@ -480,7 +480,7 @@ namespace MobileGL::MG_State::GLState { return m_publishedLiveHostWrites; } - void BufferObject::WritebackFromBackend(DataPtr data, SizeT atOffset) { + void BufferObject::WritebackFromBackend(DataPtr data, SizeT atOffset, Uint64 producedAtSeq) { MOBILEGL_ASSERT(atOffset + data.size <= m_size, "WritebackFromBackend out of bounds: atOffset (%zu) + data.size (%zu) > m_size (%zu)", atOffset, data.size, m_size); @@ -498,16 +498,42 @@ namespace MobileGL::MG_State::GLState { // P13 W4a: monolith's record arm refreshes an aliased store through this function too // (Managers.cpp RefreshAliasedStoreFromGpu), and a whole-store landing there is exactly as // current as one under split. - if (atOffset == 0 && data.size >= m_size) { + // + // ORDERED, NOT MERELY WHOLE. A writeback the client never asked for - the end of a + // transform feedback capture - is drained whenever the client next waits, which under + // run-ahead can be after a later dispatch has marked this same buffer written again. + // Those bytes are the capture's, not the dispatch's, so they may not retire the newer + // mark: only a writeback produced by a record after the one the mark was taken for can + // (the readback SyncGpuWrites emits always is). 0 is the synchronous answer - monolith + // and every no-session case - which has nothing to be ordered against. + if (atOffset == 0 && data.size >= m_size && + (producedAtSeq == 0 || producedAtSeq > m_gpuWriteMarkSeq)) { m_gpuWritePending = false; } } void BufferObject::MarkGpuWritten() { +#if MOBILEGL_BUILD_DISAGGREGATED + MarkGpuWrittenAfter(MG_Remote::Client::GpuWriteOrderSeq()); +#else + MarkGpuWrittenAfter(0); +#endif + } + + void BufferObject::MarkGpuWrittenInChannelOrder() { m_hasDefinedContent = true; m_gpuWritePending = true; } + void BufferObject::MarkGpuWrittenAfter(Uint64 orderSeq) { + m_hasDefinedContent = true; + m_gpuWritePending = true; + // Assigned, not maxed: within a session the seq only grows, and a new session (or + // none, 0) restarts the numbering - a stamp kept from the old one would outrank every + // answer the new one can give. + m_gpuWriteMarkSeq = orderSeq; + } + void BufferObject::SyncGpuWrites() { if (!m_gpuWritePending) return; #if MOBILEGL_BUILD_DISAGGREGATED @@ -642,10 +668,9 @@ namespace MobileGL::MG_State::GLState { if (bytes.size > 0 && MG_Pipe::MGPipeResourceSubsystemEnabled() && MG_Pipe::MGPipeResourceOpsHaveSubDataResident()) { MG_Pipe::MGPipeEmitBufferSubDataResident(*this, offset, bytes.data, bytes.size); - m_hasDefinedContent = true; ++m_changeSerial; MGP_NOTE_AGGREGATE(BufferChange); - m_gpuWritePending = true; + MarkGpuWritten(); return; } // No resident op: the write lands in place below, after retiring the GPU writes this diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.h b/MobileGL/MG_State/GLState/BufferState/BufferObject.h index 4ef3a66a..57214eef 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.h +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.h @@ -113,7 +113,14 @@ namespace MobileGL { // Shadow-only write used when the backend copies GPU results (e.g. ReadPixels // into a pixel-pack buffer) back into the frontend mirror. Does not issue a // backend op: the backend storage already holds these bytes. - void WritebackFromBackend(DataPtr data, SizeT atOffset); + // + // `producedAtSeq` is the command seq the bytes are current as of (the event's + // ProducedAtSeq); 0 means the writeback is synchronous with its request and needs + // no ordering. A whole-buffer writeback clears the outstanding GPU write only when + // it is newer than the record the latest mark was taken for - an older one (a + // capture's writeback drained after a later dispatch re-marked the buffer) still + // lands its bytes but cannot vouch for the write it predates. + void WritebackFromBackend(DataPtr data, SizeT atOffset, Uint64 producedAtSeq = 0); // A draw or dispatch just ran with this buffer bound where a shader can write // it (shader storage / atomic counter). The next read has to reconcile with @@ -121,6 +128,16 @@ namespace MobileGL { // memory - wait for the work that wrote them to retire. Which of the two is // the backend's business; the flag only says a GPU write is outstanding. void MarkGpuWritten(); + // The same mark, stamped with an explicit command seq: the mark stands for GPU + // work in records AFTER `orderSeq`. MarkGpuWritten stamps the session's last + // published seq (0 without a session). + void MarkGpuWrittenAfter(Uint64 orderSeq); + // The mark a server announces on the reverse channel (OnGpuWritten). That channel + // is in order with the writebacks, so every writeback drained after this mark was + // produced after the write it announces: the standing stamp is kept, not raised to + // the drain point - raising it would outrank the very readback the next read is + // already waiting on. + void MarkGpuWrittenInChannelOrder(); // Refreshes the shadow from the backend when a GPU write is outstanding. Called // from every path that reads the shadow on the app's behalf. void SyncGpuWrites(); @@ -258,6 +275,10 @@ namespace MobileGL { // and, in a split build, cleared by WritebackFromBackend instead, because there // the answer arrives later than the request. See SyncGpuWrites' definition. Bool m_gpuWritePending = false; + // The command seq the latest GPU-write mark was taken after. A monotonic record + // seq, never a count of writebacks: only a writeback produced by a LATER record + // can retire the mark (WritebackFromBackend). + Uint64 m_gpuWriteMarkSeq = 0; // The last value of MGPResourceDesc::HasLiveHostWrites this object published. // Behind the option so the pull build's layout - and therefore every inlined // constructor and accessor in it - does not move (G1). diff --git a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp index dc691f29..0c6d63ea 100644 --- a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp +++ b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp @@ -540,6 +540,39 @@ TEST_F(SplitBufferSet, UnderSplitTheWritebackClearsThePendingFlagAndNotTheReques EXPECT_FALSE(buffer->HasOutstandingGpuWrite()); } +// A writeback the client never asked for (a capture's end) drains whenever the client next +// waits, which can be after a later record marked the same buffer written again. Only a +// writeback produced by a record AFTER the mark's may retire it; an older one lands its bytes +// and leaves the mark standing, so the next read still reconciles with the newer write. +TEST_F(SplitBufferSet, AnOlderWritebackCannotRetireANewerGpuWriteMark) { + auto buffer = MakeBuffer(27u, 96); + Vector capture(96, static_cast(0x11)); + + // The capture closed at record 4; a dispatch emitted after record 6 re-marked the buffer. + buffer->MarkGpuWrittenAfter(6); + buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/4); + EXPECT_TRUE(buffer->HasOutstandingGpuWrite()) + << "the capture's writeback predates the dispatch and cannot vouch for its write"; + buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/6); + EXPECT_TRUE(buffer->HasOutstandingGpuWrite()) << "record 6 is the mark's own seq, not after it"; + + // The readback the next read emits is a later record, and it does retire the mark. + buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/7); + EXPECT_FALSE(buffer->HasOutstandingGpuWrite()); + + // An unordered (synchronous) writeback keeps the old rule. + buffer->MarkGpuWrittenAfter(9); + buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0); + EXPECT_FALSE(buffer->HasOutstandingGpuWrite()); + + // A server-announced mark is drained in channel order, so it keeps the standing stamp: the + // readback the client emitted after its own mark at 10 still answers it. + buffer->MarkGpuWrittenAfter(10); + buffer->MarkGpuWrittenInChannelOrder(); + buffer->WritebackFromBackend(DataPtr{capture.data(), capture.size()}, 0, /*producedAtSeq=*/11); + EXPECT_FALSE(buffer->HasOutstandingGpuWrite()); +} + // The tier gate in P11 B2's shape (CONTRACT-P11 §1 and B2). 0 / 1 are SETTLED AT THE HANDSHAKE by // the data plane rather than discovered at the first map_persistent: a stream refuses them by name // and runs T2 on either side; over shared segments a client with 0 ASKS T0 (0), with 1 is refused @@ -826,6 +859,7 @@ TEST(SplitBufferSet, BothEdgesOfAWriteMapPublishOneStateRecord) { MGL_SPLIT_ONLY TEST(SplitBufferSet, AZeroBlockSizeTurnsThePushOffRatherThanMakingItUnlimited) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, ADestroyedBufferLeavesTheSet) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, UnderSplitTheWritebackClearsThePendingFlagAndNotTheRequest) { MGL_SPLIT_ONLY_OR_SKIP(); } +TEST(SplitBufferSet, AnOlderWritebackCannotRetireANewerGpuWriteMark) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, TheAdoptTierIsSettledAtTheHandshakeAndNeverFatal) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, ASubPageMapIsOneProtectedPageAndAWriteThroughItFaults) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, AShadowThatFailsThePageGranularTestKeepsTheInwardAlignmentAndItsEdges) { diff --git a/MobileGL/MG_Test/Wire/SessionTest.cpp b/MobileGL/MG_Test/Wire/SessionTest.cpp index 594704ac..209aa996 100644 --- a/MobileGL/MG_Test/Wire/SessionTest.cpp +++ b/MobileGL/MG_Test/Wire/SessionTest.cpp @@ -461,11 +461,17 @@ TEST(SessionTest, AppliedSeqAdvancesExactlyOncePerRecordAndIsNeverBatched) { session.producer.PublishAndNotify(kRecords); std::uint64_t applied = 0; - while (session.consumer.ApplyOne([&](const RingRecordView&) { ++applied; })) { + EXPECT_EQ(session.consumer.ProducingSeq(), 0u); + while (session.consumer.ApplyOne([&](const RingRecordView&) { + // A reverse event posted by this apply is stamped with THIS record's seq. + EXPECT_EQ(session.consumer.ProducingSeq(), applied + 1); + ++applied; + })) { EXPECT_EQ(session.Control().Progress.appliedSeq.load(), applied) << "appliedSeq did not move with the record; a barrier waiter would be blocked on " "work that already ran"; EXPECT_EQ(session.consumer.AppliedSeq(), applied); + EXPECT_EQ(session.consumer.ProducingSeq(), applied) << "between records: the last one applied"; } EXPECT_EQ(applied, kRecords); }