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

[Jason Ekstrand]

On Sun, Aug 9, 2015 at 6:46 AM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 09.08.2015 um 12:11 schrieb Oded Gabbay:
>> 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;
>> }
>
> Aside from the endianness issues, it actually looks like this does some
> totally unnecessary math for 8bit unorm to 8bit unorm "conversion". Odd...

I *think* that gets optimized away.  It probably wouldn't hurt to put
a src_bits == dst_bits case in there though.

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