feat(rdp): decode the DP FIFO command stream (T-31-001) - #56
Merged
Conversation
Phase 3, Sprint 1's first ticket: the RDP now drains its command FIFO instead of blindly advancing one 64-bit word per tick. `Rdp::tick` reads the command at `DPC_CURRENT`, decodes the opcode (bits 61:56), and advances the pointer by the command's *full* length, so a multi-word primitive is consumed whole and the stream never desyncs mid-command. It retires one command per scheduler tick — the FIFO drains gradually, not in a burst. No opcode is dispatched to a handler yet (that is T-31-002+): each command is recognised, its length consumed, and a retired-work counter incremented. The length table (`rustyn64_rdp::command`) is sourced from the N64brew `Reality Display Processor/Commands` map: one 64-bit word for every command except the variable-length Fill Triangle forms (0x08-0x0F, a 4-word base plus shade/texture/z coefficient blocks selected by the opcode's low three bits) and the two-word Texture Rectangle pair (0x24/0x25). `commands_processed` is a retired-work tally, not a cycle position — nothing schedules against it, so it does not touch the derive-don't-increment rule (ADR 0006). Tests: the opcode->length map is unit-tested exhaustively across 0x00-0x3F, and a mixed-list walk (set-state, a 22-word STZ triangle, a no-op, a texture rectangle, Sync Full) asserts the decoder lands `DPC_CURRENT` exactly on `DPC_END`, one command per tick. Mutation-checked: forcing the length to 8 fails the walk (27 ticks != 5). Docs: `docs/rdp.md` gains a command-decoder section in the same change; `CHANGELOG.md` under [Unreleased]. Gates run locally: cargo fmt --check, clippy --workspace --all-targets -D warnings, cargo test --workspace, RUSTDOCFLAGS=-D warnings cargo doc, the thumbv7em no_std build, and markdownlint — all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RDP now decodes DP FIFO opcodes, consumes complete commands in one scheduler tick, tracks retired commands, and stalls for incomplete commands or ChangesDP FIFO decoder
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RdpTick
participant RdramBus
participant CommandDecoder
participant DpFifoState
RdpTick->>RdramBus: Read command word at DPC_CURRENT
RdramBus-->>RdpTick: Return command word
RdpTick->>CommandDecoder: Decode opcode and command length
CommandDecoder-->>RdpTick: Return full word length
RdpTick->>DpFifoState: Advance DPC_CURRENT and increment commands_processed
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
Antigravity review (Gemini via Ultra)This PR implements RDP DP FIFO command length decoding for opcodes Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustyn64-rdp/src/lib.rs`:
- Around line 352-357: Update push_cmd in the decoder alignment test to accept
an explicit fixed word count for each fixture command instead of calling
command::command_len_words(opcode). Use independently specified counts at every
call site, while preserving the existing opcode encoding and zero-padding so the
test endpoint is independent of the production decoder table.
- Around line 199-206: The command retirement logic in tick must wait for the
full encoded command before advancing cmd_current: after decoding opcode and
len_bytes, return when cmd_end - cmd_current is less than len_bytes, preserving
the header-only 0x0F until its remaining 21 words arrive; add coverage for this
incremental FIFO case. Update docs/rdp.md lines 89-95 to state that tick retires
commands only when their complete encoded length is available.
- Around line 192-205: Update tick to return immediately when DP_STATUS_XBUS is
set, before reading RDRAM, decoding an opcode, or advancing cmd_current. Add a
test covering XBUS mode that verifies both CURRENT/cmd_current and
commands_processed remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3fdfb3f9-d073-4644-94d1-355c85c26242
📒 Files selected for processing (4)
CHANGELOG.mdcrates/rustyn64-rdp/src/command.rscrates/rustyn64-rdp/src/lib.rsdocs/rdp.md
Adjudicating the PR #56 bot review: - **Wait for the whole command** (CodeRabbit #2, Antigravity blocking #2): `Rdp::tick` now returns without advancing when `DPC_END - DPC_CURRENT` is less than the decoded length. The rdpq microcode advances `DPC_END` incrementally as it fills the buffer, so it can land mid-command; consuming a partial multi-word primitive would decode unwritten RDRAM. New test drives a 22-word triangle with `DPC_END` first at word 10 (stalls) then at word 22 (consumed whole). Mutation-checked. - **XBUS stalls the decoder** (CodeRabbit #3): with `DPC_STATUS.XBUS` set the command source is DMEM, which is not yet wired, so the decoder must not fall back to reading RDRAM (that would decode parameter data as opcodes and desync). New test asserts no advance under XBUS. Mutation-checked. - **Independent decoder test** (CodeRabbit #1): the FIFO-walk fixture now states each command's word count explicitly rather than calling `command_len_words`, so the walk is a genuine check of the decoder against the N64brew map, not a tautology built from it. Rejected: Antigravity blocking #1 (texture rectangle is 3 words). The N64brew command map shows 0x24/0x25 as two 64-bit words (Word 0 coords + Word 1 s/t/dsdx/dtdy); there is no third dsdy/dtdx word — texrect is axis-aligned and carries only dsdx and dtdy. The decoder's 2 stands. Docs: docs/rdp.md gains the two stall conditions; CHANGELOG updated. Gates: fmt, clippy --workspace -D warnings, cargo test --workspace, rustdoc -D warnings, thumbv7em no_std, markdownlint — all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
Adjudication of the Antigravity reviewThanks — adjudicated each item; fixes in 3f92685. Blocking
Suggestions
Nitpicks
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustyn64-rdp/src/lib.rs (1)
96-96: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMirror
commands_processedin the documented public interface.
docs/rdp.md'sRdpdeclaration omits this new public field. Keep the API reference complete. As per path instructions, “Docs are the SPEC here”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-rdp/src/lib.rs` at line 96, Update the documented Rdp declaration in docs/rdp.md to include the public commands_processed: u64 field, matching the Rust Rdp interface. Keep the documentation’s existing declaration style and ordering consistent with the implementation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustyn64-rdp/src/lib.rs`:
- Line 418: Correct the comment on the push_cmd call using opcode 0x0E to
identify it as a shade+texture triangle, while preserving the existing opcode,
arguments, and word-count annotation.
---
Outside diff comments:
In `@crates/rustyn64-rdp/src/lib.rs`:
- Line 96: Update the documented Rdp declaration in docs/rdp.md to include the
public commands_processed: u64 field, matching the Rust Rdp interface. Keep the
documentation’s existing declaration style and ordering consistent with the
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 657daaaf-4240-4ca5-a1f3-6a71448552d2
📒 Files selected for processing (3)
CHANGELOG.mdcrates/rustyn64-rdp/src/lib.rsdocs/rdp.md
…01 review) Adjudicating CodeRabbit's re-review: the documented `Rdp` struct in docs/rdp.md now lists the `commands_processed` field added in ffb46b2, keeping the docs-as-spec interface in sync with the code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai Adopted the outside-diff finding ( |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
Adjudicating CodeRabbit: the 0x0E fixture comment now reads "Fill Triangle (shade + texture)" instead of the abbreviated "(ST)", matching the spelled-out 0x0F comment in the walk fixture and removing any read as the base (non-shaded) Fill Triangle. Keeps the N64brew wiki name "Fill Triangle" (the project's mirrored, canonical command map) rather than an alternate source's "Non-Shaded Triangle" convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustyn64-rdp/src/lib.rs`:
- Line 418: Update the opcode 0x0E fixture in the push_cmd test to use the
canonical N64brew label “Fill Triangle (ST)”, optionally retaining the “shade +
texture” explanatory gloss. Keep the existing 20-word length and command
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af55d3b5-669f-464a-839a-7254e0bf013b
📒 Files selected for processing (1)
crates/rustyn64-rdp/src/lib.rs
…1 review) Adjudicating CodeRabbit's follow-up: the 0x0E and 0x0F fixture comments now carry the canonical N64brew command labels — Fill Triangle (ST) and (STZ) — alongside the shade/texture/z gloss, matching the mirrored command map's terminology. Lengths (20, 22) and behaviour unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 22, 2026
doublegate
added a commit
that referenced
this pull request
Jul 22, 2026
…32-001) (#64) * docs(phase-3): close out Sprint 1 and plan Sprint 2 Sprint 1 (command decode, fill, VI scan-out) is complete: T-31-001..005 all merged (#56-#63). Mark the acceptance criteria met and record the deliberate deviations honestly rather than silently checking boxes: - T-31-005's golden is an inlined FNV-1a hash constant (harness is hash-based), not a file under tests/golden/; it runs unconditionally (more coverage than gating behind test-roms); and the frame is a synthetic FILL command stream, not a booted ROM (real cart boot is Phase 5). The DP-FIFO-onward path is identical, so the picture path is fully exercised now. - VI X/Y_SCALE resampling and AA/divot/de-dither are not implemented (1:1 scan, ledger R-5); per-register VI write masks are ledger R-4. - The documented RDP hazards (T-31-002) are deferred to Sprint 3, where the render pipeline they govern exists. Refine the Sprint 2 stub into real tickets (T-32-001..004), grounded in the n64brew Commands.md texture-command encodings: TMEM + tile descriptors + texture-state commands, Load Block/Load Tile, Load TLUT + texel-format decoders, and the sampler + copy-mode Texture Rectangle. Point the overview at the new plan and mark Sprint 1 COMPLETE / Sprint 2 in progress. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(rdp): TMEM, tile descriptors, and the texture-state commands (T-32-001) Give the RDP its texture state and the three commands that describe it, the foundation the texel loads and the sampler build on. Pure state: no texel is moved and no framebuffer pixel changes. - `Rdp` owns a 4 KiB TMEM and eight `TileDescriptor`s, reset at power-on. - Dispatch `Set Texture Image` (0x3D), `Set Tile` (0x35), and `Set Tile Size` (0x32), decoding every field per the N64brew command tables (format/size/line/ tmem_addr/palette + per-axis clamp/mirror/mask/shift for Set Tile; SL/TL/SH/TH for Set Tile Size; format/size/width/addr for Set Texture Image). Set Tile preserves the disjoint tile-size coords via a struct-update spread. TMEM is a lazily-allocated `Option<Box<[u8; 4096]>>`: it starts None (read as all-zero) and allocates on first write. This keeps `Rdp`'s Default cheap, which matters because `Bus::rdp_tick` does a `core::mem::take` every RCP step -- a None placeholder swaps in with no 4 KiB allocation or copy, while the real TMEM box moves by pointer. An inline array would memcpy ~12 KiB per tick; a plain boxed default would allocate per tick. Neither touches the Bus seam. The four clamp/mirror bools are the hardware's four independent bit-flags (clamp+mirror per S/T axis), decoded straight from command bits, so struct_excessive_bools is allowed with that justification rather than forcing an enum that would misrepresent the register. Field-by-field unit tests pin each decode with distinct per-field values so a swapped bit range surfaces as a wrong field. docs/rdp.md gains the T-32-001 section and its State list is updated. n64-systemtest oracle unchanged at 93 (nothing rendered changed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs,refactor(rdp): adjudicate bot review on the texture state Follow-up to the CodeRabbit and Antigravity reviews of PR #64. - TileDescriptor: order the T-axis fields (bits 19:10) before the S-axis fields (bits 9:0), matching the command word's MSB->LSB layout and the set_tile decoder (Antigravity). Field-init order in the literals was already T-first; this only aligns the struct declaration. - tmem_byte: document that `offset` is a BYTE address, not the 64-bit-word address that Set Tile's tmem_addr/line use (a word address must be *8 first) -- guards against an address-scaling bug in the upcoming load/sampler tickets (Antigravity). - sprint-1 plan: reword the VI register criterion as read/write-through (store-and-read-back), not full "hardware semantics", since the per-register write masks are deferred (ledger R-4) -- the doc is the spec (CodeRabbit). - sprint-2 plan: the TMEM acceptance criterion described an inline [u8; 4096], contradicting the lazily-allocated Option<Box<..>> the PR ships. Reword to the observable contract (reads as zero until written, resets at power-on; storage may be lazy) so the plan matches the implementation (CodeRabbit). Deferred (Antigravity): a tmem_mut/ensure_tmem allocation helper -- it lands with its first caller in T-32-002 (Load Block/Load Tile); adding it now would be dead code with no load command to use it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test,docs(rdp): adjudicate CodeRabbit outside-diff review (T-32-001) Three outside-the-diff comments from CodeRabbit's incremental review, all adopted. - set_tile_size_decodes_coords: seed descriptor 2's addressing fields (format/ size/line/tmem_addr) to non-zero via Set Tile before Set Tile Size, then assert they are preserved. The old test asserted `== 0` on a fresh descriptor, which would have passed even if Set Tile Size wrongly cleared them; this makes the preservation guard real. - sprint-2 plan, Load TLUT criterion: require an INCLUSIVE entry count SH-SL+1 (the typical (0,0,count-1,0) gives `count` entries) and require SL/TL/SH/TH to be latched into the tile descriptor. - sprint-2 plan, Load Block over-limit: attribute the "writes nothing over 2048 texels" behavior to its source (N64brew Commands.md Load Block) rather than stating it unsourced, and note the fuzz oracle supersedes it in Sprint 3 if the hardware does a partial write instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test,docs(rdp): adjudicate CodeRabbit round-3 review (T-32-001) Three more CodeRabbit comments (2 inline planning-doc, 1 outside-diff test), all adopted. - Add `dispatch_routes_set_tile_and_set_tile_size`: the field-level tests called set_tile/set_tile_size directly, bypassing the dispatch routing. This drives the actual `dispatch` entry for 0x35 and 0x32 with inputs that distinguish the handlers (Set Tile sets format=3 which Set Tile Size must preserve), so a mis-wired opcode arm is caught, not only a decode bug (outside-diff comment). - sprint-2 Load Block criterion: pin both sides of the 2048-texel boundary -- exactly 2048 loads fully, 2049 writes nothing -- to guard an off-by-one. - sprint-2 Load TLUT criterion: require the base to be in the upper TMEM half AND aligned to 16 TMEM words (128 bytes), with the test covering both a below-0x100 base and a misaligned high-half base, not only the lower bound. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 23, 2026
doublegate
added a commit
that referenced
this pull request
Jul 30, 2026
…ot lock-free The cpal callback ran `Mutex::lock()`. That callback executes on the device's real-time thread, so a blocking acquire there is a priority inversion: the audio thread waits on the emulator thread, misses its deadline, and the device underruns anyway. Blocking buys nothing and costs a glitch of unbounded length. `pull` now uses `try_lock` and emits one buffer of silence when contended. The trade is explicit and documented -- a contended callback produces silence where a blocking one MIGHT have got real samples; bounded and bursty beats unbounded. Both sides also transfer in bulk (a single drain/extend instead of per-sample pop_front/push_back with a capacity test each iteration), which shortens the critical section so the non-blocking acquire succeeds in practice rather than merely in principle. The module header called this "the lock-free audio ring" and the type "a simple lock-free-ish SPSC ring". It is neither -- it is a Mutex<VecDeque<f32>>. Both now say so, and the asymmetry that matters (producer may block, consumer must not) is stated at the type. A genuinely wait-free ring needs unsafe or a new dependency and stays a roadmap item. WHAT THIS DOES NOT FIX, recorded so it is not re-diagnosed. The ~1s on / ~1s off chopping is not a ring defect and not a resampler defect. `produce_audio` stages one EMULATED frame of audio per emulated frame while the device consumes in wall-clock time, so the supply ratio is exactly fps/60. At the ~10 FPS this core sustains, the ring receives under a fifth of what it must deliver. No buffering strategy manufactures the missing 80%; it closes when the core gets faster and not before. Since docs/performance.md records that 60 FPS is out of reach for this execution model, an explicit slow-running audio policy will eventually be needed instead -- noted, not designed. #56 stays open for that reason; this PR is scoped to the part that is a defect at ANY frame rate. Tests: a contended pull must return silence rather than wait. The pull runs on its own thread behind a recv_timeout so a regression FAILS in seconds with "pull must return while the lock is held, not block on it" rather than wedging the test binary until CI's job timeout -- the first version of this test did hang, which is loud but a bad citizen, and was fixed before merge rather than rationalised. Sequenced with channels, not sleeps, so there is no window to flake on; the holder is released on both the pass and fail paths so a failure cannot strand the puller. Mutation-checked both ways. Plus a test that a push larger than capacity keeps the newest tail, the behavior the bulk rewrite had to preserve. Review rejected two suggestions to recover from a poisoned mutex, on a checkable fact now recorded in the docs: the release profile sets panic = "abort", so there is no unwinding and a Mutex cannot be poisoned in a shipped build. Under test/debug a poisoned lock means a panic already happened and was already reported, so recovering the guard would continue on state that panic may have interrupted. For the callback specifically, WouldBlock and Poisoned mean the same actionable thing: no samples available to this call. Refs #56.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
First ticket of Phase 3 (v0.4.0 "Rasteriser"), Sprint 1 — T-31-001, the DP FIFO
and command decoder. Before any pixel can be drawn, the RDP has to walk its
command stream without desyncing: multi-word primitives (triangles, texture
rectangles) must be consumed whole, and unimplemented opcodes must be skipped by
exactly their length. The prior
tickstub advanced one 64-bit word per call,which would shred any multi-word command.
Changes
Rdp::tickdrains the FIFO. Reads the command word atDPC_CURRENT,decodes the opcode (bits 61:56), advances
DPC_CURRENTby the command's fulllength. One command retired per scheduler tick — gradual drain, not a burst.
No opcode is dispatched yet (T-31-002+ adds the sync commands + DP interrupt,
then the fill pipeline); each command is recognised, consumed, and counted.
rustyn64_rdp::command— the length table, sourced from the N64brewReality Display Processor/Commandsmap. One 64-bit word for every commandexcept the variable Fill Triangle forms (
0x08–0x0F: 4-word base + shade/texture/z blocks selected by the opcode's low three bits) and Texture
Rectangle (
0x24/0x25, 2 words).commands_processedis a retired-work tally, not a cycle position —nothing schedules against it, so it does not touch the derive-don't-increment
rule (ADR 0006).
docs/rdp.mdgains a command-decoder section in the same change;CHANGELOG.mdunder[Unreleased].Verification
0x00–0x3F, including alleight triangle forms and the texture-rectangle pair.
rectangle,
Sync Full) assertsDPC_CURRENTlands exactly onDPC_END, onecommand per tick. Mutation-checked: forcing the length to 8 fails the walk
(27 ticks ≠ 5).
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps, thethumbv7em-none-eabihfno_stdbuild, and
pre-commit run markdownlint.🤖 Generated with Claude Code