On Mon, 26 Jan 2026, Chad Jablonski wrote:
I don't feel strongly about it but I though it might be simpler to reuse the same type instead of adding a new struct for everything. I wonder if something liketypedef { QemuRect rect; int bpp; int stride; uint8_t *bits; } ATIBltRect; ati_2d_do_blt(ATIBltRect src, ATIBltRect dst, bool top_to_bottom, bool left_to_right) or similar could work (even if you don't use src.rect.width/height) and be simpler or if you still have ATIVGAState as parameter to ati_2d_do_blt then you don't even need to pass the bools as those could be get from the device state which should not change during host data blits (or if they do real hardware may also change what it does). Or maybe ati_2d_do_blt really only needs a copy of the relevant registers (e.g. from dst_offset to dp_write_mask copied to an ATI2DCtx or similar struct) and work from that instead of ATIVGAState directly so the host data route can pass it's own copy with modified register contents and can update these without affecting the registers. But if this does not make sense to you and you think having separate structs are clearer I'm OK with that too, these could be modified later as well. I think I'll wait for the next version before going into the details of the end of the series as I don't expect to need any more major rewrite so it will probably already be close to be good enough and only need maybe one more revision but this next version may change some these patches so the next version may look a bit different from the current version.I see. That makes a lot of sense. And now that you mention the potential ATI2DCtx solution I do like that it removes direct dependency on the registers inside of the blit function. I wonder if we could go a step even further and store the actual derived values in the struct instead of a copy of raw register values. Something like: typedef struct { /* src */ uint8_t *src_bits; int src_x, src_y; int src_stride; /* dst */ uint8_t *dst_bits; QemuRect dst_rect; int dst_stride; int bpp; QemuRect scissor; uint32_t rop3; bool left_to_right; bool top_to_bottom; uint32_t frgd_clr; uint32_t bkgd_clr; } ATI2DCtx;
That would work. For consistency I'd rather either go with dst_x dst_y dst_w, etc. and likewise for scissor (which could be abbreviated sc) or also use QemuRect src even if w and h are never used just so all usage will be uniform and you don't have to remember why src is special.
We would still need to pass VGACommonState in to access a few things (vram and the palette) and use_pixman but we wouldn't need the full device at that point. So the function signature would look like: static void ati_2d_do_blt(VGACommonState *vga, ATI2DCtx *ctx, bool use_pixman)
The use_pixman may need to be an int and mirror the x-pixman property and used like the property now so we can control different operations separately instead of just on or off for all.
A blt ctx setup function could then fill this struct in the most common case (memory) and the host_data flush could easily modify it without needing any of the register access details. One downside of course is that if we discover we need to mutate registers during the blit we will need to rethink this a bit. But so far it isn't looking like that is necessary.
Maybe if any changes to regs are needed those can be done in whatever calls ati_2d_do_blt so hopefully this won't be a problem. But we may need to add more values to ctx in the future but that should be OK.
Regards, BALATON Zoltan
