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

[Jason Ekstrand]

On Mon, Aug 10, 2015 at 12:41 AM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
> On Mon, Aug 10, 2015 at 10:30 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> 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
>
> Prior to _mesa_format_convert, there was a function called
> slow_read_rgba_pixels() which did the actual work. At least that's
> what I saw when I tested piglit sanity on BE. Although I only skimmed
> over that function, I don't see any byte-swapping there, so you didn't
> have the double-conversion you have now.

It was calling pack_ubyte_a8b8g8r8_unorm() under the hood, so there
was byte-swapping.  It just wasn't as obvious.

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