psyqo: add selectable framebuffer Layout - #2051
Conversation
Add a compile-time GPU::Layout (Default, VerticalSwitch, Horizontal) selecting where the two display buffers sit in VRAM, routed through a new setDisplayArea() helper plus the scissor/clear paths. Default is the existing vertically-stacked pair (0,0)/(0,256) and is behaviorally unchanged. VerticalSwitch offsets the first buffer down by 16 lines; Horizontal places the buffers side by side. Selected via PSYQO_USE_VERTICAL_SWITCH_LAYOUT / PSYQO_USE_HORIZONTAL_LAYOUT. Co-authored-by: Elias Daler <eliasdaler@protonmail.com> Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds compile-time selectable GPU layouts, separates register setup into ChangesGPU Layout Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GPU
participant Hardware_GPU_Ctrl
Caller->>GPU: initialize(config)
GPU->>GPU: initialize COUNTERS[1]
GPU->>GPU: reinitialize(config)
GPU->>Hardware_GPU_Ctrl: write GPU setup registers
Caller->>GPU: flip()
GPU->>GPU: setDisplayArea(firstBuffer)
GPU->>Hardware_GPU_Ctrl: write display-area offsets based on c_layout
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mips/psyqo/src/gpu.cpp (2)
73-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInitialize the display area using the selected layout.
VerticalSwitchputs the first buffer at y=16, butreinitialize()still resets the display area to(0,0), so the initial display/reinitialize path points at a region that is not one of the configured buffers.🛠️ Proposed fix
- // Display Area - Hardware::GPU::Ctrl = 0x05000000; + // Display Area + if constexpr (c_layout == Layout::VerticalSwitch) { + Hardware::GPU::Ctrl = 0x05000000 | (16 << 10); + } else { + Hardware::GPU::Ctrl = 0x05000000; + }🤖 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/src/gpu.cpp` around lines 73 - 74, The display area setup in reinitialize() is hardcoded to (0,0) instead of using the active layout, so VerticalSwitch can point at an unconfigured region. Update the GPU display-area initialization in gpu.cpp to derive the starting y offset from the selected layout (the same layout used by the buffer configuration), and make sure the initial display/reinitialize path selects one of the configured buffers rather than always resetting to the top-left.
84-101: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
Horizontallayout against 640-wide modes.With
m_width = 640, the horizontal layout needs 1280 pixels of VRAM width for two side-by-side buffers, but this file treats VRAM as 1024 pixels wide. That makes the second buffer/scissor/clear region extend past VRAM.🛠️ Proposed fix
} else { m_width = 368; } + + if constexpr (c_layout == Layout::Horizontal) { + Kernel::assert(m_width <= 512, "Horizontal GPU layout requires display width <= 512"); + } if (config.config.videoMode == Configuration::VM_NTSC) {🤖 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/src/gpu.cpp` around lines 84 - 101, The horizontal layout in the GPU width setup does not account for 640-wide mode exceeding the assumed 1024-pixel VRAM span. Update the logic in the GPU width/layout handling around the width selection and horizontal buffer placement so that 640-wide modes are rejected, clamped, or routed to a safe layout before `Horizontal` computes the two side-by-side buffers, scissor, and clear regions.
🧹 Nitpick comments (1)
src/mips/psyqo/src/gpu.cpp (1)
291-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize buffer rectangle calculation.
The same layout/parity mapping is repeated across scissor and clear paths, and it has already diverged from
setDisplayArea. A small helper returning the current/next buffer origin would reduce the risk of future layout bugs. As per static analysis, CodeScene flags duplication acrossgetClear,getNextClear,getNextScissor, andgetScissor.🤖 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/src/gpu.cpp` around lines 291 - 372, Centralize the buffer-origin logic used by getScissor, getNextScissor, getClear, and getNextClear in psyqo::GPU, since the current layout/parity mapping is duplicated and has already drifted from setDisplayArea. Introduce a small helper that computes the current/next buffer origin for each Layout variant and parity state, then reuse it in those methods so the scissor and clear rectangles stay consistent and future layout changes only need one update.Source: Linters/SAST tools
🤖 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`:
- Around line 112-118: Reject conflicting layout defines at compile time in the
GPU layout selection block by updating the conditional around `c_layout` to
detect when both `PSYQO_USE_VERTICAL_SWITCH_LAYOUT` and
`PSYQO_USE_HORIZONTAL_LAYOUT` are defined and fail the build instead of silently
choosing `Layout::VerticalSwitch`. Keep the existing `c_layout` selection logic
for the valid single-define cases, but add a compile-time guard near the
`Layout` static constant so configuration mistakes are caught immediately.
In `@src/mips/psyqo/src/gpu.cpp`:
- Around line 223-224: The horizontal display-area mapping in flip() is reversed
relative to the vertical/default path, causing firstBuffer to point at the wrong
buffer. Update the Layout::Horizontal branch so it maps the next drawing buffer
to the opposite display buffer, matching the existing behavior in the other
layout case. Refer to flip() and the c_layout/Layout::Horizontal conditional
when making the change.
---
Outside diff comments:
In `@src/mips/psyqo/src/gpu.cpp`:
- Around line 73-74: The display area setup in reinitialize() is hardcoded to
(0,0) instead of using the active layout, so VerticalSwitch can point at an
unconfigured region. Update the GPU display-area initialization in gpu.cpp to
derive the starting y offset from the selected layout (the same layout used by
the buffer configuration), and make sure the initial display/reinitialize path
selects one of the configured buffers rather than always resetting to the
top-left.
- Around line 84-101: The horizontal layout in the GPU width setup does not
account for 640-wide mode exceeding the assumed 1024-pixel VRAM span. Update the
logic in the GPU width/layout handling around the width selection and horizontal
buffer placement so that 640-wide modes are rejected, clamped, or routed to a
safe layout before `Horizontal` computes the two side-by-side buffers, scissor,
and clear regions.
---
Nitpick comments:
In `@src/mips/psyqo/src/gpu.cpp`:
- Around line 291-372: Centralize the buffer-origin logic used by getScissor,
getNextScissor, getClear, and getNextClear in psyqo::GPU, since the current
layout/parity mapping is duplicated and has already drifted from setDisplayArea.
Introduce a small helper that computes the current/next buffer origin for each
Layout variant and parity state, then reuse it in those methods so the scissor
and clear rectangles stay consistent and future layout changes only need one
update.
🪄 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: 4f19267a-91c5-4670-8068-7952ccf42e65
📒 Files selected for processing (2)
src/mips/psyqo/gpu.hhsrc/mips/psyqo/src/gpu.cpp
| #if defined(PSYQO_USE_VERTICAL_SWITCH_LAYOUT) | ||
| static constexpr Layout c_layout = Layout::VerticalSwitch; | ||
| #elif defined(PSYQO_USE_HORIZONTAL_LAYOUT) | ||
| static constexpr Layout c_layout = Layout::Horizontal; | ||
| #else | ||
| static constexpr Layout c_layout = Layout::Default; | ||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject conflicting layout defines at compile time.
If both PSYQO_USE_VERTICAL_SWITCH_LAYOUT and PSYQO_USE_HORIZONTAL_LAYOUT are defined, this silently selects VerticalSwitch, which can hide a build configuration mistake.
🛠️ Proposed fix
+#if defined(PSYQO_USE_VERTICAL_SWITCH_LAYOUT) && defined(PSYQO_USE_HORIZONTAL_LAYOUT)
+#error "Only one PSYQO GPU layout macro can be defined"
+#endif
+
`#if` defined(PSYQO_USE_VERTICAL_SWITCH_LAYOUT)
static constexpr Layout c_layout = Layout::VerticalSwitch;📝 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.
| #if defined(PSYQO_USE_VERTICAL_SWITCH_LAYOUT) | |
| static constexpr Layout c_layout = Layout::VerticalSwitch; | |
| #elif defined(PSYQO_USE_HORIZONTAL_LAYOUT) | |
| static constexpr Layout c_layout = Layout::Horizontal; | |
| #else | |
| static constexpr Layout c_layout = Layout::Default; | |
| #endif | |
| `#if` defined(PSYQO_USE_VERTICAL_SWITCH_LAYOUT) && defined(PSYQO_USE_HORIZONTAL_LAYOUT) | |
| `#error` "Only one PSYQO GPU layout macro can be defined" | |
| `#endif` | |
| `#if` defined(PSYQO_USE_VERTICAL_SWITCH_LAYOUT) | |
| static constexpr Layout c_layout = Layout::VerticalSwitch; | |
| `#elif` defined(PSYQO_USE_HORIZONTAL_LAYOUT) | |
| static constexpr Layout c_layout = Layout::Horizontal; | |
| `#else` | |
| static constexpr Layout c_layout = Layout::Default; | |
| `#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/gpu.hh` around lines 112 - 118, Reject conflicting layout
defines at compile time in the GPU layout selection block by updating the
conditional around `c_layout` to detect when both
`PSYQO_USE_VERTICAL_SWITCH_LAYOUT` and `PSYQO_USE_HORIZONTAL_LAYOUT` are defined
and fail the build instead of silently choosing `Layout::VerticalSwitch`. Keep
the existing `c_layout` selection logic for the valid single-define cases, but
add a compile-time guard near the `Layout` static constant so configuration
mistakes are caught immediately.
| if constexpr (c_layout == Layout::Horizontal) { | ||
| x = firstBuffer ? 0 : m_width; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Flip the horizontal display-area mapping.
flip() passes whether the next drawing buffer is the first buffer; vertical/default map that to the opposite buffer for display, but the horizontal branch maps it to the same buffer. After the first flip, it displays the untouched second buffer instead of the first rendered buffer.
🐛 Proposed fix
uint32_t y = 0;
if constexpr (c_layout == Layout::Horizontal) {
- x = firstBuffer ? 0 : m_width;
+ x = firstBuffer ? m_width : 0;📝 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.
| if constexpr (c_layout == Layout::Horizontal) { | |
| x = firstBuffer ? 0 : m_width; | |
| if constexpr (c_layout == Layout::Horizontal) { | |
| x = firstBuffer ? m_width : 0; |
🤖 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/src/gpu.cpp` around lines 223 - 224, The horizontal
display-area mapping in flip() is reversed relative to the vertical/default
path, causing firstBuffer to point at the wrong buffer. Update the
Layout::Horizontal branch so it maps the next drawing buffer to the opposite
display buffer, matching the existing behavior in the other layout case. Refer
to flip() and the c_layout/Layout::Horizontal conditional when making the
change.
1 participant
Add a compile-time GPU::Layout (Default, VerticalSwitch, Horizontal) selecting
where the two display buffers sit in VRAM, routed through a new setDisplayArea()
helper plus the scissor/clear paths. Default is the existing vertically-stacked
pair (0,0)/(0,256) and is behaviorally unchanged. VerticalSwitch offsets the
first buffer down by 16 lines; Horizontal places the buffers side by side.
Selected via PSYQO_USE_VERTICAL_SWITCH_LAYOUT / PSYQO_USE_HORIZONTAL_LAYOUT.
Stacked on #2050 (the reinitialize() split); rebase once that merges.