perf(vcd): stop re-equipping BDMA on every launch when the card already carries a driver pair - #243
Conversation
…dy carries a driver pair NathanNeurotic: "BDMA operations are overboard... especially when the user already has the right one equipped. It feels like we are doing unnecessary over work, causing extended wait on game launch (handing over to POPSTARTER waiting on unnecessary MC transfer method?)" -- and the 1.0.1 anchor: MMCE VCD launches there were a dumb, fast handoff; everything slow is machinery we added in front. THE TRAP: vcdReadBdmaMode() collapses "marker file ABSENT" into VCD_BDMA_FAT32. A card that already has the right usbd.irx + usbhdfsd.irx pair but no bdma_config.txt (manual setup, or any install predating the marker) therefore read as a MISMATCH -- and vcdEnsureBdmaForLaunch ran the FULL equip on EVERY VCD launch: source-device module force-loads with bounded waits, POPS/ probes, two module copies onto the memory card, and a marker write. That is the extended pre-handoff wait, and it also CLOBBERED the user's manually-placed pair every launch. FIX: new no-marker fast path. Marker absent + BOTH driver modules present on the card = a manually managed pair -> trust it, touch nothing, hand off (1.0.1-style). An EXPLICIT different marker (a real variant switch) or a missing/partial pair still does the copy work -- ONCE -- after which the marker matches and every later launch takes the existing cheap path. Fast-path cost when it fires: ~4-5 mc metadata ops, replacing bounded-wait module loads + 4 file copies. Adversarially verified (SHIP): gate matrix walked (corrupt-marker self-heal survives; explicit-FAT32 marker still equips; partial pair still repairs; no-card unchanged); the Settings-screen equip path is untouched (helper is static, sole call site is the launch prep); POSIX-only IO rule kept; no interaction with the POPSTARTER net/SMB co-tenants. KNOWN TRADE-OFF (deliberate, doctrine): a manual pair of the WRONG variant now launches uncorrected -- previously it was overwritten every launch. The modules carry no variant identity, so this is undetectable without hashing; one Settings-screen equip writes the marker and restores auto-correction forever. Also by design: the fast path writes NO marker (it cannot know which variant the pair is), so Settings' BDMA MODE keeps showing FAT32 for manually-managed cards. Builds clean (make opl.elf, exit 0). HW-pending: FifthFox/MMCE VCD launch time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesBDMA manual installation handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/vcdsupport.c`:
- Around line 897-902: Update the marker check near VCD_BDMA_MARKER so only an
open() failure with errno equal to ENOENT is treated as marker absence; return 0
immediately for other errors, including I/O or permission failures. Preserve the
existing success path that closes the descriptor and returns 0 when the marker
is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 344f59f9-5373-4d34-abcf-8566d312c26f
📒 Files selected for processing (1)
src/vcdsupport.c
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: build-debug (ingame_ppctty_debug, :v20250725-2)
- GitHub Check: build-debug-ps2dev-latest (ingame_debug)
- GitHub Check: build-debug-ps2dev-latest (ingame_ppctty_debug)
🔇 Additional comments (1)
src/vcdsupport.c (1)
936-937: LGTM!
| snprintf(path, sizeof(path), "%s/%s", mcDir, VCD_BDMA_MARKER); | ||
| fd = open(path, O_RDONLY); | ||
| if (fd >= 0) { | ||
| close(fd); | ||
| return 0; // marker PRESENT -> its verdict stands (the caller already compared it) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only treat ENOENT as an absent marker.
A marker open() failure is currently indistinguishable from “no marker.” If the marker exists but the card returns an I/O or access error, this helper can trust an arbitrary module pair and suppress the repair/equip attempt. Return false for all errors other than ENOENT, so the fast path is used only when marker absence is confirmed.
Proposed fix
fd = open(path, O_RDONLY);
if (fd >= 0) {
close(fd);
return 0; // marker PRESENT -> its verdict stands (the caller already compared it)
}
+ if (errno != ENOENT)
+ return 0; // marker state is unknown; do not trust the manual pair📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| snprintf(path, sizeof(path), "%s/%s", mcDir, VCD_BDMA_MARKER); | |
| fd = open(path, O_RDONLY); | |
| if (fd >= 0) { | |
| close(fd); | |
| return 0; // marker PRESENT -> its verdict stands (the caller already compared it) | |
| } | |
| snprintf(path, sizeof(path), "%s/%s", mcDir, VCD_BDMA_MARKER); | |
| fd = open(path, O_RDONLY); | |
| if (fd >= 0) { | |
| close(fd); | |
| return 0; // marker PRESENT -> its verdict stands (the caller already compared it) | |
| } | |
| if (errno != ENOENT) | |
| return 0; // marker state is unknown; do not trust the manual pair |
🤖 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 `@src/vcdsupport.c` around lines 897 - 902, Update the marker check near
VCD_BDMA_MARKER so only an open() failure with errno equal to ENOENT is treated
as marker absence; return 0 immediately for other errors, including I/O or
permission failures. Preserve the existing success path that closes the
descriptor and returns 0 when the marker is present.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fast-path check for VCD launch equipping in src/vcdsupport.c by adding the vcdBdmaManualPairPresent helper function. This function checks if a memory card lacks a marker file but contains both driver modules, indicating a manually managed pair. Integrating this check into vcdEnsureBdmaForLaunch avoids unnecessary module copies and waits on launch. No review comments were provided, so there is no additional feedback.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
CodeRabbit's ENOENT-gating suggestion: declined after vetting. (1) errno is unreliable on PS2 mc drivers in this codebase — mcman/mmceman return blanket -1s and PS2SDK's errno mapping has burned us before (the config.c EIO/ENODEV comment was proven wrong for exactly this reason) — so gating on errno==ENOENT risks disabling the fast path on the very setups it serves. (2) The feared consequence (marker unreadable → trust the present pair → launch, touching nothing) is the doctrine-preferred behavior: running a 4-file copy on a card actively returning IO errors is the riskier action. (3) vcdReadBdmaMode() has collapsed open-failure into 'absent' since its introduction — the concern predates this PR. Launch-as-is on ambiguity is the dumb-handoff contract. |
1 participant
The complaint
With the 1.0.1 anchor: MMCE VCD launches there were a dumb, fast handoff — everything slow is machinery we added in front of it.
The trap — found exactly where you pointed
vcdReadBdmaMode()collapses "marker file absent" intoVCD_BDMA_FAT32. So a card that already has the rightusbd.irx+usbhdfsd.irxpair but nobdma_config.txt(manual setup, or any install predating the marker) read as a mismatch — and the launch prep ran the full equip on every single VCD launch: source-device module force-loads with bounded waits, POPS/ probes, two module copies onto the memory card, and a marker write. That's the extended pre-handoff wait — and it also clobbered the user's manually-placed pair every launch.Fix
New no-marker fast path: marker absent + both driver modules present = manually managed pair → trust it, touch nothing, hand off. An explicit different marker (a real variant switch) or a missing/partial pair still does the copy work — once — after which the marker matches and every later launch takes the existing cheap path. Fast-path cost: ~4–5 mc metadata ops, replacing bounded-wait module loads + 4 file copies.
Adversarially verified (SHIP)
Gate matrix walked: corrupt-marker self-heal survives; an explicit-FAT32 marker still equips; a partial pair still repairs; no-card behavior unchanged. Settings-screen equip path untouched (the helper is static; sole call site is the launch prep). POSIX-only IO rule kept; no interaction with the POPSTARTER net/SMB co-tenants.
Known trade-offs (deliberate, per the dumb-handoff doctrine)
Builds clean. HW-pending: FifthFox's MMCE VCD launch time.
🤖 Generated with Claude Code
Summary by CodeRabbit