Phase 1 v3 amendment: VP9 writes 4 segments not 5, narrow common-file split
Janet AMEND-cycle re-review of v2 returned 2 surgical corrections: A. OFFSET_POC_HIGHBIT_REGS is HEVC/H.264 only — POC isn't a VP9 concept. rkvdec-vdpu381-vp9.c writes 4 segments (COMMON, CODEC_PARAMS, COMMON_ADDR, CODEC_ADDR), not 5. Confirmed via BSP hal_vp9d_vdpu382.c — no poc_highbit reference anywhere in VP9 path. B. Common-file split narrowed: only spec-computation functions (probability init, segmap state, entropy update) move into a rkvdec-vp9-common.h header as static-inline helpers. The register-writing functions (config_seg_registers, config_ref_registers, config_registers) STAY in legacy rkvdec-vp9.c — they touch the legacy flat struct rkvdec_regs and can't share with the new vdpu381 struct without abstracting the register backend (over-engineering). The new backend rewrites these against the vdpu381 struct. Bonus implementation hint: VDPU381_MODE_VP9=2 is already defined in rkvdec-vdpu381-regs.h:22 — Casanova's v7.0 series had VP9 in mind from the start, just didn't ship a backend. RCB sizing tuning deferred to Phase 6 (vp9d_refine_rcb_size formula from BSP). With both amendments applied, Janet's verdict carry-forward is PROCEED. Phase 2 implementation begins next session. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,49 @@
|
||||
# Phase 1 plan v3 — amendment per Janet re-review of v2
|
||||
|
||||
Date: 2026-05-17 ~01:55. Janet AMEND-cycle re-review of v2 returned verdict AMEND with two surgical corrections that don't require re-architecture. This document is the v3 delta; v2 stands as the architecture-of-record except for these two paragraphs.
|
||||
|
||||
## Amendment A: VP9 writes FOUR register-block segments, not five
|
||||
|
||||
v2 said `rkvdec-vdpu381-vp9.c` would model on `rkvdec-vdpu381-hevc.c`'s five-segment write pattern: COMMON_REGS / CODEC_PARAMS_REGS / COMMON_ADDR_REGS / CODEC_ADDR_REGS / POC_HIGHBIT_REGS.
|
||||
|
||||
**Correction**: `OFFSET_POC_HIGHBIT_REGS` is HEVC + H.264 only — POC (Picture Order Count) is not a VP9 concept. BSP `hal_vp9d_vdpu382.c` writes only four segments (COMMON, CODEC_PARAMS, COMMON_ADDR, CODEC_ADDR) plus reads INTERRUPT_REGS + STATISTIC_REGS for status. No `poc_highbit` reference exists in any VP9 BSP file.
|
||||
|
||||
**Implementation impact**: `rkvdec_vdpu381_vp9_run` issues 4 `rkvdec_memcpy_toio` calls (one per write segment), not 5. The `struct rkvdec_vp9_regs` definition in the extended `rkvdec-vdpu381-regs.h` omits a `poc_highbit` sub-struct.
|
||||
|
||||
## Amendment B: Common-file split — static-inline helpers in a header, not a separate `.c`
|
||||
|
||||
v2 proposed extracting `rkvdec-vp9-common.{c,h}` with both probability-table init AND `config_seg_registers` / `config_ref_registers` from legacy `rkvdec-vp9.c`.
|
||||
|
||||
**Correction**: `config_registers`, `config_seg_registers`, `config_ref_registers` all write to `regs->...` (the LEGACY flat `struct rkvdec_regs`). They cannot move to a common module that the vdpu381 backend can consume, because the vdpu381 backend uses a different register struct (`struct rkvdec_vp9_vdpu381_regs`). Moving them would either require an abstract register-backend layer (over-engineering) or break RK3399 by replacing them with a vdpu381-flavoured version.
|
||||
|
||||
**Revised boundary**: extract ONLY pure-computation / spec-only functions into `rkvdec-vp9-common.h` as `static inline` helpers (no `.c` file):
|
||||
- Probability table init (`init_intra_only_probs`, `init_inter_probs`, `init_probs`)
|
||||
- `rkvdec_vp9_priv_tbl` allocation/free (the buffer-layout logic, not the register-write side)
|
||||
- Segmap state machine (`segmapid` tracking)
|
||||
- Entropy update path (`rkvdec_vp9_done` → `rkvdec_init_v4l2_vp9_count_tbl`)
|
||||
|
||||
`config_seg_registers` and `config_ref_registers` are **REWRITTEN** in `rkvdec-vdpu381-vp9.c` against the vdpu381 register struct; the legacy versions stay in `rkvdec-vp9.c` unchanged. Code duplication is acceptable here because the two register structs are genuinely different; abstracting them is more code than duplicating.
|
||||
|
||||
**Regression risk**: drops to ~zero. The legacy `rkvdec-vp9.c` retains all its register-writing functions; the only delta is its `#include "rkvdec-vp9-common.h"` and the deletion of the spec functions whose bodies moved into the header. Compile-test of `rk3399_defconfig` becomes a fast sanity check, not a deep validation effort. R8 from v2 stands but is downgraded from BLOCKER-risk to AMENDMENT-risk.
|
||||
|
||||
## Free implementation hint
|
||||
|
||||
`VDPU381_MODE_VP9 = 2` is **already defined** in `rkvdec-vdpu381-regs.h:22`. The new backend's `dec_mode` field assignment is `regs->common.reg009_dec_mode.dec_mode = VDPU381_MODE_VP9` — constant exists, just write to it. Janet flagged this is the single most likely "copy-paste-bug" point: legacy writes `regs->common.reg02.dec_mode = RKVDEC_MODE_VP9` against a different struct. Do not let muscle-memory bridge the two namespaces.
|
||||
|
||||
## RCB sizing — Phase 6 follow-up
|
||||
|
||||
v2 marked RCB sizing as PROCEED-with-note (HEVC-tuned, oversized but harmless). Adding to Phase 6 punch-list: derive VP9-correct RCB sizes from BSP `vp9d_refine_rcb_size` (`mpp/hal/rkdec/vp9d/hal_vp9d_vdpu382.c:269`) as a follow-up patch after first-light VP9 decode works. Sub-optimal SRAM use, not blocking.
|
||||
|
||||
## File list (final for Phase 2)
|
||||
|
||||
| File | Action | Est. lines |
|
||||
|---|---|---|
|
||||
| `rkvdec-vdpu381-vp9.c` | NEW backend, 4-segment register write | 550-700 |
|
||||
| `rkvdec-vp9-common.h` | NEW header, static-inline helpers only | 200-300 |
|
||||
| `rkvdec-vdpu381-regs.h` | EXTEND with `struct rkvdec_vp9_regs` (4-segment) | +150-200 |
|
||||
| `rkvdec-vp9.c` | MINIMAL refactor — `#include "rkvdec-vp9-common.h"` + delete moved bodies | -50, +1 |
|
||||
| `rkvdec.c` | WIRE — `vdpu38x_vp9_ctrls` + entry in `vdpu381_coded_fmts[]` | +25 |
|
||||
|
||||
## Verdict carry-forward
|
||||
|
||||
Janet v2 verdict was AMEND with these two amendments. With Amendment A + B applied (as documented above), the plan is **PROCEED**. Phase 2 implementation can begin.
|
||||
Reference in New Issue
Block a user