From efe58eac146b13dedf4d38476db47d3926bbbaa7 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 18 Sep 2026 14:10:04 -0400 Subject: [PATCH] [MG_Backend, MG_Pipe, MG_Impl, Docs] (Disaggregated): P5e ra2 ID-136 - retire the indirect-binding pull instead of escalating for it, and withdraw escalation (iii) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TOUCHING mv's FILE UNDER ID-136, deliberately and by ruling: MG_Backend/DirectGLES/ MultiDraw.cpp is mv's per ID-113, and the integrator overrode that boundary for this one change because the two halves must not exist apart on any head. The boundary has not drifted. - BoundDrawIndirectBufferId (MultiDraw.cpp) gets the handle arm its neighbour ResolveBoundIndexBuffer already had, in mv's own idiom rather than a second one beside it: the arm is `MG_Config::Transport != Monolith`, STATED (ID-81) and never inferred from a null handle (ID-110); a null handle is "this verb bound no indirect buffer", the same 0 the monolith arm returns for an empty slot; a missing backend resource is the named refusal Fatal{RoleViolation, "multidraw-indirect-buffer-arm"}, a sibling of mv's RefuseMissingIndexBufferRecord, never a quiet fall-back to the frontend. The monolith body below it is unchanged token for token. The function moved down beside BoundIndexBufferId, which is the same question about the other target. - WHY A HANDLE ARM AND NOT A MIGRATION: the tier never reads a byte of the client's indirect buffer. It binds its OWN scratch command buffer and wants to put back the name that was there - a save/restore. With a transport the only other writer of this process's GL_DRAW_INDIRECT_BUFFER is DirectGLES.cpp's DrawSyncBit::IndirectBuffer arm, which binds from MGPipeApplier().VerbIndirectBuffer and nothing else, so "what was bound" IS "what the verb's record named". - Escalation (iii) withdrawn in the same commit - MGPipeBarriered's clause, the client's verb-keyed half in ClientVerbIsBarriered, and the contract row. It was never wrong, it was the wrong instrument: there is one draw opcode and the tier is chosen server-side per batch, so the narrowest key both roles could compute charged EVERY plain glMultiDraw* on the auto -> ext default - a per-batch rendezvous on the arm the phone ships, added to make a lane green. THE CHECK ON AN ESCALATION IS "WHICH ARM PAYS FOR IT", NOT "WHICH LANE GOES GREEN"; the rule is recorded in PipeApply.h beside the three that remain and in CONTRACT-P5E §2.1. - Red-once D: `if (false && ...)` on the new arm and the entry dies 3/3, deterministically, on `Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"} [BARRIER-PULLED, UNBARRIERED, the client did not fill it, retires in P8 (indirect), P9 (readback), P13 (transfer)]`. Its counterpart is this head itself: the escalation is GONE and the lane is green, so nothing needed it - and the strict census's admitted set drops from nine rows back to ID-128's eight, because the pull no longer happens at all rather than being legalised. - Gate on the flipped head: unit 2256/2256, isplit 179/179, gens rc=0, lane counts unchanged (179 / 1357 / 2 / 2). --- MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp | 78 ++++++++++++++++++-- MobileGL/MG_Impl/Pipe/PipeFill.cpp | 30 +++----- MobileGL/MG_Pipe/PipeApply.cpp | 44 +++++------ MobileGL/MG_Pipe/PipeApply.h | 15 ++-- MobileGL/MG_Remote/CONTRACT-P5E.md | 53 ++++++------- 5 files changed, 132 insertions(+), 88 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp index 7229ec79..c8f0060c 100644 --- a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp +++ b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp @@ -84,13 +84,8 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { constexpr SizeT kMaxComputeWorkGroups = 65535; constexpr SizeT kMaxComputeFlattenedIndices = kMaxComputeWorkGroups * kComputeWorkGroupSize; - Uint BoundDrawIndirectBufferId() { - const auto& indirect = - MGB_CTX->GetBufferBindingSlot(BufferTarget::DrawIndirect).GetBoundObject(); - if (!indirect) return 0; - const auto* resource = BufferImpl::EnsureBufferResource(indirect); - return resource ? resource->id : 0; - } + // BoundDrawIndirectBufferId MOVED (P5e ra2, ID-136) to sit beside BoundIndexBufferId, + // which is the same question about the other target and now has the same two arms. // --------------------------------------------------------------------------- // The bound index buffer, and WHICH SIDE ANSWERS FOR IT @@ -150,6 +145,24 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { std::abort(); } + // ---- P5e (ra2), ID-136: THE SAME REFUSAL FOR THE INDIRECT TARGET ------------------- + // + // A sibling of mv's rather than a second idiom, because it is the same sentence about + // the other buffer target: a missing record on the handle arm aborts by name and never + // falls back to the frontend binding slot, which answers for whatever the CLIENT has + // bound NOW - a later verb than the one being applied. + [[noreturn]] void RefuseMissingIndirectBufferRecord(const char* entry, + MG_Pipe::MGPipeHandle res) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"multidraw-indirect-buffer-arm\"} - %s found no " + "backend resource for the indirect buffer {%u, %u} this verb's record named. " + "The indirect buffer of this family is MGPipeApplier().VerbIndirectBuffer " + "(CONTRACT-P5E §2.1) and the arm is selected by Transport != Monolith " + "(ID-81); falling back to the frontend GL_DRAW_INDIRECT_BUFFER slot here " + "would restore whatever the CLIENT has bound now", + entry, res.Slot, res.Gen); + std::abort(); + } + // Does the applier's record say the application SUPPLIED this store's content? The M-3 // rule ScopedRestartIndexSubstitution spells: for a supplied store a coverage gap in the // server shadow is a missing record and Fatal by name; for one the application ORPHANED @@ -247,6 +260,57 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { return ResolveBoundIndexBuffer(IndexBufferQuestion::DriverName, "BoundIndexBufferId").Id; } + // The GL name on GL_DRAW_INDIRECT_BUFFER, i.e. what a tier that swaps in its own scratch + // COMMAND buffer has to put back - the exact twin of BoundIndexBufferId above, and now + // with the same two arms. + // + // ---- P5e (ra2), ID-136: THE SEAT THIS FILE'S OWN RULE HAD MISSED ------------------- + // + // This was the LAST unguarded frontend read on the multi-draw apply path, and it sat + // twenty lines above the function that retired its neighbour. It asked + // `MGB_CTX->GetBufferBindingSlot(BufferTarget::DrawIndirect)` and then + // `EnsureBufferResource()` - a registry lookup keyed by the client's + // identity, which is §4.4's rule and not only the allocator's. Under run-ahead that is + // `Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"}` on every batch the + // indirect tiers execute: 18 lane entries, and the only thing standing between the flip + // and a green lane once the fill race was fixed. + // + // WHAT IT IS NOT: a data dependency. The tier does not want the client's indirect + // buffer - it never reads a byte of it. It binds its OWN scratch command buffer + // (g_indirectCommands) and wants to put back the name that was there. So the answer is + // not "migrate the value" (which is what P8 owes for the ordinary indirect draw path); + // it is "ask the side that did the binding". ID-133's escalation (iii) legalised the + // pull instead, at the price of a rendezvous on every plain glMultiDraw* on the DEFAULT + // tier - a real cost on the shipping arm, paid to make a lane green. ID-136 withdraws + // that escalation and retires the read, in the commit that adds this arm. + // + // WHY MGPipeApplier().VerbIndirectBuffer IS THE WHOLE ANSWER ON THIS ARM: with a + // transport, the ONLY writer of this process's GL_DRAW_INDIRECT_BUFFER outside this + // function is DirectGLES.cpp's DrawSyncBit::IndirectBuffer arm, which binds from exactly + // that handle and nothing else. So "what was bound" IS "what the verb's record named", + // and a null handle is "this verb bound none" - the same 0 the monolith arm returns for + // an empty slot, and not the arm test (ID-110: the arm was decided by the transport + // above, never inferred from a null). + Uint BoundDrawIndirectBufferId() { +#if MOBILEGL_BUILD_DISAGGREGATED + if (MG_Config::Transport != MG_Config::TransportMode::Monolith) { + const MG_Pipe::MGPipeHandle res = MG_Pipe::MGPipeApplier().VerbIndirectBuffer; + if (MG_Pipe::MGPipeHandleIsNull(res)) return 0; + auto* resource = BufferImpl::EnsureBufferResourceForHandle(nullptr, res); + if (resource == nullptr) { + RefuseMissingIndirectBufferRecord("BoundDrawIndirectBufferId", res); + } + return resource->id; + } +#endif + // MONOLITH GLUE from here down, token for token what this function did before. + const auto& indirect = + MGB_CTX->GetBufferBindingSlot(BufferTarget::DrawIndirect).GetBoundObject(); + if (!indirect) return 0; + const auto* resource = BufferImpl::EnsureBufferResource(indirect); + return resource ? resource->id : 0; + } + // --------------------------------------------------------------------------- // Scratch GL objects // diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 654eba0f..72ff9c24 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -423,27 +423,15 @@ namespace MobileGL::MG_Pipe { static_assert(MGPipeVerbForWireOp(MGPWireOp::DrawVbo) == MGPipeVerb::DrawArrays, "the draw family's representative row moved; the fallback below names " "draw_vbo because that is the record every kDraw verb emits"); - // ---- P5e (ra2), ID-133: AND THE CLIENT'S HALF OF ESCALATION (iii) -------------- - // - // The server escalates a PLAIN multi-draw record (`NumDraws > 1` and not indirect) - // because Espryt's indirect tiers still pull the client's GL_DRAW_INDIRECT_BUFFER - // binding at apply. The fill decision is asked HERE, before the record exists, so - // it cannot read NumDraws - it asks the verb instead, and the three verbs below are - // exactly the ones whose record can carry NumDraws > 1 without kDrawIsIndirect. - // - // THE DIRECTION OF THE INEQUALITY IS THE SAFETY ARGUMENT, and it is worth stating - // because the two clauses are spelled differently. The client's set must CONTAIN - // the server's: a record the server calls barriered whose fields the client did not - // fill would be an ADMITTED pull of a stale value - legal-looking and wrong, which - // is worse than the abort. It does contain it, because `NumDraws > 1` can only come - // from a glMultiDraw* entry point, and the `MultiDraw*Indirect*` verbs set - // kDrawIsIndirect and are therefore NOT escalated on either side. The reverse - // inclusion is not needed: a verb the client fills for and the server then calls - // unbarriered costs a fill nobody reads, never a wrong answer. - const Bool plainMultiDraw = verb == MGPipeVerb::MultiDrawArrays || - verb == MGPipeVerb::MultiDrawElements || - verb == MGPipeVerb::MultiDrawElementsBaseVertex; - if (plainMultiDraw) return true; + // P5e (ra2): THE CLIENT'S HALF OF ESCALATION (iii) WAS HERE AND WENT WITH IT + // (ID-133, withdrawn by ID-136). It named MultiDrawArrays / MultiDrawElements / + // MultiDrawElementsBaseVertex so the client would FILL for the records the server + // was escalating - a fill the server then read as a barriered pull. Retiring that + // pull outright (MultiDraw.cpp's BoundDrawIndirectBufferId takes a handle arm) left + // nothing for the fill to serve, so a plain multi-draw is an ordinary kDraw verb + // again and takes the draw_vbo answer below. The pair is the phase's own lesson + // written twice: a wait added to make a lane green is paid by an arm, and here the + // arm was the default tier the phone ships. if (op == MGPWireOp::kOpCount && kMGPipeVerbClass[static_cast(verb)] == MGPipeVerbClass::kDraw) { op = MGPWireOp::DrawVbo; diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index 5e957459..c10c8153 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -3015,33 +3015,27 @@ namespace MobileGL::MG_Pipe { if (op == MGPWireOp::DrawVbo && payload != nullptr) { const auto& draw = *static_cast(payload); if ((draw.Flags & static_cast(kDrawClientArrays)) != 0) return true; - // 4. Escalation (iii), ID-133: a PLAIN multi-draw - NumDraws > 1 and not indirect. - // Espryt's multi-draw tiers are chosen on the SERVER and per batch - // (MultiDraw.cpp's ResolveTierForBatch), and the two indirect tiers reach - // MultiDrawImpl::RunIndirect, whose apply still reads the client's - // GL_DRAW_INDIRECT_BUFFER binding slot (BoundDrawIndirectBufferId, :87). That - // is a BARRIER_PULLED field whose retiring phase is P8, so under run-ahead an - // unbarriered record reading it is `Fatal{UnmigratedPipeInput, - // "GetBufferBindingSlot@DrawArrays"}` - 18 lane entries on the two tier lanes. + // ---- ESCALATION (iii) WAS HERE AND IS WITHDRAWN (P5e ra2, ID-133 then ID-136) --- // - // WHY THE KEY IS THE RECORD AND NOT THE TIER, which is the thing a reader will - // want to check: there is exactly ONE draw opcode. Every one of the twenty draw - // entry points collapses onto draw_vbo (MGPDrawInfo's header says so), and the - // tier is resolved on the server, per batch, from driver caps the client does - // not hold - so "escalate the indirect multi-draw op" has no op to name and no - // predicate both roles could compute. NumDraws > 1 is the narrowest wire fact - // that CONTAINS the reaching set: RunIndirect is reachable only from - // DrawElementsBatch, which only a multi-draw record enters. + // ID-133 escalated a PLAIN multi-draw (`NumDraws > 1 && !kDrawIsIndirect`) so that + // MultiDrawImpl::RunIndirect's read of the client's GL_DRAW_INDIRECT_BUFFER binding + // became a legal barriered pull instead of an unbarriered Fatal. It worked, and it + // cost a rendezvous on EVERY plain glMultiDraw* on the DEFAULT tier: there is one + // draw opcode - all twenty entry points collapse onto draw_vbo - and the tier is + // chosen on the server per batch (MultiDraw.cpp's ResolveTierForBatch), so no + // predicate both roles can compute names the arm that actually needed it. // - // AND kDrawIsIndirect IS EXCLUDED, which is what keeps Sodium's draw call out of - // it: a genuinely indirect record carries its buffer as a handle in the second - // tail and its apply resolves from that (ResolveIndirectCommandBytes' split - // arm), so it pulls nothing and needs no wait. NumDraws is 0 there, so the test - // is written `> 1` and the flag test is belt to that brace - both are stated - // because the reason each holds is different. - if (draw.NumDraws > 1 && (draw.Flags & static_cast(kDrawIsIndirect)) == 0) { - return true; - } + // THE CHECK ON AN ESCALATION IS "WHICH ARM PAYS FOR IT", NOT "WHICH LANE GOES + // GREEN", and that is the rule this pair of rulings exists to record. The read was + // a save/restore of a GL binding NAME around the tier's own scratch buffer, not a + // data dependency, so the answer was to ask the side that did the binding: + // BoundDrawIndirectBufferId now takes the handle arm its neighbour + // ResolveBoundIndexBuffer already had, and the pull is GONE rather than legalised. + // + // Both halves landed in ONE commit on purpose: the pull retired without this clause + // withdrawn is a cost with no reason, and this clause withdrawn without the pull + // retired puts 18 lane entries back on the unbarriered arm. Neither is a state to + // gate or to measure, so neither was ever a head. } return false; } diff --git a/MobileGL/MG_Pipe/PipeApply.h b/MobileGL/MG_Pipe/PipeApply.h index 3b95ca7d..7d41d405 100644 --- a/MobileGL/MG_Pipe/PipeApply.h +++ b/MobileGL/MG_Pipe/PipeApply.h @@ -916,13 +916,18 @@ namespace MobileGL::MG_Pipe { // 1. MGPipeWaitClassFor(op) != kWaitNone the static column (§2.2) // 2. kCtxVerb with an open transform-feedback span escalation (i): XFB stays lockstep // 3. draw_vbo carrying kDrawClientArrays escalation (ii): client vertex arrays - // 4. draw_vbo with NumDraws > 1 and NOT indirect escalation (iii): plain multi-draw // // NO OTHER RUNTIME ESCALATION EXISTS. Adding one is an integrator ruling and a row in the - // contract, not a condition somebody adds at a call site - because a fifth clause that only - // one side computes is the same silent failure one level down. (iii) is ID-133's, and - // ID-118's precedent is the one it follows: move the record into a barriered class rather - // than special-case the field its apply still pulls. + // contract, not a condition somebody adds at a call site - because a fourth clause that only + // one side computes is the same silent failure one level down. + // + // THERE WAS AN ESCALATION (iii) AND IT WAS WITHDRAWN, which is worth a line here rather than + // only in the history: ID-133 barriered a plain multi-draw to legalise a pull in Espryt's + // indirect multi-draw tier, ID-136 retired the pull instead (MultiDraw.cpp's + // BoundDrawIndirectBufferId) and took the clause back out. The rule it leaves behind is the + // one to apply to the next candidate: ask WHICH ARM PAYS for an escalation, not which lane + // it turns green - (iii) was keyed on the only wire fact available and therefore charged the + // default tier for a read only two opt-in tiers made. // // Escalation (ii) is a REFUSAL under run-ahead (§5.1), so on a run-ahead server it never // reaches the sink at all; it is here so the predicate is TOTAL and so the lockstep arm, diff --git a/MobileGL/MG_Remote/CONTRACT-P5E.md b/MobileGL/MG_Remote/CONTRACT-P5E.md index 48101830..d87f3814 100644 --- a/MobileGL/MG_Remote/CONTRACT-P5E.md +++ b/MobileGL/MG_Remote/CONTRACT-P5E.md @@ -90,8 +90,6 @@ MGPipeBarriered(op, payload, st) = MGPipeWaitClassFor(op) != kWaitNone // static column, §2.2 || (MGPipeCallClassFor(op) == kCtxVerb && st.IsTransformFeedbackActive) // (i) XFB stays lockstep || (op == DrawVbo && (payload.Flags & kDrawClientArrays)) // (ii) client arrays - || (op == DrawVbo && payload.NumDraws > 1 - && !(payload.Flags & kDrawIsIndirect)) // (iii) plain multi-draw ``` `st.IsTransformFeedbackActive` is `MGPContextValues`' value (`MGPipeTypes.h:1698`): on the client it is `ctx.IsTransformFeedbackActive()` at emit, on the server the applied value — `set_context_values` @@ -105,36 +103,31 @@ must not be told a draw is unbarriered on that account. Escalation (ii) is a ref (§5.1), so on a run-ahead server it never reaches the sink; it is listed so the predicate is total. **No other runtime escalation exists**; adding one is an integrator ruling and a row here. -**Escalation (iii) is ID-133's, added by ra2, and it follows ID-118's precedent** — move the -record into a barriered class rather than special-case the field its apply still pulls. Espryt's -multi-draw tier is chosen on the SERVER and per batch (`MultiDraw.cpp`'s `ResolveTierForBatch`), -and the two indirect tiers reach `MultiDrawImpl::RunIndirect`, whose apply still reads the -client's `GL_DRAW_INDIRECT_BUFFER` binding slot (`BoundDrawIndirectBufferId`, `MultiDraw.cpp:87`) -— `GetBufferBindingSlot`, whose retiring phase is P8. Unbarriered, that is -`Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"}`: 18 lane entries on the two -tier-pinned lanes. +**There was an escalation (iii) and it was WITHDRAWN — ID-133 added it, ID-136 took it back out, +and the reason is the most reusable thing P5e learned about escalations.** ID-133 barriered a +plain multi-draw (`NumDraws > 1 && !kDrawIsIndirect`) so that `MultiDrawImpl::RunIndirect`'s read +of the client's `GL_DRAW_INDIRECT_BUFFER` binding became a legal barriered pull instead of +`Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"}` on 18 lane entries. It worked, and +it was the wrong instrument: -**Why the key is the RECORD and not the tier.** There is exactly one draw opcode — all twenty -draw entry points collapse onto `draw_vbo` — and the tier is resolved on the server from driver -caps the client does not hold, so "escalate the indirect multi-draw op" has no op to name and no -predicate both roles could compute. `NumDraws > 1` is the narrowest wire fact that CONTAINS the -reaching set (`RunIndirect` is reachable only from `DrawElementsBatch`, which only a multi-draw -record enters). `kDrawIsIndirect` is excluded because a genuinely indirect record carries its -buffer as a handle in the second tail and resolves from that, pulling nothing. **The cost is -therefore NOT confined to the opt-in tier lanes**: a plain `glMultiDraw*` now waits on every tier, -including the `auto` → `ext` default. That is a real charge against §0's goal and the device arm -owns measuring it. **It can be withdrawn outright** once `BoundDrawIndirectBufferId` takes the -handle arm its own neighbour `ResolveBoundIndexBuffer` already has (three lines, `MultiDraw.cpp`, -mv's file per ID-113): the read is a SAVE/RESTORE of a GL binding name around the tier's scratch -buffer, not a data dependency, so the server can answer it from its own binding record and the -pull disappears rather than being legalised. +- There is exactly **one draw opcode** — all twenty draw entry points collapse onto `draw_vbo` — + and the multi-draw tier is resolved on the SERVER, per batch (`MultiDraw.cpp`'s + `ResolveTierForBatch`), from driver caps the client does not hold. So "escalate the indirect + multi-draw op" has no op to name and no predicate both roles can compute, and the narrowest + available key charged **every plain `glMultiDraw*` on every tier**, including the `auto` → `ext` + default the phone ships — a per-batch rendezvous on the shipping arm, added to satisfy a lane. +- And the read was never a data dependency: `BoundDrawIndirectBufferId` SAVES AND RESTORES a GL + binding name around the tier's own scratch command buffer. Giving it the handle arm its + neighbour `ResolveBoundIndexBuffer` already had retires the pull outright. -The client's half is asked at the validate point, before the record exists, so it cannot read -`NumDraws`; it asks the verb, and `{MultiDrawArrays, MultiDrawElements, MultiDrawElementsBaseVertex}` -is exactly the set whose record can carry `NumDraws > 1` without `kDrawIsIndirect`. **The -inclusion must go that way round**: a record the server calls barriered whose fields the client -did not fill would be an ADMITTED pull of a stale value, which is worse than the abort; a verb the -client fills for and the server then calls unbarriered costs only a fill nobody reads. +**The rule this leaves for the next candidate escalation: ask WHICH ARM PAYS FOR IT, not which +lane it turns green.** A wait added to make a lane green is a real cost on a real path; if the +predicate cannot name the arm that needs it, the escalation is charging arms that do not. + +The two halves — retiring the pull and withdrawing the clause — landed in one commit (ID-136 +overrode ID-113's package boundary for it): the pull retired without the clause withdrawn is a +cost with no reason, and the clause withdrawn without the pull retired puts the 18 entries back +on the unbarriered arm. ### 2.2 The static column, row by row