From a6ddeb0816cfd230037cdc58f584c5dca4489f56 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 18 Sep 2026 12:02:02 -0400 Subject: [PATCH] [MG_Pipe, MG_Test] (Disaggregated): P5e ID-118 - resource_copy_region becomes barriered - CopyImageSubData's apply reads GetTextureObject, a BARRIER_PULLED row that retires in P7, not in P5e. resource_copy_region was kWaitNone, so after the flip that is a genuinely UNBARRIERED record reading client memory: an unconditional Fatal, no knob, standing for two phases. - The wait class moves rather than the field being special-cased into the allowlist. The allowlist is derived (ID-116), so an exception written into it would be a real defect kept quiet instead of a debt stated - and with the class moved the derivation admits the pair with no exception at all: 101 -> 111 pairs, CopyImageSubData joins the 12 barriered verbs. - kWaitApplied AND NOT kWaitReply: the row posts no reply slot, so the client parks until the record applies. That is exactly what copy_framebuffer_to_texture - the two framebuffer-sourced copies, same kBlitOrCopy verb class - already does. THE COST IS MEASURED: glCopyImageSubData is zero per frame in the Minecraft 26.3-rc-3 scene, so the wait is on no per-frame path. - PipeCatalogue's wait-class census moves with it, both halves in this commit: the named kWaitApplied set gains the row, the count goes 9 -> 10, and the kWaitNone offset 24 -> 25. - Red-once: putting the row back to kWaitNone drops CopyImageSubData out of the derived allowlist (111 -> 101 pairs, 0 @CopyImageSubData) and fails "FieldOwnershipTest.cpp:422: Failure / resource_copy_region is unbarriered again: after the flip, a copy record reading the client's texture object is an unconditional Fatal and P7 is two phases away", with PipeCatalogueTest red on three counts beside it. --- MobileGL/MG_Pipe/PipeCalls.def | 17 ++++++++++++++++- .../MG_Pipe/generated/PipeFieldOwnership.inc | 4 ++-- MobileGL/MG_Pipe/generated/PipeWire.inc | 2 +- MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp | 9 +++++++++ MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp | 19 +++++++++++++++---- 5 files changed, 43 insertions(+), 8 deletions(-) diff --git a/MobileGL/MG_Pipe/PipeCalls.def b/MobileGL/MG_Pipe/PipeCalls.def index db4083cf..c8b1b808 100644 --- a/MobileGL/MG_Pipe/PipeCalls.def +++ b/MobileGL/MG_Pipe/PipeCalls.def @@ -236,7 +236,22 @@ /* only way bytes reach it - a blobref here would be a SECOND way to say the same thing. */ \ X(ResourceFlushRange, MGPFlushRange, kCtxObject, kNone, kWaitNone) \ X(ResourceReadback, MGPReadback, kCtxObject, kReplySlot, kWaitReply) \ - X(ResourceCopyRegion, MGPCopyRegion, kCtxObject, kNone, kWaitNone) \ + /* P5e (gl), ID-118: kWaitNone -> kWaitApplied, AND IT IS THE FIELD'S RETIRING PHASE THAT */ \ + /* forces it. This row's verb is CopyImageSubData, whose apply reads GetTextureObject - */ \ + /* a BARRIER_PULLED row whose retiring phase is P7, not P5e. Left kWaitNone, the flip */ \ + /* would make a genuinely UNBARRIERED record read client memory: an unconditional Fatal */ \ + /* (PipeInputs.cpp), no knob, for two whole phases. The alternative was to special-case */ \ + /* the field into section 7's allowlist, which ID-118 refused: the allowlist is DERIVED */ \ + /* (ID-116), and an exception written into it would be a real defect kept quiet rather */ \ + /* than a debt stated. With the wait class moved, the derivation admits the pair with no */ \ + /* exception at all and P7 retires the field on its own schedule. */ \ + /* kWaitApplied AND NOT kWaitReply: the call posts no reply slot and needs no answer - the */ \ + /* client parks until the record applies, which is exactly what CopyFramebufferToTexture */ \ + /* below already does for the two framebuffer-sourced copies. Same verb class */ \ + /* (kBlitOrCopy), same shape, a wait that is already carried in this lane. THE COST IS */ \ + /* MEASURED, NOT ASSUMED: glCopyImageSubData is zero per frame in the Minecraft 26.3-rc-3 */ \ + /* scene (~849 draws/frame), so the wait is not on any per-frame path. */ \ + X(ResourceCopyRegion, MGPCopyRegion, kCtxObject, kNone, kWaitApplied) \ X(GenerateMipmap, MGPMipPlan, kCtxObject, kNone, kWaitApplied) \ X(GetTextureImage, MGPReadbackInfo, kCtxObject, kReplySlot, kWaitReply) \ /* ---- context: transfer calls that read whole-context state, and the commands ---- */ \ diff --git a/MobileGL/MG_Pipe/generated/PipeFieldOwnership.inc b/MobileGL/MG_Pipe/generated/PipeFieldOwnership.inc index c03b22e5..72df2b01 100644 --- a/MobileGL/MG_Pipe/generated/PipeFieldOwnership.inc +++ b/MobileGL/MG_Pipe/generated/PipeFieldOwnership.inc @@ -356,7 +356,7 @@ inline constexpr MGPipeFieldMask kMGPipeAdmittedPullMask[kMGPipeVerbCount] = { {{0x0000000000000000ull, 0x0000000000000000ull}}, // BlitNamedFramebuffer {{0x5800c02000300180ull, 0x0000000000000000ull}}, // CopyTexImage2D {{0x0000000000000000ull, 0x0000000000000000ull}}, // CopyTexSubImage2D - {{0x0000000000000000ull, 0x0000000000000000ull}}, // CopyImageSubData + {{0x5800c02000300180ull, 0x0000000000000000ull}}, // CopyImageSubData {{0x5800c02000300180ull, 0x0000000000000000ull}}, // GenerateMipmap {{0x5800c02000300140ull, 0x0000000000000000ull}}, // ReadPixels {{0x0000000000000000ull, 0x0000000000000000ull}}, // GetTexImage @@ -394,7 +394,7 @@ inline constexpr MGPipeFieldMask kMGPipeAdmittedPullMask[kMGPipeVerbCount] = { {{0x0000000000000000ull, 0x0000000000000000ull}}, // GetGpuTimestampNs }; -inline constexpr SizeT kMGPipeAdmittedPullPairCount = 101; +inline constexpr SizeT kMGPipeAdmittedPullPairCount = 111; // THE PREDICATE THE STRICT KNOB ASKS (ID-117). A 69 x 128-bit table, so this costs a shift and // a test on the apply thread - the same shape MGPipeFieldMaskHas already has. diff --git a/MobileGL/MG_Pipe/generated/PipeWire.inc b/MobileGL/MG_Pipe/generated/PipeWire.inc index d1bb7246..906e7f59 100644 --- a/MobileGL/MG_Pipe/generated/PipeWire.inc +++ b/MobileGL/MG_Pipe/generated/PipeWire.inc @@ -428,7 +428,7 @@ inline constexpr Uint8 kMGPipeWaitClasses[static_cast(MGPWireOp::kOpCount /* 50 ResourceSubDataComplete */ static_cast(kWaitNone), /* 51 ResourceFlushRange */ static_cast(kWaitNone), /* 52 ResourceReadback */ static_cast(kWaitReply), - /* 53 ResourceCopyRegion */ static_cast(kWaitNone), + /* 53 ResourceCopyRegion */ static_cast(kWaitApplied), /* 54 GenerateMipmap */ static_cast(kWaitApplied), /* 55 GetTextureImage */ static_cast(kWaitReply), /* 56 Blit */ static_cast(kWaitNone), diff --git a/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp b/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp index 579af56b..4b923f74 100644 --- a/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp +++ b/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp @@ -413,6 +413,15 @@ TEST_F(FieldOwnershipTest, TheAdmittedPullTableIsID84sDerivationAndNotAList) { "entries this phase is being run to fix"; EXPECT_FALSE(MGPipeBarrierPullAdmitted(MGPipeInputField::GetBoundVertexArray, MGPipeVerb::DrawArrays)); + // P5e (gl), ID-118: the pair that made resource_copy_region's wait class move. The field + // retires in P7, so it had to be admitted somewhere - and it is admitted HERE, by the + // derivation and with no exception, only because the op is kWaitApplied. Put that row back + // to kWaitNone in PipeCalls.def and this goes red, which is the whole argument for moving + // the wait class rather than writing the field into a list. + EXPECT_TRUE(MGPipeBarrierPullAdmitted(MGPipeInputField::GetTextureObject, + MGPipeVerb::CopyImageSubData)) + << "resource_copy_region is unbarriered again: after the flip, a copy record reading " + "the client's texture object is an unconditional Fatal and P7 is two phases away"; // A field that is not a debt at all is never admitted, on any verb. EXPECT_FALSE(MGPipeBarrierPullAdmitted(MGPipeInputField::GetRenderStateParameters, MGPipeVerb::ReadPixels)); diff --git a/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp b/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp index b44f265f..f9a571ad 100644 --- a/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp +++ b/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp @@ -763,6 +763,15 @@ TEST(PipeCatalogue, EveryRowCarriesTheWaitClassTheContractGivesIt) { MGPWireOp::EndStreamOutput, MGPWireOp::PauseStreamOutput, MGPWireOp::ResumeStreamOutput, MGPWireOp::BindStreamOutput, MGPWireOp::CopyFramebufferToTexture, + // P5e (gl), ID-118. resource_copy_region JOINED THIS SET, and it is the retiring phase + // of a field that put it here: its verb is CopyImageSubData, whose apply reads + // GetTextureObject, and that row retires in P7. An unbarriered record reading client + // memory is an unconditional Fatal, so leaving it kWaitNone would have meant a real + // defect standing for two phases - or an exception hand-written into a DERIVED + // allowlist, which is the same thing with a comment on it. It sits beside + // copy_framebuffer_to_texture here for the reason it took that class: same verb class, + // no reply slot, and zero calls per frame in the measured scene. + MGPWireOp::ResourceCopyRegion, }; for (const MGPWireOp op : appliedRows) { EXPECT_EQ(MGPipeWaitClassFor(op), kWaitApplied) << WireOpNameForDiag(op); @@ -787,8 +796,10 @@ TEST(PipeCatalogue, EveryRowCarriesTheWaitClassTheContractGivesIt) { EXPECT_EQ(MGPipeWaitClassFor(op), kWaitNone) << WireOpNameForDiag(op); } - // The partition, by count. 14 + 1 + 9 = 24 rows wait; every other row of the catalogue does - // not. A row that changed class moves two of these numbers at once. + // The partition, by count. 14 + 1 + 10 = 25 rows wait; every other row of the catalogue does + // not. A row that changed class moves two of these numbers at once - which is why the + // kWaitApplied count went 9 -> 10 and the kWaitNone offset 24 -> 25 in the SAME commit that + // moved resource_copy_region (P5e gl, ID-118). SizeT reply = 0, applied = 0, present = 0, none = 0, other = 0; for (SizeT i = 1; i < static_cast(MGPWireOp::kOpCount); ++i) { switch (MGPipeWaitClassFor(static_cast(i))) { @@ -800,9 +811,9 @@ TEST(PipeCatalogue, EveryRowCarriesTheWaitClassTheContractGivesIt) { } } EXPECT_EQ(reply, 14u); - EXPECT_EQ(applied, 9u); + EXPECT_EQ(applied, 10u); EXPECT_EQ(present, 1u); - EXPECT_EQ(none, static_cast(kMGPipeCallCount) - 24u); + EXPECT_EQ(none, static_cast(kMGPipeCallCount) - 25u); EXPECT_EQ(other, 0u) << "a row carries the kWaitClassCount terminator as its class"; // And the reply half of the partition BOTH WAYS, over the whole catalogue: exactly the rows