nv2a/pgraph: refactor blit into shared implementation with GL/VK wrap… #2675

KraftMacAndChee · 2026-01-16T01:34:34Z

gemini-code-assist · 2026-01-16T01:34:52Z

Summary of Changes

Hello @KraftMacAndChee, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the image blitting functionality within the NV2A PGRAPH component by extracting the core blit logic into a shared, renderer-agnostic implementation. This change eliminates code duplication between the OpenGL and Vulkan rendering backends, making the blitting process more maintainable and easier to extend in the future. The renderer-specific files now act as lightweight wrappers, providing their unique surface management functions to the unified blit routine.

Highlights

Shared Blit Implementation: A new common image blit function, pgraph_common_image_blit, has been introduced to centralize the core blitting logic for the NV2A PGRAPH renderer.
Renderer Agnostic Interface: A PGRAPHSurfaceOps struct was added, defining a set of function pointers for surface operations, allowing the common blit function to interact with renderer-specific surface management.
GL Blit Refactoring: The OpenGL-specific blit implementation (pgraph_gl_image_blit) has been refactored to delegate its operations to the new common blit function, passing its own GL-specific surface operation callbacks.
Vulkan Blit Refactoring: Similarly, the Vulkan-specific blit implementation (pgraph_vk_image_blit) has been refactored to utilize the common blit function, providing its Vulkan-specific surface operation callbacks.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request is a great refactoring that consolidates duplicated image blitting logic from the OpenGL and Vulkan backends into a shared implementation. This significantly improves maintainability. I've reviewed the new common implementation and found a few potential correctness issues that were likely present in the original code. My comments below provide suggestions to make the implementation more robust.

gemini-code-assist · 2026-01-16T01:37:16Z

hw/xbox/nv2a/pgraph/blit.c

+    PGRAPHState *pg = &d->pgraph;
+    ContextSurfaces2DState *context_surfaces = &pg->context_surfaces_2d;
+    ImageBlitState *image_blit = &pg->image_blit;
+    BetaState *beta = &pg->beta;
+
+    ops->surface_update(d, false, true, true);
+
+    assert(context_surfaces->object_instance == image_blit->context_surfaces);


Blits with zero width or height should be handled as no-ops. The current code proceeds with calculations that can lead to integer underflow when calculating buffer sizes (e.g., image_blit->height - 1 when height is 0). This can cause the bounds-checking assertions to fail or behave incorrectly.

It's best to add an early return at the beginning of the function if the width or height is zero.

PGRAPHState *pg = &d->pgraph; ContextSurfaces2DState *context_surfaces = &pg->context_surfaces_2d; ImageBlitState *image_blit = &pg->image_blit; if (image_blit->width == 0 || image_blit->height == 0) { return; } BetaState *beta = &pg->beta; ops->surface_update(d, false, true, true); assert(context_surfaces->object_instance == image_blit->context_surfaces);

It probably does make sense to bail early on zero-sized draws. I don't think we perform any interesting side effects that wouldn't be covered elsewhere in the pipeline.

gemini-code-assist · 2026-01-16T01:37:16Z

hw/xbox/nv2a/pgraph/blit.c

