fresnel-fourier/phase5_iter7_review.md

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:

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.