psyqo: groundwork for a host PC port#2047
Open
nicolasnoble wants to merge 3 commits into
Open
Conversation
The GTE kernels/registers, the BIOS syscall wrappers, the cop0 helpers in kernel.hh, and the AdvancedPad busy loop are thin wrappers around MIPS instructions that only assemble for the target. Guard them behind PS1_PC_PORT so psyqo can be built on the host as part of a PC port: with the macro defined those paths compile out (the GTE register readers return 0), and on a normal PS1 build the macro is undefined and codegen is unchanged. Co-authored-by: Elias Daler <eliasdaler@protonmail.com> Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Fragments and ordering tables encode their DMA linked-list pointer as a
packed 24-bit-address + 8-bit-count word ("head"). That packing is
PS1-specific: on a 64-bit host the next-pointer alone fills the word, so a
PC port needs the pointer and size stored separately. Introduce a ChainEntry
type that resolves to the packed head on PS1 (ChainEntryPS1) and to a
{next, size} pair on the host (ChainEntryPC), and have every fragment type
inherit it. Because the DMA node begins at the link word, the fixed
fragments' count field moves after the payload so it is never transferred.
Descend the head type through the GPU chaining API: chain(), scheduleOTC()
and the internal chain-walk pointers become uintptr_t so the link word is
pointer-width on the host. sendFragment() keeps uint32_t*, since it carries
primitive words rather than a link pointer.
Co-authored-by: Elias Daler <eliasdaler@protonmail.com>
Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
📝 WalkthroughWalkthroughThis PR adds a ChangesPS1_PC_PORT Inline Assembly Guards
Fragment ChainEntry Abstraction and GPU Chaining
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GPU
participant OrderingTable
participant ChainEntry
GPU->>OrderingTable: request ordering-table chain
OrderingTable->>ChainEntry: set bucket links and end marker
GPU->>ChainEntry: read chain head pointers
GPU->>GPU: schedule DMA chain and OTC
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mips/psyqo/fragments.hh (1)
66-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding
ChainEntryPC/ChainEntryPS1definitions with#ifdef.Both structs are always compiled regardless of
PS1_PC_PORT. WhenPS1_PC_PORTis not defined,ChainEntryisChainEntryPS1, soChainEntryPC::next(typed asChainEntry*) resolves toChainEntryPS1*— a semantically incorrect member type that could confuse future developers. Guarding each definition behind its respective#ifdef/#ifndefwould eliminate this latent type mismatch and make the intent clearer.♻️ Proposed refactor
-struct ChainEntryPS1; -struct ChainEntryPC; - -#ifdef PS1_PC_PORT -using ChainEntry = ChainEntryPC; -#else -using ChainEntry = ChainEntryPS1; -#endif - -struct ChainEntryPC { - ChainEntry* next; - unsigned size; - void setEndMarker() { - next = nullptr; - size = 0; - } - void set(ChainEntry* next_, unsigned size_) { - next = next_; - size = size_; - } -}; - -struct ChainEntryPS1 { - uintptr_t head; - void setEndMarker() { - head = 0xffffff; - } - void set(ChainEntryPS1* next, unsigned size) { - head = (size << 24) | (next->head & 0xffffff); - } -}; +#ifdef PS1_PC_PORT +struct ChainEntryPC { + ChainEntryPC* next; + unsigned size; + void setEndMarker() { + next = nullptr; + size = 0; + } + void set(ChainEntryPC* next_, unsigned size_) { + next = next_; + size = size_; + } +}; +using ChainEntry = ChainEntryPC; +#else +struct ChainEntryPS1 { + uintptr_t head; + void setEndMarker() { + head = 0xffffff; + } + void set(ChainEntryPS1* next, unsigned size) { + head = (size << 24) | (next->head & 0xffffff); + } +}; +using ChainEntry = ChainEntryPS1; +#endif🤖 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/mips/psyqo/fragments.hh` around lines 66 - 96, Guard the ChainEntryPC definition with `#ifdef` PS1_PC_PORT and the ChainEntryPS1 definition with the corresponding `#ifndef` PS1_PC_PORT. Keep the existing ChainEntry alias and each platform’s methods unchanged, ensuring only the selected platform’s struct is compiled.
🤖 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/mips/psyqo/gpu.hh`:
- Line 231: Guard the GPU fragment and chain templates, including sendFragment,
chain(Frag&), chain(Frag1*, Frag2*), and chain(OrderingTable&), so PS1_PC_PORT
builds never access .head on Fragments::ChainEntryPC. Mirror the conditional
path used by ordering-table.hh::insert(), or use the ChainEntry abstraction for
PC-port fragments while preserving the existing non-PC behavior.
---
Nitpick comments:
In `@src/mips/psyqo/fragments.hh`:
- Around line 66-96: Guard the ChainEntryPC definition with `#ifdef` PS1_PC_PORT
and the ChainEntryPS1 definition with the corresponding `#ifndef` PS1_PC_PORT.
Keep the existing ChainEntry alias and each platform’s methods unchanged,
ensuring only the selected platform’s struct is compiled.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18b22e80-7479-4b63-897e-0eb774ea49d5
📒 Files selected for processing (11)
src/mips/common/syscalls/syscalls.hsrc/mips/psyqo/advancedpad.hhsrc/mips/psyqo/fragment-concept.hhsrc/mips/psyqo/fragments.hhsrc/mips/psyqo/gpu.hhsrc/mips/psyqo/gte-kernels.hhsrc/mips/psyqo/gte-registers.hhsrc/mips/psyqo/kernel.hhsrc/mips/psyqo/ordering-table.hhsrc/mips/psyqo/src/gpu.cppsrc/mips/psyqo/src/ordering-table.cpp
| template <Fragment Frag> | ||
| void sendFragment(const Frag &fragment) { | ||
| sendFragment(&fragment.head + 1, fragment.getActualFragmentSize()); | ||
| sendFragment(reinterpret_cast<const uint32_t *>(&fragment.head + 1), fragment.getActualFragmentSize()); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all .head references in gpu.hh templates that lack PS1_PC_PORT guards.
rg -n '\.head' src/mips/psyqo/gpu.hh | grep -v 'm_chainHead\|m_chainTail\|chainHead\|chainTail\|head ='Repository: grumpycoders/pcsx-redux
Length of output: 603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## gpu.hh outline\n'
ast-grep outline src/mips/psyqo/gpu.hh --view expanded || true
printf '\n## fragment-concept.hh outline\n'
ast-grep outline src/mips/psyqo/fragment-concept.hh --view expanded || true
printf '\n## ordering-table.hh outline\n'
ast-grep outline src/mips/psyqo/ordering-table.hh --view expanded || true
printf '\n## gpu.hh relevant ranges\n'
sed -n '200,380p' src/mips/psyqo/gpu.hh | cat -n
printf '\n## fragment-concept.hh relevant ranges\n'
sed -n '1,140p' src/mips/psyqo/fragment-concept.hh | cat -n
printf '\n## ordering-table.hh relevant ranges\n'
sed -n '1,220p' src/mips/psyqo/ordering-table.hh | cat -nRepository: grumpycoders/pcsx-redux
Length of output: 20849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## fragments.hh outline\n'
ast-grep outline src/mips/psyqo/fragments.hh --view expanded || true
printf '\n## fragments.hh relevant ranges\n'
sed -n '1,260p' src/mips/psyqo/fragments.hh | cat -n
printf '\n## search for ChainEntryPC / ChainEntryPS1\n'
rg -n 'ChainEntryPC|ChainEntryPS1|struct ChainEntry|class ChainEntry|using ChainEntry' src/mips/psyqoRepository: grumpycoders/pcsx-redux
Length of output: 8340
Guard GPU fragment/chain templates for PS1_PC_PORT
sendFragment, chain(Frag&), chain(Frag1*, Frag2*), and chain(OrderingTable&) still dereference .head unconditionally. Fragments::ChainEntryPC has no head, so these templates won’t instantiate for PC-port fragments even though the Fragment concept already drops that requirement. Mirror the #ifdef PS1_PC_PORT split used in ordering-table.hh::insert(), or route the PC path through the ChainEntry abstraction.
🤖 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/mips/psyqo/gpu.hh` at line 231, Guard the GPU fragment and chain
templates, including sendFragment, chain(Frag&), chain(Frag1*, Frag2*), and
chain(OrderingTable&), so PS1_PC_PORT builds never access .head on
Fragments::ChainEntryPC. Mirror the conditional path used by
ordering-table.hh::insert(), or use the ChainEntry abstraction for PC-port
fragments while preserving the existing non-PC behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2047 +/- ##
==========================================
- Coverage 12.63% 12.63% -0.01%
==========================================
Files 538 541 +3
Lines 150069 150140 +71
==========================================
- Hits 18967 18964 -3
- Misses 131102 131176 +74 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Groundwork so psyqo can be compiled on a non-MIPS host as part of a PC port,
originating from Elias Daler's fork.
Two pieces:
Gate the non-portable inline assembly (GTE kernels/registers, BIOS syscall
wrappers, the cop0 helpers, the AdvancedPad busy loop) behind PS1_PC_PORT.
When the macro is defined those paths compile out; on a normal PS1 build the
macro is undefined and the generated code is unchanged.
Abstract the DMA chain link word behind a ChainEntry type. The packed
24-bit-address + 8-bit-count "head" is PS1-specific and does not survive a
64-bit host, where the next-pointer needs its own word; ChainEntry resolves
to the packed head on PS1 and to a {next, size} pair on the host. The head
type descends through chain()/scheduleOTC() as uintptr_t.
The macro is not defined by any in-tree build; the swappable pieces (the GPU
class and friends) remain the porter's responsibility. Native builds are
unaffected - the psyqo examples (cube, gte, torus, tetris) build unchanged.