+            dest_row += context_surfaces->dest_pitch;
+        }
+    } else if (image_blit->operation == NV09F_SET_OPERATION_BLEND_AND) {
+        // NV2A_GL_DPRINTF(false, "NV09F_SET_OPERATION_BLEND_AND");


The BLEND_AND operation implementation assumes a 32bpp format by using x * 4 for pixel addressing. This will cause incorrect memory access if used with formats that have a different number of bytes per pixel (e.g., 1 or 2).

An assertion should be added to ensure this operation is only used with 32bpp formats.

assert(bytes_per_pixel == 4 && "BLEND_AND is only implemented for 32bpp formats");

gemini-code-assist · 2026-01-16T01:37:16Z

hw/xbox/nv2a/pgraph/blit.c

+        uint32_t beta_mult = beta->beta >> 16;
+        uint32_t inv_beta_mult = max_beta_mult - beta_mult;


If beta_mult is greater than max_beta_mult, inv_beta_mult will wrap around due to unsigned integer arithmetic, resulting in a very large positive value. This will lead to incorrect blending calculations.

beta_mult should be clamped to max_beta_mult to prevent this.

uint32_t beta_mult = MIN(beta->beta >> 16, max_beta_mult); uint32_t inv_beta_mult = max_beta_mult - beta_mult;

abaire

Mostly nitpicky comments

abaire · 2026-01-16T01:50:41Z

hw/xbox/nv2a/pgraph/gl/blit.c

+#include "renderer.h"
+#include "hw/xbox/nv2a/pgraph/blit.h"
+
+// TODO: Optimize.


I think we can remove this comment now. There's not much that could be done to optimize the one function call and the comment doesn't give any detail on how to do renderer-specific optimization.

abaire · 2026-01-16T01:51:27Z

hw/xbox/nv2a/pgraph/blit.c

+#include "hw/xbox/nv2a/pgraph/blit.h"
+
+
+// TODO: Optimize. Ideally this should all be done via OpenGL.


nit: Remove, now that this is a shared implementation it doesn't make sense to suggest GL.

abaire · 2026-01-16T01:52:40Z

hw/xbox/nv2a/pgraph/blit.c

+    PGRAPHState *pg = &d->pgraph;
+    ContextSurfaces2DState *context_surfaces = &pg->context_surfaces_2d;
+    ImageBlitState *image_blit = &pg->image_blit;
+    BetaState *beta = &pg->beta;
+
+    ops->surface_update(d, false, true, true);
+
+    assert(context_surfaces->object_instance == image_blit->context_surfaces);


It probably does make sense to bail early on zero-sized draws. I don't think we perform any interesting side effects that wouldn't be covered elsewhere in the pipeline.

abaire · 2026-01-16T01:53:30Z

hw/xbox/nv2a/pgraph/blit.c

+    uint8_t *dest_row = dest + dest_offset;
+
+    if (image_blit->operation == NV09F_SET_OPERATION_SRCCOPY) {
+        // NV2A_GL_DPRINTF(false, "NV09F_SET_OPERATION_SRCCOPY");


nit: While you're in here, we can probably lose these commented-out DPRINTF's as well.

abaire · 2026-01-16T01:55:05Z

hw/xbox/nv2a/pgraph/blit.h

@@ -0,0 +1,13 @@
+// Header for blit.c


I think we should bring over the comment from the original GL impl.

abaire · 2026-01-16T01:56:25Z

hw/xbox/nv2a/pgraph/blit.h

@@ -0,0 +1,13 @@
+// Header for blit.c
+
+#pragma once


I don't think we use pragma once outside of C++ files. Would be good to use old C-style guards like in

xemu/hw/xbox/nv2a/nv2a_regs.h

Line 21 in 24087af

#ifndef HW_NV2A_REGS_H

abaire · 2026-01-16T01:57:16Z

hw/xbox/nv2a/pgraph/blit.h

+
+#pragma once
+
+#include "hw/xbox/nv2a/nv2a_int.h"   // NV2AState, hwaddr, SurfaceBinding


nit: I don't think we declare the types used from headers elsewhere and these can easily go stale without automation. I think we should remove them.

abaire · 2026-01-16T02:00:02Z

hw/xbox/nv2a/pgraph/blit.h

+
+#include "hw/xbox/nv2a/nv2a_int.h"   // NV2AState, hwaddr, SurfaceBinding
+
+typedef struct PGRAPHSurfaceOps {


I wonder if we should put this in the PGRAPHRenderer so they're available everywhere?

xemu/hw/xbox/nv2a/pgraph/pgraph.h

Line 108 in 24087af

typedef struct PGRAPHRenderer {

mborgerson · 2026-01-16T07:32:21Z

Looks like this change was made on top of be882c0 and not 24087af (which fixed a bug). Please rebase the patch

mborgerson · 2026-01-16T07:41:44Z

hw/xbox/nv2a/pgraph/gl/blit.c

+void pgraph_gl_image_blit(NV2AState *d)
+{
+    static const PGRAPHSurfaceOps ops = {
+        .surface_update = pgraph_gl_surface_update,


Let's make this even simpler and drop the extra indirection here.

surface_update is already a renderer op

Just add surface_get, add surface_download_if_dirty and remove image_blit in PGRAPHRenderer::ops

Drop gl/blit.c, vk/blit.c, blit.h

mborgerson

Thanks for the patch. See comments about rebase and structure inline. Also don't worry about trying to fix pre-existing bugs with this patch. I've not evaluated the feedback of Gemini, but we should focus only on the unification in this patch. The delta between pgraph/blit.c, gl/blit.c, and vk/blit.c should be near 0. We can fix any bugs in the current implementation afterwards.

…pers

Used a shared implementation and dropped gl/blit.c. vk/blit.c, and blit.h

KraftMacAndChee · 2026-01-16T20:19:38Z

Thanks for the patch. See comments about rebase and structure inline. Also don't worry about trying to fix pre-existing bugs with this patch. I've not evaluated the feedback of Gemini, but we should focus only on the unification in this patch. The delta between pgraph/blit.c, gl/blit.c, and vk/blit.c should be near 0. We can fix any bugs in the current implementation afterwards.

I rebased it as you requested, I then worked to unify everything following your instruction and also removed gl/blit.c, vk/blit.c, and blit.h as requested. I spent a long time looking everything over and trying to double check myself and make sure I did everything correctly. However, I would appreciate you looking over it as I'm still getting my feet wet on this kind of stuff again.

abaire

This still looks like it has dropped all of the changes from 24087af

You can use https://github.com/abaire/xemu-dev_pgraph_test_results to look for other regressions (I haven't had a chance to update the goldens for the latest xemu yet, so it wouldn't catch the blit regression that this PR will cause).

hw/xbox/nv2a/pgraph/blit.c

hw/xbox/nv2a/pgraph/pgraph.h

KraftMacAndChee · 2026-01-16T20:53:05Z

This still looks like it has dropped all of the changes from 24087af

You can use https://github.com/abaire/xemu-dev_pgraph_test_results to look for other regressions (I haven't had a chance to update the goldens for the latest xemu yet, so it wouldn't catch the blit regression that this PR will cause).

Ah yes, you’re right. I wasn’t thinking and just rebased it without modifying anything. So it would be a regression. I’ll have to fix that when I get time.

mborgerson · 2026-01-17T02:26:29Z

hw/xbox/nv2a/pgraph/vk/renderer.c

 #include "hw/xbox/nv2a/nv2a_int.h"
 #include "renderer.h"
-
+#include "hw/xbox/nv2a/pgraph/surface.h"


Already included (via nv2a_int.h, which includes pgraph.h, which includes surface.h)

mborgerson · 2026-01-17T02:27:55Z

hw/xbox/nv2a/pgraph/blit.c

+        dest += dest_pitch;
+    }
+}
+void pgraph_common_image_blit(NV2AState *d)


Fix whitespace

Use name pgraph_image_blit (this was the former name, and since there is only one 'common' is redundant)

abaire · 2026-01-17T02:31:24Z

hw/xbox/nv2a/pgraph/pgraph.c


            if (image_blit->width && image_blit->height) {
-                d->pgraph.renderer->ops.image_blit(d);
+                pgraph_common_image_blit(d);


Should've pointed it out before, but "common" seems unnecessary if there are no renderer-spec. Maybe just "pgraph_image_blit"?

abaire · 2026-01-17T02:32:14Z

hw/xbox/nv2a/pgraph/blit.c

+    }
+}
+void pgraph_common_image_blit(NV2AState *d)
+


Remove newline

abaire · 2026-01-17T02:38:29Z

hw/xbox/nv2a/pgraph/blit.c

+            break;
+    }
+
+    hwaddr source_dma_len, dest_dma_len;


This is (I assume inadvertently) reverted from the split version in the current head, see line 104 and 111 in the diff.

abaire · 2026-01-17T02:39:36Z

hw/xbox/nv2a/pgraph/blit.c

+                     leftover_bytes / bytes_per_pixel, 1, leftover_bytes,
+                     context_surfaces->source_pitch,
+                     context_surfaces->dest_pitch, beta);
+        }


