When writing pixels we have to take into account if the frame buffer
endianness matches the host endianness or we need to swap to correct
endianness. This caused wrong colors e.g. with PPC Linux guest that
uses big endian frame buffer when running on little endian host.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
ROP3_BLACKNESS, ROP3_WHITENESS is probably still broken with <24 bit
modes but I don't know anything that uses it so I leave it for now
because I'm not sure which palette entries should that use in 15/16
bit modes so I'm not trying to fix that without a test case
hw/display/ati_2d.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 37fe6c17ee..0cbbdc33f4 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -50,6 +50,7 @@ typedef struct {
bool host_data_active;
bool left_to_right;
bool top_to_bottom;
+ bool need_swap;
uint32_t frgd_clr;
const uint8_t *palette;
const uint8_t *vram_end;
@@ -89,6 +90,7 @@ static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
ctx->host_data_active = s->host_data.active;
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->need_swap = HOST_BIG_ENDIAN != s->vga.big_endian_fb ? true : false;
ctx->frgd_clr = s->regs.dp_brush_frgd_clr;
ctx->palette = s->vga.palette;
ctx->dst_offset = s->regs.dst_offset;
@@ -131,6 +133,17 @@ static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
(ctx->top_to_bottom ? 'v' : '^'));
}
+static uint32_t make_filler(int bpp, uint32_t color)
+{
+ if (bpp < 24) {
+ color |= color << 16;
+ if (bpp < 15) {
+ color |= color << 8;
+ }
+ }
+ return color;
+}
+
static bool ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
{
QemuRect vis_src, vis_dst;
@@ -255,7 +268,7 @@ static bool ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
switch (ctx->rop3) {
case ROP3_PATCOPY:
- filler = ctx->frgd_clr;
+ filler = make_filler(ctx->bpp, ctx->frgd_clr);
break;
case ROP3_BLACKNESS:
filler = 0xffUL << 24 | rgb_to_pixel32(ctx->palette[0],
@@ -268,10 +281,12 @@ static bool ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
ctx->palette[5]);
break;
}
-
DPRINTF("pixman_fill(%p, %ld, %d, %d, %d, %d, %d, %x)\n",
ctx->dst_bits, ctx->dst_stride / sizeof(uint32_t), ctx->bpp,
vis_dst.x, vis_dst.y, vis_dst.width, vis_dst.height, filler);
+ if (ctx->need_swap) {
+ bswap32s(&filler);
+ }
#ifdef CONFIG_PIXMAN
if (!(use_pixman & BIT(0)) ||
!pixman_fill((uint32_t *)ctx->dst_bits,
@@ -325,11 +340,8 @@ void ati_2d_blt(ATIVGAState *s)
bool ati_host_data_flush(ATIVGAState *s)
{
ATI2DCtx ctx, chunk;
- uint32_t fg = s->regs.dp_src_frgd_clr;
- uint32_t bg = s->regs.dp_src_bkgd_clr;
unsigned bypp, pix_count, row, col, idx;
uint8_t pix_buf[ATI_HOST_DATA_ACC_BITS * sizeof(uint32_t)];
- uint32_t byte_pix_order = s->regs.dp_datatype & DP_BYTE_PIX_ORDER;
uint32_t src_source = s->regs.dp_mix & DP_SRC_SOURCE;
uint32_t src_datatype = s->regs.dp_datatype & DP_SRC_DATATYPE;
@@ -360,21 +372,27 @@ bool ati_host_data_flush(ATIVGAState *s)
}
bypp = ctx.bpp / 8;
-
+ pix_count = ATI_HOST_DATA_ACC_BITS;
if (src_datatype == SRC_COLOR) {
- pix_count = ATI_HOST_DATA_ACC_BITS / ctx.bpp;
- memcpy(pix_buf, &s->host_data.acc[0], sizeof(s->host_data.acc));
+ pix_count /= ctx.bpp;
+ memcpy(pix_buf, s->host_data.acc, sizeof(s->host_data.acc));
} else {
- pix_count = ATI_HOST_DATA_ACC_BITS;
/* Expand monochrome bits to color pixels */
+ uint32_t byte_pix_order = s->regs.dp_datatype & DP_BYTE_PIX_ORDER;
+ uint32_t fg = make_filler(ctx.bpp, s->regs.dp_src_frgd_clr);
+ uint32_t bg = make_filler(ctx.bpp, s->regs.dp_src_bkgd_clr);
+
+ if (ctx.need_swap) {
+ bswap32s(&fg);
+ bswap32s(&bg);
+ }
idx = 0;
for (int word = 0; word < 4; word++) {
for (int byte = 0; byte < 4; byte++) {
uint8_t byte_val = s->host_data.acc[word] >> (byte * 8);
for (int i = 0; i < 8; i++) {
bool is_fg = byte_val & BIT(byte_pix_order ? i : 7 - i);
- uint32_t color = is_fg ? fg : bg;
- stn_he_p(&pix_buf[idx], bypp, color);
+ stn_he_p(&pix_buf[idx], bypp, is_fg ? fg : bg);
idx += bypp;
}
}
--
2.41.3
>
> +static uint32_t make_filler(int bpp, uint32_t color)
> +{
> + if (bpp < 24) {
> + color |= color << 16;
> + if (bpp < 15) {
> + color |= color << 8;
> + }
> + }
> + return color;
> +}
> +
Does this assume that the upper bits of color are zeroed when bpp < 24? Is that
a safe assumption or could those bits be garbage?
On Wed, 18 Mar 2026, Chad Jablonski wrote:
>> +static uint32_t make_filler(int bpp, uint32_t color)
>> +{
>> + if (bpp < 24) {
>> + color |= color << 16;
>> + if (bpp < 15) {
>> + color |= color << 8;
>> + }
>> + }
>> + return color;
>> +}
>> +
>
> Does this assume that the upper bits of color are zeroed when bpp < 24? Is that
> a safe assumption or could those bits be garbage?
I think to be a valid <24 bit color the upper bits should be 0 here. In
practice this is called with register values where we only support 32 bit
writes so the only way to get garbage there if the guest does a 32 bit
write with garbage bits. (I.e. it can't do an 8 or 16 bit write and leave
garbage in upper bits from earier value.) It seems unlikely that a guest
doing 32 bit write would leave garbage in upper bits and if this could
happen it's better to take care of that at register write not here so I'd
assume color is some valid color for the bpp and the caller should make
sure if needed. This function just expands that to 32 bits so we don't
have to care about other sizes later.
Regards,
BALATON Zoltan
> > I think to be a valid <24 bit color the upper bits should be 0 here. In > practice this is called with register values where we only support 32 bit > writes so the only way to get garbage there if the guest does a 32 bit > write with garbage bits. (I.e. it can't do an 8 or 16 bit write and leave > garbage in upper bits from earier value.) It seems unlikely that a guest > doing 32 bit write would leave garbage in upper bits and if this could > happen it's better to take care of that at register write not here so I'd > assume color is some valid color for the bpp and the caller should make > sure if needed. This function just expands that to 32 bits so we don't > have to care about other sizes later. > Makes sense, thanks. Tested again with 8, 16, and 32-bit XOrg modes and all display a gray background in fluxbox now. Tested-by: Chad Jablonski <chad@jablonski.xyz> Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
© 2016 - 2026 Red Hat, Inc.