----- Original Message ----- > ----- Original Message ----- > > Jose Fonseca <jfons...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