This indentation is wrong.

Since this is a new file you can probably just run clang-format on it to avoid having to waste time with whitespace changes. I believe I clang-formatted the original blit file when I fixed the clipping bug.

abaire · 2026-01-17T02:40:56Z

hw/xbox/nv2a/pgraph/blit.c

+
+    SurfaceBinding *surf_dest = pg->renderer->ops.surface_get(d, dest_addr);
+    if (surf_dest) {
+         if (adjusted_height < surf_dest->height ||


This indentation looks wrong

abaire · 2026-01-17T02:44:20Z

hw/xbox/nv2a/pgraph/blit.c

+        dest += dest_pitch;
+    }
+}
+void pgraph_common_image_blit(NV2AState *d)


Add the newline above this back

gemini-code-assist bot reviewed Jan 16, 2026

View reviewed changes

abaire reviewed Jan 16, 2026

View reviewed changes

mborgerson reviewed Jan 16, 2026

View reviewed changes

mborgerson requested changes Jan 16, 2026

View reviewed changes

nv2a/pgraph: refactor blit into shared implementation with GL/VK wrap…

f74e009

…pers

KraftMacAndChee force-pushed the refactor-pgraph-blit branch from b7aad53 to f74e009 Compare January 16, 2026 15:57

nv2a/pgraph: remove renderer image_blit op and use shared implementation

