[Mesa-dev] [PATCH 03/13] gallium: Introduce 32-bit bytewise format names

[Richard Sandiford]

Jose Fonseca <jfonseca at vmware.com> writes:
>> Yeah, that's what the patch was trying to do.  Even though the formats
>> were defined as "int"s, the int layout was extra information on top of
>> what's already there.  The int information didn't change or replace the
>> array information.
>> 
>> So the idea is that the array nature of the formats doesn't really change.
>> R8G8B8A8 was originally renamed to ABGR8888 on little endian (with R8G8B8A8
>> as a #defined alias)[*], but the util_format description was the same as
>> before.  I.e. quoting the comment change:
>> 
>>     /**
>> -    * Input channel description.
>> +    * Input channel description, in the order XYZW.
>>      *
>>      * Only valid for UTIL_FORMAT_LAYOUT_PLAIN formats.
>> +    *
>> +    * If each channel is accessed as an individual N-byte value, X is always
>> +    * at the lowest address in memory, Y is always next, and so on.  For all
>> +    * currently-defined formats, the N-byte value has native endianness.
>> 
>> ...this gives the "array" layout for all plain formats for which that
>> makes sense, even "int" ones, and the patch doesn't change that.
>
> I'm afraid it does change..  Because this description is paradoxical for
> formats that can be seen both as "array" and for "int", on big endian.
> Before we didn't have to choose how to interpret formats like r8g8b8a8
> (we could access them either as 4 x bytes, or a 32bit packed integer, we
> didn't have to care, and unfortunately we didn't care at all and this
> assumption crept into many places), but now we can't (we have to pick
> one).  And the mere fact we have to pick one, is a change -- a deep
> reaching change that can easily cause performance/correctness regression
> -- want it or not.

That wasn't the intention.  Even for big endian, we _can_ choose between
accessing formats like .8.8.8.8 as either array or int.  That seems like
a useful thing to do.  And the patch series doesn't want to change the
choice of which access is used where.  It also doesn't want to change
the channel order in util_format or the byte layout of formats that can
be seen as "array".

Obviously something has to change, and that change was supposed to be
a relatively simple one: when something chooses to access channels
as an int, it uses the new shift field instead of trying to count bits
from the first channel.  The change is intended to be much less deep
than the one you suggested later.  I don't really see the paradox in
the description above.

As far as not breaking things for little-endian goes: the current
channel information remains unchanged.  Only a new field is added.
The process of fixing things for big-endian is to go through and:

(a) find the places that should be using PIPE_FORMAT_INT_x8y8z8w8
    instead of PIPE_FORMAT_x8y8z8w8 (assuming the revised naming
    convention).  This is trivially a no-op for little endian
    since one is #defined to the other.

(b) find the places that should be using the shift field,
    because they chose to access as an "int".

If we start out with a patch that lays the groundwork but doesn't
do any of (b), then nothing changes for little endian.  If you like,
we could restrict the use (and even the existence) of the shift field
to big-endian targets at first.  I.e. wrap every change under (b)
under an endianness test and continue to use the current shift
calculation for little endian.  But the shift is really a simple
calculation.

FWIW, Adam did test this patch on little-endian and I believe it's being
used unconditionally for all targets in Fedora 19.

As far as future patches for the non-array formats go: if I ever
end up being the one who does it, I'll try to follow your recipe
and restrict the new PIPE_FORMAT_INT_.5.6.5 formats (say) to
big-endian at first.

Thanks,
Richard