iter8 Phase 7c + 8: close iter8 PARTIAL — Bug 4 narrowed via 5 eliminations
α-2 (POC strip removal) changed wire bytes (POC now matches kdirect's sentinel-encoded 0x10000) but H.264 output unchanged. POC not load-bearing. 5-codec regression sweep on α-2 backend: all 4 non-H.264 anchors hold. Zero regression. Iter8 close: 5/6 PASS, criterion-1 PARTIAL. Bug 4 narrowed but not fixed. Eliminations achieved: 1. libva-readback bug (γ dump) 2. Slot-binding wrong (γ dump shows correct slot per surface) 3. Stale residue (IMP-1 memset confirmed deterministic kernel write) 4. constraint_set_flags (Phase 5b CRIT-1: rkvdec source review) 5. POC sentinel strip (α-2 wire change, no output change) Remaining candidates for iter9: PPS diff (α-3), DECODE_PARAMS post-DPB fields (α-6), DPB entry order (α-4), slice data encoding (α-5). Fork tip 0226684 carries γ + IMP-1 diagnostic + α-2 hygiene. All env-gated off by default; α-2 is a wire-payload cleanup with zero behavior effect. Lessons distilled: - Reviews are never skippable — Phase 5b CRIT-1 saved a build cycle. - Wire-byte equivalence ≠ behavior equivalence. - Per-driver kludges in shared codec code need explicit gating. - Bug carryover labels can mislead (Bug 4 != "inter race-loss"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,98 @@
|
||||
# Iteration 8 — Phase 5c Review (α-2 POC sentinel strip removal)
|
||||
|
||||
Reviewed 2026-05-13. Reviewer: claude-sonnet-4-6.
|
||||
|
||||
Plan under review: `/home/mfritsche/src/fresnel-fourier/phase4c_iter8_plan.md`
|
||||
|
||||
Sources read:
|
||||
- `/usr/include/linux/v4l2-controls.h` — `struct v4l2_h264_dpb_entry` (lines 1635–1644)
|
||||
- `/home/mfritsche/src/linux-rfc/drivers/staging/media/rkvdec/rkvdec-h264.c` — lines 845–864, 975–991
|
||||
- `/home/mfritsche/src/linux-rfc/drivers/staging/media/rkvdec/rkvdec-regs.h` — `RKVDEC_CUR_POC` macro (line 150)
|
||||
- `/home/mfritsche/src/libva-multiplanar/libva-v4l2-request-fourier/src/h264.c` — lines 191–228 (helper + docblock), 308–313 (DPB call sites), 461–466 (current-frame call sites)
|
||||
|
||||
---
|
||||
|
||||
## Struct layout verification
|
||||
|
||||
The plan decodes the 32-byte strace blob using this layout:
|
||||
|
||||
| Offset | Field | Width |
|
||||
|---|---|---|
|
||||
| 0–7 | reference_ts | u64 |
|
||||
| 8–11 | pic_num | u32 |
|
||||
| 12–13 | frame_num | u16 |
|
||||
| 14 | fields | u8 |
|
||||
| 15–19 | reserved[5] | u8[5] |
|
||||
| 20–23 | top_field_order_cnt | s32 |
|
||||
| 24–27 | bottom_field_order_cnt | s32 |
|
||||
| 28–31 | flags | u32 |
|
||||
|
||||
Verified against `/usr/include/linux/v4l2-controls.h` lines 1635–1644. `struct calcsize` = 32. The reserved[5] padding gap (bytes 15–19) in the system header is explicit (`__u8 reserved[5]`) — no compiler-inserted padding. The plan's offset table is correct.
|
||||
|
||||
The decoded values from the kdirect blob are:
|
||||
`top_field_order_cnt = 0x10000 (65536)`, `bottom_field_order_cnt = 0x10000 (65536)`, `flags = 0x03`.
|
||||
|
||||
The Phase 5b CRIT-1 re-read ("kdirect flags = 0x30001") was a misalignment error — the plan's correction is empirically confirmed.
|
||||
|
||||
---
|
||||
|
||||
## CRIT findings
|
||||
|
||||
**No CRIT findings.** The strace re-decode is structurally correct. The POC diff is real.
|
||||
|
||||
---
|
||||
|
||||
## IMP findings
|
||||
|
||||
### IMP-1: RKVDEC_CUR_POC masks to 32 bits — no masking of the sentinel specifically
|
||||
|
||||
`rkvdec-regs.h:150`:
|
||||
```c
|
||||
#define RKVDEC_CUR_POC(x) ((x) & 0xffffffff)
|
||||
```
|
||||
|
||||
This is a 32-bit mask on a 32-bit `s32` argument — it passes the value through unchanged. No bit-16 masking, no sentinel suppression. The full 32-bit POC value including the sentinel `0x10000` is written verbatim to `RKVDEC_REG_CUR_POC0` / `RKVDEC_REG_CUR_POC1`.
|
||||
|
||||
Similarly, lines 975–978 write `dpb[i].top_field_order_cnt` and `dpb[i].bottom_field_order_cnt` directly via `writel_relaxed` with no intervening mask.
|
||||
|
||||
The hardware therefore receives POC = 65536 from kdirect (sentinel preserved) vs POC = 0 from libva (sentinel stripped). These are numerically different inputs to the hardware's POC-based reference picture matching. The fix hypothesis is structurally sound.
|
||||
|
||||
### IMP-2: Strip removal also changes current-frame POC at h264.c:462-465 — flag it, but it's correct
|
||||
|
||||
Call sites at lines 461–466 strip the sentinel from `VAPicture->CurrPic.TopFieldOrderCnt` / `BottomFieldOrderCnt` before writing to `decode->top_field_order_cnt` / `decode->bottom_field_order_cnt`. Removing the strip (Option A) changes these fields too, not just the DPB entries.
|
||||
|
||||
This is the right behaviour: rkvdec line 988 writes `dec_params->top_field_order_cnt` to `RKVDEC_REG_CUR_POC0` via `RKVDEC_CUR_POC`, and the hardware compares current-frame POC against reference POC for temporal distance calculations. Both sides must agree on whether the sentinel is present. kdirect sends the sentinel on both sides; Option A makes libva consistent with kdirect.
|
||||
|
||||
**The plan's LOC estimate covers only 4 call sites (h264.c:309, 312, 462, 465) — confirmed by source read. The estimate is accurate.**
|
||||
|
||||
### IMP-3: Option B not needed now, but add a TODO comment if Option A is chosen
|
||||
|
||||
The plan argues Option A is safe because hantro-decoder doesn't support H.264 on RK3399. This is correct in the campaign context. However, the docblock at h264.c:191-218 documents the hantro-specific reason the strip was added. If Option A removes the function entirely, that rationale disappears.
|
||||
|
||||
**Amendment**: Keep the helper function body but change it to return `poc` (or `poc` when valid, 0 when INVALID) without the sentinel subtraction. Add a one-line comment: `/* rkvdec writes POC verbatim to MMIO; stripping hantro's sentinel is wrong here */`. If a future port to hantro+H.264 needs the strip, the comment shows where to re-gate it. This costs 0 extra LOC net (same function, different body).
|
||||
|
||||
Alternative: delete the function and inline `return poc;` at each call site — also fine, slightly cleaner.
|
||||
|
||||
---
|
||||
|
||||
## MIN findings
|
||||
|
||||
### MIN-1: R-2 probability rating ("medium") is reasonable
|
||||
|
||||
The plan rates "POC diff isn't root cause" at medium. Given that (a) flags match, (b) fields match, (c) POC is the only confirmed numerical diff, and (d) kdirect with sentinel decodes correctly, medium is slightly conservative but appropriate — there may be slice-header fields that also differ (Phase 5b IMP-2 PPS concern). The rating doesn't need changing.
|
||||
|
||||
---
|
||||
|
||||
## Amendments
|
||||
|
||||
1. **Option A greenlit with one amendment**: When removing the sentinel subtraction, preserve the `VA_PICTURE_H264_INVALID` guard (`if (flags & VA_PICTURE_H264_INVALID) return 0;`) — empty DPB slots must still return 0. Only remove the `if (poc & (1 << 16)) return poc - (1 << 16);` branch. The function becomes a conditional identity, which can be inlined or kept.
|
||||
|
||||
2. **Document both sides**: The current frame's POC call sites (h264.c:462, 465) are affected by Option A — this is correct and intentional. Phase 6c commit message should call this out explicitly so the reviewer doesn't flag it as an unintended scope change.
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
**Proceed with Option A.** The strace re-decode is correct. The struct layout is verified. rkvdec writes the full 32-bit POC with no masking. The strip is the confirmed diff between libva and kdirect. No CRIT objections.
|
||||
|
||||
Phase 6c is a 5-LOC change. Phase 7c will confirm whether full-plane writes appear in the γ dump.
|
||||
Reference in New Issue
Block a user