>>
>> -static int ati_bpp_from_datatype(ATIVGAState *s)
>> +typedef struct {
>> + QemuRect rect;
>> + int bpp;
>> + int stride;
>> + bool top_to_bottom;
>> + bool left_to_right;
>> + uint8_t *bits;
>> +} ATIBltDst;
>
> I'll need to look at this and further patches in more detail but I wonder
> if we need separate types for BltDst and BltSrc or could we use a single
> type for both maybe not using some fields for Src. But if that would not
> simplify things then these can be separate too.
>
I do think that we would end up with unused fields in some cases. To me
this mapped well conceptually to the ROP3 operations. For example the
fill operations won't use src at all. It's possible we would even end up
with ATIBltBrush or similar in the future. But definitely open to
changing it if you feel strongly after looking at it in more detail!
>> + ATIBltDst _dst; /* TEMP: avoid churn in future patches */
>> + ATIBltDst *dst = &_dst;
>
> These will be removed but names beginning with _ are reserved and should
> not be used so maybe this (and _src in next patch) should be dst_ or
> something else? Can the churn be avoided by reorganising changes or moving
> converting to pointer in a separate patch? This patch is quite long and
> mixes pointer conversion with other refactoring so separating those may
> help in review.
>
Ah, yep, renaming the _dst and _src makes sense! As far as the churn, as I
look at this again I think it's primarily because there's an inconsistency
in how the register values are accessed and this is attempting to unify that.
The local variables become struct members (e.g. bpp becomes dst->bpp) and so
do the direct register references (e.g. s->regs.dst_pitch becomes dst->stride).
Whether dst is a pointer or a local struct doesn't change that.
I was thinking we could initialize the dst struct and then assign the locals to
those values to avoid churn (e.g. unsigned dst_x = dst->rect.x) and anything
using those locals wouldn't need to be updated. But then the problem still
remains with the direct register references.
Another thought is that we could first create local variables for each of these,
even those that are currently direct register references, and do the replacement
in the function. Then in a separate patch assign the locals to the struct
members. The locals could then either stick around indefinitely or be replaced
themselves with direct references to the struct in another patch.
Do you have any preference, or maybe even other ideas about which might be
clearer?
