On Mon, Aug 10, 2015 at 10:30 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Mon, Aug 10, 2015 at 12:07 AM, Oded Gabbay <oded.gab...@gmail.com> wrote:
>> On Mon, Aug 10, 2015 at 7:58 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>>> On Sun, Aug 9, 2015 at 3:11 AM, Oded Gabbay <oded.gab...@gmail.com> wrote:
>>>> On Sun, Aug 9, 2015 at 2:43 AM, Jason Ekstrand <ja...@jlekstrand.net>
>>>> wrote:
>>>>> On Sat, Aug 8, 2015 at 3:14 PM, Oded Gabbay <oded.gab...@gmail.com> wrote:
>>>>>> On Sun, Aug 9, 2015 at 12:26 AM, Jason Ekstrand <ja...@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.
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
