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

[Jose Fonseca]

----- Original Message -----
> Michel Dänzer <michel at daenzer.net> writes:
> > On Fre, 2013-05-24 at 09:11 -0700, Jose Fonseca wrote:
> >> 
> >> I agree that with non-array formats, like B5G6R5 and R5G6B5, replacing
> >> them with endian-variant BGR565 and RGB565 makes a lot of sense (as
> >> the swapped version will probably never be needed).
> >> 
> >> But I'm not sure about RGBA8 variants...
> >> 
> >>  - On one hand, it is often more efficient to read/write them as 32bit
> >> integers than as an array of bytes.
> >>  
> >>  - On the other hand it is easier to think of then as an array of
> >> bytes than an integer quantity.
> >> 
> >> One thing is clear -- a given format can't be both -- either it is
> >> endianess-variant packed color or a endianness-invariant array color.
> >
> > Actually, I think it should be possible to make the RGBA8 variants
> > usable as either an array of bytes or a packed integer, given the right
> > setup of definitions and aliases (which would differ between little and
> > big endian hosts). But I'm not sure offhand what would be the best way
> > to achieve that for the util_format stuff.

This is only possible for little endian hosts. For big endian hosts, no format can be interpreted simultaneously as array or arithmetically packed integer, as the opposite ordering ensues. For example, 8bits red 8 bits green 8 bits blue 8 bits alpha, on big endian is four bytes with R G B A if seen as array, four bytes with A B G R if seen as packed integer. 

> 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.


Honestly, I think that the mere fact we are going in circles in this thread implies we're still biting more than we can chew.

I strongly recommend stepping back, and avoiding invasive (or any) semantic changes. Merely stick to add new things instead of changing the things already in there.  In particular, I suggest on a first stage to do the following:
- introduce _new_ native endianness formats (RGB565, RGBA8888 and friends) as necessary
  - do not alias these with anything
  - do not change any of the of existing formats or their interpretation
- introduce a new util_format_description::native_endianess or "packed_integer" for these new formats.
- implement the new formats where needed (u_format, gallivm, llvmpipe, etc)
  - introduce new paths for them "if (format_description->native_endianess) { ... } else { /* precisely the same as before */ }"
- use the new formats in mesa state tracker on big endian hosts
  - leave little endian hosts alone until big endian work well

If you do this, we all can be sure that we won't break what's there while making progress on this issue: that is, it should improve support on big endian without any regressions on little endian. Therefore we can commit such changes immediately, one by one as soon as you're done with them, without these long discussion on "why some change can cause regression somewhere", or "what's the ideal".


After this is done, and if we are confident on how to do it, we would have a cleanup stage:
- use the new formats also on little endian
  - let dust settle, see if there are any correctness/performance regressions, and either address them immediately, or flip back and continue using the old formats on little endian
- remove the old formats like R5G6B5 in favour of the new RGB565 (as we all agree that we don't need the old native-independent formats)
- _maybe_ remove either RGBA8888/etc or R8G8B8A8/etc -- via aliases or something else -- (what to do, if anything, is still unclear to me)

We'll get the same end result (proper support big endian), but with much less grief from everybody.

I really see no other way to make progress on this.

Jose