diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp index 4d437083..4814410e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp @@ -270,7 +270,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { } Bool VkBufferManager::WaitForWireBufferHostAccess(WireBufferResource& resource) { - if (resource.lastUseSerial <= GetCompletedSerial() && !resource.gpuWritesPending) return true; + // B3 review: the early return must consult the same submission term WriteWireBuffer + // does, or a caller that reaches here with the serial term idle skips the wait the + // term would have demanded. + if (resource.lastUseSerial <= GetCompletedSerial() && !resource.gpuWritesPending && + (pVulkanRenderer == nullptr || pVulkanRenderer->IsSubmitIndexComplete(resource.lastUseSubmitIndex))) { + return true; + } if (!m_copyProvider || !pVulkanRenderer) return false; const VkCommandBuffer commands = m_copyProvider->AcquireBufferCopyCommandBuffer(); if (commands == VK_NULL_HANDLE) return false; @@ -303,27 +309,34 @@ namespace MobileGL::MG_Backend::DirectVulkan { const Uint64 probeLastUse = resource->lastUseSerial; const Uint64 probeCompleted = GetCompletedSerial(); - // ---- P7 wave 2 package B3: WHY THIS PREDICATE HAS TWO TERMS -------------------------- + // ---- P7 wave 2 package B3: WHAT EACH TERM OF THIS PREDICATE IS FOR ----------------- // // A streamed `glBufferSubData -> draw` pair is only correct if the write is ordered - // against every draw that still reads the bytes it overwrites. The question "is this - // buffer still busy?" used to be answered by the FRAME SERIAL alone, and that answer is - // a guess: GetCompletedSerial() is max(m_frameSerial - frameCount, m_completedSerialFloor) - // and its first term is PURE COUNTING - it asserts that work from frameCount serials ago - // has finished with no fence behind it. On the wire arm the serial is advanced not only - // by Present but by TryDrainFrameTransients -> VkBufferManager::BeginFrame on every 8th - // drain, which happens MID-FRAME (VulkanRenderer.cpp's drain block; ReadWireBuffer below - // already carries a comment acknowledging "even if the idle drain advanced the frame"), - // and package B established that mid-frame pooled-fence submissions are not covered by - // frame.imageInFlightFence. So the counting term can say "idle" about a buffer a - // recorded-or-in-flight draw is still reading, and the write is then applied as an - // unsynchronised host memcpy. The draw executes and reads bytes that are not its own - - // it renders another batch's geometry, or nothing at all. That is the OpenRA device - // divergence: one draw's quads missing, the terrain under them intact. + // against every draw that still reads the bytes it overwrites; a write this predicate + // answers "idle" is applied as an unsynchronised host memcpy, and a draw still reading + // the range then renders bytes that are not its own - silently, with no log line, no GL + // error and no Fatal. That was the OpenRA device divergence (magma-b3.md §2). // - // The second term is the fence-backed one and it is the fix. IsSubmitIndexComplete - // polls the real fences and answers false for an index that is not even submitted yet, - // which is exactly the state a draw being recorded right now is in. + // THE GUARANTEE IS THE SERIAL TERM, and it is sound because of the FLOOR, not because + // of anything here. GetCompletedSerial() is max(m_frameSerial - frameCount, + // m_completedSerialFloor). The floor used to be raised to a retired submission's frame + // serial even while another submission carrying the SAME serial was still executing - + // on the wire arm a serial has two (a mid-frame FlushPendingCommands under a pooled + // fence, then Present) - so "frame N-1 complete" was asserted on a fence nobody waited. + // VulkanRenderer::OnSubmitsCompletedUpTo now raises it only to a serial no in-flight + // submission still carries, and run_trace_case.cmake reds a split retrace on any + // "MGWIRE-FLOOR unsound-serial-complete" line, which is that fix's lane. + // + // THE SUBMISSION TERM IS A PROBE AND A DEFENCE, NOT A SECOND GUARANTEE. It is not sound + // on its own: lastUseSubmitIndex is stamped at AcquireWireSlice with the next submission + // index, but the draw that acquired the slice can still be pushed into a LATER + // submission - BindProgramUniformBuffers -> SyncWireTextureShape's preserve path -> + // FlushWirePendingCommandsForTextureUpdate submits the stamped index without the draw - + // so the stamp can name S1 while the draw rides S2. What it does do: with the floor + // sound it should never be the term that says "busy" (the WBUF probe reports bySubmit + // separately, so a device that shows one has found a floor hole), and it is the only + // thing the MGITEST_MAGMA_FORCE_STALE_BUFFER_SERIAL knob leaves standing, which is what + // the StaleSerial. entries pin. Bool busyBySerial = resource->lastUseSerial > GetCompletedSerial(); if (busyBySerial && ForceStaleSerialSelector() != 0) { const Uint64 index = ++g_orderedWireWriteCounter; @@ -403,8 +416,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { auto* resource = FindWireBuffer(res); if (!resource || !resource->buffer.IsValid() || resource->size == 0) return false; resource->lastUseSerial = m_frameSerial; - // B3: the draw this slice is being acquired for is recorded into the NEXT submission. - // Until that submission's fence signals, a host write to these bytes is a data race. + // B3: the draw this slice is being acquired for is normally recorded into the NEXT + // submission - normally, not always: a mid-draw flush (SyncWireTextureShape's preserve + // path) can submit this index without the draw. This stamp is the defence term's input + // (see WriteWireBuffer); the floor behind lastUseSerial is the guarantee. if (pVulkanRenderer) resource->lastUseSubmitIndex = pVulkanRenderer->GetWireNextSubmitIndex(); outSlice = resource->buffer.GetSlice(0, resource->size); outSlice.mapped = nullptr; @@ -429,6 +444,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { // work, not the draw that will bind that previously acquired slice. // Preserve its reservation even if the idle drain advanced the frame. resource->lastUseSerial = m_frameSerial; + // B3 review: and the submission half of the same reservation - the wait cleared it, + // and the draw holding the slice has not been submitted yet. + if (pVulkanRenderer) resource->lastUseSubmitIndex = pVulkanRenderer->GetWireNextSubmitIndex(); } if (!resource->buffer.Invalidate(size, offset)) return false; Memcpy(dst, static_cast(resource->buffer.GetMappedData()) + offset, static_cast(size)); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h index ff8e77ba..77f2211d 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h @@ -228,11 +228,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { VkBufferObject buffer; Uint64 size = 0; Uint64 lastUseSerial = 0; - // P7 wave 2 package B3: the submission that carries the most recent GPU use of - // this buffer. lastUseSerial above is a FRAME COUNTER and cannot answer "has the - // GPU finished reading these bytes" (see WriteWireBuffer); this can, because - // IsSubmitIndexComplete polls the submission's own fence and reports an - // unsubmitted index as incomplete. + // P7 wave 2 package B3: the submission expected to carry the most recent GPU use + // of this buffer - the busy predicate's DEFENCE term, not its guarantee (see + // WriteWireBuffer). IsSubmitIndexComplete polls the real fence and reports an + // unsubmitted index as incomplete, but the stamp is taken before the draw is + // recorded and a mid-draw flush can submit it without the draw. Uint64 lastUseSubmitIndex = 0; Bool gpuWritesPending = false; // Only ranges actually submitted by resource_subdata are covered. No diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index b2003509..41339e71 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -13772,7 +13772,9 @@ void main() { ++m_drainsSinceLastPresent; #if MOBILEGL_BUILD_DISAGGREGATED // B3 probe: a drain that reaches here found the GPU caught up and nothing recording. - // Every 8th one is treated as a FRAME BOUNDARY - mid-frame. See the note. + // Every 8th one is treated as a FRAME BOUNDARY - mid-frame. This probe is what REFUTED + // that as the OpenRA mechanism (one successful drain, zero boundary works across the + // replay; magma-b3.md §2.4): the cause was the floor in OnSubmitsCompletedUpTo. { static const Bool probe = [] { const char* value = std::getenv("MOBILEGL_MAGMA_WIREBUF_PROBE"); diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 54c37301..a5a57251 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -3108,12 +3108,13 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # ---- P7 wave 2 package B3, EXIT GATE 3: the streamed `glBufferSubData -> draw` pair. # # TWO ENTRIES OVER ONE CASE, for the reason the ShaderMip pair gives. The plain entry is - # a regression gate that runs on every driver. The forced entry is the RED-ONCE: it makes - # the frame-counting half of VkBufferManager's busy predicate answer "idle", which is the - # state a mid-frame idle drain puts the wire arm into on the device and which no host - # lane reaches on its own - measured, zero mid-frame frame-boundary drains across the - # whole OpenRA replay, which is why the divergence is device-only and why the lane could - # not arm against it before this knob existed. + # a regression gate that runs on every driver. The forced entry is the red-once of the + # DEFENCE term: it makes the serial half of VkBufferManager's busy predicate answer + # "idle" - the state an unsound completed-serial floor put the wire arm into on the + # device - so only the submission-fence half is left to order the copy. It does NOT + # cover the floor fix itself (forcing the serial term bypasses the floor entirely); the + # floor's lane is the OpenRA split retrace, which run_trace_case.cmake reds on any + # "MGWIRE-FLOOR unsound-serial-complete" line. # # ITS OWN ENTRY RATHER THAN A LANE-WIDE FLIP: the knob changes how EVERY streamed buffer # write in the process is answered, so putting it in the shared Fm environment would diff --git a/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp index f03c94b8..e668ec30 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/F1WireScenario.cpp @@ -2078,12 +2078,13 @@ TEST_F(F1WireScenario, CopyImageInPlaceOverlapDeclinesAndLeavesTheLevelAlone) { // somebody else's vertices. So the case cannot assert "no error"; it has to assert the pixels. // // Registered TWICE per arm. The plain entry is a regression gate on every driver. The -// `StaleSerial.` entry carries MGITEST_MAGMA_FORCE_STALE_BUFFER_SERIAL=1, which forces the -// frame-counting half of the busy predicate to answer "idle" - the state a mid-frame idle drain -// puts the wire arm into on the device, and one no host lane reaches by itself (measured: zero -// mid-frame frame-boundary drains across the whole OpenRA replay). With the counting half -// forced to lie, only the submission-fence half can still order the copy, which is exactly what -// this package added and exactly what the entry pins. +// `StaleSerial.` entry (split and spawn only - the tcp arm's server never sees an entry's +// environment) carries MGITEST_MAGMA_FORCE_STALE_BUFFER_SERIAL=1, which forces the serial half +// of the busy predicate to answer "idle" - the state the unsound completed-serial floor put the +// wire arm into on the device. With the serial half forced to lie, only the submission-fence +// half can still order the copy, so the entry pins that DEFENCE term. The guarantee is the +// floor (VulkanRenderer::OnSubmitsCompletedUpTo), which the knob bypasses; its lane is the +// OpenRA split retrace's "MGWIRE-FLOOR unsound-serial-complete" red condition. TEST_F(F1WireScenario, StreamedBufferSubDataBeforeEachDrawIsOrdered) { if (!Ready()) return; constexpr int kSize = 8, kBatches = 4, kColumn = kSize / kBatches;