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

[Jason Ekstrand]

On Aug 7, 2015 21:08, "Rob Clark" <robdclark at gmail.com> wrote:
>
> On Fri, Aug 7, 2015 at 8:11 PM, 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, 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 <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20150807/e690f2f7/attachment-0001.html>