[Mesa-stable] [Mesa-dev] [PATCH v2 2/2] mesa: _mesa_format_convert should be endian agnostic

Sat Aug 8 14:12:08 PDT 2015

On Sat, Aug 8, 2015 at 2:10 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Aug 8, 2015 at 4:26 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Sat, Aug 8, 2015 at 1:01 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>> On Sat, Aug 8, 2015 at 7:34 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>> On Fri, Aug 7, 2015 at 10:38 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>>>> On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>>> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay <oded.gabbay at gmail.com> wrote:
>>>>>>> This patch fixes a bug that is manifested in the read path of mesa when
>>>>>>> running on big-endian machines. The effects can be seen when running
>>>>>>> piglit sanity test and/or taking a screen capture.
>>>>>>
>>>>>> piglit sanity isn't all that convincing.  It's quite possible that
>>>>>> there are two byte-swapping bugs that just happen to cancel out.  If
>>>>>> it fixes huge numbers of piglit tests, that's more convincing.
>>>>>>
>>>>>
>>>>> Well, without it about 65% of piglit tests fails. With this fix, about
>>>>> 30% fails. I would say that accounts for something...
>>>>
>>>> Yes, that counts for far more than just sanity.  It would be
>>>> instructive to see the piglit diff between 2ec8718d, 8ec6534b2, and
>>>> 5038d839b.  Those three patches are sequential and the one in the
>>>> middle, uses _mesa_format_convert for texture upload.
>>>>
>>>>>>> The bug is caused when _mesa_format_convert receives src_format as
>>>>>>> mesa_format, which it thens changes to mesa_array_format. During this
>>>>>>> change, it checks for endianness and swaps the bytes accordingly.
>>>>>>> However, because the bytes are _already_ swapped in the memory itself
>>>>>>> (being written there by llvmpipe/softpipe/sw rast), and src_format
>>>>>>> value matches the _actual_ contents of the memory, the result of the
>>>>>>> read is wrong.
>>>>>>>
>>>>>>> Therefore, because other layers/functions, such as llvm or
>>>>>>> read_rgba_pixels() takes care whether we are dealing with big-endian or
>>>>>>> little-endian, _mesa_format_convert should be endian agnostic to avoid
>>>>>>> duplicate swapping of bytes.
>>>>>>
>>>>>> As far as I know, the core mesa format conversion code has handled
>>>>>> byte-swapping for a very long time.  it is *not* simply punted to the
>>>>>> driver.  I'm 100% ready to believe that there is a problem but it's
>>>>>> probably far more subtle than just removing support for byte-swapping
>>>>>> in _mesa_format_to_array_format.
>>>>>
>>>>> First of all, this bug has been in a very long time as well (I tracked
>>>>> it to between 10.4 to 10.5). It has shown up since mesa started to use
>>>>> _mesa_format_convert
>>>>> From my debugging, I see that llvmpipe (and other) write the data to
>>>>> memory in BE format. e.g if the format is RGBA, then the data is
>>>>> actually written in AGBR format.
>>>>> Now, in _mesa_format_convert you get the src_format as AGBR (already
>>>>> swapped). However, because it arrives as format and not array_format,
>>>>> _mesa_format_convert needs to build the array_format in
>>>>> _mesa_format_to_array_format()
>>>>>
>>>>> In _mesa_format_to_array_format, after the array_format is created,
>>>>> you swap it if it is BE, so you now have RGBA in array_format
>>>>> representing the source. And that is the bug!
>>>>
>>>> Not quite... MESA_FORMAT formats are specified one of two ways: array
>>>> and packed.  Packed formats are specified in terms of low bits and
>>>> high bits in the word while array formats are specified in terms of an
>>>> array of elements of a certain type.  For the most part, packed is
>>>> used for things like 565 which obviously aren't array formats.
>>>> However, packed is also used for the normal 8-bit RGBA formats.  Since
>>>> they are specified in terms of low vs. high bits in a byte, they *do*
>>>> need to be swapped when going to an array format.  There was, indeed a
>>>> bug in there: we were swapping even if the MESA_FORMAT was already an
>>>> array format.  I just sent out a patch to fix that bug.
>>>>
>>>> It would also be instructive to know what the actual enums looked like
>>>> and what the data that you're refering to looked like.  The enum
>>>> values are described in mesa/main/formats.h.
>>>>
>>>> It's quite possible that, in all the BE cases, we aren't actually
>>>> treating the MESA_FORMATs as described above.  If that is the case,
>>>> then we should figure out what they do with the formats and either
>>>> change the BE cases or redefine the MESA_FORMATs in such a way that it
>>>> makes more sense on BE hardware.  However, as it currently stands
>>>> _mesa_format_to_array_format (as patched by the one I just sent)
>>>> matches the definitions.
>>>>
>>>>> of course I checked that _mesa_format_to_array_format() is only called
>>>>> from _mesa_format_convert so I can safely modify it according to
>>>>> _mesa_format_convert usage.
>>>>>
>>>>> So I don't think it is "far more subtle" than removing this
>>>>> byte-swapping. If you still think so, please elaborate.
>>>>
>>>> There are many places in the driver where we look at endianness and do
>>>> something with formats.  Obviously, something changed with 5038d839b;
>>>> we were taking something into account and aren't now or we weren't
>>>> then and are now.  What that something was, I'm not sure.
>>>>
>>>> Where all could the bug be?  Lots of places.  It could be in generated
>>>> pack/unpack code but I find that unlikely.  It could be where your
>>>> patch indicates but that seems to violate the definition of the
>>>> MESA_FORMATs.  It's possible that piglit is setting LSB_FIRST and
>>>> that's not getting respected.  If you're looking at gallium swrast or
>>>> llvmpipe, it could be in st/mesa where we convert from PIPE_FORMAT_*
>>>> to MESA_FORMAT_*.
>>>>
>>>> If you want to try and fix mesa for BE hardware, please do!  As ilia
>>>> said, most of the core mesa devs don't have BE hardware available.
>>>> However, that's probably going to be a long road (but possibly quite
>>>> rewarding).  I'm sure there are a *lot* of places where we get it
>>>> wrong and readpixels is just the start.
>>>>
>>>> --Jason
>>>>
>>> From my comparison of software rendering paths between ppc64, ppc64le
>>> and x86_64 using piglit gpu.py (software rendering only) as reference,
>>> ppc64 (BE) is definitely quite broken. Around 2000 tests are failing,
>>> vs. around 500 on ppc64le and 100 on x86_64
>>>
>>> I think my patch is a small step in a long road, but a step in the
>>> right direction nonetheless. I'm more focused on ppc64le but if I will
>>> be able to fix things as well for ppc64, I will certainly do it.
>>>
>>> Regarding the technical side, I looked at this problem from different
>>> angles. There maybe some mismatch here between mesa core and
>>> llvmpipe/softpipe that cause this problem (and others possibly), but
>>> in my opinion, the fix should be in mesa core for this specific
>>> problem, because:
>>> 1. llvmpipe/softpipe write the bytes *correctly* to the memory (so
>>> fixing there might break that)
>>
>> If it's claiming the format is BGRA and writting as ARGB then that's
>> not correct.
>
> are mesa formats defined as little endian or host endian?  I suspect
> that is where the confusion is..

