Skip to content

psyqo: add selectable framebuffer Layout #2051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nicolasnoble wants to merge 2 commits into
base: main
Choose a base branch
from
Open

psyqo: add selectable framebuffer Layout #2051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b71308
psyqo: add selectable framebuffer Layout
nicolasnoble Jul 8, 2026
f5bea32
Merge branch 'main' into psyqo-gpu-layout
nicolasnoble Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/mips/psyqo/gpu.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,17 @@ class GPU {
enum class ColorMode { C15BITS, C24BITS };
enum class Interlace { PROGRESSIVE, INTERLACED };
enum class MiscSetting { CLEAR_VRAM, KEEP_VRAM };
// Where the two display buffers sit in VRAM. Selected at compile time; Default is the
// classic vertically-stacked pair (0,0)/(0,256). VerticalSwitch offsets the first buffer
// down by 16 lines; Horizontal places the buffers side by side.
enum class Layout { Default, VerticalSwitch, Horizontal };
#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
Comment on lines +112 to +118

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
#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.

void initialize(const Configuration &config);
void reinitialize(const Configuration &config);

Expand Down Expand Up @@ -539,6 +550,7 @@ class GPU {
void scheduleChainedDMA(uintptr_t head);
void chain(uintptr_t *first, uintptr_t *last, size_t count);
void scheduleOTC(uintptr_t *start, uint32_t count);
void setDisplayArea(bool firstBuffer);
void checkOTCAndTriggerCallback();
void prepareForTakeover();

Expand Down
82 changes: 68 additions & 14 deletions src/mips/psyqo/src/gpu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,19 @@
}
}

void psyqo::GPU::setDisplayArea(bool firstBuffer) {
uint32_t x = 0;
uint32_t y = 0;
if constexpr (c_layout == Layout::Horizontal) {
x = firstBuffer ? 0 : m_width;
Comment on lines +223 to +224

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

} else if constexpr (c_layout == Layout::VerticalSwitch) {
y = firstBuffer ? 256 : 16;
} else {
y = firstBuffer ? 256 : 0;
}
Hardware::GPU::Ctrl = 0x05000000 | (x << 0) | (y << 10);
}

