<p dir="ltr"><br>
On Aug 7, 2015 21:08, "Rob Clark" <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote:<br>
><br>
> On Fri, Aug 7, 2015 at 8:11 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> > On Fri, Aug 7, 2015 at 12:24 PM, Oded Gabbay <<a href="mailto:oded.gabbay@gmail.com">oded.gabbay@gmail.com</a>> wrote:<br>
> >> This patch fixes a bug that is manifested in the read path of mesa when<br>
> >> running on big-endian machines. The effects can be seen when running<br>
> >> piglit sanity test and/or taking a screen capture.<br>
> ><br>
> > piglit sanity isn't all that convincing.  It's quite possible that<br>
> > there are two byte-swapping bugs that just happen to cancel out.  If<br>
> > it fixes huge numbers of piglit tests, that's more convincing.<br>
><br>
> well, if sanity fails, basically all piglit tests would fail..</p>
<p dir="ltr">Sure, if sanity fails, that's bed. However, sanity passing doesn't mean much.</p>
<p dir="ltr">> that said, I'm a bit uncertain about how actual-hw gpu's deal w/ b/e..<br>
> ie. do they swap everything around in the driver, or do we really need<br>
> a concept of "gpu"[1] endianess vs cpu endianess in mesa?<br>
><br>
> [1] where "gpu" endianess would be same as cpu endianess for swrast..<br>
> but potentially different for !swrast..<br>
><br>
><br>
> BR,<br>
> -R<br>
><br>
> >> The bug is caused when _mesa_format_convert receives src_format as<br>
> >> mesa_format, which it thens changes to mesa_array_format. During this<br>
> >> change, it checks for endianness and swaps the bytes accordingly.<br>
> >> However, because the bytes are _already_ swapped in the memory itself<br>
> >> (being written there by llvmpipe/softpipe/sw rast), and src_format<br>
> >> value matches the _actual_ contents of the memory, the result of the<br>
> >> read is wrong.<br>
> >><br>
> >> Therefore, because other layers/functions, such as llvm or<br>
> >> read_rgba_pixels() takes care whether we are dealing with big-endian or<br>
> >> little-endian, _mesa_format_convert should be endian agnostic to avoid<br>
> >> duplicate swapping of bytes.<br>
> ><br>
> > As far as I know, the core mesa format conversion code has handled<br>
> > byte-swapping for a very long time.  it is *not* simply punted to the<br>
> > driver.  I'm 100% ready to believe that there is a problem but it's<br>
> > probably far more subtle than just removing support for byte-swapping<br>
> > in _mesa_format_to_array_format.<br>
> ><br>
> >> btw, it is also mentioned in the comment of the function that it doesn't<br>
> >> handle byte-swapping, so the original code contradicts the documentation.<br>
> >><br>
> >> Signed-off-by: Oded Gabbay <<a href="mailto:oded.gabbay@gmail.com">oded.gabbay@gmail.com</a>><br>
> >> CC: "10.5 10.6" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>><br>
> >> Signed-off-by: Oded Gabbay <<a href="mailto:oded.gabbay@gmail.com">oded.gabbay@gmail.com</a>><br>
> >> ---<br>
> >>  src/mesa/main/formats.c | 6 ++----<br>
> >>  1 file changed, 2 insertions(+), 4 deletions(-)<br>
> >><br>
> >> diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c<br>
> >> index baeb1bf..7a41cb0 100644<br>
> >> --- a/src/mesa/main/formats.c<br>
> >> +++ b/src/mesa/main/formats.c<br>
> >> @@ -372,10 +372,8 @@ uint32_t<br>
> >>  _mesa_format_to_array_format(mesa_format format)<br>
> >>  {<br>
> >>     const struct gl_format_info *info = _mesa_get_format_info(format);<br>
> >> -   if (_mesa_little_endian())<br>
> >> -      return info->ArrayFormat;<br>
> >> -   else<br>
> >> -      return _mesa_array_format_flip_channels(info->ArrayFormat);<br>
> >> +<br>
> >> +   return info->ArrayFormat;<br>
> >>  }<br>
> >><br>
> >>  static struct hash_table *format_array_format_table;<br>
> >> --<br>
> >> 2.4.3<br>
> >><br>
> >> _______________________________________________<br>
> >> mesa-dev mailing list<br>
> >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
> > _______________________________________________<br>
> > mesa-dev mailing list<br>
> > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</p>