Previously all state derived from registers was moved to locals. Now we
can mechanically replace those locals with fields on the new ATI2DCtx
struct.
Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---
vram_end feels a little out of place to me in the ATI2DCtx struct but moving
it into the ctx allows us to eventually remove ATIVGAState entirely. I
also considered moving the dst validation that requires it outside of
ati_2d_blt and making it the responsibility of the caller but that has its
own downsides (forgetting to validate). In the end I left it in the ctx.
---
hw/display/ati_2d.c | 216 +++++++++++++++++++++++++-------------------
1 file changed, 122 insertions(+), 94 deletions(-)
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index a368950182..bc41ad37f6 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -13,6 +13,7 @@
#include "qemu/log.h"
#include "ui/pixel_ops.h"
#include "ui/console.h"
+#include "ui/rect.h"
/*
* NOTE:
@@ -43,6 +44,26 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
}
}
+typedef struct {
+ int bpp;
+ uint32_t rop3;
+ bool left_to_right;
+ bool top_to_bottom;
+ uint32_t frgd_clr;
+ const uint8_t *palette;
+ const uint8_t *vram_end;
+
+ /* dst */
+ QemuRect dst;
+ int dst_stride;
+ uint8_t *dst_bits;
+
+ /* src */
+ QemuRect src;
+ int src_stride;
+ const uint8_t *src_bits;
+} ATI2DCtx;
+
void ati_2d_blt(ATIVGAState *s)
{
/* FIXME it is probably more complex than this and may need to be */
@@ -50,105 +71,111 @@ void ati_2d_blt(ATIVGAState *s)
DisplaySurface *ds = qemu_console_surface(s->vga.con);
bool use_pixman_fill = s->use_pixman & BIT(0);
bool use_pixman_blt = s->use_pixman & BIT(1);
- int rop3 = s->regs.dp_mix & GMC_ROP3_MASK;
- bool left_to_right = s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT;
- bool top_to_bottom = s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM;
- uint32_t frgd_clr = s->regs.dp_brush_frgd_clr;
- uint8_t *palette = s->vga.palette;
+ ATI2DCtx ctx;
+ ctx.rop3 = s->regs.dp_mix & GMC_ROP3_MASK;
+ ctx.left_to_right = s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT;
+ ctx.top_to_bottom = s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM;
+ ctx.frgd_clr = s->regs.dp_brush_frgd_clr;
+ ctx.palette = s->vga.palette;
DPRINTF("%p %u ds: %p %d %d rop: %x\n", s->vga.vram_ptr,
s->vga.vbe_start_addr, surface_data(ds), surface_stride(ds),
surface_bits_per_pixel(ds),
- rop3 >> 16);
+ ctx.rop3 >> 16);
unsigned dst_offset = s->regs.dst_offset;
- unsigned dst_width = s->regs.dst_width;
- unsigned dst_height = s->regs.dst_height;
- unsigned dst_x = (left_to_right ?
- s->regs.dst_x : s->regs.dst_x + 1 - dst_width);
- unsigned dst_y = (top_to_bottom ?
- s->regs.dst_y : s->regs.dst_y + 1 - dst_height);
- int bpp = ati_bpp_from_datatype(s);
- if (!bpp) {
+ ctx.dst.width = s->regs.dst_width;
+ ctx.dst.height = s->regs.dst_height;
+ ctx.dst.x = (ctx.left_to_right ?
+ s->regs.dst_x : s->regs.dst_x + 1 - ctx.dst.width);
+ ctx.dst.y = (ctx.top_to_bottom ?
+ s->regs.dst_y : s->regs.dst_y + 1 - ctx.dst.height);
+ ctx.bpp = ati_bpp_from_datatype(s);
+ if (!ctx.bpp) {
qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
return;
}
- int dst_stride = s->regs.dst_pitch;
- if (!dst_stride) {
+ ctx.dst_stride = s->regs.dst_pitch;
+ if (!ctx.dst_stride) {
qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
return;
}
- uint8_t *dst_bits = s->vga.vram_ptr + dst_offset;
+ ctx.dst_bits = s->vga.vram_ptr + dst_offset;
if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
- dst_bits += s->regs.crtc_offset & 0x07ffffff;
- dst_stride *= bpp;
+ ctx.dst_bits += s->regs.crtc_offset & 0x07ffffff;
+ ctx.dst_stride *= ctx.bpp;
}
- int dst_stride_words = dst_stride / sizeof(uint32_t);
- uint8_t *end = s->vga.vram_ptr + s->vga.vram_size;
- if (dst_x > 0x3fff || dst_y > 0x3fff || dst_bits >= end
- || dst_bits + dst_x
- + (dst_y + dst_height) * dst_stride >= end) {
+ int dst_stride_words = ctx.dst_stride / sizeof(uint32_t);
+ ctx.vram_end = s->vga.vram_ptr + s->vga.vram_size;
+ if (ctx.dst.x > 0x3fff || ctx.dst.y > 0x3fff || ctx.dst_bits >= ctx.vram_end
+ || ctx.dst_bits + ctx.dst.x
+ + (ctx.dst.y + ctx.dst.height) * ctx.dst_stride >= ctx.vram_end) {
qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
return;
}
DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
s->regs.src_offset, dst_offset, s->regs.default_offset,
- s->regs.src_pitch, dst_stride, s->regs.default_pitch,
- s->regs.src_x, s->regs.src_y, dst_x, dst_y,
- dst_width, dst_height,
- (left_to_right ? '>' : '<'),
- (top_to_bottom ? 'v' : '^'));
- switch (rop3) {
+ ctx.src_stride, ctx.dst_stride, s->regs.default_pitch,
+ ctx.src.x, ctx.src.y, ctx.dst.x, ctx.dst.y,
+ ctx.dst.width, ctx.dst.height,
+ (ctx.left_to_right ? '>' : '<'),
+ (ctx.top_to_bottom ? 'v' : '^'));
+ switch (ctx.rop3) {
case ROP3_SRCCOPY:
{
bool fallback = false;
- unsigned src_x = (left_to_right ?
- s->regs.src_x : s->regs.src_x + 1 - dst_width);
- unsigned src_y = (top_to_bottom ?
- s->regs.src_y : s->regs.src_y + 1 - dst_height);
- int src_stride = s->regs.src_pitch;
- if (!src_stride) {
+ ctx.src.x = (ctx.left_to_right ?
+ s->regs.src_x : s->regs.src_x + 1 - ctx.dst.width);
+ ctx.src.y = (ctx.top_to_bottom ?
+ s->regs.src_y : s->regs.src_y + 1 - ctx.dst.height);
+ ctx.src_stride = s->regs.src_pitch;
+ if (!ctx.src_stride) {
qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
return;
}
- uint8_t *src_bits = s->vga.vram_ptr + s->regs.src_offset;
+ ctx.src_bits = s->vga.vram_ptr + s->regs.src_offset;
if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
- src_bits += s->regs.crtc_offset & 0x07ffffff;
- src_stride *= bpp;
+ ctx.src_bits += s->regs.crtc_offset & 0x07ffffff;
+ ctx.src_stride *= ctx.bpp;
}
- int src_stride_words = src_stride / sizeof(uint32_t);
- if (src_x > 0x3fff || src_y > 0x3fff || src_bits >= end
- || src_bits + src_x
- + (src_y + dst_height) * src_stride >= end) {
+ int src_stride_words = ctx.src_stride / sizeof(uint32_t);
+ if (ctx.src.x > 0x3fff || ctx.src.y > 0x3fff
+ || ctx.src_bits >= ctx.vram_end
+ || ctx.src_bits + ctx.src.x
+ + (ctx.src.y + ctx.dst.height) * ctx.src_stride >= ctx.vram_end) {
qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
return;
}
DPRINTF("pixman_blt(%p, %p, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d)\n",
- src_bits, dst_bits, src_stride_words, dst_stride_words,
- bpp, bpp, src_x, src_y, dst_x, dst_y,
- dst_width, dst_height);
+ ctx.src_bits, ctx.dst_bits, src_stride_words, dst_stride_words,
+ ctx.bpp, ctx.bpp, ctx.src.x, ctx.src.y, ctx.dst.x, ctx.dst.y,
+ ctx.dst.width, ctx.dst.height);
#ifdef CONFIG_PIXMAN
- if (use_pixman_blt && left_to_right && top_to_bottom) {
- fallback = !pixman_blt((uint32_t *)src_bits, (uint32_t *)dst_bits,
- src_stride_words, dst_stride_words, bpp, bpp,
- src_x, src_y, dst_x, dst_y,
- dst_width, dst_height);
+ if (use_pixman_blt && ctx.left_to_right && ctx.top_to_bottom) {
+ fallback = !pixman_blt((uint32_t *)ctx.src_bits,
+ (uint32_t *)ctx.dst_bits,
+ src_stride_words, dst_stride_words,
+ ctx.bpp, ctx.bpp,
+ ctx.src.x, ctx.src.y, ctx.dst.x, ctx.dst.y,
+ ctx.dst.width, ctx.dst.height);
} else if (use_pixman_blt) {
/* FIXME: We only really need a temporary if src and dst overlap */
- int llb = dst_width * (bpp / 8);
+ int llb = ctx.dst.width * (ctx.bpp / 8);
int tmp_stride = DIV_ROUND_UP(llb, sizeof(uint32_t));
uint32_t *tmp = g_malloc(tmp_stride * sizeof(uint32_t) *
- dst_height);
- fallback = !pixman_blt((uint32_t *)src_bits, tmp,
- src_stride_words, tmp_stride, bpp, bpp,
- src_x, src_y, 0, 0,
- dst_width, dst_height);
+ ctx.dst.height);
+ fallback = !pixman_blt((uint32_t *)ctx.src_bits, tmp,
+ src_stride_words, tmp_stride,
+ ctx.bpp, ctx.bpp,
+ ctx.src.x, ctx.src.y, 0, 0,
+ ctx.dst.width, ctx.dst.height);
if (!fallback) {
- fallback = !pixman_blt(tmp, (uint32_t *)dst_bits,
- tmp_stride, dst_stride_words, bpp, bpp,
- 0, 0, dst_x, dst_y,
- dst_width, dst_height);
+ fallback = !pixman_blt(tmp, (uint32_t *)ctx.dst_bits,
+ tmp_stride, dst_stride_words,
+ ctx.bpp, ctx.bpp,
+ 0, 0, ctx.dst.x, ctx.dst.y,
+ ctx.dst.width, ctx.dst.height);
}
g_free(tmp);
} else
@@ -157,18 +184,19 @@ void ati_2d_blt(ATIVGAState *s)
fallback = true;
}
if (fallback) {
- unsigned int y, i, j, bypp = bpp / 8;
- for (y = 0; y < dst_height; y++) {
- i = dst_x * bypp;
- j = src_x * bypp;
- if (top_to_bottom) {
- i += (dst_y + y) * dst_stride;
- j += (src_y + y) * src_stride;
+ unsigned int y, i, j, bypp = ctx.bpp / 8;
+ for (y = 0; y < ctx.dst.height; y++) {
+ i = ctx.dst.x * bypp;
+ j = ctx.src.x * bypp;
+ if (ctx.top_to_bottom) {
+ i += (ctx.dst.y + y) * ctx.dst_stride;
+ j += (ctx.src.y + y) * ctx.src_stride;
} else {
- i += (dst_y + dst_height - 1 - y) * dst_stride;
- j += (src_y + dst_height - 1 - y) * src_stride;
+ i += (ctx.dst.y + ctx.dst.height - 1 - y) * ctx.dst_stride;
+ j += (ctx.src.y + ctx.dst.height - 1 - y) * ctx.src_stride;
}
- memmove(&dst_bits[i], &src_bits[j], dst_width * bypp);
+ memmove(&ctx.dst_bits[i], &ctx.src_bits[j],
+ ctx.dst.width * bypp);
}
}
break;
@@ -179,38 +207,38 @@ void ati_2d_blt(ATIVGAState *s)
{
uint32_t filler = 0;
- switch (rop3) {
+ switch (ctx.rop3) {
case ROP3_PATCOPY:
- filler = frgd_clr;
+ filler = ctx.frgd_clr;
break;
case ROP3_BLACKNESS:
- filler = 0xffUL << 24 | rgb_to_pixel32(palette[0],
- palette[1],
- palette[2]);
+ filler = 0xffUL << 24 | rgb_to_pixel32(ctx.palette[0],
+ ctx.palette[1],
+ ctx.palette[2]);
break;
case ROP3_WHITENESS:
- filler = 0xffUL << 24 | rgb_to_pixel32(palette[3],
- palette[4],
- palette[5]);
+ filler = 0xffUL << 24 | rgb_to_pixel32(ctx.palette[3],
+ ctx.palette[4],
+ ctx.palette[5]);
break;
}
DPRINTF("pixman_fill(%p, %d, %d, %d, %d, %d, %d, %x)\n",
- dst_bits, dst_stride_words, bpp, dst_x, dst_y,
- dst_width, dst_height, filler);
+ ctx.dst_bits, dst_stride_words, ctx.bpp, ctx.dst.x, ctx.dst.y,
+ ctx.dst.width, ctx.dst.height, filler);
#ifdef CONFIG_PIXMAN
if (!use_pixman_fill ||
- !pixman_fill((uint32_t *)dst_bits, dst_stride_words,
- bpp, dst_x, dst_y,
- dst_width, dst_height, filler))
+ !pixman_fill((uint32_t *)ctx.dst_bits, dst_stride_words,
+ ctx.bpp, ctx.dst.x, ctx.dst.y,
+ ctx.dst.width, ctx.dst.height, filler))
#endif
{
/* fallback when pixman failed or we don't want to call it */
- unsigned int x, y, i, bypp = bpp / 8;
- for (y = 0; y < dst_height; y++) {
- i = dst_x * bypp + (dst_y + y) * dst_stride;
- for (x = 0; x < dst_width; x++, i += bypp) {
- stn_he_p(&dst_bits[i], bypp, filler);
+ unsigned int x, y, i, bypp = ctx.bpp / 8;
+ for (y = 0; y < ctx.dst.height; y++) {
+ i = ctx.dst.x * bypp + (ctx.dst.y + y) * ctx.dst_stride;
+ for (x = 0; x < ctx.dst.width; x++, i += bypp) {
+ stn_he_p(&ctx.dst_bits[i], bypp, filler);
}
}
}
@@ -218,15 +246,15 @@ void ati_2d_blt(ATIVGAState *s)
}
default:
qemu_log_mask(LOG_UNIMP, "Unimplemented ati_2d blt op %x\n",
- rop3 >> 16);
+ ctx.rop3 >> 16);
return;
}
- if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
- dst_bits < s->vga.vram_ptr + s->vga.vbe_start_addr +
+ if (ctx.dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
+ ctx.dst_bits < s->vga.vram_ptr + s->vga.vbe_start_addr +
s->vga.vbe_regs[VBE_DISPI_INDEX_YRES] * s->vga.vbe_line_offset) {
memory_region_set_dirty(&s->vga.vram, s->vga.vbe_start_addr +
- dst_offset + dst_y * surface_stride(ds),
- dst_height * surface_stride(ds));
+ dst_offset + ctx.dst.y * surface_stride(ds),
+ ctx.dst.height * surface_stride(ds));
}
}
--
2.52.0On Fri, 30 Jan 2026, Chad Jablonski wrote:
> Previously all state derived from registers was moved to locals. Now we
> can mechanically replace those locals with fields on the new ATI2DCtx
> struct.
>
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>
> ---
>
> vram_end feels a little out of place to me in the ATI2DCtx struct but moving
> it into the ctx allows us to eventually remove ATIVGAState entirely. I
> also considered moving the dst validation that requires it outside of
> ati_2d_blt and making it the responsibility of the caller but that has its
> own downsides (forgetting to validate). In the end I left it in the ctx.
> ---
> hw/display/ati_2d.c | 216 +++++++++++++++++++++++++-------------------
> 1 file changed, 122 insertions(+), 94 deletions(-)
>
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index a368950182..bc41ad37f6 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -13,6 +13,7 @@
> #include "qemu/log.h"
> #include "ui/pixel_ops.h"
> #include "ui/console.h"
> +#include "ui/rect.h"
>
> /*
> * NOTE:
> @@ -43,6 +44,26 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
> }
> }
>
> +typedef struct {
> + int bpp;
> + uint32_t rop3;
> + bool left_to_right;
> + bool top_to_bottom;
> + uint32_t frgd_clr;
> + const uint8_t *palette;
> + const uint8_t *vram_end;
> +
> + /* dst */
> + QemuRect dst;
> + int dst_stride;
> + uint8_t *dst_bits;
> +
> + /* src */
Since these are already prefixed with src_ and dst_ I'm not sure the
comments add much.
> + QemuRect src;
> + int src_stride;
> + const uint8_t *src_bits;
> +} ATI2DCtx;
> +
> void ati_2d_blt(ATIVGAState *s)
> {
> /* FIXME it is probably more complex than this and may need to be */
> @@ -50,105 +71,111 @@ void ati_2d_blt(ATIVGAState *s)
> DisplaySurface *ds = qemu_console_surface(s->vga.con);
> bool use_pixman_fill = s->use_pixman & BIT(0);
> bool use_pixman_blt = s->use_pixman & BIT(1);
> - int rop3 = s->regs.dp_mix & GMC_ROP3_MASK;
> - bool left_to_right = s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT;
> - bool top_to_bottom = s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM;
> - uint32_t frgd_clr = s->regs.dp_brush_frgd_clr;
> - uint8_t *palette = s->vga.palette;
> + ATI2DCtx ctx;
To reduce churn later maybe doing the pointer here as you had in the
previous version could help just not with _name but name_ or similar. Now
next patches convert these from ctx.something to ctx->something so this
patch does note help that much.
Regards,
BALATON Zoltan
© 2016 - 2026 Red Hat, Inc.