panvk-bifrost/mesa-panvk-bifrost/phase5_iter13_close.md

Phase 5 close — iter13 second-model review

Reviewer: janet (ARM/DDR bare-metal specialist agent — closest available to driver/NIR review). Verdict: NEEDS FIX BEFORE MERGE (one CRITICAL, two HIGH, two MEDIUM, two LOW). Outcome: all CRITICAL + HIGH addressed in this phase, MEDIUM + LOW addressed where cheap.

CRITICAL: single-variant ships, dual-variant was Phase 2 lock

Janet's catch: a pipeline with XFB-decorated outputs used in a NON-XFB draw (or in an XFB draw with fewer buffers bound than declared) would write nir_store_global to address 0 → GPU page fault → DEVICE_LOST.

The Phase 2 lock specified dual-variant (B). Phase 4 shipped single-variant (closer to option C). Janet recommended the dual-variant refactor.

Resolution: option Z (better than B or C) — Panfrost-Gallium memory-sink idiom.

While re-reading gallium/drivers/panfrost/pan_cmdstream.c:1339-1366, I found the Gallium PAN_SYSVAL_XFB handler does exactly this: when no XFB target is bound, it sets the address sysval to 0x8000_0000_0000_0000 (= PAN_SHADER_OOB_ADDRESS). The Bifrost MMU silently discards stores to this address. No fault. No dual variants. Single-variant solution at no runtime cost.

Applied in panvk_vX_cmd_draw.c:

uint64_t _xa0 = PAN_SHADER_OOB_ADDRESS, _xa1 = PAN_SHADER_OOB_ADDRESS,
         _xa2 = PAN_SHADER_OOB_ADDRESS, _xa3 = PAN_SHADER_OOB_ADDRESS;
if (_gfx->xfb.active) {
   if (_gfx->xfb.buffer_count > 0 && _gfx->xfb.buffers[0].addr)
      _xa0 = _gfx->xfb.buffers[0].addr + _gfx->xfb.buffers[0].offset;
   /* ... 1..3 ... */
}

Plus #include "pan_compiler.h" for the constant.

Saved as project memory project_pan_shader_oob_address.md — the canonical conditional-write idiom for Panfrost. Will be useful for any future feature with driver-state-conditional shader writes.

A new regression probe probe_xfb_nodraw.c covers Janet's exact scenario: same XFB-capable pipeline as probe_xfb.c but no Bind/Begin/End and no buffer bound, just a raw vkCmdDraw. Expected: [PASS] XFB-capable pipeline survives non-XFB draw — memory-sink active. (DEVICE_LOST = FAIL).

HIGH: _pad_xfb ghost padding

Janet's catch: the explicit uint32_t _pad_xfb after num_vertices was supposed to keep 8-byte alignment for aligned_u64 xfb_address[4], but the compiler inserts another 4 anonymous bytes regardless because aligned_u64's alignment attribute already triggers padding. The named field is misleading.

Fix: removed _pad_xfb entirely. aligned_u64 does the right thing on its own. Comment in struct explains.

HIGH: dirty-mark on Begin/End

Janet's concern was about variant re-selection. With the memory-sink fix, only one variant exists — variant selection is moot. The sysval address value changing across Begin/End is caught by set_gfx_sysval's memcmp + BITSET mechanism, which marks the FAU upload dirty. No additional dirty-mark needed.

Confirmed by the probe: Begin → buffer addr propagates → store_global writes captured data; End → buffer addr would flip back to OOB on the next draw if there was one. (No new probe needed; this path is exercised by probe_xfb.c.)

MEDIUM: counter-buffer silent drop

CmdBeginTransformFeedbackEXT silently ignored pCounterBuffers != NULL despite advertising transformFeedbackDraw=false. Apps reading the spec carefully will not pass counter buffers, but defensive logging helps debugging.

Fix: loud mesa_logw when counter buffers are passed:

panvk: CmdBeginTransformFeedbackEXT: counter buffers not implemented
       (transformFeedbackDraw=false); XFB resume will restart at buffer offset 0

MEDIUM: buffer_count not reset on cmd buffer reset

Stale xfb.buffer_count from a previous recording could leak into a new one.

Fix: in panvk_per_arch(BeginCommandBuffer) for JM, memset(&cmdbuf->state.gfx.xfb, 0, sizeof(...)). Three-line change. Gated on PAN_ARCH < 9 because the xfb substruct only exists there.

LOW: UINT32_MAX vs (1ULL<<32)-1

Janet noted Phase 2 spec was (1ULL << 32) - 1 but I used UINT32_MAX. Numerically identical. Type expression matters less here than I'd thought — VkDeviceSize is uint64 and both forms widen identically. Left as UINT32_MAX for terseness.

LOW: duplicated transformFeedbackPreservesProvokingVertex

Janet noted a duplicated = false between the features block and the EXT_provoking_vertex properties block. Both correctly false; no behavior impact. Defer to upstream Mesa style — this is how mainline panvk_vX_physical_device.c shapes its physical-device fill-in. Not iter13's place to refactor.

What's open

Items Janet flagged as missing for ANGLE/Chromium GLES3 emulation later:

1. vkCmdDrawIndirectByteCountEXT — needed if ANGLE hits glDrawTransformFeedback. Deferred — transformFeedbackDraw=false is spec-compliant. If iter14 hits this in Brave testing, add then.
2. VK_QUERY_TYPE_TRANSFORM_FEEDBACK_STREAM_PRIMITIVES_WRITTEN_EXT — needed if ANGLE uses GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN. Deferred for the same reason — transformFeedbackQueries=false.

Both are loud-fail (extension/feature not present), not silent-broken. Acceptable.

Verdict

Review obstacles cleared. All CRITICAL + HIGH addressed; both MEDIUM addressed. iter13 ready for Phase 6 integration test.

— claude-noether, 2026-05-20