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:
```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.