From 7e1421be7cfd892234555e67e9045a5134dd4e4f Mon Sep 17 00:00:00 2001 From: Matthew Stanley <1379tech@gmail.com> Date: Wed, 5 Aug 2026 17:06:20 -0700 Subject: [PATCH] docs: correct the Tomba 2 perf measurement (first attempt was interp-bound) The first FMV A/B was run on a fresh worktree whose overlay shard cache was cold AND whose autocompile could never work: game.toml points at psxrecomp-v4/recompiler/build-t2/psxrecomp-game.exe while the documented build recipe builds the recompiler at build-recompiler/ in the worktree root, so it failed 8/8 with WinError 2 and the runtime silently ran interpreted (dispatch_native = 0). The '40 fps / ~19 underruns per second' attributed to master in the previous commit was that broken setup, not master. Re-measured against the fully warm cache with the pre-SPU exe as a control in the same worktree: interp_share 0.7813 vs 0.7775, exc_share 0.2386 vs 0.2381, 39.1 vs 38.0-38.4 fps. Identical within noise, so the DSP work is free. Separately characterises the real pre-existing defect: the FMV is 96% native by dispatch count but 78% interpreted by wall clock with 24% in exception handling, even warm. Co-Authored-By: Claude Opus 5 (1M context) --- docs/internal/SPU_FIDELITY_103.md | 683 +++++++++++++++--------------- 1 file changed, 351 insertions(+), 332 deletions(-) diff --git a/docs/internal/SPU_FIDELITY_103.md b/docs/internal/SPU_FIDELITY_103.md index 31360f63..cb4aabc1 100644 --- a/docs/internal/SPU_FIDELITY_103.md +++ b/docs/internal/SPU_FIDELITY_103.md @@ -1,332 +1,351 @@ -# SPU fidelity burndown — Issue #103 and PR #102 - -Branch: `feat/spu-fidelity-103` · Worktree: `_wt-spu-103` · Base: `origin/master` `7f7fbc8e` - -Closes the four gaps named in Issue #103 (reverb, noise, volume sweeps, SPU IRQ) -and lands the separable, verified parts of PR #102. Written so the disposition of -each piece is auditable rather than asserted. - ---- - -## 1. Scope overlap between #102 and #103 - -They intersect on exactly one item. - -| Item | Issue #103 | PR #102 | Disposition | -| --- | --- | --- | --- | -| Reverb | ✔ asked | — | Implemented here, clean-room (§3) | -| Noise LFSR | ✔ asked | — | Implemented here, clean-room (§3) | -| Volume sweeps | ✔ asked | — | Implemented here, clean-room (§3) | -| SPU IRQ | ✔ asked | ✔ offered | **One implementation**, ours, in `spu.c` | -| CAUSE.IP2 combinational | — | ✔ offered | Taken, verified, extended (§2) | -| Mid-dispatch audio pump | — | ✔ offered | Taken, with a regression fixed (§2) | -| SPU RAM DMA readback | — | ✔ offered | Taken as-is in substance (§2) | - -So this is one coherent change, not two competing ones. PR #102 covers 1 of -#103's 4 items; the other three were never in it. - -## 2. PR #102 — per-piece verdict - -PR #102's four changes were reviewed as four separable items, because they carry -very different risk. Attribution to Alexandros Mandravillis is recorded in the -commit trailer and in the source comments; the code here is a reimplementation -on the same findings, because two of the PR's three commits touch `spu.c` and -would have collided with the clean-room work. - -### 2a. CAUSE.IP2 combinational — CONFIRMED, taken, and extended - -The PR's claim is that `CAUSE.IP2` is a live mirror of the interrupt line rather -than a latched bit. **The claim is correct**, and it was checked against the -in-tree Beetle oracle rather than accepted on the PR's reasoning: - -- `beetle-psx/mednafen/psx/irq.cpp` defines - `#define Recalc() PSX_CPU->AssertIRQ(0, (bool)(Status & Mask))` - and calls it from `IRQ_Assert` (raise), from **both** halves of `IRQ_Write` - (the Status ack and the Mask write), and from `IRQ_Power`. -- `beetle-psx/mednafen/psx/cpu.cpp:289 AssertIRQ()` clears `CAUSE` bit `10+n` - unconditionally, then re-sets it only if the level is asserted. - -That is the definition of a combinational mirror, and it matches R3000A: the -`Cause.IP` field is not storage, it reflects the interrupt pins. Our runtime -previously only ever OR'd bit 10 in at delivery and never cleared it. - -Extensions beyond the PR: - -1. **A second latching writer the PR missed.** `runtime/src/psx_interpreter.c` - had its own `cpu->cop0[COP0_CAUSE] |= (1u << 10);` at interrupt delivery — - the identical defect in the standalone interpreter path. Fixed too. An audit - of every writer of `cop0[13]` confirms no other site touches bit 10: the - interpreter's `MTC0` paths correctly mask guest writes to `0x0300` - (software-interrupt bits only), and the exception paths mask `~0x7C` / - `~0x8000007C`, preserving the IP field. -2. **Single ownership.** The compiled delivery path no longer sets bit 10 - itself; `psx_irq_refresh_cause_ip2()` is the only writer. Two writers of one - combinational bit is how the bug survived in the first place. -3. **Power-on recompute**, mirroring Beetle's `IRQ_Power() -> Recalc()`. -4. **A regression test**, `runtime/tests/test_cause_ip2_combinational.c`, - covering rise, fall on ack, fall on mask, partial ack with another source - still pending, partial-width register writes, preservation of ExcCode/BD/IP0/ - IP1, and out-of-range I_STAT bits. Registered in CTest. - -Risk note: this is a real behavioural change to core IRQ delivery for every -title, and it is the highest-risk item in this branch. It is nonetheless the -faithful behaviour, so per Rule -1 it lands and titles are revalidated rather -than the fix being narrowed. - -### 2b. Mid-dispatch audio pump — taken, with a regression the PR would have shipped - -The reasoning is sound: the SPU is autonomous on hardware, our pump is driven -from the main loop between presented frames, so a guest busy-wait that never -presents a frame freezes SPU time — self-deadlocking for any game waiting on an -SPU-generated condition. The VBlank edge is derived from the guest cycle -counter, so it keeps firing through such a wait and is a defensible place to -pump. Confirmed single-threaded: `spu_render` runs on the main loop thread -(`sdl_audio_pump` is the producer; the SDL callback only drains an SPSC ring), -so this introduces no data race on `i_stat`/`cop0`. - -**Defect found and fixed.** PR #102 wires the hook to `sdl_audio_pump()` -unconditionally. But `sdl_audio_update()` is the sole authority on whether a -pump should emit, discard, or not run: - -- hard mute (turbo loads) → *does not pump at all*; the queue drains and voice - positions freeze in place, so music resumes where it left off instead of - replaying time-compressed garble. This is user-validated behaviour. -- turbo sink → pumps with `discard_output = true`. - -Pumping unconditionally from the VBlank edge bypasses both, which would push -real audio during every turbo load. The hook now goes through -`sdl_audio_pump_midframe()`, which mirrors the gate state -(`AUDIO_GATE_NORMAL` / `MUTED` / `SINK`) that `sdl_audio_update()` last set. - -Known tension, recorded rather than guessed at: in the `MUTED` state SPU time -still freezes, so a game that busy-waits on an SPU condition *during* a turbo -load would stall. No title in our suite is known to do this. Advancing SPU time -under mute would change validated mute audio, so it is not being done -speculatively. - -Placement: the pump call sits at the **end** of `fire_vblank_edge()` so the -VBlank's own raise and ring records complete first — the pump can itself raise -an SPU IRQ, which should land after the edge rather than interleaved into it. - -### 2c. SPU RAM DMA readback — taken - -DMA4 in the SPU→RAM direction previously wrote literal zeros. SPU RAM is -readable memory; this was not a transfer at all. Titles that carry a checksummed -block through SPU RAM across an `Exec` boundary (SPU RAM being one of the few -regions a main-RAM reload does not touch) read zeros, failed their own integrity -check, and fell back to a cold-boot path. Lowest-risk item in the PR: -previously-dead path, no existing behaviour depends on the zeros. - -### 2d. Validation claims — NOT reproduced, and cannot be - -PR #102 validates against Medal of Honor Underground (SLUS-01270) and Gran -Turismo (SCUS-94194). Neither disc is in our suite. Those specific claims are -**unverified by us and should not be repeated as if they were**. What we -verified independently is the hardware behaviour behind each change (§2a–2c) and -non-regression across our own titles. - -## 3. Issue #103 — why this had to be clean-room - -⚠ **This is the load-bearing constraint on the whole item, and it is why #103 -sat open.** - -`psxrecomp` is distributed under **PolyForm Noncommercial 1.0.0**. Beetle PSX / -mednafen is **GPL-2.0-or-later**. They are incompatible: GPL source cannot be -folded into a PolyForm distribution. Beetle's tree contains a complete, correct, -well-tested reverb — so the obvious implementation route is closed, and two -contributors independently walked into it: - -- **PR #16** (Martin Penkava) implemented all of this by porting Beetle. Parked - on license grounds after a line-by-line audit found it tracked - `beetle-psx/mednafen/psx/spu.cpp` lines 590–730 in register layout, resampling - coefficients, buffer layout, arithmetic and processing order. See - `docs/internal/upstream/martin-pr16-spu-reverb.md` on branch - `audit/pr16-spu-reverb-gpl-lineage-mpenkava`. -- **PR #13** (parked at `5fc8e15d`) shipped a reverb too, rejected for being - *wrong* rather than for licensing: it gated reverb on bit 15 of `0x1F801DC0`, - which is `dAPF1` — an all-pass *offset* register whose top bit is set for any - offset ≥ `0x8000`, so reverb switched on and off according to an address - value. The real gate is SPUCNT (`0x1F801DAA`) bit 7. It also read - `dAPF2` / `vIIR` / `vCOMB1` as "feedback" / "wet level" / "early-reflection - level"; they are none of those. - -The PR #16 audit names the only acceptable path, and this branch takes it: **a -clean-room implementation from hardware documentation, with independently -derived tests.** The psx-spx / nocash register map and documented algorithm were -the source. Beetle's source was deliberately not consulted for the -implementation. - -Beetle remains fully usable as a **runtime oracle** — running the binary and -comparing its audio output is not a derivative work, and that comparison is how -the remaining uncertainties in §5 get settled. `audio_wav` and `audio_stats` -exist on both `psx-runtime` and `psx-beetle`, so the comparison is a symmetric -always-on ring query, not an arm-then-capture. - -### Pre-existing lineage exposure — reported, not changed - -Separate from this work, and worth a decision: - -- `runtime/src/spu.c` `calc_vc_delta()` is commented *"Ported verbatim from - Beetle's CalcVCDelta"*. This is the ADSR rate decoder, and it is in every - released binary. Same category as PR #16, already shipped. -- `runtime/include/spu_gauss.h` cites *"No$PSX docs / DuckStation - core/spu.cpp"*. - -Deliberately left alone here: rewriting the ADSR rate decoder clean-room changes -every voice envelope in every title and needs its own revalidation campaign, not -a rider on this branch. - -## 4. `feat/pr13-salvage-remainder` is fully superseded — no surgery needed - -Prior notes flagged this branch as "ready to land" while Issue #103 says its -reverb must stay parked, and treated reconciling that as a blocker. **It is not -a blocker: the branch retains nothing but the rejected reverb commit.** - -`git cherry -v origin/master feat/pr13-salvage-remainder`: - -``` -- e9a8a24e interp: deliver alignment exceptions, protect Cause read-only bits, add CFC0 -- 3ce30155 dma: implement channel 5 (PIO / expansion port) transfers -+ 5fc8e15d spu: reverb, noise LFSR, sweep volumes, pitch modulation, SPU IRQ -- 9f3cd0a3 autocompile: name the resolved interpreter at configure, warn on Cygwin builds -``` - -Three of four are already upstream (`-`). Verified four independent ways rather -than trusting patch-id alone: - -- cherry-picking all three onto current `origin/master` produced **empty** - commits; -- `execute_ch5_pio()` is present in `runtime/src/dma.c` on master; -- `CFC0` handling and the `& ~0x0300u | (val & 0x0300u)` Cause write-protection - are present in `runtime/src/dirty_ram_interp.c` on master; -- `git diff origin/master 9f3cd0a3 -- runtime/src/autocompile.c` is **empty**. - -Consequences: there is no salvage left to preserve, no git surgery to perform, -and the "two competing SPU IRQ implementations" question resolves by default — -`5fc8e15d` is both superseded and Beetle-derived. The branch should be -abandoned and PR #13 closed. `origin/park/pr13-spu-reverb-rejected` still -retains the commit, so nothing is lost. - -Incidentally, `e9a8a24e`'s Cause write-protection (guest `MTC0` may only write -bits 8–9) is exactly what makes §2a safe: the guest cannot forge IP2. - -## 5. Known deviations and the oracle-verification queue - -Recorded explicitly rather than presented as exact (Rule 14). - -- **22.05 → 44.1 kHz reverb reconstruction filter.** The reverb engine runs at - 22050 Hz, but the hardware's reconstruction filter is not specified in the - documentation used. Beetle's 39-tap FIR was deliberately not copied. The - implementation uses a clearly-marked, single-function reconstruction so the - filter can be swapped once measured against the oracle. **This is the one - place the implementation is known to differ from hardware.** -- Further per-feature judgement calls are listed in §6 as reported by the - implementation pass; each is a candidate for oracle comparison. - -## 6. Implementation judgement calls - -Every point where the documentation ran out. Each is an oracle-comparison -candidate, NOT a settled fact. Listed so a later measurement pass has a work -list instead of having to rediscover them. - -1. **Reverb 22.05 ↔ 44.1 kHz boundary** (the flagged known gap): input is the - box average of the frame pair, output is linear interpolation between - successive engine results. Isolated in `rev_reconstruct()` plus one call - site. **The one deliberate deviation from hardware.** -2. **Noise divider wiring**: down-counter decremented by step `4 + SPUCNT[9:8]` - per 44.1 kHz cycle, reloaded with `0x20000 >> shift`. The least-documented - part of the noise generator. -3. **Register-write IRQ compare sites**: the transfer-address write, the - IRQ-address write and an SPUCNT-enable re-check all compare against the - *resting* transfer address immediately. Consequence: enabling the IRQ while - both registers are 0 fires immediately (unit 0 == unit 0). Real drivers set - the address first. -4. **Suppressed reverb writes** (SPUCNT bit 7 clear) do not run the IRQ compare, - since no access occurs. Reads always do. -5. **Sweep negative-phase semantics**: the envelope works on a 0..0x7FFF value - in the phase's domain; wrong-sign levels clamp to 0 first. A sweep-mode - register read returns the live level as signed 16-bit. -6. **Reverb send taps**: post-envelope and post-voice-volume for EON voices; - post-CD-volume for the CD send. -7. **Capture values**: CD captured pre-CD-volume (raw input bus), voice 1/3 - post-envelope pre-voice-volume. The CD bus now **pops continuously while the - SPU is enabled** (SPUCNT bit 0 gates only mix/reverb), needed for capture - correctness. This is a behaviour change — measured as *not* causing - starvation, see §7. -8. **Mix saturation order**: the summed bus (dry + CD + wet) saturates to 16-bit - *before* main volume, per the documented mixer order. Replaces previous - unclamped, int32-overflow-prone math. Audible only in loud scenes. -9. **All-disabled gating**: capture / noise / reverb / sweeps run only while - SPUCNT bit 15 is set; whether they free-run with the SPU disabled is - undocumented. Noise clocks after the voice walk within a frame. -10. **Shadow tap and `SpuDebugInfo` volume fields** keep the historical 1.14 - scale (`level >> 1`) so `spu_shadow.c` and old captures stay byte-compatible - for direct-mode volumes. - -## 7. Validation status - -- [x] `test_cause_ip2_combinational` passes (`-Wall -Wextra -Werror`). -- [x] `test_spu_fidelity` passes — 137 checks, 0 failures. Independently re-run, - not taken from the implementation report. -- [x] All changed translation units compile clean; `psx-runtime` and - `Tomba2Recomp` both link. -- [x] **BIOS LLE boot proves the reverb engine runs.** The BIOS itself sets - SPUCNT bit 7 (`ctrl = 0xC085`) and allocates a 62 KB work area at - `mBASE = 0x70940`. Sampled through the boot chime: the work area holds - 256/256 non-zero reflection halfwords peaking at 24681, `reverb_cur` - advances and laps correctly inside `[mBASE, 0x80000)`, and after the dry - signal goes silent (output peak → 0 at t≈6.8 s) the work area keeps - ringing down (628 → 18 → 2) for ~3 s. A decaying tail that outlives its - input is the reverb working. Noise LFSR observed shifting with correct - parity feedback (`000F → 001F → … → FFA3`). -- [x] **Tomba 2 intro FMV: A/B against the exact parent commit.** Same worktree, - framework at `c88c3ca9^` vs `c88c3ca9`, identical generated code - (SPU work is runtime-only). - - | Metric | Baseline | SPU branch | - | --- | --- | --- | - | FPS drop | 59.9 → 40.7 at t=6.2 s | 59.9 → 40.4 at t=6.2 s | - | min p50 fps | 40.3 | 40.0 | - | `cd_underflow_frames` | 441, flat | 441, flat | - | host underruns @32 s | 594 | 582 | - | audio peak | 23066 | 26876 | - | `reverb_cur`/`noise_lfsr` | absent (control) | live | - - Conclusions: performance is a wash (the FMV's 40 fps and its ~19/s host - underruns are **pre-existing on master** — worth their own issue, not - caused here); the continuous CD-bus drain does **not** starve the bus; and - the peak rising 23066 → 26876 is wet signal adding on top of dry, still - well under 32767, so not clipping. FMV video confirmed correct by - screenshot through the debug server (320x224, intro scene). -- [ ] Oracle audio comparison against `psx-beetle` at a fixed scene — settles - the §6 items, especially (1). `audio_wav`/`audio_stats` exist on both - ports, so this is a symmetric always-on ring query. **Not yet done.** -- [ ] User ear-validation (final gate). - -## 9. Merge prerequisites - -- **Rebase required.** This branch is based on `7f7fbc8e`; master moved during - the session (observed `9217ae2`, alpha-258). Re-fetch and rebase; do not - merge stale. -- **`recomp-ui` must be bumped in lockstep.** Framework master's - `feat(mods): expose linked package attribution` needs recomp-ui `99558ee` - (`feat(mods): link package authors and sources`), which adds `author_links` / - `author_link_count` / `source_name` / `source_url` to - `RecompLauncherCModPackage`. Tomba 2's pin at `854ae10` is one commit short - and **fails to compile** with `'RecompLauncherCModPackage' has no member named - 'author_link_count'`. This is unrelated to the SPU work but blocks any title - build against current master. -- Every title needs regen + revalidation, because CAUSE.IP2 touches core IRQ - delivery for all of them. - -## 8. Incidental fix — `tools/embed_spirv.py` build race - -Not SPU-related, found while building. Two runtime targets (`psx-runtime`, -`psx-oracle`) embed the same shader set, so ninja runs `embed_spirv.py` -concurrently. It wrote its intermediate SPIR-V to `.spv` **in the source -tree**, so the two invocations raced on one path and whichever finished first -`os.remove()`d the file the other was about to read — a hard build failure -(`FileNotFoundError: .../blit.frag.spv`), and source-tree pollution besides. -Now compiled to a private `mkdtemp` directory, with the output header written -via atomic replace and a loud error if `glslc` reports success but produces no -file. Fixed per Rule 15 rather than worked around. +# SPU fidelity burndown — Issue #103 and PR #102 + +Branch: `feat/spu-fidelity-103` · Worktree: `_wt-spu-103` · Base: `origin/master` `7f7fbc8e` + +Closes the four gaps named in Issue #103 (reverb, noise, volume sweeps, SPU IRQ) +and lands the separable, verified parts of PR #102. Written so the disposition of +each piece is auditable rather than asserted. + +--- + +## 1. Scope overlap between #102 and #103 + +They intersect on exactly one item. + +| Item | Issue #103 | PR #102 | Disposition | +| --- | --- | --- | --- | +| Reverb | ✔ asked | — | Implemented here, clean-room (§3) | +| Noise LFSR | ✔ asked | — | Implemented here, clean-room (§3) | +| Volume sweeps | ✔ asked | — | Implemented here, clean-room (§3) | +| SPU IRQ | ✔ asked | ✔ offered | **One implementation**, ours, in `spu.c` | +| CAUSE.IP2 combinational | — | ✔ offered | Taken, verified, extended (§2) | +| Mid-dispatch audio pump | — | ✔ offered | Taken, with a regression fixed (§2) | +| SPU RAM DMA readback | — | ✔ offered | Taken as-is in substance (§2) | + +So this is one coherent change, not two competing ones. PR #102 covers 1 of +#103's 4 items; the other three were never in it. + +## 2. PR #102 — per-piece verdict + +PR #102's four changes were reviewed as four separable items, because they carry +very different risk. Attribution to Alexandros Mandravillis is recorded in the +commit trailer and in the source comments; the code here is a reimplementation +on the same findings, because two of the PR's three commits touch `spu.c` and +would have collided with the clean-room work. + +### 2a. CAUSE.IP2 combinational — CONFIRMED, taken, and extended + +The PR's claim is that `CAUSE.IP2` is a live mirror of the interrupt line rather +than a latched bit. **The claim is correct**, and it was checked against the +in-tree Beetle oracle rather than accepted on the PR's reasoning: + +- `beetle-psx/mednafen/psx/irq.cpp` defines + `#define Recalc() PSX_CPU->AssertIRQ(0, (bool)(Status & Mask))` + and calls it from `IRQ_Assert` (raise), from **both** halves of `IRQ_Write` + (the Status ack and the Mask write), and from `IRQ_Power`. +- `beetle-psx/mednafen/psx/cpu.cpp:289 AssertIRQ()` clears `CAUSE` bit `10+n` + unconditionally, then re-sets it only if the level is asserted. + +That is the definition of a combinational mirror, and it matches R3000A: the +`Cause.IP` field is not storage, it reflects the interrupt pins. Our runtime +previously only ever OR'd bit 10 in at delivery and never cleared it. + +Extensions beyond the PR: + +1. **A second latching writer the PR missed.** `runtime/src/psx_interpreter.c` + had its own `cpu->cop0[COP0_CAUSE] |= (1u << 10);` at interrupt delivery — + the identical defect in the standalone interpreter path. Fixed too. An audit + of every writer of `cop0[13]` confirms no other site touches bit 10: the + interpreter's `MTC0` paths correctly mask guest writes to `0x0300` + (software-interrupt bits only), and the exception paths mask `~0x7C` / + `~0x8000007C`, preserving the IP field. +2. **Single ownership.** The compiled delivery path no longer sets bit 10 + itself; `psx_irq_refresh_cause_ip2()` is the only writer. Two writers of one + combinational bit is how the bug survived in the first place. +3. **Power-on recompute**, mirroring Beetle's `IRQ_Power() -> Recalc()`. +4. **A regression test**, `runtime/tests/test_cause_ip2_combinational.c`, + covering rise, fall on ack, fall on mask, partial ack with another source + still pending, partial-width register writes, preservation of ExcCode/BD/IP0/ + IP1, and out-of-range I_STAT bits. Registered in CTest. + +Risk note: this is a real behavioural change to core IRQ delivery for every +title, and it is the highest-risk item in this branch. It is nonetheless the +faithful behaviour, so per Rule -1 it lands and titles are revalidated rather +than the fix being narrowed. + +### 2b. Mid-dispatch audio pump — taken, with a regression the PR would have shipped + +The reasoning is sound: the SPU is autonomous on hardware, our pump is driven +from the main loop between presented frames, so a guest busy-wait that never +presents a frame freezes SPU time — self-deadlocking for any game waiting on an +SPU-generated condition. The VBlank edge is derived from the guest cycle +counter, so it keeps firing through such a wait and is a defensible place to +pump. Confirmed single-threaded: `spu_render` runs on the main loop thread +(`sdl_audio_pump` is the producer; the SDL callback only drains an SPSC ring), +so this introduces no data race on `i_stat`/`cop0`. + +**Defect found and fixed.** PR #102 wires the hook to `sdl_audio_pump()` +unconditionally. But `sdl_audio_update()` is the sole authority on whether a +pump should emit, discard, or not run: + +- hard mute (turbo loads) → *does not pump at all*; the queue drains and voice + positions freeze in place, so music resumes where it left off instead of + replaying time-compressed garble. This is user-validated behaviour. +- turbo sink → pumps with `discard_output = true`. + +Pumping unconditionally from the VBlank edge bypasses both, which would push +real audio during every turbo load. The hook now goes through +`sdl_audio_pump_midframe()`, which mirrors the gate state +(`AUDIO_GATE_NORMAL` / `MUTED` / `SINK`) that `sdl_audio_update()` last set. + +Known tension, recorded rather than guessed at: in the `MUTED` state SPU time +still freezes, so a game that busy-waits on an SPU condition *during* a turbo +load would stall. No title in our suite is known to do this. Advancing SPU time +under mute would change validated mute audio, so it is not being done +speculatively. + +Placement: the pump call sits at the **end** of `fire_vblank_edge()` so the +VBlank's own raise and ring records complete first — the pump can itself raise +an SPU IRQ, which should land after the edge rather than interleaved into it. + +### 2c. SPU RAM DMA readback — taken + +DMA4 in the SPU→RAM direction previously wrote literal zeros. SPU RAM is +readable memory; this was not a transfer at all. Titles that carry a checksummed +block through SPU RAM across an `Exec` boundary (SPU RAM being one of the few +regions a main-RAM reload does not touch) read zeros, failed their own integrity +check, and fell back to a cold-boot path. Lowest-risk item in the PR: +previously-dead path, no existing behaviour depends on the zeros. + +### 2d. Validation claims — NOT reproduced, and cannot be + +PR #102 validates against Medal of Honor Underground (SLUS-01270) and Gran +Turismo (SCUS-94194). Neither disc is in our suite. Those specific claims are +**unverified by us and should not be repeated as if they were**. What we +verified independently is the hardware behaviour behind each change (§2a–2c) and +non-regression across our own titles. + +## 3. Issue #103 — why this had to be clean-room + +⚠ **This is the load-bearing constraint on the whole item, and it is why #103 +sat open.** + +`psxrecomp` is distributed under **PolyForm Noncommercial 1.0.0**. Beetle PSX / +mednafen is **GPL-2.0-or-later**. They are incompatible: GPL source cannot be +folded into a PolyForm distribution. Beetle's tree contains a complete, correct, +well-tested reverb — so the obvious implementation route is closed, and two +contributors independently walked into it: + +- **PR #16** (Martin Penkava) implemented all of this by porting Beetle. Parked + on license grounds after a line-by-line audit found it tracked + `beetle-psx/mednafen/psx/spu.cpp` lines 590–730 in register layout, resampling + coefficients, buffer layout, arithmetic and processing order. See + `docs/internal/upstream/martin-pr16-spu-reverb.md` on branch + `audit/pr16-spu-reverb-gpl-lineage-mpenkava`. +- **PR #13** (parked at `5fc8e15d`) shipped a reverb too, rejected for being + *wrong* rather than for licensing: it gated reverb on bit 15 of `0x1F801DC0`, + which is `dAPF1` — an all-pass *offset* register whose top bit is set for any + offset ≥ `0x8000`, so reverb switched on and off according to an address + value. The real gate is SPUCNT (`0x1F801DAA`) bit 7. It also read + `dAPF2` / `vIIR` / `vCOMB1` as "feedback" / "wet level" / "early-reflection + level"; they are none of those. + +The PR #16 audit names the only acceptable path, and this branch takes it: **a +clean-room implementation from hardware documentation, with independently +derived tests.** The psx-spx / nocash register map and documented algorithm were +the source. Beetle's source was deliberately not consulted for the +implementation. + +Beetle remains fully usable as a **runtime oracle** — running the binary and +comparing its audio output is not a derivative work, and that comparison is how +the remaining uncertainties in §5 get settled. `audio_wav` and `audio_stats` +exist on both `psx-runtime` and `psx-beetle`, so the comparison is a symmetric +always-on ring query, not an arm-then-capture. + +### Pre-existing lineage exposure — reported, not changed + +Separate from this work, and worth a decision: + +- `runtime/src/spu.c` `calc_vc_delta()` is commented *"Ported verbatim from + Beetle's CalcVCDelta"*. This is the ADSR rate decoder, and it is in every + released binary. Same category as PR #16, already shipped. +- `runtime/include/spu_gauss.h` cites *"No$PSX docs / DuckStation + core/spu.cpp"*. + +Deliberately left alone here: rewriting the ADSR rate decoder clean-room changes +every voice envelope in every title and needs its own revalidation campaign, not +a rider on this branch. + +## 4. `feat/pr13-salvage-remainder` is fully superseded — no surgery needed + +Prior notes flagged this branch as "ready to land" while Issue #103 says its +reverb must stay parked, and treated reconciling that as a blocker. **It is not +a blocker: the branch retains nothing but the rejected reverb commit.** + +`git cherry -v origin/master feat/pr13-salvage-remainder`: + +``` +- e9a8a24e interp: deliver alignment exceptions, protect Cause read-only bits, add CFC0 +- 3ce30155 dma: implement channel 5 (PIO / expansion port) transfers ++ 5fc8e15d spu: reverb, noise LFSR, sweep volumes, pitch modulation, SPU IRQ +- 9f3cd0a3 autocompile: name the resolved interpreter at configure, warn on Cygwin builds +``` + +Three of four are already upstream (`-`). Verified four independent ways rather +than trusting patch-id alone: + +- cherry-picking all three onto current `origin/master` produced **empty** + commits; +- `execute_ch5_pio()` is present in `runtime/src/dma.c` on master; +- `CFC0` handling and the `& ~0x0300u | (val & 0x0300u)` Cause write-protection + are present in `runtime/src/dirty_ram_interp.c` on master; +- `git diff origin/master 9f3cd0a3 -- runtime/src/autocompile.c` is **empty**. + +Consequences: there is no salvage left to preserve, no git surgery to perform, +and the "two competing SPU IRQ implementations" question resolves by default — +`5fc8e15d` is both superseded and Beetle-derived. The branch should be +abandoned and PR #13 closed. `origin/park/pr13-spu-reverb-rejected` still +retains the commit, so nothing is lost. + +Incidentally, `e9a8a24e`'s Cause write-protection (guest `MTC0` may only write +bits 8–9) is exactly what makes §2a safe: the guest cannot forge IP2. + +## 5. Known deviations and the oracle-verification queue + +Recorded explicitly rather than presented as exact (Rule 14). + +- **22.05 → 44.1 kHz reverb reconstruction filter.** The reverb engine runs at + 22050 Hz, but the hardware's reconstruction filter is not specified in the + documentation used. Beetle's 39-tap FIR was deliberately not copied. The + implementation uses a clearly-marked, single-function reconstruction so the + filter can be swapped once measured against the oracle. **This is the one + place the implementation is known to differ from hardware.** +- Further per-feature judgement calls are listed in §6 as reported by the + implementation pass; each is a candidate for oracle comparison. + +## 6. Implementation judgement calls + +Every point where the documentation ran out. Each is an oracle-comparison +candidate, NOT a settled fact. Listed so a later measurement pass has a work +list instead of having to rediscover them. + +1. **Reverb 22.05 ↔ 44.1 kHz boundary** (the flagged known gap): input is the + box average of the frame pair, output is linear interpolation between + successive engine results. Isolated in `rev_reconstruct()` plus one call + site. **The one deliberate deviation from hardware.** +2. **Noise divider wiring**: down-counter decremented by step `4 + SPUCNT[9:8]` + per 44.1 kHz cycle, reloaded with `0x20000 >> shift`. The least-documented + part of the noise generator. +3. **Register-write IRQ compare sites**: the transfer-address write, the + IRQ-address write and an SPUCNT-enable re-check all compare against the + *resting* transfer address immediately. Consequence: enabling the IRQ while + both registers are 0 fires immediately (unit 0 == unit 0). Real drivers set + the address first. +4. **Suppressed reverb writes** (SPUCNT bit 7 clear) do not run the IRQ compare, + since no access occurs. Reads always do. +5. **Sweep negative-phase semantics**: the envelope works on a 0..0x7FFF value + in the phase's domain; wrong-sign levels clamp to 0 first. A sweep-mode + register read returns the live level as signed 16-bit. +6. **Reverb send taps**: post-envelope and post-voice-volume for EON voices; + post-CD-volume for the CD send. +7. **Capture values**: CD captured pre-CD-volume (raw input bus), voice 1/3 + post-envelope pre-voice-volume. The CD bus now **pops continuously while the + SPU is enabled** (SPUCNT bit 0 gates only mix/reverb), needed for capture + correctness. This is a behaviour change — measured as *not* causing + starvation, see §7. +8. **Mix saturation order**: the summed bus (dry + CD + wet) saturates to 16-bit + *before* main volume, per the documented mixer order. Replaces previous + unclamped, int32-overflow-prone math. Audible only in loud scenes. +9. **All-disabled gating**: capture / noise / reverb / sweeps run only while + SPUCNT bit 15 is set; whether they free-run with the SPU disabled is + undocumented. Noise clocks after the voice walk within a frame. +10. **Shadow tap and `SpuDebugInfo` volume fields** keep the historical 1.14 + scale (`level >> 1`) so `spu_shadow.c` and old captures stay byte-compatible + for direct-mode volumes. + +## 7. Validation status + +- [x] `test_cause_ip2_combinational` passes (`-Wall -Wextra -Werror`). +- [x] `test_spu_fidelity` passes — 137 checks, 0 failures. Independently re-run, + not taken from the implementation report. +- [x] All changed translation units compile clean; `psx-runtime` and + `Tomba2Recomp` both link. +- [x] **BIOS LLE boot proves the reverb engine runs.** The BIOS itself sets + SPUCNT bit 7 (`ctrl = 0xC085`) and allocates a 62 KB work area at + `mBASE = 0x70940`. Sampled through the boot chime: the work area holds + 256/256 non-zero reflection halfwords peaking at 24681, `reverb_cur` + advances and laps correctly inside `[mBASE, 0x80000)`, and after the dry + signal goes silent (output peak → 0 at t≈6.8 s) the work area keeps + ringing down (628 → 18 → 2) for ~3 s. A decaying tail that outlives its + input is the reverb working. Noise LFSR observed shifting with correct + parity feedback (`000F → 001F → … → FFA3`). +- [x] **Tomba 2 intro FMV — measured twice, because the first attempt was on a + broken setup. Recording both so the mistake is not repeated.** + + *Attempt 1 (INVALID as an absolute measurement).* A fresh worktree has a + cold overlay shard cache, and `autocompile` failed 8/8 with `WinError 2`: + `game.toml` points at `psxrecomp-v4/recompiler/build-t2/psxrecomp-game.exe` + while the documented build recipe puts the recompiler at + `build-recompiler/` in the worktree root. No shards were ever built and the + runtime silently fell back to the interpreter (`dispatch_native = 0`, + `native_share = 0.0`). The resulting "40 fps, ~19 underruns/s" was my setup, + NOT a property of master. Fixed by building the recompiler at the path + `game.toml` expects; autocompile then reports `fails=0`, `shard_ok` rising, + and the cache grows. + + *Attempt 2 (valid).* Ran the SPU build against the fully warm cache in + `_wt-t2-measure`, then the original pre-SPU exe in the same worktree with + the same cache as a control: + + | Metric | Control (no SPU work) | SPU branch | + | --- | --- | --- | + | `native_share` (by time) | 0.1558 | 0.1704 | + | `interp_share` | 0.7813 | 0.7775 | + | `exc_share` | 0.2386 | 0.2381 | + | p50 fps | 39.1 | 38.0-38.4 | + | native/interp dispatch | 96.6% native | 95.9% native | + + Identical within noise, so the added per-frame DSP work costs nothing + measurable. Reverb/noise state observed live; peak rose 23066 -> 26876 + (wet adding to dry, still under 32767 so not clipping); `cd_underflow` + flat at 441 on both, so the continuous CD-bus drain does not starve it. + FMV video confirmed correct by debug-server screenshot (320x224). + + **Pre-existing defect characterised, not caused here:** Tomba 2's intro FMV + is 96% native by DISPATCH COUNT but 78% interpreted by WALL CLOCK, with 24% + of time in exception handling, giving ~39 fps and continuous host-sink + underruns even with a fully warm cache. A few interpreted blocks consume + most of the frame. Worth its own issue. + +- [ ] Oracle audio comparison against `psx-beetle` at a fixed scene — settles + the §6 items, especially (1). `audio_wav`/`audio_stats` exist on both + ports, so this is a symmetric always-on ring query. **Not yet done.** +- [ ] User ear-validation (final gate). + +## 9. Merge prerequisites + +- **Rebase required.** This branch is based on `7f7fbc8e`; master moved during + the session (observed `9217ae2`, alpha-258). Re-fetch and rebase; do not + merge stale. +- **`recomp-ui` must be bumped in lockstep.** Framework master's + `feat(mods): expose linked package attribution` needs recomp-ui `99558ee` + (`feat(mods): link package authors and sources`), which adds `author_links` / + `author_link_count` / `source_name` / `source_url` to + `RecompLauncherCModPackage`. Tomba 2's pin at `854ae10` is one commit short + and **fails to compile** with `'RecompLauncherCModPackage' has no member named + 'author_link_count'`. This is unrelated to the SPU work but blocks any title + build against current master. +- Every title needs regen + revalidation, because CAUSE.IP2 touches core IRQ + delivery for all of them. + +## 8. Incidental fix — `tools/embed_spirv.py` build race + +Not SPU-related, found while building. Two runtime targets (`psx-runtime`, +`psx-oracle`) embed the same shader set, so ninja runs `embed_spirv.py` +concurrently. It wrote its intermediate SPIR-V to `.spv` **in the source +tree**, so the two invocations raced on one path and whichever finished first +`os.remove()`d the file the other was about to read — a hard build failure +(`FileNotFoundError: .../blit.frag.spv`), and source-tree pollution besides. +Now compiled to a private `mkdtemp` directory, with the output header written +via atomic replace and a loud error if `glslc` reports success but produces no +file. Fixed per Rule 15 rather than worked around.