[Mesa-dev] [PATCH] st/mesa: Map packed gallium formats to packed mesa formats.

[Oded Gabbay]

On Mon, Aug 10, 2015 at 7:58 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Sun, Aug 9, 2015 at 3:11 AM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>> On Sun, Aug 9, 2015 at 2:43 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>> On Sat, Aug 8, 2015 at 3:14 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>>> On Sun, Aug 9, 2015 at 12:26 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>
>>>> If I understand your patch, instead of using the endian-dependent
>>>> defines, you use the LE defines all the time when converting
>>>> pipe<-->mesa formats
>>>>
>>>> I'm not saying its wrong, but these endian-dependent defines, e.g.
>>>> PIPE_FORMAT_ARGB8888_UNORM, are used in more places in the gallium
>>>> code than in the above two functions. Why change only here and not in
>>>> all places, i.e removing them ?
>>>>
>>>> Also, this fixes only llvmpipe/softpipe. It doesn't fix sw_rast. That
>>>> is still broken
>>>
>>> The fact that sw_rast is broken bothers me.  The sw_rast code doesn't
>>> leave core mesa so it shouldn't be affected by any gallium changes.
>>> From that perspective, this makes sense.  However, sw_rast should be
>>> using the auto-generated packing/unpacking functions in core mesa.
>>> Did sw_rast work prior to the readpixels changes?  It's possible it
>>> was already broken.
>>>
>> I didn't check sw_rast status before readpixels changes.
>> However, from looking at the automatic pack/unpack functions, it seems
>> strange to me they don't take into account the endianness.
>> e.g on ppc64 machine, in file format_pack.c, there is the following function:
>>
>> static inline void
>> pack_ubyte_a8b8g8r8_unorm(const GLubyte src[4], void *dst)
>> {
>>     uint8_t a = _mesa_unorm_to_unorm(src[3], 8, 8);
>>     uint8_t b = _mesa_unorm_to_unorm(src[2], 8, 8);
>>     uint8_t g = _mesa_unorm_to_unorm(src[1], 8, 8);
>>     uint8_t r = _mesa_unorm_to_unorm(src[0], 8, 8);
>>
>>     uint32_t d = 0;
>>     d |= PACK(a, 0, 8);
>>     d |= PACK(b, 8, 8);
>>     d |= PACK(g, 16, 8);
>>     d |= PACK(r, 24, 8);
>>     (*(uint32_t *) dst) = d;
>> }
>>
>> PACK is defined as:
>> #define PACK(SRC, OFFSET, BITS) (((SRC) & MAX_UINT(BITS)) << (OFFSET))
>>
>> So, we see that 'R' is written to the most-significant byte of the
>> dword and 'A' is written to the least-significant byte of the dword,
>> although in BE, that should be reversed, since this is abgr format,
>> not rgba.
>
> No, what I've been trying to tell you this whole time is that this is
> how the a8r8g8b8 format is *defined*.  It's defined in terms of
> carving up a single 32-bit word in native endianness.  This is why we
> have to flip things based on endianness if we want to get it in terms
> of an array of 8-bit values.  You're free to argue that that's a
> clumsy definition for a color format, but that's how it's defined none
> the less.
>
>> It's enough to see that the implementation of this function is the
>> same for LE and BE to suspect the code, unless I'm missing something.
>>
>>
>>>> I tested this patch on llvmpipe, combined with the previous patch you
>>>> sent on POWER8 machine running ppc64 (BE)
>>>> First of all, as I said above, it fixes the piglit sanity test.
>>>> Second, it seems that it improves piglit gpu.py results much more than
>>>> my patch. "Only" 1368 tests failed, vs. a bit more than 2000 with my
>>>> patch. Still a lot higher than the ~600 tests that fail on ppc64le
>>>
>>> I'm not surprised that it didn't get to zero regressions vs. ppc64le.
>>> This patch was just kind of thrown together and I didn't check all of
>>> the formats.  Also, as per my conversation with rolland, I may be
>>> misunderstanding the PIPE_FORMAT enums.
>>>
>>> Really, this is a problem that is going to have to be chased by
>>> someone other than me.  I have no BE hardware, so I can't really chase
>>> it down.  I I also have very limited gallium knowledge so I'm probably
>>> not the best person to start chasing it through gallium.  All I've
>>> done so far is throw darts at the wall.  I'm sure this patch is
>>> incomplete and, to be honest, I didn't even compile-test it as I don't
>>> usually build gallium.  What i do know is that, for the most part, the
>>> core mesa code is consistent with itself as far as endianness goes.
>>
>> So I'm not married to my patch, and your 2 patches seem to do a better
>> job (although sw-rast is still broken, but that's another issue and
>> frankly, it is of less importance for me atm), and I accept your
>> arguments for your solution. I would suggest that you upstream your
>> patches as a start and I will continue from there, based on my
>> schedule.
>
> Unfortunately, from what Marek and Rolland have said, this patch (the
> one to change the mesa format -> gallium format conversion) is also
> wrong.  It seems like the code was doing it correct all along.  This
> means that something else is wrong somewhere.  Possibly in llvmpipe,
> possibly somewhere else.  It's going to take some tracking.
>
> --Jason

Jason,
>From my debugging it seems as though the bug originates from the
mismatch between how llvmpipe handles BE and how mesa *expects* to
receive the information, i.e in BE machines, when llvmpipe converts
the pipe format to mesa format, it *changes* the format type, but mesa
does the same conversion, so we have a double conversion.

e.g. when running piglit sanity, st_pipe_format_to_mesa_format
receives PIPE_FORMAT_R8G8B8A8_UNORM (because that what was chosen by
mesa for the texture image) and returns MESA_FORMAT_A8B8G8R8_UNORM (in
BE machine). Thus, the formats are *reversed* in order to represent
the *actual* order of bytes in the memory.

Now, when arriving to _mesa_format_convert, src_format is
MESA_FORMAT_A8B8G8R8_UNORM (as passed by llvmpipe).
_mesa_format_convert again does the byte swapping, and therefore,
returns to the original format of RGBA. However, because the bytes are
written in memory in the format of ABGR, the result of
read_rgba_pixels (which calls _mesa_format_convert) are swapped and
piglit sanity fails.

Because the auto-generated code in mesa format_pack/unpack is *always*
using little-endian format, you *must* have the formats reversed in
order for that code to work in BE machines. No other way around it. I
thought of generating a different set of format_pack/unpack for BE
machines, but at the end decided it was simpler and cleaner to remove
the extra swap in _mesa_format_convert.

Could you please explain again why you think it is the wrong move ? As
I'm new to mesa, I'm still in the learning phase of the code.

    Oded


>
>>>
>>> If you want to see what a given format means in a more explicit
>>> fashion, just look at the auto-generated format_pack.c and
>>> format_unpack.c files.  gallium has similar auto-generated format
>>> packing functions.  If you can digest what each of them means on BE,
>>> then it should be pretty obvious how to match things up.
>>> --Jason
>> As I pointed above, I think the auto-generated code is not
>> endian-aware, which is a problem, IMO.
>>
>>>
>>> P.S. Thanks for bringing this up and taking the time to look into it.
>>> Mesa claims to work on BE but, as you've clearly demonstrated, it's
>>> completely broken.  I'm sure we'd all like to see it working again.
>>
>> Although I'm focused more on ppc64le, I will probably be able to
>> squeeze some fixes in for ppc64, and fortunately I have the H/W to
>> check it. I appreciate your help and will be glad if you could take a
>> look at future patches I will send on the subject.
>>
>>         Oded