Both supported ROPs follow the same memory set dirty logic.
This consolidates that logic to remove the duplication.

Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati_2d.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index cfc7bf9789..5d24d11d7a 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -43,15 +43,29 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
     }
 }
 
+static void ati_set_dirty(ATIVGAState *s, const uint8_t *dst_bits,
+                          unsigned dst_y, unsigned dst_height, unsigned rop3)
+{
+    VGACommonState *vga = &s->vga;
+    DisplaySurface *ds = qemu_console_surface(vga->con);
+    unsigned dst_offset = dst_bits - vga->vram_ptr;
+    DPRINTF("%p %u ds: %p %d %d rop: %x\n",
+            vga->vram_ptr, vga->vbe_start_addr,
+            surface_data(ds), surface_stride(ds), surface_bits_per_pixel(ds),
+            rop3 >> 16);
+    if (dst_bits >= vga->vram_ptr + vga->vbe_start_addr &&
+        dst_bits < vga->vram_ptr + vga->vbe_start_addr +
+        vga->vbe_regs[VBE_DISPI_INDEX_YRES] * vga->vbe_line_offset) {
+        memory_region_set_dirty(&vga->vram, vga->vbe_start_addr +
+                                dst_offset + dst_y * surface_stride(ds),
+                                dst_height * surface_stride(ds));
+    }
+}
+
 void ati_2d_blt(ATIVGAState *s)
 {
     /* FIXME it is probably more complex than this and may need to be */
     /* rewritten but for now as a start just to get some output: */
-    DisplaySurface *ds = qemu_console_surface(s->vga.con);
-    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),
-            (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
     unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
                       s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
     unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
@@ -166,14 +180,6 @@ void ati_2d_blt(ATIVGAState *s)
                 memmove(&dst_bits[i], &src_bits[j], s->regs.dst_width * bypp);
             }
         }
-        if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
-            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 +
-                                    s->regs.dst_offset +
-                                    dst_y * surface_stride(ds),
-                                    s->regs.dst_height * surface_stride(ds));
-        }
         break;
     }
     case ROP3_PATCOPY:
@@ -216,18 +222,14 @@ void ati_2d_blt(ATIVGAState *s)
                 }
             }
         }
-        if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
-            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 +
-                                    s->regs.dst_offset +
-                                    dst_y * surface_stride(ds),
-                                    s->regs.dst_height * surface_stride(ds));
-        }
         break;
     }
     default:
         qemu_log_mask(LOG_UNIMP, "Unimplemented ati_2d blt op %x\n",
                       (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
+        return;
     }
+
+    ati_set_dirty(s, dst_bits, dst_y, s->regs.dst_height,
+                  (s->regs.dp_mix & GMC_ROP3_MASK));
 }
-- 
2.52.0

On Mon, 2 Feb 2026, Chad Jablonski wrote:
> Both supported ROPs follow the same memory set dirty logic.
> This consolidates that logic to remove the duplication.
>
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> ---
> hw/display/ati_2d.c | 44 +++++++++++++++++++++++---------------------
> 1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index cfc7bf9789..5d24d11d7a 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -43,15 +43,29 @@ static int ati_bpp_from_datatype(ATIVGAState *s)
>     }
> }
>
> +static void ati_set_dirty(ATIVGAState *s, const uint8_t *dst_bits,
> +                          unsigned dst_y, unsigned dst_height, unsigned rop3)
> +{
> +    VGACommonState *vga = &s->vga;
> +    DisplaySurface *ds = qemu_console_surface(vga->con);
> +    unsigned dst_offset = dst_bits - vga->vram_ptr;

Code style suggests an empty line here.

Are you sure dst_offset calculation this way is correct? Also if 
s->regs.crtc_offset is not 0 as that seems to be added to dst_bits for 
Rage128Pro. This may be replaced with correct value from ctx later but 
then you could just keep the original here to make sure it's not broken.

> +    DPRINTF("%p %u ds: %p %d %d rop: %x\n",
> +            vga->vram_ptr, vga->vbe_start_addr,
> +            surface_data(ds), surface_stride(ds), surface_bits_per_pixel(ds),
> +            rop3 >> 16);
> +    if (dst_bits >= vga->vram_ptr + vga->vbe_start_addr &&
> +        dst_bits < vga->vram_ptr + vga->vbe_start_addr +
> +        vga->vbe_regs[VBE_DISPI_INDEX_YRES] * vga->vbe_line_offset) {
> +        memory_region_set_dirty(&vga->vram, vga->vbe_start_addr +
> +                                dst_offset + dst_y * surface_stride(ds),
> +                                dst_height * surface_stride(ds));
> +    }
> +}
> +
> void ati_2d_blt(ATIVGAState *s)
> {
>     /* FIXME it is probably more complex than this and may need to be */
>     /* rewritten but for now as a start just to get some output: */
> -    DisplaySurface *ds = qemu_console_surface(s->vga.con);
> -    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),
> -            (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
>     unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
>                       s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
>     unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> @@ -166,14 +180,6 @@ void ati_2d_blt(ATIVGAState *s)
>                 memmove(&dst_bits[i], &src_bits[j], s->regs.dst_width * bypp);
>             }
>         }
> -        if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
> -            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 +
> -                                    s->regs.dst_offset +
> -                                    dst_y * surface_stride(ds),
> -                                    s->regs.dst_height * surface_stride(ds));
> -        }
>         break;
>     }
>     case ROP3_PATCOPY:
> @@ -216,18 +222,14 @@ void ati_2d_blt(ATIVGAState *s)
>                 }
>             }
>         }
> -        if (dst_bits >= s->vga.vram_ptr + s->vga.vbe_start_addr &&
> -            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 +
> -                                    s->regs.dst_offset +
> -                                    dst_y * surface_stride(ds),
> -                                    s->regs.dst_height * surface_stride(ds));
> -        }
>         break;
>     }
>     default:
>         qemu_log_mask(LOG_UNIMP, "Unimplemented ati_2d blt op %x\n",
>                       (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
> +        return;
>     }
> +
> +    ati_set_dirty(s, dst_bits, dst_y, s->regs.dst_height,
> +                  (s->regs.dp_mix & GMC_ROP3_MASK));
> }

Last two parameters (dst_height and rop3) are not needed because they can 
be get from s. This is changed later but also removed there so maybe don't 
add it here in the first place just use values from state as copied from 
the original lines.

Regards,
BALATON Zoltan

>
> Are you sure dst_offset calculation this way is correct? Also if 
> s->regs.crtc_offset is not 0 as that seems to be added to dst_bits for 
> Rage128Pro. This may be replaced with correct value from ctx later but 
> then you could just keep the original here to make sure it's not broken.
>

Good catch on this. I totally overlooked the R128-specific update to it.
I'll keep the original here. It does eventually bite us again in patch
11. This was an attempt to avoid adding it to the ctx while also allowing us
to pass only VGACommonState into ati_set_dirty instead of the full
ATIVGAState. But it may have been a bit ambitious.

The alternative here is to either continue to pass ATIVGAState to
ati_set_dirty or add dst_offset to the ATI2DCtx. It feels a little out of
place to me (like vram_end) on ATI2DCtx but I think that's a better tradeoff
than continuing to pass the full ATIVGAState struct. I'll add it to the ctx
in v8 unless you have objections or a cleaner approach.

On Tue, 3 Feb 2026, Chad Jablonski wrote:
>> Are you sure dst_offset calculation this way is correct? Also if
>> s->regs.crtc_offset is not 0 as that seems to be added to dst_bits for
>> Rage128Pro. This may be replaced with correct value from ctx later but
>> then you could just keep the original here to make sure it's not broken.
>>
>
> Good catch on this. I totally overlooked the R128-specific update to it.
> I'll keep the original here. It does eventually bite us again in patch
> 11. This was an attempt to avoid adding it to the ctx while also allowing us
> to pass only VGACommonState into ati_set_dirty instead of the full
> ATIVGAState. But it may have been a bit ambitious.
>
> The alternative here is to either continue to pass ATIVGAState to
> ati_set_dirty or add dst_offset to the ATI2DCtx. It feels a little out of
> place to me (like vram_end) on ATI2DCtx but I think that's a better tradeoff
> than continuing to pass the full ATIVGAState struct. I'll add it to the ctx
> in v8 unless you have objections or a cleaner approach.

I can't tell without looking at the new patch version but I think 
ati_vga_set_dirty needs to determine if the update is visible so it would 
need two values for that: start of visible screen (offset from vram start) 
and length of it so maybe these two values are needed in Ctx (or maybe 
multiple of these as there could be more screens but we only emulate one 
now) but I don't mind either if we keep passing ATIVGAState to all these 
functions.

Originally I thought Ctx would only include values that override what are 
in regs such as expanded bits and current src and dest coords that aren't 
updated in regs so host data can set and update these and not include a 
copy of every reg which could still be get from ATIVGAState. That way we 
don't need a separate ATIHostDataState struct either just include an 
ATI2DCtx in ATIVGAState and the blt function can use that instead of some 
regs while can keep using regs for other values. But if this would be too 
much reverting then I'm also OK with current approach and we can refine 
it later.

Regards,
BALATON Zoltan