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

[Jose Fonseca]

----- Original Message -----
> ----- Original Message -----
> > 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.
> 
> I confess that I missed this hunk among the whole series:
> 
>   diff --git a/src/gallium/auxiliary/util/u_format.h
>   b/src/gallium/auxiliary/util/u_format.h
>   index e4b9c36..3a04d89 100644
>   --- a/src/gallium/auxiliary/util/u_format.h
>   +++ b/src/gallium/auxiliary/util/u_format.h
>   @@ -132,6 +132,7 @@ struct util_format_channel_description
>       unsigned normalized:1;
>       unsigned pure_integer:1;
>       unsigned size:9;        /**< bits per channel */
>   +   unsigned shift:16;      /** number of bits from lsb */
>    };
>  
> Indeed that solves the paradox I was worried about, and in a neat way.
> 
> Let me look again into this again...

Ok. I think this patch series is sound from an implementation POV. I see no point in delaying further. We can tweak things afterwards if deemed necessary.

Lets squash the commits, rename the XYZW8888 formats to go from low->high bit, and commit this into master.

Jose