diff --git a/MobileGL/MG_Pipe/PipeCalls.def b/MobileGL/MG_Pipe/PipeCalls.def index ff5720fb..b4bfd8d7 100644 --- a/MobileGL/MG_Pipe/PipeCalls.def +++ b/MobileGL/MG_Pipe/PipeCalls.def @@ -90,7 +90,14 @@ /* property of the payload, so its absence was simply wrong. Adding a flag does not */ \ /* move an opcode: the opcode is this line's 1-based position. */ \ X(GetCaps, MGPCaps, kScreen, kReplySlot|kHasBlob) \ - X(ResourceCreate, MGPResourceDesc, kScreen, kNone) \ + /* THE FOUR ACCEPTANCE ROWS (P5 R-5, R-16). These four and map_persistent are the only */ \ + /* applier entry points that RETURN something the client acts on destructively - clearing */ \ + /* per-level dirty flags, latching parameters, advancing its own bookkeeping - so the */ \ + /* answer has to come back, and kReplySlot is how a record says so. They carried no flag */ \ + /* until P5, because in monolith the answer is the return value of a direct call and there */ \ + /* was nothing to declare. kMGPipeCallFlags is now what sizes the reply pool and what the */ \ + /* decoder posts against, so a row that answers without the flag is a waiter that hangs. */ \ + X(ResourceCreate, MGPResourceDesc, kScreen, kReplySlot) \ /* NO kHasBlob, BY RULING (P5 R-13.3; CONTRACT-P5.md table 1 row 19). MGPResourceDesc */ \ /* owns no MGPBlobRef and gains none: under split `initialBytes` is always nullptr and */ \ /* the initial content of a glBufferData(size, data) / glTexImage*(..., data) arrives */ \ @@ -99,7 +106,7 @@ /* cost is one extra record. NOTE the SECOND uncarried companion on this call, the */ \ /* MGPRespecifiedLevel* scope (PipeApply.h:792) - it is NOT bytes and NOT covered by */ \ /* this ruling; table 1 row 19 gives it the two pad fields of MGPResourceDesc. */ \ - X(ResourceRespecify, MGPResourceDesc, kScreen, kNeedsAck) \ + X(ResourceRespecify, MGPResourceDesc, kScreen, kNeedsAck|kReplySlot) \ X(ResourceDestroy, MGPHandleOnly, kScreen, kNone) \ X(MapPersistent, MGPHandleOnly, kScreen, kReplySlot|kOptional) \ X(UnmapPersistent, MGPHandleOnly, kScreen, kOptional) \ @@ -154,8 +161,8 @@ /* final step asserts sizeof(ResidualValueBlock) == 0 (plan 6.3). */ \ X(SetResidualValueState, MGPResidualValueState, kCtxState, kHasBlob) \ /* ---- context: per-object state and transfer (plan 4.4.3 set_texture_params, 4.4.4) ---- */ \ - X(SetTextureParams, MGPTextureParams, kCtxObject, kNone) \ - X(ResourceSubData, MGPSubData, kCtxObject, kHasBlob|kVarTail) \ + X(SetTextureParams, MGPTextureParams, kCtxObject, kReplySlot) \ + X(ResourceSubData, MGPSubData, kCtxObject, kHasBlob|kVarTail|kReplySlot) \ X(BufferSubDataResident, MGPSubData, kCtxObject, kHasBlob|kOptional) \ X(ResourceSubDataComplete, MGPSubDataComplete, kCtxObject, kNone) \ /* NO kHasBlob AND NO MGPBlobRef, BY RULING (P5 R-13.2; CONTRACT-P5.md table 1 row 21). */ \ diff --git a/MobileGL/MG_Pipe/generated/PipeTables.inc b/MobileGL/MG_Pipe/generated/PipeTables.inc index 072635c4..a5d4f966 100644 --- a/MobileGL/MG_Pipe/generated/PipeTables.inc +++ b/MobileGL/MG_Pipe/generated/PipeTables.inc @@ -16,8 +16,8 @@ // call and the frontend keeps its own path (plan B section 4.1). struct MGPipeScreen { void (*GetCaps)(const MGPCaps* payload, MGPReplySlot* reply); - void (*ResourceCreate)(const MGPResourceDesc* payload); - void (*ResourceRespecify)(const MGPResourceDesc* payload); + void (*ResourceCreate)(const MGPResourceDesc* payload, MGPReplySlot* reply); + void (*ResourceRespecify)(const MGPResourceDesc* payload, MGPReplySlot* reply); void (*ResourceDestroy)(const MGPHandleOnly* payload); void (*MapPersistent)(const MGPHandleOnly* payload, MGPReplySlot* reply); void (*UnmapPersistent)(const MGPHandleOnly* payload); @@ -67,8 +67,8 @@ struct MGPipeContext { void (*SetDrawProgram)(const MGPHandleOnly* payload); void (*SetDispatchProgram)(const MGPHandleOnly* payload); void (*SetResidualValueState)(const MGPResidualValueState* payload); - void (*SetTextureParams)(const MGPTextureParams* payload); - void (*ResourceSubData)(const MGPSubData* payload, const void* varTail, Uint32 varTailCount); + void (*SetTextureParams)(const MGPTextureParams* payload, MGPReplySlot* reply); + void (*ResourceSubData)(const MGPSubData* payload, const void* varTail, Uint32 varTailCount, MGPReplySlot* reply); void (*BufferSubDataResident)(const MGPSubData* payload); void (*ResourceSubDataComplete)(const MGPSubDataComplete* payload); void (*ResourceFlushRange)(const MGPFlushRange* payload); diff --git a/MobileGL/MG_Pipe/generated/PipeThunks.inc b/MobileGL/MG_Pipe/generated/PipeThunks.inc index 6a898679..09638e04 100644 --- a/MobileGL/MG_Pipe/generated/PipeThunks.inc +++ b/MobileGL/MG_Pipe/generated/PipeThunks.inc @@ -21,12 +21,12 @@ inline void MGP_GetCaps(const MGPCaps* payload, MGPReplySlot* reply) { gMGPipeScreen.GetCaps(payload, reply); } -inline void MGP_ResourceCreate(const MGPResourceDesc* payload) { - gMGPipeScreen.ResourceCreate(payload); +inline void MGP_ResourceCreate(const MGPResourceDesc* payload, MGPReplySlot* reply) { + gMGPipeScreen.ResourceCreate(payload, reply); } -inline void MGP_ResourceRespecify(const MGPResourceDesc* payload) { - gMGPipeScreen.ResourceRespecify(payload); +inline void MGP_ResourceRespecify(const MGPResourceDesc* payload, MGPReplySlot* reply) { + gMGPipeScreen.ResourceRespecify(payload, reply); } inline void MGP_ResourceDestroy(const MGPHandleOnly* payload) { @@ -201,12 +201,12 @@ inline void MGP_SetResidualValueState(const MGPResidualValueState* payload) { gMGPipeContext.SetResidualValueState(payload); } -inline void MGP_SetTextureParams(const MGPTextureParams* payload) { - gMGPipeContext.SetTextureParams(payload); +inline void MGP_SetTextureParams(const MGPTextureParams* payload, MGPReplySlot* reply) { + gMGPipeContext.SetTextureParams(payload, reply); } -inline void MGP_ResourceSubData(const MGPSubData* payload, const void* varTail, Uint32 varTailCount) { - gMGPipeContext.ResourceSubData(payload, varTail, varTailCount); +inline void MGP_ResourceSubData(const MGPSubData* payload, const void* varTail, Uint32 varTailCount, MGPReplySlot* reply) { + gMGPipeContext.ResourceSubData(payload, varTail, varTailCount, reply); } inline void MGP_BufferSubDataResident(const MGPSubData* payload) { diff --git a/MobileGL/MG_Pipe/generated/PipeWire.inc b/MobileGL/MG_Pipe/generated/PipeWire.inc index d909caf9..6f37895f 100644 --- a/MobileGL/MG_Pipe/generated/PipeWire.inc +++ b/MobileGL/MG_Pipe/generated/PipeWire.inc @@ -34,7 +34,33 @@ struct MGPWireRecHeader { Uint16 Op; // MGPWireOp - Uint16 Flags; // MGPipeCallFlags of the call, for asserts and tracing + // RING FRAMING FLAGS - MG_Remote::Transport::RingRecordFlags - NOT MGPipeCallFlags. + // + // This field IS RingRecordHeader::flags (Ring.h): the two structs are the same eight bytes + // and the ring writes this one. The two flag spaces OVERLAP AND DISAGREE, so stamping + // MGPipeCallFlagsFor(op) in here is a silent, data-dependent corruption: + // + // bit 0 kNeedsAck == kRecNeedsAck agree + // bit 1 kHasBlob == kRecHasBlob agree + // bit 2 kVarTail vs kRecPad DISAGREE + // bit 3 kHostSpan vs kRecBorrowSlot DISAGREE + // bit 4 kReplySlot vs kRecVarTail DISAGREE + // bit 5 kOptional vs (nothing) no ring counterpart + // + // The first two agreeing is exactly what makes this dangerous: a naive stamp looks right + // in a debugger. It is not. RingProducer::Reserve masks kRecPad out of a caller's flags, + // so bit 2 is defended for anyone who goes through it - and ONLY bit 2, and only on that + // path. A producer that writes this header itself (these are the same eight bytes, so a + // codec with its own header struct does exactly that) loses even that: Pop treats kRecPad + // as a WRAP FILLER AND SKIPS THE RECORD. Bits 3 and 4 are undefended everywhere - a + // stamped kHostSpan tells the consumer this record borrowed a slot into the GPU timeline + // and must retire late, and a stamped kReplySlot claims a tail that is not there. + // + // THE TWO SPACES ARE DISJOINT BY TRANSLATION, NOT SHARED. The encoder maps one to the + // other explicitly (MG_Remote/Wire/PipeWireCodec.cpp) and nothing else may write this + // field. The CALL's flags are not on the wire at all and do not need to be: the opcode is, + // and MGPipeCallFlagsFor(op) recovers them exactly on either side. + Uint16 Flags; Uint32 Size; // bytes of this record including the header and the variable tail }; static_assert(sizeof(MGPWireRecHeader) == 8, "the wire header is 8 bytes"); @@ -138,8 +164,8 @@ enum class MGPWireOp : Uint16 { inline constexpr Uint32 kMGPipeCallFlags[static_cast(MGPWireOp::kOpCount)] = { /* 0 kInvalid */ static_cast(kNone), /* 1 GetCaps */ static_cast(kReplySlot | kHasBlob), - /* 2 ResourceCreate */ static_cast(kNone), - /* 3 ResourceRespecify */ static_cast(kNeedsAck), + /* 2 ResourceCreate */ static_cast(kReplySlot), + /* 3 ResourceRespecify */ static_cast(kNeedsAck | kReplySlot), /* 4 ResourceDestroy */ static_cast(kNone), /* 5 MapPersistent */ static_cast(kReplySlot | kOptional), /* 6 UnmapPersistent */ static_cast(kOptional), @@ -183,8 +209,8 @@ inline constexpr Uint32 kMGPipeCallFlags[static_cast(MGPWireOp::kOpCount) /* 44 SetDrawProgram */ static_cast(kNone), /* 45 SetDispatchProgram */ static_cast(kNone), /* 46 SetResidualValueState */ static_cast(kHasBlob), - /* 47 SetTextureParams */ static_cast(kNone), - /* 48 ResourceSubData */ static_cast(kHasBlob | kVarTail), + /* 47 SetTextureParams */ static_cast(kReplySlot), + /* 48 ResourceSubData */ static_cast(kHasBlob | kVarTail | kReplySlot), /* 49 BufferSubDataResident */ static_cast(kHasBlob | kOptional), /* 50 ResourceSubDataComplete */ static_cast(kNone), /* 51 ResourceFlushRange */ static_cast(kNone), @@ -239,6 +265,28 @@ static_assert((MGPipeCallFlagsFor(MGPWireOp::DrawVbo) & static_cast(kHostSpan | kVarTail)) == static_cast(kHostSpan | kVarTail), "draw_vbo is the conditional-tail plus host-span shape the codec is measured on"); + +// The bit layout of MGPipeCallFlags, read out of MGPipe.h by the generator. +// MG_Test/Wire/RingTest.cpp holds the other half: what each of these bits means +// in MG_Remote::Transport::RingRecordFlags, which is NOT the same thing. +static_assert(static_cast(kNeedsAck) == (1u << 0), + "MGPipeCallFlags::kNeedsAck moved bit; MG_Remote's translation is keyed on it"); +static_assert(static_cast(kHasBlob) == (1u << 1), + "MGPipeCallFlags::kHasBlob moved bit; MG_Remote's translation is keyed on it"); +static_assert(static_cast(kVarTail) == (1u << 2), + "MGPipeCallFlags::kVarTail moved bit; MG_Remote's translation is keyed on it"); +static_assert(static_cast(kHostSpan) == (1u << 3), + "MGPipeCallFlags::kHostSpan moved bit; MG_Remote's translation is keyed on it"); +static_assert(static_cast(kReplySlot) == (1u << 4), + "MGPipeCallFlags::kReplySlot moved bit; MG_Remote's translation is keyed on it"); +static_assert(static_cast(kOptional) == (1u << 5), + "MGPipeCallFlags::kOptional moved bit; MG_Remote's translation is keyed on it"); +// ... and this is ALL of them. A seventh flag changes this number, which is the +// build break that sends its author to the ring's flag space before it ships. +inline constexpr Uint32 kMGPipeCallFlagsAllBits = static_cast(kNeedsAck | kHasBlob | kVarTail | kHostSpan | kReplySlot | kOptional); +static_assert(kMGPipeCallFlagsAllBits == 0x3Fu, + "the MGPipeCallFlags bit set changed; see RingTest's cross-enum table"); + struct alignas(8) MGPWireRec_GetCaps { MGPWireRecHeader Header; MGPCaps Payload; diff --git a/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp b/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp index db7fbe30..a3ea0bb0 100644 --- a/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp +++ b/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp @@ -850,6 +850,36 @@ TEST(PipeCatalogue, ResourceRespecifyAcksOnlyImmutableStorage) { #undef MGP_COUNT_ACKING_CALLS EXPECT_EQ(ackingCalls, 1u); + // P5 R-16: the same shape over kReplySlot, which had no count pin at all until the flag + // became load-bearing. It is what sizes the reply pool and what the decoder posts against, + // so the number is now a protocol quantity rather than a documentation one. + // + // FOURTEEN. Ten answers that were always declared - get_caps, map_persistent, the two fence + // reads, the three query reads, the two readbacks and read_pixels - plus the FOUR ACCEPTANCE + // ROWS, whose applier entry points return a Bool the client acts on destructively and which + // carried no flag because in monolith that answer is a direct call's return value. + Uint32 replySlotCalls = 0; +#define MGP_COUNT_REPLY_SLOT_CALLS(Name, Payload, Class, Flags) \ + if ((static_cast(Flags) & static_cast(kReplySlot)) != 0) ++replySlotCalls; + MGP_CALL_LIST(MGP_COUNT_REPLY_SLOT_CALLS) +#undef MGP_COUNT_REPLY_SLOT_CALLS + EXPECT_EQ(replySlotCalls, 14u); + + // And the four by name, because a count alone would let a row lose the flag while another + // gained one. These are exactly the MGPipeApply* entry points that return Bool + // (PipeApply.h:820, :868, :897, :1023); map_persistent's void* is the fifth answer and was + // already declared. + Uint32 acceptanceWithSlot = 0; +#define MGP_COUNT_ACCEPTANCE_ROWS(Name, Payload, Class, Flags) \ + if ((std::strcmp(#Name, "ResourceCreate") == 0 || std::strcmp(#Name, "ResourceRespecify") == 0 || \ + std::strcmp(#Name, "ResourceSubData") == 0 || std::strcmp(#Name, "SetTextureParams") == 0) && \ + (static_cast(Flags) & static_cast(kReplySlot)) != 0) { \ + ++acceptanceWithSlot; \ + } + MGP_CALL_LIST(MGP_COUNT_ACCEPTANCE_ROWS) +#undef MGP_COUNT_ACCEPTANCE_ROWS + EXPECT_EQ(acceptanceWithSlot, 4u); + // glBufferStorage: an immutable store, and the one entry point allowed a synchronous ack. MGPResourceDesc immutable{}; immutable.Immutable = 1;