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

[Jason Ekstrand]

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

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