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

Sat Aug 8 09:34:55 PDT 2015

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

> 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