>
> 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 like
>
> typedef {
> 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;
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)
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.
