[Mesa-dev] [PATCH 03/29] mesa: Do not assert on integer<->non-integer direct pack/unpack fast paths

Iago Toral itoral at igalia.com
Fri Nov 21 05:53:28 PST 2014


On Thu, 2014-11-20 at 21:35 -0800, Jason Ekstrand wrote:
> 
> 
> On Thu, Nov 20, 2014 at 9:33 PM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
>         
>         
>         On Thu, Nov 20, 2014 at 12:29 AM, Iago Toral
>         <itoral at igalia.com> wrote:
>                 It is explained here:
>                 https://bugs.freedesktop.org/show_bug.cgi?id=84566#c35
>                 
>                 So one example of this was a glReadPixels where we
>                 want to store the
>                 pixel data as RGBA UINT, but the render buffer format
>                 we  read from is
>                 MESA_FORMAT_B8G8R8A8_UNORM. There are piglit tests
>                 that hit this case.
>         
>         
>         I'm still not seeing how this is allowed.  From the 4.2 core
>         spec:
>         
>         "If format is one of RED , GREEN , BLUE , RG , RGB , RGBA ,
>         BGR , or BGRA , then
>         red, green, blue, and alpha values are obtained from the
>         selected buffer at each
>         pixel location.
>         If format is an integer format and the color buffer is not an
>         integer format, or
>         if the color buffer is an integer format and format is not an
>         integer format, an
>         INVALID_OPERATION error is generated."
>         
>         
>         I also checked the 3.3 compatibility spec and it says the same
>         thing.  This seems to indicate that that combination should
>         result in GL_INVALID_OPERATION.
>         
> 
> 
> I also just CC'd Ian, our local spec expert.  Maybe he can shed a
> little light on this.

No need. I have reverted the commit and run piglit again on i965 and
swrast and I don't hit the assert any more, so I guess that when I was
hitting that it was because of a bug somewhere in the GL->Mesa type
mapping that I must have fixed after added this patch.

I'll remove the patch in the second version of the series.
>  
>                 
>                 Iago
>                 
>                 On Wed, 2014-11-19 at 12:04 -0800, Jason Ekstrand
>                 wrote:
>                 > Can you remind me again as to what formats hit these
>                 paths?  I
>                 > remember you hitting them, but I'm still not really
>                 seeing how it
>                 > happens.
>                 >
>                 > --Jason
>                 >
>                 >
>                 > On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
>                 > <itoral at igalia.com> wrote:
>                 >         We can have conversions from non-integer
>                 types to integer
>                 >         types, so remove
>                 >         the assertions for these in the pack/unpack
>                 fast paths. It
>                 >         could be that
>                 >         we do not have all the necessary pack/unpack
>                 functions in
>                 >         these cases though,
>                 >         so protect these paths with conditionals and
>                 let
>                 >         _mesa_format_convert use
>                 >         other paths to resolve these kind of
>                 conversions if necessary.
>                 >         ---
>                 >          src/mesa/main/format_utils.c | 16
>                 ++++++++--------
>                 >          1 file changed, 8 insertions(+), 8
>                 deletions(-)
>                 >
>                 >         diff --git a/src/mesa/main/format_utils.c
>                 >         b/src/mesa/main/format_utils.c
>                 >         index 1d65f2b..56a3b8d 100644
>                 >         --- a/src/mesa/main/format_utils.c
>                 >         +++ b/src/mesa/main/format_utils.c
>                 >         @@ -143,8 +143,8 @@
>                 _mesa_format_convert(void *void_dst,
>                 >         uint32_t dst_format, size_t dst_stride,
>                 >                      dst += dst_stride;
>                 >                   }
>                 >                   return;
>                 >         -      } else if (dst_array_format.as_uint
>                 ==
>                 >         RGBA8888_UBYTE.as_uint) {
>                 >         -         assert(!
>                 _mesa_is_format_integer_color(src_format));
>                 >         +      } else if (dst_array_format.as_uint
>                 ==
>                 >         RGBA8888_UBYTE.as_uint &&
>                 >         +                 !
>                 _mesa_is_format_integer_color(src_format))
>                 >         {
>                 >                   for (row = 0; row < height; ++row)
>                 {
>                 >
>                 _mesa_unpack_ubyte_rgba_row(src_format, width,
>                 >
>                 src, (uint8_t
>                 >         (*)[4])dst);
>                 >         @@ -152,8 +152,8 @@
>                 _mesa_format_convert(void *void_dst,
>                 >         uint32_t dst_format, size_t dst_stride,
>                 >                      dst += dst_stride;
>                 >                   }
>                 >                   return;
>                 >         -      } else if (dst_array_format.as_uint
>                 ==
>                 >         RGBA8888_UINT.as_uint) {
>                 >         -
>                  assert(_mesa_is_format_integer_color(src_format));
>                 >         +      } else if (dst_array_format.as_uint
>                 ==
>                 >         RGBA8888_UINT.as_uint &&
>                 >         +
>                  _mesa_is_format_integer_color(src_format)) {
>                 >                   for (row = 0; row < height; ++row)
>                 {
>                 >
>                 _mesa_unpack_uint_rgba_row(src_format, width,
>                 >                                                 src,
>                 (uint32_t
>                 >         (*)[4])dst);
>                 >         @@ -174,8 +174,8 @@
>                 _mesa_format_convert(void *void_dst,
>                 >         uint32_t dst_format, size_t dst_stride,
>                 >                      dst += dst_stride;
>                 >                   }
>                 >                   return;
>                 >         -      } else if (src_array_format.as_uint
>                 ==
>                 >         RGBA8888_UBYTE.as_uint) {
>                 >         -         assert(!
>                 _mesa_is_format_integer_color(dst_format));
>                 >         +      } else if (src_array_format.as_uint
>                 ==
>                 >         RGBA8888_UBYTE.as_uint &&
>                 >         +                 !
>                 _mesa_is_format_integer_color(dst_format))
>                 >         {
>                 >                   for (row = 0; row < height; ++row)
>                 {
>                 >
>                 _mesa_pack_ubyte_rgba_row(dst_format, width,
>                 >
>                 (const uint8_t
>                 >         (*)[4])src, dst);
>                 >         @@ -183,8 +183,8 @@
>                 _mesa_format_convert(void *void_dst,
>                 >         uint32_t dst_format, size_t dst_stride,
>                 >                      dst += dst_stride;
>                 >                   }
>                 >                   return;
>                 >         -      } else if (src_array_format.as_uint
>                 ==
>                 >         RGBA8888_UINT.as_uint) {
>                 >         -
>                  assert(_mesa_is_format_integer_color(dst_format));
>                 >         +      } else if (src_array_format.as_uint
>                 ==
>                 >         RGBA8888_UINT.as_uint &&
>                 >         +
>                  _mesa_is_format_integer_color(dst_format)) {
>                 >                   for (row = 0; row < height; ++row)
>                 {
>                 >
>                 _mesa_pack_uint_rgba_row(dst_format, width,
>                 >                                               (const
>                 uint32_t
>                 >         (*)[4])src, dst);
>                 >         --
>                 >         1.9.1
>                 >
>                 >
>                  _______________________________________________
>                 >         mesa-dev mailing list
>                 >         mesa-dev at lists.freedesktop.org
>                 >
>                  http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>                 >
>                 >
>                 
>                 
>                 
>         
>         
> 
> 




More information about the mesa-dev mailing list