The "packed" formats such as A8R8G8B8 are defined as host endian.
"array" formats such as RGBA_UINT8 are defined as "array of
uint8_t's".

> BR,
> -R
>
>>> 2. _mesa_format_convert, according to its own documentation, should
>>> not handle byte swapping
>>
>> That's referring to the GL_SWAP_BYTES parameter in glPixelStorei.  It
>> *does*, however, properly handle going from one mesa format or array
>> format (as defined in formats.h) to another regardless of endianness.
>> Modulo the patch I sent this morning, it does exactly what it claims
>> to do.
>>
>>> Regarding the enums, _mesa_format_convert gets
>>> MESA_FORMAT_A8B8G8R8_UNORM, but the "real" format is actually RGBA.
>>> However, because its BE, the data is written in memory in format of
>>> ABGR. The entity that changes RGBA to ABGR is llvmpipe in st_format.c,
>>> when converting pipe to mesa format. notice the distinction between BE
>>> and LE in p_format.h
>>
>> The way that gallium defines its formats is a bit strange.  Basically
>> it comes down to "follow C structure packing rules".  Doing a direct
>> PIPE_FORMAT to MESA_FORMAT conversion as is done in st_formats.c is
>> wrong and probably always has been wrong.  (The way mesa defines
>> formats didn't change in the format conversion series.)  The only
>> reason why no one has noticed up until this point is that gallium
>> hooks and does it's own thing for a lot of the texture upload stuff
>> and so those may not touch core mesa.  Apparently, gallium drivers
>> still use core mesa for readpixels.  It sounds to me like readpixels
>> was actually fixed by the patch in question but, because gallium was
>> passing mesa the wrong formats, it works before and doesn't now.
>>
>> My recommendation would actually be to change st_formats.c to use the
>> A8R8G8B8 forms instead of the ARGB8888 forms for formats that mesa
>> considers "packed".  The former actually matches the definition in
>> mesa because it's specified in terms of low bits to high bits like
>> mesa does.
>>
>> --Jason
>>
>>>     Oded
>>>
>>>
>>>
>>>>> Thanks,
>>>>>          Oded
>>>>>
>>>>>>
>>>>>>> btw, it is also mentioned in the comment of the function that it doesn't
>>>>>>> handle byte-swapping, so the original code contradicts the documentation.
>>>>>>>
>>>>>>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>>>>>>> CC: "10.5 10.6" <mesa-stable at lists.freedesktop.org>
>>>>>>> Signed-off-by: Oded Gabbay <oded.gabbay at gmail.com>
>>>>>>> ---
>>>>>>>  src/mesa/main/formats.c | 6 ++----
>>>>>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
>>>>>>> index baeb1bf..7a41cb0 100644
>>>>>>> --- a/src/mesa/main/formats.c
>>>>>>> +++ b/src/mesa/main/formats.c
>>>>>>> @@ -372,10 +372,8 @@ uint32_t
>>>>>>>  _mesa_format_to_array_format(mesa_format format)
>>>>>>>  {
>>>>>>>     const struct gl_format_info *info = _mesa_get_format_info(format);
>>>>>>> -   if (_mesa_little_endian())
>>>>>>> -      return info->ArrayFormat;
>>>>>>> -   else
>>>>>>> -      return _mesa_array_format_flip_channels(info->ArrayFormat);
>>>>>>> +
>>>>>>> +   return info->ArrayFormat;
>>>>>>>  }
>>>>>>>
>>>>>>>  static struct hash_table *format_array_format_table;
>>>>>>> --
>>>>>>> 2.4.3
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> mesa-dev mailing list
>>>>>>> mesa-dev at lists.freedesktop.org
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev