Move register write logic into its own function. This is in preparation
for CCE engine support for register writes. MMIO writes will have their
own distinct policy that doesn't apply to writes made by the CCE engine.

Note: Because of the recursion in the MM_DATA handler the calls to
ati_mm_write needed to be changed to ati_reg_write. This means that
tracing output changes slightly for MM_DATA writes. Otherwise, this
is purely a refactor and does not change behavior.

Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati.c     | 25 ++++++++++++++++---------
 hw/display/ati_int.h |  2 ++
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index e9c3ad2cd1..33f8e211dc 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -530,14 +530,9 @@ static inline void ati_reg_write_offs(uint32_t *reg, int offs,
     }
 }
 
-static void ati_mm_write(void *opaque, hwaddr addr,
-                           uint64_t data, unsigned int size)
+void ati_reg_write(ATIVGAState *s, hwaddr addr,
+                   uint64_t data, unsigned int size)
 {
-    ATIVGAState *s = opaque;
-
-    if (addr < CUR_OFFSET || addr > CUR_CLR1 || ATI_DEBUG_HW_CURSOR) {
-        trace_ati_mm_write(size, addr, ati_reg_name(addr & ~3ULL), data);
-    }
     switch (addr) {
     case MM_INDEX:
         s->regs.mm_index = data & ~3;
@@ -550,10 +545,10 @@ static void ati_mm_write(void *opaque, hwaddr addr,
                 stn_le_p(s->vga.vram_ptr + idx, size, data);
             }
         } else if (s->regs.mm_index > MM_DATA + 3) {
-            ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
+            ati_reg_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
-                "ati_mm_write: mm_index too small: %u\n", s->regs.mm_index);
+                "ati_reg_write: mm_index too small: %u\n", s->regs.mm_index);
         }
         break;
     case BIOS_0_SCRATCH ... BUS_CNTL - 1:
@@ -942,6 +937,18 @@ static void ati_mm_write(void *opaque, hwaddr addr,
     }
 }
 
+
+static void ati_mm_write(void *opaque, hwaddr addr,
+                         uint64_t data, unsigned int size)
+{
+    ATIVGAState *s = opaque;
+
+    if (addr < CUR_OFFSET || addr > CUR_CLR1 || ATI_DEBUG_HW_CURSOR) {
+        trace_ati_mm_write(size, addr, ati_reg_name(addr & ~3ULL), data);
+    }
+    ati_reg_write(s, addr, data, size);
+}
+
 static const MemoryRegionOps ati_mm_ops = {
     .read = ati_mm_read,
     .write = ati_mm_write,
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index f5a47b82b0..ea1a8bceab 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -105,5 +105,7 @@ struct ATIVGAState {
 const char *ati_reg_name(int num);
 
 void ati_2d_blt(ATIVGAState *s);
+void ati_reg_write(ATIVGAState *s, hwaddr addr,
+                   uint64_t data, unsigned int size);
 
 #endif /* ATI_INT_H */
-- 
2.51.2

On Wed, 31 Dec 2025, Chad Jablonski wrote:
> Move register write logic into its own function. This is in preparation
> for CCE engine support for register writes. MMIO writes will have their
> own distinct policy that doesn't apply to writes made by the CCE engine.
>
> Note: Because of the recursion in the MM_DATA handler the calls to
> ati_mm_write needed to be changed to ati_reg_write. This means that
> tracing output changes slightly for MM_DATA writes. Otherwise, this
> is purely a refactor and does not change behavior.
>
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> ---
> hw/display/ati.c     | 25 ++++++++++++++++---------
> hw/display/ati_int.h |  2 ++
> 2 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index e9c3ad2cd1..33f8e211dc 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -530,14 +530,9 @@ static inline void ati_reg_write_offs(uint32_t *reg, int offs,
>     }
> }
>
> -static void ati_mm_write(void *opaque, hwaddr addr,
> -                           uint64_t data, unsigned int size)
> +void ati_reg_write(ATIVGAState *s, hwaddr addr,
> +                   uint64_t data, unsigned int size)
> {
> -    ATIVGAState *s = opaque;
> -
> -    if (addr < CUR_OFFSET || addr > CUR_CLR1 || ATI_DEBUG_HW_CURSOR) {
> -        trace_ati_mm_write(size, addr, ati_reg_name(addr & ~3ULL), data);
> -    }
>     switch (addr) {
>     case MM_INDEX:
>         s->regs.mm_index = data & ~3;
> @@ -550,10 +545,10 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>                 stn_le_p(s->vga.vram_ptr + idx, size, data);
>             }
>         } else if (s->regs.mm_index > MM_DATA + 3) {
> -            ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
> +            ati_reg_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
>         } else {
>             qemu_log_mask(LOG_GUEST_ERROR,
> -                "ati_mm_write: mm_index too small: %u\n", s->regs.mm_index);
> +                "ati_reg_write: mm_index too small: %u\n", s->regs.mm_index);
>         }
>         break;

Is it possible to access MM_INDEX and MM_DATA through CCE? If not maybe it 
needs to be cut here instead?

>     case BIOS_0_SCRATCH ... BUS_CNTL - 1:
> @@ -942,6 +937,18 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>     }
> }
>
> +
> +static void ati_mm_write(void *opaque, hwaddr addr,
> +                         uint64_t data, unsigned int size)
> +{
> +    ATIVGAState *s = opaque;
> +
> +    if (addr < CUR_OFFSET || addr > CUR_CLR1 || ATI_DEBUG_HW_CURSOR) {
> +        trace_ati_mm_write(size, addr, ati_reg_name(addr & ~3ULL), data);
> +    }
> +    ati_reg_write(s, addr, data, size);
> +}
> +
> static const MemoryRegionOps ati_mm_ops = {
>     .read = ati_mm_read,
>     .write = ati_mm_write,
> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> index f5a47b82b0..ea1a8bceab 100644
> --- a/hw/display/ati_int.h
> +++ b/hw/display/ati_int.h
> @@ -105,5 +105,7 @@ struct ATIVGAState {
> const char *ati_reg_name(int num);
>
> void ati_2d_blt(ATIVGAState *s);
> +void ati_reg_write(ATIVGAState *s, hwaddr addr,
> +                   uint64_t data, unsigned int size);

Move this up next to ati_reg_name just so they are grouped together. 
Otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

>
> #endif /* ATI_INT_H */
>