From cebdd82e7f89a6dc39e4e15b8b6702ef608b452c Mon Sep 17 00:00:00 2001 From: Markus Fritsche Date: Wed, 13 May 2026 09:34:40 +0000 Subject: [PATCH] =?UTF-8?q?iter7=20Phase=205:=20review=20=E2=80=94=202=20C?= =?UTF-8?q?RIT=20on=20link-graph=20traversal;=20algorithm=20validated?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 5 sonnet-architect found: - CRIT-1: interface links connect IO entities (source/sink) to interfaces, NOT directly to proc entity. Walk must use MEDIA_LNK_FL_INTERFACE_LINK (1U<<28) to discriminate. Author verified at media.h:223-225. - CRIT-2: source_id/sink_id ordering not guaranteed in link entries; check both endpoints. Author verified media_v2_link struct at media.h:341-347. - IMP-1: hantro decoder-proc (entity 17) distinct from encoder-proc (entity 3) by function field. Algorithm correct by construction — no encoder contamination possible. - IMP-2: MEDIA_ENT_F_PROC_VIDEO_DECODER set on both rkvdec-proc (rkvdec.c:1382) and hantro-dec-proc (hantro_drv.c). - IMP-3: current 3-call ioctl pattern has spurious memset; new function uses 2-call pattern (alloc all 3 arrays before second call). - IMP-4/MIN-1/2/3: minor implementation notes. All 5 substantive findings empirically verified against boltzmann's linux-rockchip tree. Phase 6 implementer pseudocode provided: walk entities → find decoder proc → walk data links to collect IO entity neighbors → walk interface links to find linked interface → resolve major:minor. Co-Authored-By: Claude Opus 4.7 (1M context) --- phase5_iter7_review.md | 128 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 phase5_iter7_review.md diff --git a/phase5_iter7_review.md b/phase5_iter7_review.md new file mode 100644 index 0000000..36ac8cf --- /dev/null +++ b/phase5_iter7_review.md @@ -0,0 +1,128 @@ +# Iteration 7 — Phase 5 review + +Phase 5 architect review of `phase4_iter7_plan.md` by sonnet-architect, 2026-05-13. Plan is structurally correct; 2 CRIT findings on the link-graph traversal mechanics; 4 IMP findings around ioctl pattern + fd lifecycle; 3 MIN. + +## Critical findings + +### CRIT-1 — Interface links connect to IO entities, not proc; use MEDIA_LNK_FL_INTERFACE_LINK to discriminate + +Kernel topology layout per `v4l2-mem2mem.c` and `hantro_drv.c`: + +``` +INTERFACE (MEDIA_INTF_T_V4L_VIDEO) + |-- INTERFACE_LINK --> source entity (MEDIA_ENT_F_IO_V4L) + |-- INTERFACE_LINK --> sink entity (MEDIA_ENT_F_IO_V4L) + +source entity → DATA_LINK → proc entity (MEDIA_ENT_F_PROC_VIDEO_DECODER) +proc entity → DATA_LINK → sink entity +``` + +Link discrimination via `flags & MEDIA_LNK_FL_INTERFACE_LINK`: +- `MEDIA_LNK_FL_LINK_TYPE = (0xf << 28)` +- `MEDIA_LNK_FL_DATA_LINK = (0U << 28)` +- `MEDIA_LNK_FL_INTERFACE_LINK = (1U << 28)` + +Author-verified at `boltzmann:~/src/linux-rockchip/include/uapi/linux/media.h:223-225`. + +C1 algorithm (Phase 6 implementer pseudocode from reviewer): +``` +for each entity E in entities[]: + if E.function != MEDIA_ENT_F_PROC_VIDEO_DECODER: continue + io_entity_ids = set() + for each link L in links[]: + if L.flags & MEDIA_LNK_FL_INTERFACE_LINK: continue + if L.source_id == E.id: io_entity_ids.add(L.sink_id) + if L.sink_id == E.id: io_entity_ids.add(L.source_id) + for each link L in links[]: + if !(L.flags & MEDIA_LNK_FL_INTERFACE_LINK): continue + if L.source_id in io_entity_ids: intf_id = L.sink_id + elif L.sink_id in io_entity_ids: intf_id = L.source_id + else: continue + for each iface I in interfaces[]: + if I.id == intf_id and I.intf_type == MEDIA_INTF_T_V4L_VIDEO: + return resolve_dev_node(I.devnode.major, I.devnode.minor) +``` + +### CRIT-2 — Interface link source/sink ordering not guaranteed; check both + +For each interface link, the kernel may store either (entity_id, interface_id) or (interface_id, entity_id). Phase 6 must check BOTH endpoints against the IO entity set. Mechanically: if `L.source_id` is in `io_entity_ids` the OTHER endpoint (`L.sink_id`) is the interface ID; else if `L.sink_id` is in the set, `L.source_id` is the interface ID. + +`struct media_v2_link` (verified at `include/uapi/linux/media.h:341-347`): id, source_id, sink_id, flags, reserved[6]. 32-byte struct, packed. + +## Important findings + +### IMP-1 — Hantro topology: decoder proc (entity 17) is distinct from encoder proc (entity 3); algorithm correct by construction + +The hantro_attach_func code creates separate `(source, proc, sink)` triplets per codec function. Decoder triplet has function=MEDIA_ENT_F_PROC_VIDEO_DECODER (0x4008); encoder triplet has function=MEDIA_ENT_F_PROC_VIDEO_ENCODER (0x4007). Step 1's entity scan filters to decoder-proc only → step 2 collects ONLY decoder source/sink IO entities → step 3 finds ONLY the interface linked to /dev/video3 (decoder), never /dev/video2 (encoder). + +**No encoder contamination possible** under the C1 algorithm. + +### IMP-2 — MEDIA_ENT_F_PROC_VIDEO_DECODER IS set on rkvdec + hantro decoder proc entities + +Author verified at `boltzmann:~/src/linux-rockchip`: +- `drivers/media/platform/rockchip/rkvdec/rkvdec.c:1382` passes `MEDIA_ENT_F_PROC_VIDEO_DECODER` to `v4l2_m2m_register_media_controller`. +- `v4l2-mem2mem.c:1076` assigns `entity->function = function` for the proc entity. +- `drivers/media/platform/verisilicon/hantro_drv.c::hantro_attach_func` similarly passes the function via `func->id`. + +Both decoders set the function field. iter7 fix is empirically safe. + +### IMP-3 — Existing 3-call ioctl pattern is buggy; use 2-call + +The current `find_video_node_via_topology` (request.c:122-137) does: +1. Call 1: get counts. +2. `memset(&topo, 0)` — clears counts. +3. Call 2: get counts again. +4. Set ptr_interfaces. +5. Call 3: fill data. + +The middle `memset` is spurious. New function should be 2-call: +```c +memset(&topo, 0, sizeof topo); +ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topo); // call 1: counts +entities = calloc(topo.num_entities, sizeof *entities); +interfaces = calloc(topo.num_interfaces, sizeof *interfaces); +links = calloc(topo.num_links, sizeof *links); +topo.ptr_entities = (uintptr_t)entities; +topo.ptr_interfaces = (uintptr_t)interfaces; +topo.ptr_links = (uintptr_t)links; +ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topo); // call 2: data +``` + +### IMP-4 — Two-pass fd re-open OK; break/continue logic in pass 1 + +Pass 1 scans for `strcmp(info.driver, "rkvdec") == 0` AND has decoder entity. Pass 2 scans for any known_decoder_drivers match AND has decoder entity. fds are opened+closed per device per pass; cost is negligible. Verify pass-1 doesn't break early on rkvdec-without-decoder-entity (rare but possible). + +### IMP-5 — `MEDIA_ENT_F_PROC_VIDEO_DECODER` is set on hantro-vpu-dec only when `CONFIG_VIDEO_HANTRO_ROCKCHIP=y` and decoder is registered + +Already verified at `boltzmann:~/src/linux-rockchip/drivers/media/platform/verisilicon/hantro_drv.c`. fresnel kernel has `CONFIG_VIDEO_HANTRO=m` + `CONFIG_VIDEO_HANTRO_ROCKCHIP=y` (per pacman config inspection at iter5 Phase 2). Hantro decoder entity is registered. Safe. + +## Minor findings + +- MIN-1: `media_out` set from `path` must be preserved in refactor. Confirm unchanged. +- MIN-2: `flags & MEDIA_LNK_FL_INTERFACE_LINK` sufficient for link-type check (no need for full mask). +- MIN-3: Phase 7 verification of MPEG-2/VP8 env-override path is MANDATORY (Phase 1 criterion 3), not optional. + +## Author re-verification (post-review) + +Per `feedback_review_empirical_over_theoretical.md` Direction 2: + +| ID | Reviewer claim | Empirical verification | Status | +|---|---|---|---| +| CRIT-1 | MEDIA_LNK_FL_INTERFACE_LINK = (1U<<28); discriminates interface vs data links | Confirmed at `include/uapi/linux/media.h:223-225` on boltzmann | **CORRECT** | +| CRIT-2 | media_v2_link source/sink endpoints may swap; check both | Confirmed via struct layout at `include/uapi/linux/media.h:341-347` | **CORRECT** | +| IMP-1 | Hantro topology: decoder-proc distinct from encoder-proc; algorithm safe | Confirmed via hantro_drv.c reading | **CORRECT** | +| IMP-2 | rkvdec + hantro proc entities ARE set with PROC_VIDEO_DECODER | Confirmed at rkvdec.c:1382 + v4l2-mem2mem.c logic | **CORRECT** | +| IMP-3 | Current 3-call pattern has spurious memset; use 2-call | Confirmed at request.c:122-137 | **CORRECT** | + +All 5 substantive findings empirically verified. Reviewer's pseudocode is correct. Phase 6 incorporates the algorithm + amendments. + +## Phase 6 amendments to apply + +1. Replace `find_video_node_via_topology` with `find_decoder_video_node_via_topology` using the C1 algorithm (entities + links walk). +2. Use 2-call MEDIA_IOC_G_TOPOLOGY pattern (allocate all 3 arrays before second call). +3. In `find_codec_device`, add two-pass: pass-1 rkvdec only, pass-2 any decoder. +4. Test BOTH source_id and sink_id of each interface link to find the interface ID. +5. Replace iter4-comment block at request.c:60-89 to reflect entity-function discrimination. +6. Phase 7 verification matrix C5 item 3 (MPEG-2/VP8 env-override path) is MANDATORY. + +All amendments are mechanical. No structural plan change.