diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index ed19e4c3..cb60ebaf 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -488,6 +488,110 @@ namespace MobileGL::MG_Backend::DirectGLES { // TODO: deletion for deleted objects namespace BufferImpl { + // P5e (vi), CONTRACT-P5E §5.1 + §5.8 (ruling 1 / ID-81): THE ARM SELECTOR for this + // family, and it is a conjunction on purpose. + // + // Transport != Monolith the record arm exists because there is no frontend VAO on + // this side of a real split. Under Transport=monolith the + // push build keeps its frontend arms token for token - that + // is what the verify comparator compares against, and what + // makes MOBILEGL_IPC_RUN_AHEAD=0 a pure wait-rule A/B on + // identical server code rather than an arm swap. + // the family bit `0x0ff` (bit 7 on, bit 8 off) is a supported A/B and must + // keep running the legacy vertex-input walk; the bit is + // already the gate the rest of this family reads. + // + // It is NOT gated on run-ahead. The records carry the whole family either way, so a + // lockstep split session reads them too and the wait rule changes nothing here - which + // is the only reason ra can flip one constant at the end of the phase and change no + // backend code at all. + inline Bool VertexInputReadsRecords() { +#if MOBILEGL_BUILD_DISAGGREGATED + return MG_Config::Transport != MG_Config::TransportMode::Monolith && + VertexInputSubsystemEnabled(); +#else + return false; +#endif + } + +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi): "does this draw fetch any attribute out of the application's own memory", + // answered FROM THE RECORD instead of from the frontend VAO. The emitter publishes + // Res == kMGPipeNullHandle for a client-memory array (VertexInputEmit.h says so in as + // many words: "A client-memory array is Res == kMGPipeNullHandle, and that is not a + // hole"), and the window is truncated at the highest ENABLED attribute, so a null Res + // inside [Start, Start+Count) is exactly the client-sourced case and nothing else. + // + // BOTH VIEWS ARE NEEDED, and taking only the buffer one would be wrong in the common + // direction: the window covers [0, highest enabled + 1), so a VAO whose attribute 0 is + // DISABLED and whose attribute 1 feeds from a VBO publishes entry[0].Res == null too. + // A null Res is "client-sourced" only for an attribute the CONFIGURATION has enabled, + // which is what the vertex-elements record answers. Reading one view and not the other + // would send every such ordinary draw back to the frontend VAO and keep the row this + // package is retiring alive for no reason. + // + // A prefix scan rather than a memo: Count is 1-3 on the measured workload, not 32, and + // this replaces a cross-TU accessor call plus a SharedPtr dereference per DrawArrays. + Bool AnyClientSideVertexArrayInRecord() { + const auto& st = MG_Pipe::MGPipeApplier(); + if (st.VertexBufferCount == 0) return false; + const MG_Pipe::MGPipeHandle elements = st.BoundVertexElements; + if (MG_Pipe::MGPipeHandleIsNull(elements) || elements.Slot >= st.VertexElementsCsos.size()) { + return false; + } + const auto& record = st.VertexElementsCsos[elements.Slot]; + if (!record.Live || record.Gen != elements.Gen) return false; + + const Uint32 begin = st.VertexBufferStart; + const Uint32 end = begin + st.VertexBufferCount; + for (Uint32 i = begin; i < end && i < MG_Pipe::kMGPipeMaxVertexAttribs; ++i) { + if (!record.Attributes[i].Enabled) continue; + if (MG_Pipe::MGPipeHandleIsNull(st.VertexBuffers[i].Res)) return true; + } + return false; + } + + // See Managers.h for why these two are named functions over the applier state and + // nothing else. + Uint ResolveDrawVertexBuffersFromRecord(const MG_Pipe::MGPipeApplierState& st, + DrawVertexBufferRequest* out, Uint capacity) { + Uint count = 0; + const Uint32 begin = st.VertexBufferStart; + const Uint32 end = begin + st.VertexBufferCount; + for (Uint32 i = begin; i < end && i < MG_Pipe::kMGPipeMaxVertexAttribs; ++i) { + const MG_Pipe::MGPVertexBuffer& binding = st.VertexBuffers[i]; + if (MG_Pipe::MGPipeHandleIsNull(binding.Res)) continue; + + // A {slot, gen} compare, not a raw address compare: an address a successor + // object can reproduce is the identity hazard this phase exists to remove. + Bool alreadySeen = false; + for (Uint j = 0; j < count; ++j) { + if (out[j].Res == binding.Res) { + alreadySeen = true; + break; + } + } + if (alreadySeen) continue; + if (count >= capacity) { + MGLOG_E_ONCE("MGPipe: the vertex-buffer window named more distinct buffers than " + "a draw can hold (%u); the tail is dropped", capacity); + break; + } + out[count].Res = binding.Res; + out[count].BindingIndex = binding.BindingIndex; + ++count; + } + return count; + } + + DrawIndexBufferRequest ResolveDrawIndexBufferFromRecord(const MG_Pipe::MGPipeApplierState& st) { + DrawIndexBufferRequest request; + request.Res = st.IndexBuffer.Res; + request.Serial = st.IndexBufferSerial; + return request; + } +#endif + void SyncBufferBindingPoints(BufferTarget target, GLenum glTarget) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); @@ -784,6 +888,108 @@ namespace MobileGL::MG_Backend::DirectGLES { memo->vboCleanEpoch = 0; } } + +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi), CONTRACT-P5E §5.1: THE SAME MEMO, THE SAME THREE-VALUE KEY, AND A WALK THAT + // READS NO FRONTEND AT ALL. The arm above keeps the frontend attribute walk because the + // push-monolith build has to keep its bytes (§5.8); this one is what a server with no + // client objects on its side runs, and the difference between them is exactly three + // substitutions: + // + // the walk st.VertexBuffers[Start .. Start+Count) instead of + // GetAllAttributes(): the SAME set, because the emitter resolves the + // attributes on the client and publishes one entry per attribute slot + // (VertexInputEmit.h's EmitVertexBuffers), with Res == null for a + // client-memory array and for a disabled slot - which is why the null + // skip below is not a hole but the record's own "nothing to ensure". + // the dedupe a {slot, gen} compare instead of a raw address compare. An address + // a successor object can reproduce is the identity hazard this whole + // phase is about; a handle cannot be reproduced (Gen moves on reuse). + // the ensure EnsureBufferResourceForHandle(nullptr, Res). The frontend argument + // is only ever used for BufferObject::SyncPersistentMappedRange, which + // that function already skips under a transport (D-N), so passing + // nullptr removes a read rather than dropping work - and (nullptr, h) + // is already the live shape of the indirect-buffer ensure two hundred + // lines below. + // + // The IBO half lives in SyncNeccessaryBuffers beside the legacy one, for the reason it + // always did: the index slot is not part of the configuration (D5). + void SyncVaoAttributeBuffersByRecord( + VertexArrayImpl::BackendVertexArrayObject::ResolvedDrawBuffers* memo, Uint64 bufferEpoch) { + const auto& st = MG_Pipe::MGPipeApplier(); + const MG_Pipe::MGPipeHandle elements = st.BoundVertexElements; + Uint64 elementsSerial = 0; + Bool haveElementsRecord = false; + if (!MG_Pipe::MGPipeHandleIsNull(elements) && elements.Slot < st.VertexElementsCsos.size()) { + const auto& record = st.VertexElementsCsos[elements.Slot]; + if (record.Live && record.Gen == elements.Gen) { + elementsSerial = record.ContentSerial; + haveElementsRecord = true; + } + } + const Uint64 buffersSerial = st.VertexBuffersSerial; + + // NO LIVE ELEMENTS RECORD IS A MISS, NEVER A HIT - the arm above states the whole + // argument (MGPipeApplierReset empties the table at every context change and the + // key would then never move again). + const Bool memoKeyIsMeaningful = haveElementsRecord; + + if (memo && memo->valid && memoKeyIsMeaningful && memo->elementsHandle == elements && + memo->elementsSerial == elementsSerial && memo->buffersSerial == buffersSerial) { + if (memo->vboCleanEpoch != bufferEpoch) { + Bool allClean = true; + for (Uint i = 0; i < memo->count; ++i) { + auto& entry = memo->entries[i]; + // nullptr, not entry.frontend: the frontend question inside + // IsBufferDrawCleanByHandle is monolith-only already + // (askTheObjectWhetherItIsMapped), and a raw client address held across + // records is precisely what §4.4 stops the apply thread dereferencing. + if (IsBufferDrawCleanByHandle(entry.handle, entry.resource, nullptr)) continue; + allClean = false; + // Re-ensured BY HANDLE. The legacy repair went back through the memoed + // attribute index to re-read attrib.Buffer; here the entry already + // names the resource the record named, so the repair needs nothing the + // walk did not already have. + entry.resource = EnsureBufferResourceForHandle(nullptr, entry.handle); + } + memo->vboCleanEpoch = allClean ? bufferEpoch : 0; + } + return; + } + + // Full walk, once per distinct buffer, rebuilding the memo as it goes. WHICH + // buffers is ResolveDrawVertexBuffersFromRecord's answer and only its answer - see + // Managers.h: that separation is what lets a unit case drive the source decision + // this package changed without an ES context, and what makes a revert of it visible + // rather than duplicated. + DrawVertexBufferRequest requests[MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS]; + const Uint syncedBufferCount = ResolveDrawVertexBuffersFromRecord( + st, requests, MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + for (Uint i = 0; i < syncedBufferCount; ++i) { + auto* resource = EnsureBufferResourceForHandle(nullptr, requests[i].Res); + if (memo) { + auto& entry = memo->entries[i]; + // Explicitly cleared rather than left as whatever the previous build of + // this memo stored: nothing on this arm may dereference it, and a stale + // address that merely happens never to be read is the kind of thing a + // later reader takes for a live one. + entry.frontend = nullptr; + entry.attribIndex = static_cast(requests[i].BindingIndex); + entry.resource = resource; + entry.handle = requests[i].Res; + } + } + if (memo) { + memo->count = syncedBufferCount; + memo->elementsHandle = elements; + memo->elementsSerial = elementsSerial; + memo->buffersSerial = buffersSerial; + memo->valid = memoKeyIsMeaningful; + // Rebuilt via the ensure, not probed clean: the next probe pass stamps it. + memo->vboCleanEpoch = 0; + } + } +#endif // MOBILEGL_BUILD_DISAGGREGATED #endif // MOBILEGL_PIPE_PUSH // `vaoConfigVersion` is the caller's early read of currentVAOObject->GetConfigVersion(): @@ -804,7 +1010,12 @@ namespace MobileGL::MG_Backend::DirectGLES { // 1.VBO 2.IBO (if needed) 3.UBO 4.IndirectBuffer (if needed) // PBO is not needed since it should be handled in frontend - if (!currentVAOObject) { + // P5e (vi): on the record arm there is deliberately no frontend VAO to check - the + // question "is a VAO bound" is answered by st.BoundVertexElements at the twin + // resolve, and a null there already produced a null twin (and therefore a null + // memo) before this function was called. Asking for an object this side does not + // have would make every split draw log and return. + if (!currentVAOObject && !VertexInputReadsRecords()) { MGLOG_E_ONCE("No VAO is currently bound, cannot sync necessary buffers."); return; } @@ -825,6 +1036,11 @@ namespace MobileGL::MG_Backend::DirectGLES { const Uint64 bufferEpoch = CurrentBufferMutationEpoch(); auto* memo = vaoTwin ? &vaoTwin->GetResolvedDrawBuffersMemo() : nullptr; const Uint32 configVersion = vaoConfigVersion; +#if MOBILEGL_BUILD_DISAGGREGATED + if (VertexInputReadsRecords()) { + SyncVaoAttributeBuffersByRecord(memo, bufferEpoch); + } else +#endif #if MOBILEGL_PIPE_PUSH if (VertexInputSubsystemEnabled()) { SyncVaoAttributeBuffersByHandle(currentVAOObject, memo, bufferEpoch); @@ -894,47 +1110,88 @@ namespace MobileGL::MG_Backend::DirectGLES { // the config version). A stale identity hit is impossible in effect: the // clean probe re-validates the resource against the LIVE bound object. if (includeIBO) { - const auto& possibleIBO = currentVAOObject->GetIndexBufferBindingSlot().GetBoundObject(); - if (possibleIBO) { - // The epoch stamp alone is NOT enough here: the index slot can - // rebind another buffer with no epoch (and no config-version) move, - // so the identity compare always runs; only the clean PROBE is - // elided while the stamp holds. -#if MOBILEGL_PIPE_PUSH - if (ResourceSubsystemEnabled()) { - // Same three cases, with the identity re-keyed off the raw frontend - // address onto the resource's {slot, gen} (D-G4). - const MG_Pipe::MGPipeHandle iboHandle = HandleOfBuffer(possibleIBO.get()); - if (memo && memo->iboHandle == iboHandle && memo->iboCleanEpoch == bufferEpoch) { +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi), CONTRACT-P5E §5.1: the index buffer is st.IndexBuffer.Res and the + // "has it moved" question is st.IndexBufferSerial. The identity compare stays - + // it always did, because the index slot is outside the configuration version + // (D5) - but it now compares a handle the RECORD named instead of a frontend + // address the slot was probed for, and the serial joins it because this arm has + // no live slot to re-read: see ResolvedDrawBuffers::iboSerial in Managers.h. + // + // A null Res is "no element buffer bound" and is a no-op here exactly as the + // absent `possibleIBO` is below: SyncToBackendFromApplier is what binds + // GL_ELEMENT_ARRAY_BUFFER back to 0 for such a draw, and it keys on the same + // serial. + if (VertexInputReadsRecords()) { + const DrawIndexBufferRequest request = + ResolveDrawIndexBufferFromRecord(MG_Pipe::MGPipeApplier()); + const MG_Pipe::MGPipeHandle iboHandle = request.Res; + const Uint64 iboSerial = request.Serial; + if (!MG_Pipe::MGPipeHandleIsNull(iboHandle)) { + const Bool identityHolds = + memo != nullptr && memo->iboHandle == iboHandle && memo->iboSerial == iboSerial; + if (identityHolds && memo->iboCleanEpoch == bufferEpoch) { // probed fully clean at this epoch; nothing can have dirtied it - } else if (memo && memo->iboHandle == iboHandle && - IsBufferDrawCleanByHandle(iboHandle, memo->iboResource, - possibleIBO.get())) { + } else if (identityHolds && + IsBufferDrawCleanByHandle(iboHandle, memo->iboResource, nullptr)) { memo->iboCleanEpoch = bufferEpoch; } else { - auto* resource = EnsureBufferResource(possibleIBO); + auto* resource = EnsureBufferResourceForHandle(nullptr, iboHandle); if (memo) { memo->iboHandle = iboHandle; - memo->iboFrontend = possibleIBO.get(); + memo->iboSerial = iboSerial; + memo->iboFrontend = nullptr; // see Entry::frontend's note memo->iboResource = resource; // Repaired, not probed clean: stamp on the next clean probe. memo->iboCleanEpoch = 0; } } - } else + } + } else #endif - if (memo && memo->iboFrontend == possibleIBO.get() && memo->iboCleanEpoch == bufferEpoch) { - // probed fully clean at this epoch; nothing can have dirtied it - } else if (memo && memo->iboFrontend == possibleIBO.get() && - IsBufferDrawClean(memo->iboFrontend, memo->iboResource)) { - memo->iboCleanEpoch = bufferEpoch; - } else { - auto* resource = EnsureBufferResource(possibleIBO); - if (memo) { - memo->iboFrontend = possibleIBO.get(); - memo->iboResource = resource; - // Repaired, not probed clean: stamp on the next clean probe. - memo->iboCleanEpoch = 0; + { + const auto& possibleIBO = currentVAOObject->GetIndexBufferBindingSlot().GetBoundObject(); + if (possibleIBO) { + // The epoch stamp alone is NOT enough here: the index slot can + // rebind another buffer with no epoch (and no config-version) move, + // so the identity compare always runs; only the clean PROBE is + // elided while the stamp holds. +#if MOBILEGL_PIPE_PUSH + if (ResourceSubsystemEnabled()) { + // Same three cases, with the identity re-keyed off the raw frontend + // address onto the resource's {slot, gen} (D-G4). + const MG_Pipe::MGPipeHandle iboHandle = HandleOfBuffer(possibleIBO.get()); + if (memo && memo->iboHandle == iboHandle && memo->iboCleanEpoch == bufferEpoch) { + // probed fully clean at this epoch; nothing can have dirtied it + } else if (memo && memo->iboHandle == iboHandle && + IsBufferDrawCleanByHandle(iboHandle, memo->iboResource, + possibleIBO.get())) { + memo->iboCleanEpoch = bufferEpoch; + } else { + auto* resource = EnsureBufferResource(possibleIBO); + if (memo) { + memo->iboHandle = iboHandle; + memo->iboFrontend = possibleIBO.get(); + memo->iboResource = resource; + // Repaired, not probed clean: stamp on the next clean probe. + memo->iboCleanEpoch = 0; + } + } + } else +#endif + if (memo && memo->iboFrontend == possibleIBO.get() && memo->iboCleanEpoch == bufferEpoch) { + // probed fully clean at this epoch; nothing can have dirtied it + } else if (memo && memo->iboFrontend == possibleIBO.get() && + IsBufferDrawClean(memo->iboFrontend, memo->iboResource)) { + memo->iboCleanEpoch = bufferEpoch; + } else { + auto* resource = EnsureBufferResource(possibleIBO); + if (memo) { + memo->iboFrontend = possibleIBO.get(); + memo->iboResource = resource; + // Repaired, not probed clean: stamp on the next clean probe. + memo->iboCleanEpoch = 0; + } } } } @@ -1599,15 +1856,18 @@ namespace MobileGL::MG_Backend::DirectGLES { // legacy arm that is TwinLookupMemo's contract, and on the {slot, gen} arm it is // simply that nothing but the sweep frees a slot and the sweep only takes slots // whose frontend object is already gone. + // MONOLITH GLUE AS OF P5e (vi), CONTRACT-P5E §4.1 / §5.8, AND THE SCOPE IS GONE WITH THE + // DEBT IT NAMED. This overload is reached only when there IS a frontend VAO to resolve + // from, i.e. from the push-monolith and legacy arms of PrepareForDraw; a live transport + // takes ResolveVaoTwin(st.BoundVertexElements) instead (Managers.cpp, id's body). The + // MGPipeFrontendKeyedRegistryScope that used to wrap this - one of the three scope sites + // this family owned - is DELETED rather than narrowed, and that deletion is what makes + // the revert loud: putting `Find(vao.get())` back on the transport arm now aborts + // Fatal{RoleViolation, "MGPipeSlots"} from HandleOf instead of being quietly exempted. BackendVertexArrayObject* ResolveVaoTwin(const SharedPtr& vao) { #ifdef TRACY_ENABLE ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif -#if MOBILEGL_BUILD_DISAGGREGATED - // P5c (G6, CONTRACT-P5C §5.4): frontend-keyed twin resolution, named debt inside - // the scope - P3b/P4b rekeys the registry onto handles. - const MG_Pipe::MGPipeFrontendKeyedRegistryScope frontendKeyedRegistry; -#endif #if MOBILEGL_PIPE_PUSH if (EsprytSlotTablesEnabled()) { // No memo on this arm. The memo existed to turn the registry's hash probe into @@ -1653,6 +1913,84 @@ namespace MobileGL::MG_Backend::DirectGLES { vaoTwin->SyncToBackend(currentVAOObject); } +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi): the record arm's entry, and it takes NO frontend object - CONTRACT-P5E §4.1 + // ("the object parameter is deleted from the transport overload, not defaulted to + // null"). A null twin here is the null BoundVertexElements, i.e. "no VAO bound", which + // PrepareForDraw turns into BindBackendVAOId(0) exactly as a null frontend VAO did; + // logging about it would fire on every draw of a session that legitimately has no + // vertex-elements CSO yet. + void SyncCurrentVAOFromRecords(BackendVertexArrayObject* vaoTwin) { +#ifdef TRACY_ENABLE + ZoneScopedC(TRACY_ZONECOLOR_BACKEND); +#endif + // Kept for symmetry with the overload above and free on this arm: the slot table's + // sweep is a no-op (Managers.h's CollectGarbageIfNeeded on the handle registry). + g_backendVertexArrayObjects.CollectGarbageIfNeeded(); + if (!vaoTwin) return; + vaoTwin->SyncToBackendFromApplier(); + } +#endif // MOBILEGL_BUILD_DISAGGREGATED + + // THE CLIENT-MEMORY VERTEX ARRAY UPLOAD, and the one read of this family that P5e does + // NOT retire - it MOVES the decision instead, which is what ID-82 / ruling 2 settles. + // + // The bytes of a client-memory array exist only in the application's own memory: the + // emitter publishes Res == kMGPipeNullHandle for such an attribute by design + // (VertexInputEmit.h) and this upload is where the server used to dereference + // attrib.Offset as a raw client pointer (Managers.cpp's + // SyncClientSideAttributesForDrawArrays, kimi audit row 14). Under run-ahead those bytes + // are movable and the read is torn by construction, so the CLIENT refuses the draw by + // name before it is ever emitted (EmitTables.cpp, Fatal{UnmigratedVerb, + // "DrawArrays+CLIENT_ARRAYS"}); staging them as a record tail is P8's. + // + // What is left here is therefore reachable in exactly two situations, and both are safe: + // * monolith / the push-monolith build - one process, one thread, no wire; + // * a LOCKSTEP split session - the client is parked in WaitForApplied for + // this very record, so its memory is stable + // (rule F is scoped to unbarriered records, + // ruling 4). Escalation (ii) of §2.1 pins the + // same thing from the other side: a draw + // carrying kDrawClientArrays is BARRIERED. + // Making it monolith-only instead would silently drop the attribute upload of a + // lockstep split session, which is a wrong picture today - before ra has landed + // anything - so the refusal is the client's to raise, not this arm's to assume. + // + // The frontend VAO is taken ONLY when the applier's own vertex-buffer window says a + // client-sourced attribute exists, which is what retires the per-draw + // GetBoundVertexArray row of the ordinary (VBO-backed) DrawArrays - the strict lane's + // `GetBoundVertexArray@DrawArrays` marker - without touching this path's behaviour. + // `static` on purpose (G1): the pull build must not gain an exported name for a + // function that only re-homes two identical inline blocks the two DrawArrays entry + // points used to carry. + static void SyncClientSideVertexArraysForDrawArrays(GLint first, GLsizei count) { +#if MOBILEGL_BUILD_DISAGGREGATED + if (BufferImpl::VertexInputReadsRecords()) { + if (!BufferImpl::AnyClientSideVertexArrayInRecord()) return; + if (MG_Pipe::MGPipeApplierIsUnbarrieredApply()) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"MGPipeSlots\"} - a draw carrying a " + "client-memory vertex array is being applied UNBARRIERED. Both the " + "client's refusal (Fatal{UnmigratedVerb, \"DrawArrays+CLIENT_ARRAYS\"}) " + "and escalation (ii) of CONTRACT-P5E §2.1 exist to make this " + "unreachable; the bytes below are the application's and are moving"); + std::abort(); + } + auto* twin = ResolveVaoTwin(MG_Pipe::MGPipeApplier().BoundVertexElements); + if (twin == nullptr) return; + const auto& currentVAO = MGB_CTX->GetBoundVertexArray(); + if (!currentVAO) return; + twin->SyncClientSideAttributesForDrawArrays(currentVAO, first, count); + return; + } +#endif + const auto& currentVAO = MGB_CTX->GetBoundVertexArray(); + if (!currentVAO) return; + auto* backendVAOSlot = g_backendVertexArrayObjects.Find(currentVAO.get()); + if (backendVAOSlot && *backendVAOSlot) { + (*backendVAOSlot)->SyncClientSideAttributesForDrawArrays(currentVAO, first, count); + } + } + // GL: a shader input whose generic attribute array is DISABLED reads that attribute's *current // value* (context state set by glVertexAttrib*, default (0,0,0,1)) rather than any buffer. // MobileGL stores those values in MG_State only, so without this step the ES driver would feed @@ -1665,9 +2003,50 @@ namespace MobileGL::MG_Backend::DirectGLES { ZoneScopedC(TRACY_ZONECOLOR_BACKEND); #endif if (!program) return; + if (!vaoTwin) return; - const auto& vao = MGB_CTX->GetBoundVertexArray(); - if (!vao || !vaoTwin) return; +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi), CONTRACT-P5E §5.1: the bound-VAO row is retired here. On the record arm + // the twin IS the bound vertex-elements CSO (the caller resolved it from + // st.BoundVertexElements), so "is a VAO bound" is already answered by vaoTwin being + // non-null and GetBoundVertexArray() has nothing left to add. + // + // WHAT IS NOT RETIRED HERE, and it is S4/pg's not vi's: the two program reads below + // (GetActiveAttributeLocationMask, GetAttribType). They are frontend object rows of + // the PROGRAM family and stay until pg carries the attribute mask and types in the + // ShaderCso record. Until then this function is still a frontend reader on the + // apply thread - a BARRIERED one, which is legal (§4.4 / ruling 4) and is why vi + // does its half now rather than waiting: the VAO half is what this package owns and + // leaving it would keep GetBoundVertexArray alive for a reason that is not the + // program's. + const Bool fromRecords = BufferImpl::VertexInputReadsRecords(); + const MG_Pipe::MGPipeVertexElementsRecord* elementsRecord = nullptr; + MG_Pipe::MGPipeHandle elementsHandle = MG_Pipe::kMGPipeNullHandle; + Uint64 elementsSerial = 0; + if (fromRecords) { + const auto& st = MG_Pipe::MGPipeApplier(); + elementsHandle = st.BoundVertexElements; + if (!MG_Pipe::MGPipeHandleIsNull(elementsHandle) && + elementsHandle.Slot < st.VertexElementsCsos.size()) { + const auto& record = st.VertexElementsCsos[elementsHandle.Slot]; + if (record.Live && record.Gen == elementsHandle.Gen) { + elementsRecord = &record; + elementsSerial = record.ContentSerial; + } + } + // No record is "the configuration was never described": there is nothing to + // decide which locations lack an array from, and guessing would either + // re-issue every current value every draw or silently skip the ones that need + // it. The next create_vertex_elements re-opens the memo by serial. + if (elementsRecord == nullptr) return; + } +#else + constexpr Bool fromRecords = false; +#endif + + const SharedPtr noVao; + const auto& vao = fromRecords ? noVao : MGB_CTX->GetBoundVertexArray(); + if (!fromRecords && !vao) return; const Uint32 activeAttribMask = program->GetActiveAttributeLocationMask(); if (activeAttribMask == 0) return; @@ -1680,17 +2059,50 @@ namespace MobileGL::MG_Backend::DirectGLES { // cycled section VAOs, re-reading the cold attribute slots each time; here a // cycle re-hits every VAO's own entry. The rebuild visits only ACTIVE locations. auto& memo = vaoTwin->GetPendingAttribValueMaskMemo(); - const Uint32 configVersion = vao->GetConfigVersion(); - if (!memo.valid || configVersion != memo.configVersion || activeAttribMask != memo.activeMask) { - Uint32 pending = 0; - for (Uint32 remaining = activeAttribMask; remaining != 0; remaining &= remaining - 1) { - const Uint32 location = static_cast(std::countr_zero(remaining)); - if (!vao->GetAttribute(location).Enabled) pending |= (1u << location); +#if MOBILEGL_BUILD_DISAGGREGATED + if (fromRecords) { + // The re-keyed memo: {elementsHandle, ContentSerial, activeMask}. Every + // Enable/DisableVertexAttribArray moves the frontend configuration version, the + // client re-emits create_vertex_elements on the same handle for it, and the + // applier ++s ContentSerial - so this key opens exactly when the old one did + // and never wraps (Managers.h states the strictly-stronger argument). + if (!memo.valid || !(memo.elementsHandle == elementsHandle) || + memo.elementsSerial != elementsSerial || activeAttribMask != memo.activeMask) { + Uint32 pending = 0; + for (Uint32 remaining = activeAttribMask; remaining != 0; remaining &= remaining - 1) { + const Uint32 location = static_cast(std::countr_zero(remaining)); + // rec->Attributes[] IS what was last pushed for this configuration; the + // record carries all 32 slots and zeroes the tail + // (MGPipeApplyCreateVertexElements), so a location past AttributeCount + // reads Enabled = 0, which is the same answer the frontend's cold + // attribute slot gave. + if (location >= MG_Pipe::kMGPipeMaxVertexAttribs || + !elementsRecord->Attributes[location].Enabled) { + pending |= (1u << location); + } + } + memo.elementsHandle = elementsHandle; + memo.elementsSerial = elementsSerial; + memo.activeMask = activeAttribMask; + memo.pendingMask = pending; + memo.valid = true; + } + } else +#endif + { + const Uint32 configVersion = vao->GetConfigVersion(); + if (!memo.valid || configVersion != memo.configVersion || + activeAttribMask != memo.activeMask) { + Uint32 pending = 0; + for (Uint32 remaining = activeAttribMask; remaining != 0; remaining &= remaining - 1) { + const Uint32 location = static_cast(std::countr_zero(remaining)); + if (!vao->GetAttribute(location).Enabled) pending |= (1u << location); + } + memo.configVersion = configVersion; + memo.activeMask = activeAttribMask; + memo.pendingMask = pending; + memo.valid = true; } - memo.configVersion = configVersion; - memo.activeMask = activeAttribMask; - memo.pendingMask = pending; - memo.valid = true; } if (memo.pendingMask == 0) return; @@ -4767,12 +5179,31 @@ namespace MobileGL::MG_Backend::DirectGLES { // resolved-buffers memo on the twin), the VAO sync and the draw-time bind // below. Nothing in between can invalidate it — the bound VAO is pinned by // the context, and no step here erases or replaces a live VAO's twin. - const auto& currentVAO = MGB_CTX->GetBoundVertexArray(); - VertexArrayImpl::BackendVertexArrayObject* vaoTwin = - currentVAO ? VertexArrayImpl::ResolveVaoTwin(currentVAO) : nullptr; - // Early config-version read: see the note on SyncNeccessaryBuffers - issuing - // the (cold-line) load here overlaps its miss with the resolves below. - const Uint32 vaoConfigVersion = currentVAO ? currentVAO->GetConfigVersion() : 0; + // + // P5e (vi), CONTRACT-P5E §4.2 / §5.1: under a transport the twin is resolved from + // st.BoundVertexElements - THE SAME HANDLE the frontend probe used to mint, since + // EmitVertexElements acquires it off the VAO's own lifetime id - and neither the bound + // VAO row nor the configuration version is read at all. `currentVAO` stays null on that + // arm, which is what the two callees' record arms expect; the null twin case (no + // vertex-elements CSO bound) reaches the same BindBackendVAOId(0) below that a null + // frontend VAO always did. + const Bool vertexInputFromRecords = BufferImpl::VertexInputReadsRecords(); + const SharedPtr noFrontendVao; + const auto& currentVAO = vertexInputFromRecords ? noFrontendVao : MGB_CTX->GetBoundVertexArray(); + VertexArrayImpl::BackendVertexArrayObject* vaoTwin = nullptr; + Uint32 vaoConfigVersion = 0; +#if MOBILEGL_BUILD_DISAGGREGATED + if (vertexInputFromRecords) { + vaoTwin = VertexArrayImpl::ResolveVaoTwin(MG_Pipe::MGPipeApplier().BoundVertexElements); + } else +#endif + { + vaoTwin = currentVAO ? VertexArrayImpl::ResolveVaoTwin(currentVAO) : nullptr; + // Early config-version read: see the note on SyncNeccessaryBuffers - issuing + // the (cold-line) load here overlaps its miss with the resolves below. DEAD on the + // record arm: SyncNeccessaryBuffers only reads it in the legacy branch. + vaoConfigVersion = currentVAO ? currentVAO->GetConfigVersion() : 0; + } // One program resolve and one texture-key capture serve the whole draw, for // the same reason the twin resolve does: only frontend GL entry points move // either, and none can run inside this preparation. GetProgramForDraw is a @@ -4785,14 +5216,25 @@ namespace MobileGL::MG_Backend::DirectGLES { // counted by the callees that are instrumented; the rest is not counted (see // the inventory in PipeStats.cpp). MG_Util::PipeStats::AddCalls(MG_Util::PipeStats::CallClass::Draws, 1); - MG_Util::PipeStats::AddCalls(MG_Util::PipeStats::CallClass::AccessorCalls, 2); + // ONE on the record arm, not two: P5e (vi) retired the bound-VAO accessor call and + // the ledger has to say so, or the phase's own "accessor calls per draw" number + // would keep counting a read that no longer happens. + MG_Util::PipeStats::AddCalls(MG_Util::PipeStats::CallClass::AccessorCalls, + vertexInputFromRecords ? 1 : 2); } const TextureImpl::DrawTextureSyncKeys textureKeys = TextureImpl::CaptureDrawTextureSyncKeys(); BufferImpl::SyncNeccessaryBuffers(currentVAO, vaoTwin, vaoConfigVersion, syncBit & DrawSyncBit::IndexBuffer, syncBit & DrawSyncBit::IndirectBuffer); - VertexArrayImpl::SyncCurrentVAO(currentVAO, vaoTwin); +#if MOBILEGL_BUILD_DISAGGREGATED + if (vertexInputFromRecords) { + VertexArrayImpl::SyncCurrentVAOFromRecords(vaoTwin); + } else +#endif + { + VertexArrayImpl::SyncCurrentVAO(currentVAO, vaoTwin); + } TextureImpl::SyncNeccessaryTextures(textureKeys); // A draw reads and writes through its image units too, so the unit bindings have to be // as current as the sampled ones. Gated (see the sweep): a program with no image binding @@ -6444,8 +6886,34 @@ namespace MobileGL::MG_Backend::DirectGLES { return true; } + // MONOLITH GLUE, AND LOUD ABOUT IT AS OF P5e (vi). CONTRACT-P5E §5.1 makes the restart + // substitution read st.IndexBuffer.Res, and the constructor above already does: its + // transport arm resolves the element buffer and the bytes from the applier and its own + // staged shadow (P5c hd), and it sets serverElementBinding >= 0 BEFORE the null test, + // so the tail at BoundElementArrayBufferId() is unreachable with a live transport. + // These two are therefore only called from the monolith branch - and the refusal below + // is what keeps that a fact rather than a reading: a future caller that reaches them + // from the apply thread aborts by name instead of pulling GetBoundVertexArray behind + // the family's back. + void RefuseElementArrayBufferFromTheFrontend(const char* entry) { +#if MOBILEGL_BUILD_DISAGGREGATED + if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; + if (!BufferImpl::VertexInputReadsRecords()) return; + MGLOG_F("MGPipe: Fatal{RoleViolation, \"MGPipeSlots\"} - %s read the frontend VAO's " + "element-array slot with a live transport. The index buffer of this family is " + "MGPipeApplier().IndexBuffer.Res (CONTRACT-P5E §5.1); the frontend slot is " + "monolith glue and answers for whatever the CLIENT has bound now, not for the " + "record this draw is being applied from", + entry); + std::abort(); +#else + (void)entry; +#endif + } + const SharedPtr& BoundElementArrayBuffer() { static const SharedPtr none; + RefuseElementArrayBufferFromTheFrontend("BoundElementArrayBuffer"); const auto& vao = MGB_CTX->GetBoundVertexArray(); if (!vao) return none; return vao->GetIndexBufferBindingSlot().GetBoundObject(); @@ -6454,6 +6922,7 @@ namespace MobileGL::MG_Backend::DirectGLES { // The GL name PrepareForDraw left on GL_ELEMENT_ARRAY_BUFFER, i.e. what the // substitution has to put back. Uint BoundElementArrayBufferId() { + RefuseElementArrayBufferFromTheFrontend("BoundElementArrayBufferId"); const auto& ibo = BoundElementArrayBuffer(); if (!ibo) return 0; const auto* resource = BufferImpl::EnsureBufferResource(ibo); @@ -6721,18 +7190,7 @@ namespace MobileGL::MG_Backend::DirectGLES { #endif DrawSyncFlags syncBit = DrawSyncBit::None; PrepareForDraw(syncBit); - const auto& currentVAO = MGB_CTX->GetBoundVertexArray(); - if (currentVAO) { -#if MOBILEGL_BUILD_DISAGGREGATED - // P5c (G6, CONTRACT-P5C §5.4): frontend-keyed twin resolution, named debt inside - // the scope - P3b/P4b rekeys the registry onto handles. - const MG_Pipe::MGPipeFrontendKeyedRegistryScope frontendKeyedRegistry; -#endif - auto* backendVAOSlot = VertexArrayImpl::g_backendVertexArrayObjects.Find(currentVAO.get()); - if (backendVAOSlot && *backendVAOSlot) { - (*backendVAOSlot)->SyncClientSideAttributesForDrawArrays(currentVAO, first, count); - } - } + VertexArrayImpl::SyncClientSideVertexArraysForDrawArrays(first, count); ForEachViewportRoutingPass([&] { g_GLESFuncs.glDrawArrays(mode, first, count); }); @@ -6765,20 +7223,9 @@ namespace MobileGL::MG_Backend::DirectGLES { // ladder and the indirect executors do. Without it every sub-draw of a // glMultiDrawArrays read draw index 0. const Bool feedDrawID = CurrentProgramReadsDrawID(); - const auto& currentVAO = MGB_CTX->GetBoundVertexArray(); for (GLsizei i = 0; i < drawcount; ++i) { // Client-side arrays are uploaded per sub-draw range, like the single DrawArrays path. - if (currentVAO) { -#if MOBILEGL_BUILD_DISAGGREGATED - // P5c (G6, CONTRACT-P5C §5.4): frontend-keyed twin resolution, named debt - // inside the scope - P3b/P4b rekeys the registry onto handles. - const MG_Pipe::MGPipeFrontendKeyedRegistryScope frontendKeyedRegistry; -#endif - auto* backendVAOSlot = VertexArrayImpl::g_backendVertexArrayObjects.Find(currentVAO.get()); - if (backendVAOSlot && *backendVAOSlot) { - (*backendVAOSlot)->SyncClientSideAttributesForDrawArrays(currentVAO, first[i], count[i]); - } - } + VertexArrayImpl::SyncClientSideVertexArraysForDrawArrays(first[i], count[i]); if (feedDrawID) SetCurrentDrawID(static_cast(i)); ForEachViewportRoutingPass([&] { g_GLESFuncs.glDrawArrays(mode, first[i], count[i]); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 9e7b021b..8b06d455 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -2777,7 +2777,74 @@ namespace MobileGL::MG_Backend::DirectGLES { return g_bufferMutationEpoch.load(std::memory_order_acquire); } +#if MOBILEGL_BUILD_DISAGGREGATED + namespace { + // P5e (vi): how many times BumpBufferMutationEpoch ran off the apply thread while + // one was running, with a live transport. See the note in the bump itself; the + // accessor is what a test can assert zero on instead of reading a call graph. + std::atomic g_bufferMutationEpochBumpsOffTheApplyThread{0}; + } // namespace + + Uint64 BufferMutationEpochBumpsOffTheApplyThread() { + return g_bufferMutationEpochBumpsOffTheApplyThread.load(std::memory_order_relaxed); + } +#endif + void BumpBufferMutationEpoch() { +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi), CONTRACT-P5E §5.1's first pin (scout S1 R2, BRIEF-P5E's open question 2): + // "does any GL-THREAD path bump this under a transport". PINNED HERE RATHER THAN + // ARGUED, because the argument is the kind that is true until someone adds a caller. + // + // It matters because this counter is the ONE non-serial input left in the record + // arm's clean gate: SyncVaoAttributeBuffersByRecord stamps the pre-pass value and + // skips every IsBufferDrawCleanByHandle probe while the stamp holds. A bump from the + // GL thread would make that stamp a cross-thread read of a value the applier never + // observed - the memo would go on reading clean over buffers the client had just + // dirtied - and under run-ahead the client is a frame ahead, so the window is a + // frame wide rather than a record wide. + // + // THE MEASURED ANSWER, and it is not the clean "no" the scout expected: there is + // EXACTLY ONE class of non-apply-thread bump under a transport, and it happens + // BEFORE THERE IS AN APPLY THREAD AT ALL. BackendObject_DirectGLES::Initialize() + // runs on the app thread by design ("the backend object is BUILT on the app thread + // while its context is never OWNED by it", ServerLoop.h) and calls + // RegisterBufferBackendOps(), whose last act is a bump. An unguarded refusal aborts + // the whole split lane in EGL bring-up, which is how this was found rather than + // reasoned. + // + // That class is harmless and the apply-thread key tells it apart from the one that + // would not be: with g_applyThreadKey == 0 no record has been applied, so no twin + // exists, so no memo holds a stamp for the bump to slide under. + // + // WHAT IS COUNTED AND WHAT IS FATAL, and why they are not the same thing. Every + // bump taken off the apply thread WHILE ONE IS RUNNING is counted and named once, + // and is Fatal UNDER THE STRICT LANE - which is where this phase's pins live, and + // which is the lane the answer is claimed for. It is not Fatal by default because + // the unit fixtures that drive the production op tables directly + // (StagedShadowProductionTest / ServerLoopTest, which register the real tables and + // call them from the test thread beside a started loop) are the server role without + // being the apply thread, and taking those down would be this pin policing a + // fixture rather than the draw path. + const Uint64 applyThreadKey = + MG_Remote::Server::Detail::g_applyThreadKey.load(std::memory_order_relaxed); + if (MG_Config::Transport != MG_Config::TransportMode::Monolith && applyThreadKey != 0 && + !MG_Remote::Server::ServerLoop::OnApplyThread()) { + g_bufferMutationEpochBumpsOffTheApplyThread.fetch_add(1, std::memory_order_relaxed); + if (MG_Config::Ipc.StrictErrors) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"CurrentBufferMutationEpoch\"} - the " + "buffer-mutation epoch was bumped off the apply thread while an apply " + "thread was running, with a live transport. The draw-path memos stamp " + "this counter and skip their clean probes while the stamp holds " + "(CONTRACT-P5E §5.1), so a producer on the client's thread means the " + "backend is reachable from a thread that owns none of this state"); + std::abort(); + } + MGLOG_E_ONCE("MGPipe: the buffer-mutation epoch was bumped off the apply thread " + "with a live transport (CONTRACT-P5E §5.1's pin). Run the strict lane " + "to make this fatal at the producer"); + } +#endif g_bufferMutationEpoch.fetch_add(1, std::memory_order_release); } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index f0f426d8..ee24267a 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -789,6 +789,52 @@ namespace MobileGL::MG_Backend::DirectGLES { // AcquirePersistentMap or (FLUSH_EXPLICIT) publish only via FlushMappedRange. Uint64 CurrentBufferMutationEpoch(); void BumpBufferMutationEpoch(); +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (vi), CONTRACT-P5E §5.1's first pin: how many bumps came from a thread other than + // the apply thread WHILE an apply thread was running, under a live transport. The + // record arm's clean gate stamps this counter and skips its probes while the stamp + // holds, so "who may move it" is a question the phase has to answer with a number + // rather than with a reading of the call graph. Expected 0 for any session; the bump + // itself is Fatal on that path under the strict lane, and the answer at this head is + // that the only non-apply-thread bump in the tree happens at bring-up, BEFORE the apply + // thread exists (BackendObject_DirectGLES::Initialize's RegisterBufferBackendOps), and + // is therefore not counted here. + Uint64 BufferMutationEpochBumpsOffTheApplyThread(); + + // ---- P5e (vi): THE TWO SOURCE DECISIONS OF THE DRAW'S BUFFER SYNC ------------------ + // + // CONTRACT-P5E §5.1 is, for this family, two substitutions and nothing else: the + // attribute walk's buffer set comes from st.VertexBuffers[Start..+Count) instead of the + // frontend VAO's GetAllAttributes(), and the index buffer comes from st.IndexBuffer.Res + // + IndexBufferSerial instead of the VAO's element slot. Both are lifted out of + // SyncNeccessaryBuffers' arms and given names HERE, and the reason is the red-once: + // these take the applier state AND NOTHING ELSE, so the only way to revert the + // substitution is to put a frontend read back inside one of them, and + // MG_Test/SanityTest.cpp drives exactly these two with a frontend VAO deliberately + // pointing somewhere else. A test that drove a copy of the decision instead would stay + // green through that revert, which is the fake-green ID-102 names. + // + // The ensure, the memo and the clean probe stay where they are: they need an ES context + // and are the integration lane's to exercise. + struct DrawVertexBufferRequest { + MG_Pipe::MGPipeHandle Res = MG_Pipe::kMGPipeNullHandle; + // MGPVertexBuffer::BindingIndex, carried for the memo's diagnostics only. + Uint32 BindingIndex = 0; + }; + // The DISTINCT buffers the draw's enabled attributes fetch from, deduped on {slot, gen}. + // A null Res is skipped: it is a disabled slot below the window's high-water mark, or a + // client-memory array whose bytes have no store to ensure (P8 stages those). + Uint ResolveDrawVertexBuffersFromRecord(const MG_Pipe::MGPipeApplierState& st, + DrawVertexBufferRequest* out, Uint capacity); + + struct DrawIndexBufferRequest { + MG_Pipe::MGPipeHandle Res = MG_Pipe::kMGPipeNullHandle; + // IndexBufferSerial, which joins the identity compare because this arm has no live + // frontend slot to re-read - see ResolvedDrawBuffers::iboSerial. + Uint64 Serial = 0; + }; + DrawIndexBufferRequest ResolveDrawIndexBufferFromRecord(const MG_Pipe::MGPipeApplierState& st); +#endif // The DirectGLES storage behind one frontend buffer. Owned (refcounted) by // the frontend BufferObject; immediate BufferBackendOps keep it current, so @@ -1203,6 +1249,16 @@ namespace MobileGL::MG_Backend::DirectGLES { BackendVertexArrayObject(); ~BackendVertexArrayObject(); void SyncToBackend(const SharedPtr& stateVAOObject); +#if MOBILEGL_PIPE_PUSH + // PUBLIC AS OF P5e (vi), and the move is the point rather than a convenience: with + // a live transport there is no frontend VAO on this side to hand to the overload + // above, so VertexArrayImpl::SyncCurrentVAOFromRecords calls this ENTRY directly + // instead of passing a null SharedPtr into a function whose signature promises one. + // CONTRACT-P5E §4.1's rule ("the object parameter is deleted from the transport + // overload, not defaulted to null") is what this obeys. The body is unchanged and + // was already record-only - it is this family's existence proof (scout S1 §2). + void SyncToBackendFromApplier(); +#endif void SyncClientSideAttributesForDrawArrays( const SharedPtr& stateVAOObject, GLint first, GLsizei count); Uint GetBackendVertexArrayId() const { return m_backendVAOId; } @@ -1222,6 +1278,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // and is not covered by the config version). struct ResolvedDrawBuffers { struct Entry { + // MONOLITH GLUE AFTER P5e (vi), CONTRACT-P5E §5.1: the RECORD arm + // (SyncVaoAttributeBuffersByRecord) never writes this and never reads it - + // it stores nullptr and hands nullptr to IsBufferDrawCleanByHandle, whose + // frontend question is itself monolith-only (Managers.cpp's + // askTheObjectWhetherItIsMapped). It stays DECLARED because the legacy arm + // and the push-monolith arm still key on it and because moving it would + // move sizeof(ResolvedDrawBuffers) in the pull build (G1). That also + // retires, on the split arm, the dangling-pointer hazard the note below + // has to state. MG_State::GLState::BufferObject* frontend = nullptr; // A RAW TWIN POINTER, AND IT MAY DANGLE - the invariant that makes that safe // is stated here rather than left in the two callers (espryt-v3 §8, m8). @@ -1240,6 +1305,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // owning a reference from a per-draw memo - is what keeps a dead driver // buffer alive, which is the leak class P2's death notice exists to remove. BufferImpl::GLESBufferResource* resource = nullptr; + // P5e (vi): on the record arm this is the BINDING index the applier's + // vertex-buffer entry carried (MGPVertexBuffer::BindingIndex, which for + // Espryt's resolved attributes IS the attribute index) and it is kept for + // DIAGNOSTICS only - that arm's repair re-ensures by `handle`, never by + // walking back into a frontend attribute slot. Uint8 attribIndex = 0; #if MOBILEGL_PIPE_PUSH // P3a re-key: the entry's identity on the handle arm. A {slot, gen} cannot @@ -1266,6 +1336,18 @@ namespace MobileGL::MG_Backend::DirectGLES { Uint64 elementsSerial = 0; Uint64 buffersSerial = 0; MG_Pipe::MGPipeHandle iboHandle = MG_Pipe::kMGPipeNullHandle; + // P5e (vi), CONTRACT-P5E §5.1: the index slot's own server-owned serial, and + // the reason it has to join the identity compare is the one D5 gives for the + // slot version - the ELEMENT BUFFER IS NOT PART OF THE CONFIGURATION. The + // legacy and push-monolith arms re-read the frontend slot on every indexed + // draw and so notice a rebind for free; the record arm reads nothing, so + // "the applier was told about this index buffer again" has to be a value in + // the key. A rebind of the SAME handle still moves IndexBufferSerial + // (MGPipeApplySetIndexBuffer bumps unconditionally), which is exactly the + // case an identity-only compare would call a hit: a client that respecified + // the store behind an unchanged {slot, gen} re-emits set_index_buffer and + // this is what re-opens the ensure. + Uint64 iboSerial = 0; #endif // Buffer-mutation epoch (BufferImpl::CurrentBufferMutationEpoch) at which // the LAST probe pass found every entry / the IBO clean; 0 = not stamped @@ -1290,6 +1372,18 @@ namespace MobileGL::MG_Backend::DirectGLES { Uint32 configVersion = 0; Uint32 activeMask = 0; Uint32 pendingMask = 0; +#if MOBILEGL_PIPE_PUSH + // P5e (vi), CONTRACT-P5E §5.1: the record arm's half of the key. It replaces + // the frontend VAO's configuration version with the bound vertex-elements CSO's + // identity and its server-owned content serial, and the substitution is + // STRICTLY STRONGER rather than merely equivalent: GetConfigVersion() is a + // wrapping Uint16-derived counter shared by every VAO, while every + // configuration change re-creates the record on the same handle and ++s + // ContentSerial (PipeApply.h's MGPipeVertexElementsRecord), and a DIFFERENT VAO + // is a different {slot, gen} rather than another value of the same counter. + MG_Pipe::MGPipeHandle elementsHandle = MG_Pipe::kMGPipeNullHandle; + Uint64 elementsSerial = 0; +#endif }; PendingAttribValueMask& GetPendingAttribValueMaskMemo() { return m_pendingAttribValueMask; } @@ -1312,8 +1406,8 @@ namespace MobileGL::MG_Backend::DirectGLES { // the applier's records - the bound CSO's two views, the vertex-buffer set, the // index buffer and the resolved fetch base instance - so it takes no argument at // all and touches no frontend type. The legacy arm above it is unchanged and both - // compile in every push build (ARCHITECTURE.md 9.6). - void SyncToBackendFromApplier(); + // compile in every push build (ARCHITECTURE.md 9.6). DECLARED IN THE PUBLIC SECTION + // as of P5e (vi) - see the note there. // Same narrowing, same memo, same Adreno disable; the source bytes are the shadow // base the resource call carried and the memo key is the buffer's {slot, gen}. Bool SyncFloat64AttributeAsFloat32ByHandle(Uint attribIndex, const MGPVertexAttribWire& attrib, diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 06974e5e..2671b81c 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -77,6 +77,7 @@ add_executable(MobileGLIntegrationTest Scenarios/PixelStoreSweepScenario.cpp Scenarios/PrimitiveRestartScenario.cpp Scenarios/FragCoordOriginScenario.cpp + Scenarios/ClientVertexArrayScenario.cpp Scenarios/ClearThenReadPixelsScenario.cpp Scenarios/SampleVariablesScenario.cpp Scenarios/DepthStencilReadbackScenario.cpp @@ -2069,6 +2070,26 @@ if (MOBILEGL_BUILD_DISAGGREGATED) ENVIRONMENT "${MGL_ITEST_GLES_SPLIT_ENVIRONMENT}" ) + # ---- P5e package vi (MG_Remote/CONTRACT-P5E.md §5.1, ID-82 / ruling 2) -------------------- + # + # THE LANE ENTRY THAT DID NOT EXIST, and its absence was the brief's open item (a). Nothing + # else registered here draws from a vertex array in the APPLICATION's own memory: every + # other scenario binds GL_ARRAY_BUFFER first, so its glVertexAttribPointer argument is a + # byte offset. This is the one draw shape whose bytes have no wire form - the server uploads + # them by dereferencing a client pointer per draw - and it is therefore both the positive + # control for vi's decision to KEEP that arm alive under lockstep and the only entry that + # can reach the run-ahead refusal when ra publishes the caps bit. + gtest_discover_tests(MobileGLIntegrationTest + TEST_PREFIX "DirectGLES.Split." + TEST_LIST MGL_SPLIT_VI_CLIENT_ARRAY_TESTS + TEST_FILTER "ClientVertexArrayScenario.*" + DISCOVERY_TIMEOUT 30 + PROPERTIES + LABELS "integration-gpu\;integration-split" + TIMEOUT ${MGL_ITEST_TIMEOUT} + ENVIRONMENT "${MGL_ITEST_GLES_SPLIT_ENVIRONMENT}" + ) + gtest_discover_tests(MobileGLIntegrationTest TEST_PREFIX "DirectGLES.Split." TEST_LIST MGL_SPLIT_SYNC_TESTS diff --git a/MobileGL/MG_IntegrationTest/Harness/SplitLogPaths.cmake.in b/MobileGL/MG_IntegrationTest/Harness/SplitLogPaths.cmake.in index ca76b5c5..3bc1c1f6 100644 --- a/MobileGL/MG_IntegrationTest/Harness/SplitLogPaths.cmake.in +++ b/MobileGL/MG_IntegrationTest/Harness/SplitLogPaths.cmake.in @@ -3,8 +3,12 @@ file(MAKE_DIRECTORY "@CMAKE_CURRENT_BINARY_DIR@/split-logs") # P5b t2's two lanes ride the same rule: one private log path per entry, or # SplitLogPaths.PrivateAndDistinct is red for them (ID-53). +# P5e vi's client-array lane joins the same list for the same reason: its two entries are the +# only ones that can reach Fatal{UnmigratedVerb, "DrawArrays+CLIENT_ARRAYS"} once ra publishes +# the caps bit, and a shared log file is a marker nobody can attribute. foreach(entry IN LISTS MGL_SPLIT_CLEAR_TESTS MGL_SPLIT_TRIANGLE_TESTS MGL_SPLIT_PMAP_TESTS - MGL_SPLIT_T2_TESS_TESTS MGL_SPLIT_T2_XFB_TESTS MGL_SPLIT_SYNC_TESTS) + MGL_SPLIT_T2_TESS_TESTS MGL_SPLIT_T2_XFB_TESTS MGL_SPLIT_SYNC_TESTS + MGL_SPLIT_VI_CLIENT_ARRAY_TESTS) set_tests_properties("${entry}" PROPERTIES ENVIRONMENT "MOBILEGL_LOG_FILE_PATH=@CMAKE_CURRENT_BINARY_DIR@/split-logs/${entry}.log") endforeach() diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ClientVertexArrayScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ClientVertexArrayScenario.cpp new file mode 100644 index 00000000..ac615a74 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/ClientVertexArrayScenario.cpp @@ -0,0 +1,189 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/ClientVertexArrayScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - A VERTEX ARRAY THAT LIVES IN THE APPLICATION'S OWN MEMORY, DRAWN OVER THE WIRE. +// +// WHY THIS FILE EXISTS AT ALL (P5e vi, ID-82 / BRIEF-P5E §6 open item (a)). The brief asked +// whether any existing scenario exercises a client-memory vertex array under split, and named +// DoublePrecisionScenario as the candidate. IT DOES NOT, and nothing else does either: every +// glVertexAttribPointer in this directory is issued with a buffer bound to GL_ARRAY_BUFFER, so +// its last argument is a byte OFFSET and not a host pointer (DoublePrecisionScenario uses the +// binding-model calls against real buffer objects, DoublePrecisionScenario.cpp:944-959). So the +// one draw shape whose bytes have no wire form was, until this file, never drawn over the wire +// in any lane. +// +// WHAT IT PINS, in the two arms it can be in: +// +// TODAY (lockstep, and the monolith control) the draw is NOT refused - ruling 4: the client +// is parked in WaitForApplied for exactly this record, so the server's per-draw upload of the +// application's bytes (Managers.cpp's SyncClientSideAttributesForDrawArrays, the kimi audit's +// row 14) reads memory that is not moving. The assertion is therefore about PIXELS: the quad +// has to arrive. That is what makes vi's "monolith-only would have been wrong here" concrete +// rather than an argument in a report - delete the split arm of the upload and this goes red. +// +// UNDER RUN-AHEAD it is REFUSED BY NAME on the GL thread, Fatal{UnmigratedVerb, +// "DrawArrays+CLIENT_ARRAYS"} (CONTRACT-P5E §5.1), because the client is a frame ahead and +// those bytes are moving under the reader. This file is the red-once for that refusal: it is +// the only lane entry that can reach it. Staging the bytes as a per-attribute {BindingIndex, +// MGHostSpan} tail - the shape kDrawHasUserIndices already has for client INDICES - is P8's, +// and retires the refusal along with this scenario's second half. +// +// Both draw entry points that carry the upload are driven, because they are two arms and not +// one: DrawArrays uploads the single range, MultiDrawArrays uploads one range per sub-draw. + +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr const char* kVS = R"(#version 330 core +in vec2 aPos; +void main() { gl_Position = vec4(aPos, 0.0, 1.0); } +)"; + + constexpr const char* kFS = R"(#version 330 core +out vec4 o_color; +void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); } +)"; + + class ClientVertexArrayScenario : public ScenarioTest {}; + + // THE WHOLE POINT IS THE ABSENCE OF A BUFFER. glBindBuffer(GL_ARRAY_BUFFER, 0) before + // glVertexAttribPointer makes the last argument a HOST POINTER rather than an offset, + // which is the one vertex source EmitVertexBuffers publishes as Res == + // kMGPipeNullHandle - "not a hole: it is exactly how the server learns this attribute + // is client-sourced, upload it yourself" (VertexInputEmit.h). + // + // A VAO is still bound, because ES core requires one and because the server's upload + // hangs off the VAO's twin. + struct ClientArrayQuad { + GLuint vao = 0; + const float* vertices = nullptr; + + void Bind(const float* quad) { + vertices = quad; + glGenVertexArrays(1, &vao); + glBindVertexArray(vao); + glBindBuffer(GL_ARRAY_BUFFER, 0); + glEnableVertexAttribArray(0); + glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, 2 * sizeof(float), vertices); + } + void Release() { + glBindVertexArray(0); + if (vao != 0) glDeleteVertexArrays(1, &vao); + vao = 0; + } + }; + + // Full-viewport strip, so "did the draw arrive" is one pixel read rather than a shape + // comparison - the claim is about the vertex SOURCE, not about rasterisation. + const float kQuad[] = {-1.0f, -1.0f, 1.0f, -1.0f, -1.0f, 1.0f, 1.0f, 1.0f}; + + Rgba8 CentreAfter(int width, int height) { + const Image image = ReadPixelsRect(0, 0, width, height); + return image.At(width / 2, height / 2); + } + + } // namespace + + TEST_F(ClientVertexArrayScenario, AClientMemoryVertexArrayReachesTheDrawItFeeds) { + if (!Ready()) return; + HeadlessGL& gl = Gl(); + const int width = gl.Width(); + const int height = gl.Height(); + + std::string error; + const unsigned int program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(program, 0u) << error; + + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + ClearTo(0.0f, 0.0f, 1.0f, 1.0f); + + ClientArrayQuad quad; + quad.Bind(kQuad); + glUseProgram(program); + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + + const Rgba8 centre = CentreAfter(width, height); + EXPECT_GT(static_cast(centre.g), 200) + << "the client-memory vertex array never reached the draw: the centre is " << centre + << ", i.e. still the clear colour. Under split this means the per-draw upload of " + "the application's own bytes did not run - the arm CONTRACT-P5E §5.1 keeps alive " + "for lockstep, and refuses only under run-ahead"; + EXPECT_LT(static_cast(centre.b), 60) + << "the quad drew, but the clear colour is still showing through: " << centre; + + quad.Release(); + glUseProgram(0); + glDeleteProgram(program); + EXPECT_EQ(FirstGLError(), 0u); + } + + TEST_F(ClientVertexArrayScenario, EverySubDrawOfAMultiDrawArraysGetsItsOwnClientRange) { + if (!Ready()) return; + HeadlessGL& gl = Gl(); + const int width = gl.Width(); + const int height = gl.Height(); + + std::string error; + const unsigned int program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(program, 0u) << error; + + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + ClearTo(0.0f, 0.0f, 1.0f, 1.0f); + + // Two strips out of one client array: the left half and the right half, so a + // MultiDrawArrays that uploaded only the FIRST sub-draw's range leaves one side blue. + static const float kTwoHalves[] = { + -1.0f, -1.0f, 0.0f, -1.0f, -1.0f, 1.0f, 0.0f, 1.0f, + 0.0f, -1.0f, 1.0f, -1.0f, 0.0f, 1.0f, 1.0f, 1.0f, + }; + ClientArrayQuad quad; + quad.Bind(kTwoHalves); + glUseProgram(program); + + const GLint firsts[2] = {0, 4}; + const GLsizei counts[2] = {4, 4}; + glMultiDrawArrays(GL_TRIANGLE_STRIP, firsts, counts, 2); + + const Image image = ReadPixelsRect(0, 0, width, height); + const Rgba8 left = image.At(width / 4, height / 2); + const Rgba8 right = image.At((3 * width) / 4, height / 2); + EXPECT_GT(static_cast(left.g), 200) + << "the FIRST sub-draw's client range did not reach the draw: " << left; + EXPECT_GT(static_cast(right.g), 200) + << "the SECOND sub-draw's client range did not reach the draw (" << right + << "): an upload that ran once for the whole call instead of once per sub-draw " + "leaves exactly this half unpainted"; + + quad.Release(); + glUseProgram(0); + glDeleteProgram(program); + EXPECT_EQ(FirstGLError(), 0u); + } + +} // namespace MGITest diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index ac5dc9e4..1d28e23c 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -382,8 +382,39 @@ namespace MobileGL::MG_Remote::Client { nullptr, 0, nullptr, 0, nullptr); } + // P5e (vi), ID-82: declared here because the REDUCED DrawArrays path below predates the + // nineteen-slot block that defines them, and it needs the same two answers. One + // definition each, further down, beside the rest of the draw plan. + [[noreturn]] void RefuseDrawByName(const char* slot, const char* qualifier); + Bool RunAheadWouldSkipTheWait(ClientSession& session); + + // Does the bound VAO fetch any ENABLED attribute out of the application's own memory. + // The test is EmitVertexBuffers' own (`attrib.Enabled && !attrib.Buffer` is exactly what + // it publishes as Res == kMGPipeNullHandle), so the flag on the wire and the record the + // server applies agree by construction. + Bool BoundVaoHasClientVertexArrays(MG_State::GLState::GLContext* ctx) { + if (ctx == nullptr) return false; + const auto& vao = ctx->GetBoundVertexArray(); + if (!vao) return false; + const auto& attributes = vao->GetAllAttributes(); + for (SizeT i = 0; i < attributes.size(); ++i) { + if (attributes[i].Enabled && !attributes[i].Buffer) return true; + } + return false; + } + void EmitDrawArrays(GLenum mode, GLint first, GLsizei count) { ClientSession& session = RequireSession("DrawArrays"); + + // P5e (vi): the same refusal the nineteen-slot block raises in EmitDrawRecord, and + // this is the entry point the contract names by name - "DrawArrays+CLIENT_ARRAYS". + // The probe runs only on this path's own draw, before the hooks, and it is inert + // under lockstep (see RunAheadWouldSkipTheWait). + const Bool clientArrays = BoundVaoHasClientVertexArrays(MG_State::pGLContext.get()); + if (clientArrays && RunAheadWouldSkipTheWait(session)) { + RefuseDrawByName("DrawArrays", "CLIENT_ARRAYS"); + } + BeforeDrawVerb(); if (g_dropDrawEmission) { @@ -399,7 +430,10 @@ namespace MobileGL::MG_Remote::Client { MG_Pipe::MGPDrawInfo info{}; info.Mode = static_cast(mode); info.IndexSize = 0; // arrays - info.Flags = 0; // NO kDrawHasUserIndices: the reduced path draws from a VBO + // NO kDrawHasUserIndices: the reduced path draws from a VBO. kDrawClientArrays IS + // set when one is present, because the wait rule is computed from the record on + // both sides and this path publishes its own record rather than PlanDrawInfo's. + info.Flags = clientArrays ? static_cast(MG_Pipe::kDrawClientArrays) : 0; info.InstanceCount = 1; info.StartInstance = 0; info.RestartIndex = 0; @@ -446,6 +480,27 @@ namespace MobileGL::MG_Remote::Client { // indices (or past 2^32 of them): the record spells Start in // indices, and rounding would draw from the wrong element. + // P5e (vi), ID-82 / CONTRACT-P5E §5.1: "would this session publish a draw and move on". + // + // IT IS THE CONJUNCTION MGPipeTypes.h STATES FOR RunAheadArmed(), spelled here rather + // than called, and that is a SEAM, not a duplication: ClientSession::RunAheadArmed() is + // package ra's to land (it latches the same three at the first caps adoption after Start + // and may afterwards only turn OFF). Until it exists this is the same answer computed + // from the same three inputs, and it is inert for the whole phase because the third one + // - the server's kCapRunAheadApply - is not published until the integration commit + // (kMGPipeP5eRunAheadReady). WHEN ra LANDS, THIS BODY BECOMES `session.RunAheadArmed()`: + // one latch, one answer, and no way for the refusal to disagree with the wait rule about + // which arm the session is on. + Bool RunAheadWouldSkipTheWait(ClientSession& session) { +#if MOBILEGL_BUILD_DISAGGREGATED + return session.BarrierArmed() && MG_Config::Ipc.RunAhead != 0 && + session.Caps().HasCap(MG_Pipe::kCapRunAheadApply); +#else + (void)session; + return false; +#endif + } + [[noreturn]] void RefuseDrawByName(const char* slot, const char* qualifier) { char name[96]; std::snprintf(name, sizeof(name), "%s+%s", slot, qualifier); @@ -465,6 +520,19 @@ namespace MobileGL::MG_Remote::Client { b.ElementBufferBound = true; b.ElementBuffer = MG_Pipe::MGPipeResourceTrackerInstance().Find(*bound); } + // P5e (vi): the client-array probe, in the SAME single read of the bindings the + // rest of the plan is made from rather than in a second walk at the refusal - + // this runs on the GL thread once per draw and the refusal must not add its own + // frontend pass. The test is the emitter's own: EmitVertexBuffers publishes + // Res == kMGPipeNullHandle for exactly `attrib.Enabled && !attrib.Buffer`, so + // the flag and the record agree by construction instead of by inspection. + const auto& attributes = vao->GetAllAttributes(); + for (SizeT i = 0; i < attributes.size(); ++i) { + if (attributes[i].Enabled && !attributes[i].Buffer) { + b.ClientVertexArrays = true; + break; + } + } } if (const auto& di = ctx->GetBufferBindingSlot(::MobileGL::BufferTarget::DrawIndirect).GetBoundObject()) { b.DrawIndirectBuffer = MG_Pipe::MGPipeResourceTrackerInstance().Find(*di); @@ -486,6 +554,24 @@ namespace MobileGL::MG_Remote::Client { const void* clientIndices, Uint64 clientIndexBytes, const MG_Pipe::MGPDrawIndirect* indirect) { ClientSession& session = RequireSession(slot); + + // P5e (vi), ID-82 / ruling 2, BEFORE the pre-verb hooks and before the record: a + // draw that fetches vertices out of the application's own memory has no wire form, + // and under run-ahead the server would dereference a client pointer into memory + // this thread is already past. It is refused BY NAME on the GL thread, in the + // shape ID-57 gives every unmigrated draw shape, rather than rendered wrong (R-4). + // + // Under LOCKSTEP it is not refused and nothing here changes: the client is parked + // in WaitForApplied for exactly this record, so the server's upload reads memory + // that is not moving - which is ruling 4's "barriered records keep P5C's + // semantics", and is why this is a run-ahead gate and not a transport gate. + // Staging the bytes as a per-attribute {BindingIndex, MGHostSpan} tail, shaped like + // kDrawHasUserIndices, is P8's and retires the refusal with it. + if ((info.Flags & static_cast(MG_Pipe::kDrawClientArrays)) != 0 && + RunAheadWouldSkipTheWait(session)) { + RefuseDrawByName(slot, "CLIENT_ARRAYS"); + } + BeforeDrawVerb(); if (g_dropDrawEmission) { @@ -1853,6 +1939,13 @@ namespace MobileGL::MG_Remote::Client { // on its side). kDrawHasUserIndices / kDrawIsIndirect are added by the emission itself, // kDrawHasIndexRange by the two DrawRangeElements* callers. info.Flags = bindings.PrimitiveRestart ? static_cast(MG_Pipe::kDrawPrimitiveRestart) : 0; + // P5e (vi), CONTRACT-P5E §2.1 (ii) / §5.1. On the wire BECAUSE the server needs it: the + // barriered predicate is computed identically by both roles from the record alone, and + // "does this draw fetch from client memory" is not derivable from anything else in it - + // a null Res inside the vertex-buffer window is also what a DISABLED attribute below + // the high-water mark publishes. Set for every draw shape, not just the array ones: a + // glDrawElements can fetch its VERTICES from client memory too. + if (bindings.ClientVertexArrays) info.Flags |= static_cast(MG_Pipe::kDrawClientArrays); // A negative count has already been refused by the frontend (INVALID_VALUE); 0 crosses // as 0 and draws nothing, which is what the driver does with it. info.InstanceCount = instanceCount > 0 ? static_cast(instanceCount) : 0u; diff --git a/MobileGL/MG_Remote/Client/EmitTables.h b/MobileGL/MG_Remote/Client/EmitTables.h index f076c2aa..45707409 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.h +++ b/MobileGL/MG_Remote/Client/EmitTables.h @@ -187,6 +187,16 @@ namespace MobileGL::MG_Remote::Client { // carried verbatim (informational in P5b: the backend reads its own barrier-pulled copy). Bool PrimitiveRestart = false; Uint32 RestartIndex = 0; + // P5e (vi), ID-82 / CONTRACT-P5E §5.1: TRUE when the bound VAO has at least one ENABLED + // attribute with no buffer object behind it, i.e. an array whose vertices live in the + // application's own memory. The server has no such memory: today it dereferences + // `attrib.Offset` as a raw client pointer from the apply thread + // (Managers.cpp's SyncClientSideAttributesForDrawArrays), which is legal only while the + // client is parked behind the record. It rides in MGPDrawInfo::Flags as kDrawClientArrays + // so BOTH roles can decide from the wire - the client refuses such a draw under + // run-ahead, and MGPipeBarriered's escalation (ii) keeps it barriered if one ever + // arrives anyway. Staging the bytes is P8's. + Bool ClientVertexArrays = false; }; // 1 / 2 / 4 for the three GL index types, 0 for anything else (the frontend has already diff --git a/MobileGL/MG_Test/Pipe/VertexInputEmitTest.cpp b/MobileGL/MG_Test/Pipe/VertexInputEmitTest.cpp index b826465a..e1ac461f 100644 --- a/MobileGL/MG_Test/Pipe/VertexInputEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/VertexInputEmitTest.cpp @@ -129,6 +129,7 @@ namespace { X(VertexInputEmit, IsLongAndFloat64TravelSeparately) \ X(VertexInputEmit, ABaseInstanceChangeAloneStillEmitsTheVertexBufferSet) \ X(VertexInputEmit, AnUnchangedSetWithAnUnchangedBaseInstanceEmitsNothing) \ + X(VertexInputEmit, TheVertexBufferWindowCoversEveryAttributeTheCsoDeclaresEnabled) \ X(VertexInputEmit, RebindingTheSameVaoEmitsABindAndNoCreate) \ X(VertexInputEmit, PingPongingBetweenTwoVaosNeverRecreatesEither) \ X(VertexInputEmit, DestroyedVertexArraysReturnTheirCsoSlotsAndRecords) \ @@ -433,6 +434,78 @@ namespace { EXPECT_EQ(Emitter().VertexBufferSetCount(), 2u); } + // ==================== P5e (vi), ID-95 / ruling 19 / CONTRACT-P5E §5.3 ==================== + // + // THE WINDOW RULE, PINNED RATHER THAN ARGUED: set_vertex_buffers' window must COVER every + // attribute the vertex-elements CSO declares ENABLED. + // + // Why it needs a case at all (scout S1's caveat R4). The two views are emitted from the + // same validate step and both walk the same `Enabled` bits, so today they cannot disagree - + // which is exactly the shape of unpinned invariant that a later narrowing turns into a + // silent wrong picture. An attribute the RECORD says is enabled but that falls outside the + // window resolves VertexBufferForAttributeIndex == nullptr on the server and is SKIPPED at + // the attribute walk (Managers.cpp): the array stays enabled in the driver VAO with no + // buffer bound under it, which is not a refusal and not a black triangle, it is whatever + // that attribute last pointed at. + // + // The oracle is the RECORD, not the emitter's arithmetic: whatever create_vertex_elements + // declared enabled must be inside [Start, Start+Count). Sparse on purpose - a hole at 1..6 + // is what distinguishes "the window covers the enabled set" from "the window happens to be + // as long as the attribute array". + // + // The sink half is CheckUnitWindows (tx2's, called at every draw/dispatch), whose verdict + // for a narrow window is Fatal{ProtocolCorruption, "SetVertexBuffers.Count"}. This case is + // the PRODUCER half and is the one that goes red if the emitter narrows. + TEST(VertexInputEmit, TheVertexBufferWindowCoversEveryAttributeTheCsoDeclaresEnabled) { + EmitterScope scope; + const SharedPtr vao = MakeVao(1); + const SharedPtr buffer = Ctx().CreateBufferObject(1); + buffer->Respecify(4096, nullptr); + + // Enabled: 0 and 7. Disabled: everything between, and everything above. + const Uint kEnabled[] = {0u, 7u}; + for (Uint index : kEnabled) { + vao->SetAttributeFormat(index, 4, DataType::Float32, false, 16, 0, false); + vao->BindAttributeBuffer(index, buffer); + vao->EnableAttribute(index); + } + + ASSERT_GT(Emitter().EmitVertexElements(Ctx()), 0u); + ASSERT_GT(Emitter().EmitVertexBuffers(Ctx(), 0), 0u); + + const MGPVertexBuffers& window = Emitter().LastVertexBuffers(); + const auto& declared = Emitter().LastAttributes(); + const Uint32 declaredCount = Emitter().LastElements().AttributeCount; + + Uint32 covered = 0; + for (Uint32 i = 0; i < declaredCount; ++i) { + if (!declared[i].Enabled) continue; + ++covered; + SCOPED_TRACE(::testing::Message() << "attribute " << i); + EXPECT_GE(i, window.Start) + << "create_vertex_elements declares attribute " << i + << " ENABLED but set_vertex_buffers' window starts at " << window.Start + << ". The backend resolves no binding for it and SKIPS it, leaving the driver " + "array enabled over whatever it last pointed at (ID-95 / ruling 19)"; + EXPECT_LT(i, window.Start + window.Count) + << "create_vertex_elements declares attribute " << i + << " ENABLED but set_vertex_buffers' window ends at " << (window.Start + window.Count) + << " - the same skip, at the other end. A narrower window is the sink's " + "Fatal{ProtocolCorruption, \"SetVertexBuffers.Count\"}"; + } + ASSERT_EQ(covered, 2u) + << "the record did not declare the two attributes this case enabled, so the window " + "check above had nothing to be about"; + + // And the window's own entries agree with the record about WHICH attribute each is, so + // "covered" cannot be satisfied by a window of the right LENGTH over the wrong indices. + for (Uint32 i = 0; i < window.Count; ++i) { + EXPECT_EQ(Emitter().LastEntries()[i].BindingIndex, window.Start + i) + << "entry " << i << " of the window does not name attribute " << (window.Start + i) + << "; the server indexes this window BY ATTRIBUTE"; + } + } + // D-G3's per-handle latch. create_vertex_elements is re-issued on the SAME handle when a // configuration moves, and the latch is stored per handle rather than globally so that // rebinding cannot look like a configuration change. diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 80beaa1a..9c25a928 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -5004,6 +5004,168 @@ TEST(DirectGLESSlotTable, ACompositeHandleDoesNotGrowTheOrdinaryTable) { EXPECT_EQ(table.CompositeCapacityForTest(), 4u) << "the recycle re-grew the band"; } +// ========================================================================================== +// P5e (vi), CONTRACT-P5E §5.1: THE DRAW'S BUFFERS COME FROM THE RECORD, NOT FROM THE VAO. +// ========================================================================================== +// +// The two cases below are BRIEF-P5E's red-onces (2) and (3) for this package, and they are +// built the way ID-102 says a red-once has to be: the thing under test is THE PRODUCTION +// DECISION ITSELF (BufferImpl::ResolveDrawVertexBuffersFromRecord and +// ResolveDrawIndexBufferFromRecord, which are the only enumeration +// SyncNeccessaryBuffers' record arm has), not a copy of it written here. Both take the applier +// state and NOTHING ELSE, so the only way to revert §5.1's substitution is to put a frontend +// read back inside one of them - and then these go red naming the field. +// +// THE SETUP IS THE DIVERGENCE ITSELF: the frontend VAO is pointed at a buffer the RECORD does +// not name. That state is unreachable in one process today (the client re-emits at every +// validate point, so the two always agree); under run-ahead it is the ordinary state of the +// world, because the client has moved on by the time the record is applied. Producing it here +// by hand is the only way to ask "which one did you read" at all. +// +// WHAT IS NOT HERE, and deliberately: the ensure, the memo and the clean probe. They need an ES +// context and a driver, and DirectGLES.Split.* is where they are exercised +// (ClientVertexArrayScenario, TriangleScenario, IndexedDrawFamilyScenario and the rest of the +// lane draw through exactly this arm). +TEST(DirectGLESVertexInputDraw, TheAttributeWalkTakesItsBuffersFromTheRecordNotTheFrontendVao) { + using namespace MobileGL; + using namespace MobileGL::MG_Backend::DirectGLES; + using namespace MobileGL::MG_State::GLState; +#if !MOBILEGL_BUILD_DISAGGREGATED + GTEST_SKIP() << "the record arm of the attribute walk is compiled only under " + "MOBILEGL_BUILD_DISAGGREGATED"; +#else + // A, B and C, with handles minted the way the client mints them. + auto bufferA = MakeShared(0u); + auto bufferB = MakeShared(0u); + auto bufferC = MakeShared(0u); + const MG_Pipe::MGPipeHandle a = + MG_Pipe::MGPipeSlots().Acquire(MG_Pipe::MGPipeKind::Buffer, bufferA->GetLifetimeId()); + const MG_Pipe::MGPipeHandle b = + MG_Pipe::MGPipeSlots().Acquire(MG_Pipe::MGPipeKind::Buffer, bufferB->GetLifetimeId()); + const MG_Pipe::MGPipeHandle c = + MG_Pipe::MGPipeSlots().Acquire(MG_Pipe::MGPipeKind::Buffer, bufferC->GetLifetimeId()); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(a)); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(b)); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(c)); + + // THE PUBLISHED STATE: attribute 0 fetches from A, attribute 1 from B, attribute 2 from A + // again (the dedupe), attribute 3 is a client-memory array (null Res, nothing to ensure). + MG_Pipe::MGPipeApplierState& st = MG_Pipe::MGPipeApplier(); + const MG_Pipe::MGPipeApplierState saved = st; + st.VertexBufferStart = 0; + st.VertexBufferCount = 4; + for (Uint32 i = 0; i < 4; ++i) { + st.VertexBuffers[i] = MG_Pipe::MGPVertexBuffer{}; + st.VertexBuffers[i].BindingIndex = i; + } + st.VertexBuffers[0].Res = a; + st.VertexBuffers[1].Res = b; + st.VertexBuffers[2].Res = a; + st.VertexBuffers[3].Res = MG_Pipe::kMGPipeNullHandle; + + // THE FRONTEND, MOVED ON: every enabled attribute now points at C, and nothing was + // re-emitted. A walk that read GetAllAttributes() answers {C}. The VAO is BOUND IN A REAL + // CONTEXT rather than free-standing, because that is the only shape in which the revert + // this case exists to catch - MGB_CTX->GetBoundVertexArray()->GetAllAttributes() - has + // anything to read at all; a free-standing object would make the revert crash instead of + // disagree, which is a red for the wrong reason. + UniquePtr previousContext = Move(MG_State::pGLContext); + MG_State::pGLContext = MakeUnique(); + MG_State::pGLContext->CreateVertexArrayObject(1); + MG_State::pGLContext->BindVertexArray(1); + const SharedPtr vao = MG_State::pGLContext->GetBoundVertexArray(); + ASSERT_NE(vao, nullptr); + for (Uint i = 0; i < 4; ++i) { + vao->SetAttributeFormat(i, 2, DataType::Float32, false, 8, 0, false, false, 8); + vao->BindAttributeBuffer(i, bufferC); + vao->EnableAttribute(i); + } + + BufferImpl::DrawVertexBufferRequest requests[VertexArrayObject::MAX_VERTEX_ATTRIBS]; + const Uint count = BufferImpl::ResolveDrawVertexBuffersFromRecord( + st, requests, VertexArrayObject::MAX_VERTEX_ATTRIBS); + + ASSERT_EQ(count, 2u) + << "the walk did not resolve st.VertexBuffers[Start..+Count): two DISTINCT handles are " + "named there (A twice and B), plus one client-memory array with no store to ensure"; + EXPECT_EQ(requests[0].Res, a) + << "the first buffer is not st.VertexBuffers[0].Res. If it is the frontend VAO's C, the " + "walk went back to GetAllAttributes() and §5.1's substitution is reverted"; + EXPECT_EQ(requests[1].Res, b) << "the second buffer is not st.VertexBuffers[1].Res"; + EXPECT_EQ(requests[0].BindingIndex, 0u); + EXPECT_EQ(requests[1].BindingIndex, 1u); + for (Uint i = 0; i < count; ++i) { + EXPECT_NE(requests[i].Res, c) + << "the walk ensured the buffer the FRONTEND VAO points at, which is the object a " + "run-ahead client has already moved on from"; + } + + st = saved; + MG_State::pGLContext.reset(); + MG_State::pGLContext = Move(previousContext); + MG_Pipe::MGPipeSlots().Free(MG_Pipe::MGPipeKind::Buffer, a); + MG_Pipe::MGPipeSlots().Free(MG_Pipe::MGPipeKind::Buffer, b); + MG_Pipe::MGPipeSlots().Free(MG_Pipe::MGPipeKind::Buffer, c); +#endif +} + +// Red-once (3), the index half. The claim has TWO parts and the second is what this package +// added: the handle comes from st.IndexBuffer.Res, and IndexBufferSerial comes with it - the +// legacy and push-monolith arms re-read the VAO's element slot on every indexed draw and so see +// a rebind for free, while this arm reads nothing and needs the serial in the key +// (ResolvedDrawBuffers::iboSerial). A rebind of the SAME buffer still moves the serial, which is +// exactly the case an identity-only compare would call a hit. +TEST(DirectGLESVertexInputDraw, TheIndexArmTakesItsBufferAndItsSerialFromTheRecord) { + using namespace MobileGL; + using namespace MobileGL::MG_Backend::DirectGLES; + using namespace MobileGL::MG_State::GLState; +#if !MOBILEGL_BUILD_DISAGGREGATED + GTEST_SKIP() << "the record arm of the index-buffer sync is compiled only under " + "MOBILEGL_BUILD_DISAGGREGATED"; +#else + auto published = MakeShared(0u); + auto rebound = MakeShared(0u); + const MG_Pipe::MGPipeHandle publishedHandle = + MG_Pipe::MGPipeSlots().Acquire(MG_Pipe::MGPipeKind::Buffer, published->GetLifetimeId()); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(publishedHandle)); + + MG_Pipe::MGPipeApplierState& st = MG_Pipe::MGPipeApplier(); + const MG_Pipe::MGPipeApplierState saved = st; + st.IndexBuffer = MG_Pipe::MGPIndexBuffer{}; + st.IndexBuffer.Res = publishedHandle; + st.IndexBufferSerial = 11; + + // The frontend's element slot has moved to another buffer with nothing emitted, in a real + // context for the reason the case above gives. + UniquePtr previousContext = Move(MG_State::pGLContext); + MG_State::pGLContext = MakeUnique(); + MG_State::pGLContext->CreateVertexArrayObject(1); + MG_State::pGLContext->BindVertexArray(1); + const SharedPtr vao = MG_State::pGLContext->GetBoundVertexArray(); + ASSERT_NE(vao, nullptr); + vao->GetIndexBufferBindingSlot().Bind(rebound); + + BufferImpl::DrawIndexBufferRequest request = BufferImpl::ResolveDrawIndexBufferFromRecord(st); + EXPECT_EQ(request.Res, publishedHandle) + << "the index arm did not read st.IndexBuffer.Res; if it answered the VAO's element " + "slot it is reading a binding the client has already changed"; + EXPECT_EQ(request.Serial, 11u) << "IndexBufferSerial did not travel with the handle"; + + // The same buffer re-bound: the handle is unchanged and ONLY the serial says so. + st.IndexBufferSerial = 12; + request = BufferImpl::ResolveDrawIndexBufferFromRecord(st); + EXPECT_EQ(request.Res, publishedHandle); + EXPECT_EQ(request.Serial, 12u) + << "a re-emitted set_index_buffer on the SAME handle is invisible to this arm without " + "the serial, and the memo would read clean over it"; + + st = saved; + MG_State::pGLContext.reset(); + MG_State::pGLContext = Move(previousContext); + MG_Pipe::MGPipeSlots().Free(MG_Pipe::MGPipeKind::Buffer, publishedHandle); +#endif +} + #else // G2 wants the pull and the push build to list the SAME ctest entries. The twin table only // exists under MOBILEGL_PIPE_PUSH, so in the pull build each case above keeps its name and @@ -5103,4 +5265,11 @@ TEST(DirectGLESBufferDrawProbe, UnderSplitTheRecordAloneAnswersTheLiveHostMapQue TEST(DirectGLESBufferDrawProbe, ACapsMaskWithoutTheResourceFamilyEmitsNothingAndCountsTheRefusal) { GTEST_SKIP() << "the handle-keyed resource table is compiled only under MOBILEGL_PIPE_PUSH"; } +// P5e (vi): G2/G14's skip twins for the two record-arm cases above. +TEST(DirectGLESVertexInputDraw, TheAttributeWalkTakesItsBuffersFromTheRecordNotTheFrontendVao) { + GTEST_SKIP() << "the record arm of the attribute walk is compiled only under MOBILEGL_PIPE_PUSH"; +} +TEST(DirectGLESVertexInputDraw, TheIndexArmTakesItsBufferAndItsSerialFromTheRecord) { + GTEST_SKIP() << "the record arm of the index-buffer sync is compiled only under MOBILEGL_PIPE_PUSH"; +} #endif // MOBILEGL_PIPE_PUSH