The first P6 code that runs. ITransport over a connected AF_UNIX SOCK_STREAM
socket, with SCM_RIGHTS descriptor passing on a dedicated SOCK_DGRAM aux pair.
No shared memory: this is the CONTROL plane, and the hot path bypasses
ITransport entirely by its own header's account. What the socket carries is the
handshake, surface ops, resync, aux requests and fatals - and, once sm exists,
the four segment descriptors that let the peer map the data plane. It moves the
keys, not the bytes.
CONTRACT-P6 §8 and the spawn plan both require this green over a plain
socketpair with two threads BEFORE a child is ever forked, for the reason
SessionRings.h gives about the attach half: a transport first exercised on the
day the process appears is a transport whose bugs all arrive at once, on the
hardest day to debug them. That requirement is now met.
It reinvents nothing. Framing.h already had AppendFrame and a real byte-stream
reassembler with exactly the BUFFER_TOO_SMALL keep-the-message semantics
ITransport specifies - written for this and never before driven by a socket.
FdPassing.h already had SCM_RIGHTS over a dedicated datagram pair. What is new
is ~250 lines joining them, plus the platform refusal: Windows has no
SCM_RIGHTS and ShmSegment::Adopt is POSIX-only, so CreatePair refuses BY NAME
at construction rather than handing back a transport that fails every later
call.
SOCK_STREAM rather than SOCK_SEQPACKET, deliberately: Doorbell.h:390-398
already chose a stream socket so a closed peer surfaces as POLLIN|POLLHUP with
recv()==0 rather than as a hang, and the control plane may not disagree with
the bell about what death looks like. Framing.h is what makes a stream safe -
the message boundary is in the payload, not in the socket.
TESTS: the same nine ITransport-contract cases InProcessTransportTest runs,
copied rather than shared. Folding both suites into one parameterised fixture
would rename the sixteen existing cases and G14 says names only grow; and the
two suites do not share a handle anyway, since InProcessTransport's tests reach
into condvar doorbells this class does not have. Nine names added, zero
removed. The seven Doorbell* cases are deliberately absent: they exercise
CondVarDoorbell, not ITransport.
Plus three a stream has and a message queue cannot: a 256 KiB payload that
provably crosses in several reads; a garbage stream that must latch failed and
must NOT resync onto a boundary it invented; and adopting an already-connected
fd with no aux channel, which answers UNSUPPORTED to ShareFd rather than
pretending - rule G, a transport declares what it supports.
Red-once, both run: drop the message on BUFFER_TOO_SMALL and six cases go red;
make the framing latch recoverable and AGarbageStreamIsLatchedFailed fails by
name.
Gates: 12/12 in 677 ms. Unit suite 2340 tests, up 12, with the same two
pre-existing environmental failures. G1 pull 0 added / 0 removed, .text size
10822147 unchanged, and zero SocketTransport symbols in the pull build.
One note for the VM track this eventually serves: swapping AF_UNIX for
AF_VSOCK is one line here and nothing at all in Framing. It is the shared-memory
half that does not cross a kernel.
The shared control page both roles map had a mixed-writer cache line:
{appliedSeq, submittedSeq, retiredSeq, completedFrameSerial, presentAckSerial}
sat together, but submittedSeq is producer-written (Ring.h's "THE FIVE
WATERMARKS") while the other four are consumer-written. That is why no
single-writer progress block could be aliased over it, and it is what this
package fixes: submittedSeq moves up beside cmdHead, the other thing the
producer writes, and the four that remain become a named LinkProgress member -
the struct c6 declared in ILink.h - so a data plane with no shared page can own
the same four values and every wait predicate in SessionRings.h keeps ONE
implementation at source level.
The two event flags deliberately stay in the doorbell group. Promoting them
would put the client's per-drain exchange(0) on eventRingFull (EventRing.h:200)
onto the line the client's own spin predicate reads 1e5-1e6 times a frame.
73 call sites rewritten, 15 in the library and 58 in tests. The rewrite is
receiver-scoped rather than field-scoped, because SplitRuntimePeek.h:77 has its
own struct with a field of the same name and SplitRuntimePeek.cpp:146 has both
on one line - a field-scoped rename would have silently broken the peek struct
that MagmaRunAheadScenario reads nine times.
THE ASSERTS ARE THE PART THAT NEEDED A SECOND PASS. The first revision shipped
four aggregate asserts and an adversarial review broke them by counterexample:
swapping retiredSeq and completedFrameSerial inside LinkProgress compiled
clean, passed all four, and passed the runtime layout test too - because
appliedSeq stays at offset 0. Three wire-visible offsets on a shared page were
pinned by nothing. Nothing else in the tree covers it either: the ABI
fingerprint takes no input from this page, and sizeof(RingControl) is 4096
whatever the members do, because alignas(4096) makes it so.
So the block is now per-field as CONTRACT-P6 §8.3 actually asked, plus a
standard-layout precondition, a tail bound that alignas cannot satisfy for us,
and the LinkEventFlags mirror. Each was run red: the field swap reds the
per-field asserts by name, a fifth member inside LinkProgress reds sizeof.
One of them cannot go red, and Ring.h now says so rather than implying
otherwise. A SMALL member inserted between Progress and serverEpoch lands at
offset 288, inside Progress's own line, and serverEpoch's alignas(64) absorbs
up to 32 bytes of it - so both line asserts still pass. The review's proposed
fix does not catch it either; I tried it. A gate nobody can falsify is worse
than no gate, so the limit is written down beside the asserts.
Also: RingTest.ControlPageLayoutIsTheSharedContract said nothing about the one
field this package moved, and now asserts the producer/consumer separation and
the event flags' exclusion itself rather than borrowing the claim from Ring.h.
Ring.h grew 30 lines, so the Ring.h:NN citations this package and c6 authored
are re-anchored to named blocks instead of line numbers. ARCHITECTURE.md §11.2
is updated for the new layout, and a factual error introduced in the previous
revision of that paragraph - submittedSeq annotated as releasing staging, which
retiredSeq does - is corrected.
Gates: pull 0 added / 0 removed, .text size 10822147 unchanged; split 0/0,
.text size 11681395 unchanged; unit suite 2326/2328 with the same two
pre-existing environmental failures as the baseline. The five edits behind
MGITEST_SPLIT_RUNTIME_PEEK were proven compiled by a red-once, not assumed.
Note on the .text claim: that is a SIZE, not the bytes - field offsets moved,
so immediates moved with them. And this page's layout IS observable across
binaries; what is unobservable is behaviour within one binary. The contract
already books the layout change deliberately, before spawn exists to care.
CONTRACT-P6 §8.5 requires lk to publish its blast radius before it starts,
because an earlier draft's ratchet baseline was wrong by a factor of three and
a ratchet installed on the wrong number is red on its first CI run for reasons
that have nothing to do with a leak. The itemisation is mechanical, per file,
with the grep spelling stated: 250 seam-identifier references outside
Transport/ across 23 files (127 production, 123 test), and 44 member-access
sites that the regroup itself must rewrite, across 9 files - 17 of them in
MG_IntegrationTest, which is why lk builds with the integration tests ON.
The earlier build did not, and a missed edit there would have been green.
THE CORRECTION. Measuring that radius turned up a mistake in c6, landed two
commits ago. §8.3 and ILink.h both said lk should promote eventRingFull and
eventDropped into the watermark group and called the result single-writer.
eventRingFull has TWO writers: the server latches it with store(1) when
SEG_EVENT fills (EventRing.h:140) and the CLIENT clears it with an exchange(0)
on EVERY DRAIN (:200). Promoting it would put a per-drain read-modify-write by
the client on the same cache line as appliedSeq - which the server
release-stores every 64 records and the client's spin predicate reads 1e5-1e6
times a frame. That is new false sharing on the hottest line in the system,
and it would land in exactly the numbers this package's gate has to hold
constant. It is also the same class of error the earlier draft made with
submittedSeq, one field over.
So LinkProgress is now the four consumer-written watermarks and nothing else -
genuinely single-writer - and the two flags stay in the doorbell/generation
group, where mixed writers already live beside consumerParked/producerParked,
crossing the seam through a separate EventFlags() accessor on its own line.
The contract records the correction and its reasoning rather than quietly
restating the rule.
ALSO CORRECTED, from running the baseline suite: a6's link-experiment report
said the silently-empty build stamp had "no log and no gate to say so". That
is false. SessionHandshakeTest.cpp:140 asserts BuildStamp[0] != '\0' and :180
asserts that perturbing it changes the fingerprint, and BOTH GO RED in this
configuration right now - the worktree's .git holds a Windows path WSL's git
cannot resolve, so GIT_COMMIT_HASH_SHORT is "". The finding is therefore
stronger, not weaker: the gate that catches it already exists and is already
red. What is silent is the BUILD, and hs owes a named configure-time failure
rather than a new gate.
Baseline for lk, recorded: 3878 tests, two failures, both environmental and
understood - the fingerprint one above, and PipeCatalogue.FrontendNeverTakes-
AnApplierAddress, which walks up from cwd looking for MobileGL/MG_Impl and so
fails for any out-of-source build directory.
No behaviour change. The headers are still included by nothing.
c6 is P6's contract package. It lands CONTRACT-P6.md plus two headers and
changes nothing: no caller moves, no byte moves on any wire.
THE CONTRACT supersedes P6-CONTRACT-DRAFT.md, which was written before its own
audit and is wrong in four places - the ABI row, "the latch is implemented
unchanged", "cp is transport only", and "a transport swap and nothing else".
a6 measured all four. It carries 21 rulings, of which the load-bearing one is
D1: the spawned child keeps Transport == Spawn and anti-recursion becomes a
separate Dial axis. ARCHITECTURE.md:488's literal instruction is amended,
because 226 live lines test `Transport != Monolith` to select the SERVER arm
and forcing Monolith flips 148 MG_Backend lines to frontend glue in the one
process that has no frontend.
That ruling can be stated normatively without changing behaviour because of
something a6 found and the contract exploits: TransportMode::Spawn is
UNREACHABLE at this head. ConfigLoader.cpp:344 is the only writer for the
string "spawn" and it writes Monolith. So every rule conditioned on
Transport == Spawn constrains a state no process can enter - and the same fact
is why ConfigLoader's named refusal cannot currently falsify itself, which is
sm's to fix and §9.5's arm-proof gate to catch.
THE SEAM. ITransport says in its own header that the hot path bypasses it;
below it there was no interface of any kind, because Ring.h, ReplySlot.h and
EventRing.h each take a raw void* plus a capacity. ILink.h is that missing
thing. It is setup-time, not hot-path: no method is called per record or inside
a spin predicate, because Doorbell::Wait's predicate runs 1e5-1e6 times a frame
reading watermarks straight off the page, and because the only lane that could
measure a virtual there has four times the dispersion of the effect - a gate
that can only print "within noise" is ID-124 one level down.
StreamLink.h is declared and implemented by nobody, and that is its job: ILink's
adequacy is a claim only a compile can settle. Writing it forced three methods
the brief did not name - ResolveSpan, FlushProgress, and the reply pair - so it
has already earned its keep. Delete one ILink method it declares and the build
goes red.
Deliberately NOT here, each for a reason the contract records: Watermark::Reached
is not redefined (SessionRings.h owns it); Ring.cpp's reserve/pop bodies are not
relocated into a header that would then need the logger; RingControl is not
regrouped, because that rename is 109 production sites with five behind a
non-Android #if, so it is lk's with a published itemisation; and the schema rows
are specified but left to hs, because deprecating SurfaceReply.defaultFb while
appending `result` changes CreateSurfaceReply's arity and the existing
six-argument call would silently bind its defaultFb 0 to result.
INERTNESS IS STRUCTURAL, not measured. Zero object dependency files reference
either header, zero build rules do, and nothing in the tree includes them.
Verified anyway on both flavours: pull and split each have 0 symbols added,
0 removed, and .text byte-identical (10822147 / 11681395). check_doc_citations
resolves all 61 citations.
a6 is the read-only audit P6-SPAWN-PLAN puts first. Seven static items, each
produced by one auditor and re-resolved line by line by one adversarial
verifier; 198 rows, 65 of them contested, and only the surviving ones carry
into the report. Item 5 - can the server link without MG_Impl - is the one
experiment, and it is answered.
The headline is that P6's transport half is smaller than the plan assumed
(fc and fs really did land) and its process/role half is much larger. Four
statements in P6-SPAWN-PLAN / P6-CONTRACT-DRAFT are false at this head:
- `spawn` is not an observable state at all. ConfigLoader.cpp:339-346
recognises it and then writes Transport = Monolith, so "the child was
forced to monolith" and "the parent fell back" are the same value and
that named refusal cannot falsify itself. Meanwhile 226 live lines test
`Transport != Monolith` to select the SERVER arm, 148 of them in
MG_Backend - so ARCHITECTURE.md:488's rule, implemented literally, flips
the whole backend to frontend glue in the one process with no frontend.
- `cp` is not transport-only: the InitCapabilities REQUEST has no wire form,
and the codec refuses to encode the kind a live client still calls.
- the device-lost latch is not implemented; it was booked as carriage.
- the server cannot link without MG_Impl: 184 symbols, 177 of them real
function calls, owed 101 to P7 / 14 to P3b-P4b / 60 shared / 6 to P6.
There is also no module target to link, so the question could not have
been answered by editing a link line.
Four more blockers the earlier review missed: the apply thread aborts in
ConsumerDoorbell before popping its first record (contract 3.9's ruling, which
lk should retire rather than amend); neither side of the control plane is
wired to a socket; the control-reply wait has no deadline, so a frozen peer
wedges the GL thread inside eglMakeCurrent; and SurfaceReply cannot carry a
MobileGLResult, so a client cannot tell a dead server from a legitimate
refusal.
The abort census makes dl's funnel a redesign rather than a rename: 92 sites,
30 distinct family words against a 7-value wire enum, and three unbounded
peer-driven allocations with no try/catch anywhere under MG_Remote.
P6-ENDSTATE-REVIEW.md is the re-examination of P6 against the gfxstream-shaped
end state that produced a6's three added items. a6 overturned four of its own
claims - the MG_State eight's shape, the abort count, m_windowHandle's
injection points and a predicate name collision - and those are corrected in
place rather than left to be rediscovered.
Docs only. No code, no gate, no behaviour.
Two comments from the previous commit, corrected. The CMakeLists.txt reference in
DirectGLES.cpp pointed at the link libraries instead of the Debug/Release
visibility preset it names (707-719, not 629-641). The Managers.h note said the
overload is "reached by the pull, verify and push build" through "the three
wrappers", which reads as if all three called it; only
VertexArrayImpl::SyncClientSideVertexArraysForFetch does, and what those flavors
compile is the declaration, which is the point being made.
No code change: the four flavors' libraries are byte-identical to the previous
commit's (comment-only).
The indexed and indirect client-memory upload added by 62bfe461 landed inside
`#if MOBILEGL_BUILD_DISAGGREGATED`, and that guard was wrong in the one way a
guard can be wrong without a test saying so: the code exists FOR the monolith
arm - every entry point in the family early-returns when `MG_Config::Transport
!= Monolith` - and the monolith arm is what the pull, verify and push builds
run, not a split-arm fallback. Only the split lane was building the change, so
only the split lane could compile it.
Both CI jobs that build without MOBILEGL_BUILD_DISAGGREGATED were red on the
same twenty diagnostics: `build explicit pull runtime control` (the pull
library retrace's monolith control measures) and `build-linux-verify`
(MOBILEGL_PIPE_VERIFY=ON). DirectGLES.cpp:8437/8455/8521/8537 are the two
indirect executors and :9225/9251/9623/9637/9675/9700 the indexed family,
every one of them in an entry point that compiles in all four flavors, calling
names the pull build had no declaration for:
error: no member named 'SyncClientSideVertexArraysForIndirectFetch' in
namespace 'MobileGL::MG_Backend::DirectGLES::VertexArrayImpl'
... and SyncClientSideVertexArraysForIndexedFetch, likewise.
The block moves out of the guard rather than being duplicated per flavor, so
the split build's preprocessed text and behaviour are unchanged: the three
wrappers keep their `Transport != Monolith` early return, and in the
transport-free flavors that comparison folds to a compile-time false
(Config.h:566) exactly as it does in every other monolith-only body in this
file.
What the guard also covered, and moves with it:
* ClientFetchPlan.h - no longer behind `MOBILEGL_BUILD_DISAGGREGATED &&
MOBILEGL_PIPE_PUSH`. The plan IS the monolith arm's arithmetic (the file's
own header says so: "the monolith arm asks it to size the per-draw upload"),
so a pull build that cannot see it cannot answer the question.
* BackendVertexArrayObject::SyncClientSideAttributesForDraw - declaration and
definition, the callee of SyncClientSideVertexArraysForFetch.
* The `#include <MG_Impl/Pipe/ClientFetchPlan.h>` in Managers.h, which had
been inside the same `#if MOBILEGL_PIPE_PUSH` as the two P3a/P4a includes
it was appended to.
G1: the three wrappers and the byte reader take internal linkage (`static`),
the spelling the sibling SyncClientSideVertexArraysForDrawArrays already uses
at DirectGLES.cpp:2422 for exactly this reason. Verified on the pull build
(Release/INFO, LTO off, library target only, the monolith-symbol-report job's
own flags): 0 added / 0 removed / 0 resized / 0 renamed, .text +0, and every
one of the new names is LOCAL (`t`/`b`) with 0 of them in the dynamic table.
Had they kept external linkage they would have been 0 in the pull symbol delta
anyway (CXX_VISIBILITY_PRESET hidden) and 4 in a Debug build's dynamic table,
which is the configuration CMakeLists.txt:629-641 switches to `default`.
Verified: build-pull rc=0, build-push rc=0, build-verify
(MOBILEGL_PIPE_VERIFY=ON, DISAGGREGATED=OFF) rc=0, build-split rc=0; split
regressions green under both MOBILEGL_TRANSPORT=monolith and =inproc for
ClientVertexArrayScenario (18/18 each) and DrawRangeElements|MultiDraw (119/119
each), so the client-array behaviour 62bfe461..cb64b62b repaired is not
regressed.
The step located the Android CMake identity by reading CMakeCache.txt next
to each compile_commands.json, but the trace flavor's build also produces
.cxx/tools/<flavor>/<abi>/compile_commands.json copies whose cache lives in
the real per-ABI build dir, so the proof failed on a file-layout assumption
rather than on missing evidence. Remove the step and its artifact upload.
MEASUREMENTS 7.3 records what 82683d4a did, so the 30 s in that row stays as
written; a note under the table now says which value is live and where the
reason for the change is argued, so a reader looking up the barrier budget by
grep does not come away with the superseded number.
lavapipe/llvmpipe stall the apply thread for tens of seconds inside texture-handle
registration (vkCreateImageView -> lvp_CreateImageView -> llvmpipe_register_texture,
a JIT/futex path): one measured stall is 29.3-39.7 s and a whole inproc retrace run
accumulates ~44.6 s of it. At a 30 s bound those runs red intermittently on
Fatal{BarrierTimeout, "ResourceRespecify"} while the same stall under monolith is
merely slow - monolith has no cap on that path. Since the monolith shares the stall,
the bound, not the workload, was the defect.
kBarrierTimeoutMs becomes 120000, >3x the largest observed stall, which still leaves
the watchdog job: a record that is never retired still terminates the run and the
Fatal still names the seq. The comment now says deadlock detector rather than
performance gate and carries the measurement.
The fake-clock case that pins the budget moves with it: FenceWaitBudgetHonorsGl-
TimeoutAndFiniteTransportChunks expected 30000/30001/90000 and now expects
120000/120001/180000, so the assertion still pins the value instead of drifting.
Texts that wrote the old number down are updated: ARCHITECTURE 17.1, CONTRACT-P5E
2.4 (which now names kBarrierTimeoutMs rather than the literal), and the two
bring-up comments in PipeRoute.h / WireTables.cpp that dated a self-wait to
"thirty seconds".
Three follow-ups the first pass needed:
DirectGLES glGetTexImage has to attach a STENCIL_INDEX texture at
GL_STENCIL_ATTACHMENT. It went through the colour branch, which left the scratch
READ framebuffer incomplete for a stencil-only texture, so the readback bailed out
before the new stencil branch could run. ScratchFBOImpl::EnsureDepthAttachment2D
gains the stencil-only point; the shadow remembers which of the three points it
holds so a later depth attach is not deduped against it.
DirectVulkan's aspect staging buffer is recorded into the current frame's command
buffer and outlives the glBlitFramebuffer call that records it, so it retires with
the frame. VkBufferManager::DeferRelease is the manager's own deferred-release
entry point and moves to its public section for that.
f5234b9e taught the WIRE depth/stencil readback to scale a depth into the signed
integer widths' own range and to answer GL_HALF_FLOAT; 7b926429 taught the WIRE
depth/stencil resolve to copy only the requested aspect. Both landings stopped at
the wire entry points, so with MOBILEGL_TRANSPORT=monolith the same seven
DepthStencilReadbackMatrixScenario cases still failed on the same assertions.
DirectGLES: IsSupportedDepthStencilReadPixelsPair turned GL_BYTE/GL_SHORT/GL_INT/
GL_HALF_FLOAT depth and GL_HALF_FLOAT stencil away before the helper that can serve
them, and glGetTexImage had no GL_STENCIL_INDEX branch at all, so the read fell
through to a native ES glReadPixels the driver refuses and nothing was written.
ReadPixelsDepthComponent and ReadPixelsStencilViaNative now answer the signed
widths out of their own ranges and encode the half-float widths.
DirectVulkan: ReadDepthStencilImageToClient carried its own copy of the packing
and had drifted - GL_SHORT was scaled into 65535 (a 0.5 depth came back as
0x8000, i.e. -32768), GL_BYTE/GL_UNSIGNED_BYTE emitted the stencil byte for a
DEPTH_COMPONENT read, and GL_HALF_FLOAT was rejected outright. The packing moves
into one PackDepthStencilTight that both arms call, so they cannot disagree again.
DirectVulkan blit: a single-aspect depth/stencil copy of a packed format went out
as a whole-image vkCmdCopyImage, which moves the packed native word and therefore
replaced the destination's other aspect with the source's - a depth-only
multisample resolve overwrote the destination's stencil. Same-format,
same-size, single-layer single-aspect copies now stage that one aspect through a
device-local buffer, exactly as ResolveWireDepthStencil does.
The indexed wrapper reads the bound VAO to tell a client index array from an
element-buffer offset, and the indirect one reconciles the command buffer's
shadow - both frontend reads, and under a transport the entry points run on the
apply thread where the client owns those bytes (Fatal{RoleViolation}, which the
split lane aborted on).
The client-memory upload's index scan waits for the GPU (SyncGpuWrites), and that
wait flushes the batch the frame's command buffer belongs to - so a handle taken
before it is stale by the time the draw records into it. Reconciling in the entry
point, before SetupDraw starts recording, keeps the wait out of the recording.
The backend draw entry points run on the apply thread when a transport is live,
where MGB_CTX->GetBoundVertexArray() is a frontend read the server role must not
make (Fatal{RoleViolation}) - which is what the split lane aborted on. With a
transport the CLIENT owns those bytes and snapshots them from the GL thread, so
the server has nothing to stage.
The commands live in GPU memory, so the draw is issued natively and the vertex
range was left unknown (vertexCount = 0, instanceCount = 1). That is exactly
right for a buffer-backed array and wrong for a client-memory one: its bytes must
be in the store before the GPU reads the command, and the range the upload is
bounded by came out as one element - so the quad the command named was never
staged and the draw painted nothing.
The words are now resolved on the CPU for exactly that case, with the shadow
reconciled first (they may be shader-written), and the draw stays native.
DirectGLES: the client snapshot's baseInstance shift belongs to the backend's
emulation, not to the element set. A driver that applies baseInstance itself
(UseNativeBaseInstance) must not also receive a shifted pointer - doing both
moved instance 0 from element 1 to element 2 and left the instance that reads
element 1 outside the snapshot. EmulatedFetchBaseInstance is not declared this
early in the file, so the wrapper states the same answer inline.
Magma: ResolveIndirectCommandBytes reconciles a GPU-written command block before
the callers read it. Every one of them reads the words on the CPU - the
per-command expansion, the baseInstance the shader emulation is fed, and the
range a client-memory vertex array's upload is bounded by - so a command a
compute shader wrote this frame was expanded as the poison the shadow still
held, which is no draw at all for glDrawArraysIndirect.
Magma: a client-memory INSTANCE-RATE binding is bounded by its instances
(baseInstance + (instanceCount - 1) / divisor), not by the vertex range; the
upload now covers the union of the two, which is what instances shifted off the
front of the array needed.
The element set a divisor'd client array fetches starts at GL's raw
baseInstance, but the pointer shift that carries it belongs to this backend's
emulation: when the driver applies baseInstance itself (UseNativeBaseInstance),
EmulatedFetchBaseInstance answers 0 and shifting the pointer as well would move
the fetch twice - instance 0 read element 2 instead of element 1, and the
instance that used element 1 read past the snapshot and vanished.
Also drops the two 'continue' statements the indirect executors' lambdas cannot
carry (a lambda is not a loop; 'return' leaves the pass).
The wire arm snapshots the elements a draw fetches out of the application's own
vertex/index memory (beba0256's MGPipeOwnedDrawInputs). The monolith arm's upload
answered only the non-indexed family's contiguous (first, count) run, so an
INDEXED or INDIRECT draw over a client-memory array staged nothing: the enabled
array kept whatever the ES context last held for it - normally disabled, i.e. every
vertex reading the generic current value - and the draw painted nothing with no GL
error anywhere.
ClientFetchPlan.h is that question factored out once (the fetches, the referenced
vertices, the per-divisor element sets) so both arms answer it from one place;
OwnedDrawInputs now calls it and keeps its behaviour byte for byte.
DirectGLES: SyncClientSideAttributesForDraw stages the sparse snapshot for a draw
whose elements the (first, count) run does not describe, driven by the plan; the
indexed entry points and both indirect executors ask for it, and the arrays path
learns the baseInstance shift a divisor'd client array needs.
W3: DirectGLES' six monolith failures are the four indexed/indirect shapes.
5310ff62 generates the R11F_G11F_B10F chain with a real GPU shader blit and gives
F1WireScenario.GenerateMipmapPackedFloatPixels pixel assertions, so the entry now
passes under strict split state and `Magma:mipmap-native-format@P7` survives in no
source file. The old arm would therefore have failed its own `test rc -ne 0`.
Both halves of the arm are retired in one commit, because either alone is a lie:
CMakeLists' fm loop drops the special case that gave this slot the
`integration-magma-p7-known-red` label, so the entry is discovered by the ordinary
`integration-magma-split` lane - the same label, environment and pixels assertion
as GenerateMipmapPixels and GenerateMipmapDepthPixels next to it, and the same
label the dual-block census lane already re-runs.
The lane membership alone would not be the old gate's equal: a lane counts its
entries' exit codes, not their absence. So the CI step keeps its shape and inverts
its expectation - the same single named entry, discovered and run by name under
MOBILEGL_IPC_STRICT_ERRORS=1 and MOBILEGL_IPC_ROLE_SPLIT_STATE=1, now asserted
through split_log_paths.py's require_green, which rejects a skip, a missing entry
and a failure alike. No assertion is skipped away or weakened: the pixels are
checked by the test, and the named-refusal check the log carried is what the GPU
blit retired.
40c5443f appended LevelWidth/LevelHeight/LevelDepth/LevelPad to MGPSubData and raised
MGP_ASSERT_POD to 88, but left MGP_FIELDS_MGPSubData at its old eight rows.
gen_pipe.py's check_field_lists_cover_struct_members runs in both modes and in
pipe-gates, so "member(s) with no F(...) in PipeFields.def: LevelWidth, LevelHeight,
LevelDepth, LevelPad" reddened both 'Regenerate the MGPipe interface' and
'--self-test'.
The four rows go in declaration order. LevelWidth/Height/Depth are ordinary members
(the record's full staged mip extent, which the ResourceSubData codec's
"ResourceSubData.LevelExtent" arm bounds the dirty box against). LevelPad is padding
by role only: PADDING_MEMBER_RE is ^Pad\d*$, so a member named LevelPad is a member
to the cover check and MUST have a row. That row risks no false positive - every
producer builds the record as `MGPSubData{}` and the payload rides the wire as one
POD memcpy of all 88 bytes, so both sides of a comparison read the same zero.
Field tables only describe the existing struct, so no generated file changes:
--check stays byte-identical.
The record carries eleven surfaces, but the walk writes the attachment points
only - eight colour plus depth and stencil - and the read surface is applied by
ApplyReadBufferFromRecord rather than by the walk. Say "the SAME attachment
points on it" instead of a count that is wrong for the walk.
Magma's generate path (VulkanRenderer::GenerateMipmap) took its window from the
BOUND object and its blits from the STORAGE image, and the two do not coincide
for a texture view: glTextureView gives a view its own BASE_LEVEL/MAX_LEVEL and
layer slice over somebody else's image. Three things followed from the mix-up,
all visible through a two-layer view over a four-layer 2D array:
* EnsureGenerateMipmapStorageAllocated allocated the chain THROUGH the bound
object from that object's OWN base extent, so the storage owner's level was
resized to the view's layer count - the storage behind its other layers was
gone - and the levels outside the view's window were redefined with it.
Immutable storage and views already own every level generation can write,
so neither allocates anything now, and a mutable owner gets only the levels
its window is missing (AllocateStorage RESIZES the level it is handed, so
an existing level keeps the shadow bytes its own upload path wrote).
* The level the chain starts at was the view's, applied to the storage image:
a view whose MIN_LEVEL is 1 generated from the storage's level 1 instead of
its level 2. The window now comes from the frontend's own plan
(MipmapGenerationPlan.h - the same numbers MGPMipPlan::LevelCount publishes)
and is shifted into the storage's numbering before any blit or barrier.
* Every layer of the storage was written instead of the view's window. The
blits carry the view's baseArrayLayer and layerCount now; every layer
outside that window belongs to the storage owner and is left as it was.
The end of the window follows MAX_LEVEL for the same reason the frontend's plan
does: a chain MAX_LEVEL cuts short is no longer generated past it.
The readback had the same shape of bug pointing the other way.
VulkanRenderer::GetTextureImage sized its copy by the storage image's array
layer count while starting it at the caller's layer origin, so a two-layer view
of a four-layer array copied four layers - two outside the window, and the last
one past the end of the image. glGetTexImage carries no destination size, while
the size glGetTextureImage validates is built from the VIEW's own extents, so a
caller that sized its buffer for the view had it overrun by exactly the layers
outside its window. The copy covers the caller's own layers now.
TextureViewScenario.GenerateMipmapThroughAViewLeavesTheOwnerShapeAlone runs on
both backends now. Its DirectVulkan skip named the reallocation above, and the
sub-image readback the skip blamed as well is not broken: a control run of the
same scenario with the generate removed reads every seeded level and layer of a
2D array back, mip 2 and 3 included, through both glGetTexImage and a per-layer
FBO attachment - the two mismatches that control reported are the colours the
generate was supposed to write. GetTexImageThroughAViewReturnsOnlyItsOwnLayers
is new and pins the readback window with canary slices past the view's last
layer; it fails on the reverted readback and passes on DirectGLES too.
Every place that stated "P5 does not chunk" as current behaviour except the
Fatal texts (previous commit) now records the stage chunk budget and what it
does not cover.
- CONTRACT-P5.md section 5: the STAGE_MB row names MGPipeStageChunkBytes() =
clamp(segment/4, 4096, segment), the two buffer walks (1e7c372e) and one
texture level's whole-width slabs (9469d48e, assembled by
StagedTextureStore::AdoptRun), and the Fatal that stays the backstop for a row
with no cut; the RING_MB row says the record bound is about the record's own
bytes.
- BRIEF-P5.md's R-10 section is no longer headed "does not chunk": it keeps the
record bound and the maxrec proof obligation unchanged and records that
content is cut, with the encoder Fatal as the fallback.
- ROADMAP.md's staging debt row and open question 11, ARCHITECTURE.md's
record-bound bullet and SEG_STAGE table note, README.md's knob row,
MEASUREMENTS.md section 6.3 and CURRENT_STAGE_PROGRESS.md's open item: the
explicit MOBILEGL_IPC_STAGE_MB=256 profile is no longer needed for the two cut
paths, and the carrier design for the rest stays P8.
- Historical results (notes/**/p5-results/, p5b/) are left exactly as recorded.
R-10's "P5 does not chunk (R-10)" wording predates the stage chunk budget: buffer
content ranges are cut at MGPipeStageChunkBytes() (1e7c372e) and a texture level
at whole-width slabs (9469d48e), so the claim is false for the two rows that can
outgrow SEG_STAGE - and these messages are what a bring-up reads.
- PipeWireEncoder::StageAllocate's two Fatal{RingOverrun, "SEG_STAGE"} sites now
say what reaching them means: a blob is staged whole, a row that cuts its
content at the stage chunk budget never arrives, so this is a record type with
no cut or a piece still larger than the arena.
- EncodeRecord's MaxRecordBytes arm says the bound is about the record's own
bytes - the stage chunk budget cuts blobs, not a header, payload or inline
tail - and keeps the integrator decision it names.
- Comments in PipeWireCodec.h, EmitTables.cpp's maxrecop line, Config.h's
STAGE_MB knob, PipeStats.h's MaxRecordBytes gauge, SplitRuntimePeek.h and
WireLedgerChecks.h said P5 does no chunking; they now say what is cut and what
is not.
- PipeWireCodecTest.ARecordLargerThanHalfTheRingIsFatalRingOverrun asserts the
new diagnostic text instead of "does not chunk (R-10)".
SyncToBackendByHandle gated its record-driven attachment walk (the eleven
applyPoint calls plus NoteFramebufferTextureAttachments) on
m_syncedRecordHashes[asTarget], a PER-TARGET slot, while the work inside is
object-level: glFramebufferTexture2D/glFramebufferRenderbuffer write the
driver FBO behind whichever binding is current, so a Draw walk and a Read walk
write the same points. Two slots can therefore hold two individually true,
jointly false accounts of one physical state.
improved-transparency flips a composite framebuffer between two attachment
states and syncs one of them as DRAW in a depth pre-pass. That walk detaches
the colour point, stamps the Draw slot; the end-of-frame READ-side sync of the
other record then found its own slot already stamped by an earlier identical
sync, walked nothing, left the point detached, and the driver refused the
following glBlitFramebuffer with INVALID_OPERATION - the default framebuffer
froze from frame 835. The object form is immune because a per-point version
array gates it underneath; the handle arm has no frontend array, so this memo
is its only gate.
Replace it there with m_syncedAttachmentRecordHash, one slot for the object.
The per-target array stays for the object arm, where it is a re-arm trigger
over that version array rather than a claim. InvalidateSyncedState now zeroes
both memos: it invalidates the version array, which is not the handle arm's
gate, so a memo left standing would let a re-sync of the same record skip the
re-attach that call exists to force.
Both cases were written against the wrong property and went red on the first WGB
build, which is the one thing a case may not do.
TextureEmit.ALevelTooLargeForTheStageChunkIsCutIntoSlabs asserted D == 8 slices for
every slab of the 512x128x33 level, whose last slab is the 33rd slice alone. The
per-slab check now names the scheme (Z = 8 * index, D = 8 for every slab but the
remainder) instead of restating one number for all of them.
StagedTextureStoreTest.ALevelWithAPieceMissingIsRefusedAsAWholeLevelReadByName
wanted "not covered" out of a single LEADING run, and the store cannot answer that:
it knows no format, so it cannot know how many bytes the level should have - the
check is "the range the reader asks is covered". The case now uses the shape the
check really is about, two runs with a hole between them (ServerLoopTest's
R-11 coverage case, one store over), and the looseness is written down: in the
header's coverage ruling, and as an assertion on the leading-run case beside it.
ONE RECORD'S BLOB IS STAGED WHOLE IN SEG_STAGE, a linear arena, so a texture
level larger than the segment's chunk budget was Fatal{RingOverrun, "SEG_STAGE"}
at the encoder rather than a split - and the CI traces hit it: the texture half of
resource_subdata stages the whole level shadow regardless of how little of it
changed, and a 512x128x33 GL_RGBA32F level is 34,603,008 bytes while a 192-cube
GL_RGBA16 level is 56,623,104, both against the default 32 MiB segment. Three
retrace cases aborted there: iterationt-in-world and iterationt-nodsa-in-world in
both backends, and iterationrp-in-world in DirectVulkan.
THE CLIENT now cuts such a level into WHOLE-WIDTH SLABS, one resource_subdata
each (TextureEmit.h's MGPipeForEachTextureSlab, driven at MGPipeStageChunkBytes -
the same cap PipeFill.cpp's buffer walks are cut at). A piece is a set of
consecutive rows of the level shadow, so its box names both its texels and its
byte run; whole slices are the coarsest cut, a slice bigger than the cap falls
back to whole rows and a row bigger than the cap to whole texels inside one row.
Every piece is emitted, dirty or not: the server assembles one level image out of
the runs, so a piece left out would leave a hole no texel read can answer. Each
piece carries at least one region (its clipped dirty rects, or its own box when
none reaches it) because RegionCount == 0 remains the whole-level spelling, and
each region's SrcOffset is rebased onto its own piece's run - MGPipeTypes.h has
always said SrcOffset is "into the blob" - while the strides stay the level's. A
level that FITS keeps the single whole-level record, field for field what it has
always been, which is what every existing gate is written against.
THE SERVER now assembles levels from those runs. StagedTextureStore::AdoptRun
places one run at an image offset, its coverage set is exactly the runs it has
adopted (StagedShadowStore's CoverageAdd, the buffer half's helpers), and only a
FULLY covered level satisfies IsCovered / RequireLevelBytes - a level with one
piece missing is as unreadable as one that never arrived, and it is the same
Fatal{StageSnapshotTooNarrow}. The whole-level spelling survives as Adopt, its
offset-0 replacement, and the two are told apart by the run's own LENGTH: the
whole-level run is the level's complete shadow whatever its box says, a piece's
run is exactly its box's byte extent. Both adoption call sites -
Ops_H_TextureSubData and PipeApply.cpp's hookless fallback, which is what Magma
takes - read the offset that way.
THE PROTOCOL IS UNCHANGED: no field moved, no caps bit was added. The one
compatibility statement is that a split level now crosses as several records of
one apply batch, so an OLD server (which replaces the level on every record) would
keep only the last piece - inproc runs both roles out of one library, so the
constraint is noted here rather than armed with a gate.
Tests. TextureEmit.ALevelTooLargeForTheStageChunkIsCutIntoSlabs drives the split
at MGPipeStageChunkBytesFor's clamp with both trace shapes (5 and 7 slabs), the
row-granularity and texel-granularity fallbacks, the three shapes that refuse to
cut, the region clipping/rebasing, and the piece no rect reaches.
StagedTextureStoreTest gains the multi-run assembly, the gap that must not read as
coverage, the partial-level Fatal by name, the run-offset discriminator, and two
production cases - the applier's hookless arm and DirectGLES's real TextureSubData
hook - that would put both halves at the front of the image if the offset were
dropped.
EnsureGeneratedMipmapStorageAllocated branched on the transport. The split arm
planned the generation window from BASE_LEVEL/MAX_LEVEL and refused to touch
immutable storage (70c8230b); the monolith arm rebuilt the chain from the bound
object's level-0 extent and then truncated it. Through a view that is
destructive in a way the split arm had already been fixed for: a view's level-0
extent is one slice of the owner's layers, so a two-layer view of a four-layer
array resized every owner level the generation touched to two layers - the
storage behind the owner's other layers was gone - and redefined the owner's
levels out of the view's window as well.
Both arms now take one path. An immutable texture or a view returns before any
allocation: it already owns every level generation can write. A mutable texture
gets levels from the plan's Base..End window and nothing else moves - the chain
is not truncated, because neither MAX_LEVEL nor a generate is a truncation (a
level-0 respecification drops the stale tail itself, in
DiscardMipmapChainOnBaseRespecification). MipmapGenerationPlan.h is no longer
behind MOBILEGL_BUILD_DISAGGREGATED for the same reason: the window is a
property of the GL call, not of how the backend is reached, and the plan the
wire carrier publishes (MGPMipPlan::LevelCount, end-exclusive) is this one.
The window rule was asserted only by the split-only F1WireScenario, whose
fixture skips under every other transport - so the monolith arm, which is what
a non-disaggregated library runs for every application, had no coverage at all.
TextureViewScenario.GenerateMipmapThroughAViewLeavesTheOwnerShapeAlone runs in
an integration-gpu lane under both transports and pins the owner's layer counts,
its own level extents, and the pixels of the levels and layers outside the
view's window. It is DirectGLES-only, and the skip names why: Magma reallocates
the chain through the bound object in its own generate path
(DirectVulkan/Renderer/VulkanRenderer.cpp's EnsureGenerateMipmapStorageAllocated),
and a CPU sub-image into the layers of mip 2 or 3 does not read back through
Magma at all - a run of the same scenario with the generate removed fails
identically - so the per-level colours this case is built on are unobservable on
that backend rather than merely wrong.
DirectVulkan resolved every default-framebuffer role against
m_imageIndexAcquired. Present() advances that to the image it acquires for
the frame that is about to start, and a present record is kWaitPresent - the
client does not wait for the apply - so a readback belonging to the frame
being presented (the retrace snapshot, taken at the swap call) can be applied
after the acquire and then resolve an image nothing had written. That is the
pure-black sundial-lite DirectVulkan retrace frame; DirectGLES is immune
because the driver keeps handing back the buffer the app drew into.
Split the GL-visible index out (VulkanRenderer::m_defaultFramebufferImageIndex)
and keep it out of the present/acquire path: it only re-points at the acquired
image when something WRITES the default framebuffer (DefaultFramebufferWriteIndex,
called at the FBO-0 render-pass creation, the deferred-clear-free direct writes
and the wire draw/clear/blit resolutions) and is reset on RecreateSwapchain.
Reads - glReadPixels both arms, the parked-clear materialization, a blit or copy
SOURCED from FBO 0, ResolveWireImage's read role (carrying the resolved index in
WireImage) - resolve DefaultFramebufferReadIndex instead, i.e. whatever that is.
A readback that resolves a swapchain image with no recorded write now says so
once (MGLOG_W_ONCE, both arms) instead of silently handing back raw pixels.
New integration scenario DirectVulkan|DirectGLES.DefaultFramebufferAcrossSwap
pins both halves: a readback after a swap still sees the drawn frame, and the
first draw after a swap lands in the image the readback then resolves.
glGenerateMipmap's native arm carries BASE_LEVEL/MAX_LEVEL of the view's own
ES name, so the LEVEL axis was already clipped the way the view describes, but
the layer set it writes is every layer of the storage the view aliases. A
generation through a windowed texture view therefore rewrote the owner's layers
outside the window (F1WireScenario
.GenerateMipmapThroughViewKeepsOwnerLevelsAndLayersOutsideItsWindow).
Both arms now read the window and generate the chain with per-layer blits
between the owner's levels MinLevel + L, over layers
[MinLayer, MinLayer + NumLayers), and leave the native call for the shapes it
already gets right: no view, a view whose layer window covers the whole
storage, a multisample view, and aspects the layer blits do not carry. The
record arm reads the window from the sampler view CSO with the same validation
SyncTextureViewToBackendByRecord uses (Fatal{ProtocolCorruption} on a stale or
foreign record); the monolith arm reads the same numbers off the frontend view
object. ScratchFBOImpl gains EnsureDepthAttachmentLayer for the depth-only
aspect.
PipeFill.cpp reaches MG_Remote::Client::MGPipeStageChunkBytes through
GpuWritePending.h, which the TU did not include: the declaration was visible to
the helper's own TU and to no other, so both MobileGL translation units for
PipeFill.cpp failed to compile while the shared objects it defines built fine.
PipeFill.cpp.
ResourceEmit.AWideBufferSubDataSplitsAtTheStageChunkBytes: the walk driven at
MGPipeStageChunkBytesFor's clamp (a quarter of the segment, floored at 4096 and
never above the segment), with the trace's own 128 MiB arena against the default
32 MiB segment - sixteen contiguous ascending pieces, each encodable by the
record builder, covering the range exactly once. The no-session answer (0 = keep
the record's own bound) is asserted beside it.
ResourceEmit.AZeroByteRespecifyDefinesTheStoreAndCarriesNoContent: armed the way
the split-only unit cases are (Transport = InProcess, an op table, and the caps
mirror R-8's liveness gate reads), BufferObject::Respecify(0, nullptr) must not
abort, must land Width == 0 && HasDefinedContent == 1 on the applier, and must
emit no resource_subdata at all. The drive runs in a child because the defect's
verdict is std::abort() and a case that aborts its own binary reports nothing.
MG_Test/Pipe/ResourceEmitTest.cpp.
glBufferData(target, 0, NULL, usage) DEFINES its store - that is not the
orphaning idiom - so HasDefinedContent is set, and MappedData() answers a
non-null pointer for it because the shadow reserves one byte whatever the size
(PipeResource.h:140-143). Reading MappedData() alone therefore put the call on
the split arm's respecify(nullptr)-plus-follow-up shape, whose walk emits
nothing for a zero-length range (ResourceTracker.h:253), and the
InitialBytesNotCarried self-check aborted by name on the first such call in the
bsl-esc-menu trace.
The predicate is 'HasDefinedContent && GetSize() > 0': there are bytes here,
not merely a defined store. The split arm's own condition is untouched - moving
it instead would send the zero-byte case to the UncarriedInitialBytes refusal
in the wire escape.
PipeFill.cpp.
One resource_subdata record's blob is staged whole in SEG_STAGE, so a range
larger than that arena is Fatal{RingOverrun, SEG_STAGE} at the encoder rather
than a split: the CI traces abort there on a 128 MiB arena's whole-buffer
follow-up against a 32 MiB segment.
- ClientSession::StageCapacityBytes(), the twin of EventRingCapacityBytes, so
the emitter can size against the arena the codec allocates from.
- MGPipeStageChunkBytes(): clamp(segment/4, 4096, segment), 0 when there is
nothing to fit (monolith, the server role's own uploads, no session).
- PipeFill.cpp's two buffer content walks take that cap instead of the
record's own 2^32-1 bound, so an over-long range becomes several contiguous
ascending records. StagedShadowStore::Adopt already accumulates coverage.
ClientSession.h/.cpp, GpuWritePending.h/.cpp, PipeFill.cpp.
The remove-artifact-clutter jobs deleted trace fixtures and results while
reading job/artifact inventories through unchecked process substitution, so
a failed GitHub API page looked like 'no failed cases' and fixtures for a
red retrace could be deleted. test.yml cleanup also did not wait for the
retrace-split lane even though those jobs download the same fixtures.
Read both paginated inventories into files before any deletion so an API
failure fails the step, add retrace-split to the cleanup dependencies, and
retain fixtures plus result artifacts for failed cases in all three lanes.
swung0x48