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

[Jason Ekstrand]

On Mon, Aug 10, 2015 at 12:07 AM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
> 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.

I'm not sure what you mean by "always using little-endian format".
There are multiple different ways of packing 4 8-bit things into a
32-bit space.  This one happens to be by packing them into a 32-bit
integer and then storing it in native byte-order. It happens to be the
same as using an array of 8-bit things on LE so I guess you could call
that "little-endian format".  In any case, format_as_array_format has
to give you a format that's in terms of an array of 8-bit things so it
has to swap on BE.  If it didn't, then the different parts of the
format conversion code would be out-of-sync.

It seems like it might be a good idea to write a quick unit test for
this (it would go in main/tests).  In particular, we should verify
that converting some known value to/from the packed 32-bit format
yields the same values as converting to/from the array format.  I
don't know that we need to go over the top and test every format, but
it should be thorough enough to make sure that endianness checks
aren't getting out of sync.  We should test both RGBA and RGBX style
formats (see also the swizzle fixing patch I just sent).

What really gets me here are two things 1) that classic sw_rast is
broken and 2) that it broke when we started using
_mesa_format_convert.  Prior to the switch over toe
_mesa_format_convert, it used the format_pack/unpack functions to
convert to/from floats.  If this really was a format mismatch issue
between mesa and gallium, you'd think we would have seen it before.

--Jason

> 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