On Aug 7, 2015 21:08, "Rob Clark" <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote: > > On Fri, Aug 7, 2015 at 8:11 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay <<a href="mailto:oded.gabbay@gmail.com">oded.gabbay@gmail.com</a>> 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, if sanity fails, basically all piglit tests would fail.. Sure, if sanity fails, that's bed. However, sanity passing doesn't mean much. > that said, I'm a bit uncertain about how actual-hw gpu's deal w/ b/e.. > ie. do they swap everything around in the driver, or do we really need > a concept of "gpu"[1] endianess vs cpu endianess in mesa? > > [1] where "gpu" endianess would be same as cpu endianess for swrast.. > but potentially different for !swrast.. > > > BR, > -R > > >> 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. > > > >> 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 <<a href="mailto:oded.gabbay@gmail.com">oded.gabbay@gmail.com</a>> > >> CC: "10.5 10.6" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> > >> Signed-off-by: Oded Gabbay <<a href="mailto:oded.gabbay@gmail.com">oded.gabbay@gmail.com</a>> > >> --- > >> 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 > >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>