void psyqo::GPU::flip() {
do {
pumpCallbacks();
Expand All @@ -227,12 +240,7 @@
parity ^= 1;
if (!m_interlaced) {
bool firstBuffer = !parity;
// Set Display Area
if (firstBuffer) {
Hardware::GPU::Ctrl = 0x05000000 | (256 << 10);
} else {
Hardware::GPU::Ctrl = 0x05000000;
}
setDisplayArea(firstBuffer);

Check notice on line 243 in src/mips/psyqo/src/gpu.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Complex Method 

psyqo::GPU::flip decreases in cyclomatic complexity from 14 to 13, threshold = 9

This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid
adding more conditionals and code to it without refactoring.

Check notice on line 243 in src/mips/psyqo/src/gpu.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ No longer an issue: Bumpy Road Ahead 

psyqo::GPU::flip is no longer above the threshold for logical blocks with deeply nested code

The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The
deeper the nesting and the more bumps, the lower the code health.
} else if (!pcsx_present()) {
while (1) {
uint32_t stat = Hardware::GPU::Ctrl;
Expand Down Expand Up @@ -280,9 +288,22 @@
int16_t height = m_height;
bool firstBuffer = !parity || m_interlaced;

scissor.start = Prim::DrawingAreaStart(Vertex{{.x = 0, .y = firstBuffer ? int16_t(0) : int16_t(256)}});
scissor.end = Prim::DrawingAreaEnd(Vertex{{.x = width, .y = firstBuffer ? height : int16_t(256 + height)}});
scissor.offset = Prim::DrawingOffset(Vertex{{.x = int16_t(0), .y = firstBuffer ? int16_t(0) : int16_t(256)}});
if constexpr (c_layout == Layout::Horizontal) {
int16_t x = firstBuffer ? int16_t(0) : width;
scissor.start = Prim::DrawingAreaStart(Vertex{{.x = x, .y = 0}});
scissor.end = Prim::DrawingAreaEnd(Vertex{{.x = int16_t(x + width), .y = height}});
scissor.offset = Prim::DrawingOffset(Vertex{{.x = x, .y = 0}});
} else {
int16_t y;
if constexpr (c_layout == Layout::VerticalSwitch) {
y = firstBuffer ? int16_t(16) : int16_t(256);
} else {
y = firstBuffer ? int16_t(0) : int16_t(256);
}
scissor.start = Prim::DrawingAreaStart(Vertex{{.x = 0, .y = y}});
scissor.end = Prim::DrawingAreaEnd(Vertex{{.x = width, .y = int16_t(y + height)}});
scissor.offset = Prim::DrawingOffset(Vertex{{.x = int16_t(0), .y = y}});
}

Check warning on line 306 in src/mips/psyqo/src/gpu.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Code Duplication 

The module contains 7 functions with similar structure: psyqo::GPU::getClear,psyqo::GPU::getNextClear,psyqo::GPU::getNextScissor,psyqo::GPU::getScissor
and 3 more functions

Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
}

void psyqo::GPU::getNextScissor(Prim::Scissor &scissor) {
Expand All @@ -291,9 +312,22 @@
int16_t height = m_height;
bool firstBuffer = !parity || m_interlaced;

scissor.start = Prim::DrawingAreaStart(Vertex{{.x = 0, .y = firstBuffer ? int16_t(256) : int16_t(0)}});
scissor.end = Prim::DrawingAreaEnd(Vertex{{.x = width, .y = firstBuffer ? int16_t(256 + height) : height}});
scissor.offset = Prim::DrawingOffset(Vertex{{.x = int16_t(0), .y = firstBuffer ? int16_t(256) : int16_t(0)}});
if constexpr (c_layout == Layout::Horizontal) {
int16_t x = firstBuffer ? width : int16_t(0);
scissor.start = Prim::DrawingAreaStart(Vertex{{.x = x, .y = 0}});
scissor.end = Prim::DrawingAreaEnd(Vertex{{.x = int16_t(x + width), .y = height}});
scissor.offset = Prim::DrawingOffset(Vertex{{.x = x, .y = 0}});
} else {
int16_t y;
if constexpr (c_layout == Layout::VerticalSwitch) {
y = firstBuffer ? int16_t(256) : int16_t(16);
} else {
y = firstBuffer ? int16_t(256) : int16_t(0);
}
scissor.start = Prim::DrawingAreaStart(Vertex{{.x = 0, .y = y}});
scissor.end = Prim::DrawingAreaEnd(Vertex{{.x = width, .y = int16_t(y + height)}});
scissor.offset = Prim::DrawingOffset(Vertex{{.x = int16_t(0), .y = y}});
}
}

void psyqo::GPU::clear(Color bg) {
Expand All @@ -307,15 +341,35 @@
int16_t height = m_height;
bool firstBuffer = !m_parity || m_interlaced;
ff.setColor(bg);
ff.rect = Rect{0, firstBuffer ? int16_t(0) : int16_t(256), width, height};
if constexpr (c_layout == Layout::Horizontal) {
ff.rect = Rect{firstBuffer ? int16_t(0) : width, 0, width, height};
} else {
int16_t y;
if constexpr (c_layout == Layout::VerticalSwitch) {
y = firstBuffer ? int16_t(16) : int16_t(256);
} else {
y = firstBuffer ? int16_t(0) : int16_t(256);
}
ff.rect = Rect{0, y, width, height};
}
}

void psyqo::GPU::getNextClear(Prim::FastFill &ff, Color bg) const {
int16_t width = m_width;
int16_t height = m_height;
bool firstBuffer = !m_parity || m_interlaced;
ff.setColor(bg);
ff.rect = Rect{0, firstBuffer ? int16_t(256) : int16_t(0), width, height};
if constexpr (c_layout == Layout::Horizontal) {
ff.rect = Rect{firstBuffer ? width : int16_t(0), 0, width, height};
} else {
int16_t y;
if constexpr (c_layout == Layout::VerticalSwitch) {
y = firstBuffer ? int16_t(256) : int16_t(16);
} else {
y = firstBuffer ? int16_t(256) : int16_t(0);
}
ff.rect = Rect{0, y, width, height};
}
}

void psyqo::GPU::uploadToVRAM(const uint16_t *data, Rect rect) {
Expand Down
Loading