7176193

Used a shared implementation and dropped gl/blit.c. vk/blit.c, and blit.h

abaire reviewed Jan 16, 2026

View reviewed changes

hw/xbox/nv2a/pgraph/blit.c Show resolved Hide resolved

hw/xbox/nv2a/pgraph/pgraph.h Show resolved Hide resolved

nv2a/pgraph: fixed image blit regression after refactor

abc6606

mborgerson requested changes Jan 17, 2026

View reviewed changes

abaire reviewed Jan 17, 2026

View reviewed changes

		uint32_t beta_mult = beta->beta >> 16;
		uint32_t inv_beta_mult = max_beta_mult - beta_mult;

		#include "hw/xbox/nv2a/pgraph/blit.h"


		// TODO: Optimize. Ideally this should all be done via OpenGL.


		#pragma once

		#include "hw/xbox/nv2a/nv2a_int.h" // NV2AState, hwaddr, SurfaceBinding


		#include "hw/xbox/nv2a/nv2a_int.h" // NV2AState, hwaddr, SurfaceBinding

		typedef struct PGRAPHSurfaceOps {

Uh oh!

nv2a/pgraph: refactor blit into shared implementation with GL/VK wrap… #2675

Are you sure you want to change the base?

nv2a/pgraph: refactor blit into shared implementation with GL/VK wrap… #2675

Conversation

KraftMacAndChee commented Jan 16, 2026 • edited by mborgerson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Jan 16, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

abaire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mborgerson commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mborgerson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KraftMacAndChee commented Jan 16, 2026

Uh oh!

abaire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KraftMacAndChee commented Jan 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KraftMacAndChee commented Jan 16, 2026 •

edited by mborgerson

Loading

mborgerson commented Jan 16, 2026 •

edited

Loading

mborgerson left a comment •

edited

Loading