On Sat, Aug 8, 2015 at 3:11 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay <oded.gab...@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...
>> 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!
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.
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.gab...@gmail.com>
>> CC: "10.5 10.6" <mesa-sta...@lists.freedesktop.org>
>> Signed-off-by: Oded Gabbay <oded.gab...@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
