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

[Jose Fonseca]

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

Jose