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

[Roland Scheidegger]

Am 10.08.2015 um 07:00 schrieb 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.
Some very quick test seems to show you're half-right: llvm (clang) can
optimize it away (3.6), whereas gcc (4.8.3) can't. It is not all that
bad though, because while it can't optimize away the math it does turn
the div into a mul, so the math doesn't look all that expensive...
(I was mostly just wondering because it seemed like it could be a very
common case.)

Roland

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