Re: [Mesa-dev] [PATCH 03/29] mesa: Do not assert on integer<->non-integer direct pack/unpack fast paths
On Thu, 2014-11-20 at 21:35 -0800, Jason Ekstrand wrote:
>
>
> On Thu, Nov 20, 2014 at 9:33 PM, Jason Ekstrand
> wrote:
>
>
> On Thu, Nov 20, 2014 at 12:29 AM, Iago Toral
> 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
> > 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
> ==
> > RGBA_UBYTE.as_uint) {
> > - assert(!
> _mesa_is_format_integer_color(src_format));
> > + } else if (dst_array_format.as_uint
> ==
> > RGBA_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,
>
Re: [Mesa-dev] [PATCH 03/29] mesa: Do not assert on integer<->non-integer direct pack/unpack fast paths
On Thu, Nov 20, 2014 at 9:33 PM, Jason Ekstrand
wrote:
>
>
> On Thu, Nov 20, 2014 at 12:29 AM, Iago Toral 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.
>
>> 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
>> > 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 ==
>> > RGBA_UBYTE.as_uint) {
>> > - assert(!_mesa_is_format_integer_color(src_format));
>> > + } else if (dst_array_format.as_uint ==
>> > RGBA_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 ==
>> > RGBA_UINT.as_uint) {
>> > - assert(_mesa_is_format_integer_color(src_format));
>> > + } else if (dst_array_format.as_uint ==
>> > RGBA_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 ==
>> > RGBA_UBYTE.as_uint) {
>> > - assert(!_mesa_is_format_integer_color(dst_format));
>> > + } else if (src_array_format.as_uint ==
>> > RGBA_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;
>> >
Re: [Mesa-dev] [PATCH 03/29] mesa: Do not assert on integer<->non-integer direct pack/unpack fast paths
On Thu, Nov 20, 2014 at 12:29 AM, Iago Toral 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.
>
> 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
> > 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 ==
> > RGBA_UBYTE.as_uint) {
> > - assert(!_mesa_is_format_integer_color(src_format));
> > + } else if (dst_array_format.as_uint ==
> > RGBA_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 ==
> > RGBA_UINT.as_uint) {
> > - assert(_mesa_is_format_integer_color(src_format));
> > + } else if (dst_array_format.as_uint ==
> > RGBA_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 ==
> > RGBA_UBYTE.as_uint) {
> > - assert(!_mesa_is_format_integer_color(dst_format));
> > + } else if (src_array_format.as_uint ==
> > RGBA_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 ==
> > RGBA_UINT.as_uint) {
> > - assert(_mesa_is_format_integer_color(dst_format));
> > + } else if (src_array_format.as_uint ==
> > RGBA_UINT.as_uint &&
> > +
Re: [Mesa-dev] [PATCH 03/29] mesa: Do not assert on integer<->non-integer direct pack/unpack fast paths
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.
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
> 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 ==
> RGBA_UBYTE.as_uint) {
> - assert(!_mesa_is_format_integer_color(src_format));
> + } else if (dst_array_format.as_uint ==
> RGBA_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 ==
> RGBA_UINT.as_uint) {
> - assert(_mesa_is_format_integer_color(src_format));
> + } else if (dst_array_format.as_uint ==
> RGBA_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 ==
> RGBA_UBYTE.as_uint) {
> - assert(!_mesa_is_format_integer_color(dst_format));
> + } else if (src_array_format.as_uint ==
> RGBA_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 ==
> RGBA_UINT.as_uint) {
> - assert(_mesa_is_format_integer_color(dst_format));
> + } else if (src_array_format.as_uint ==
> RGBA_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@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
Re: [Mesa-dev] [PATCH 03/29] mesa: Do not assert on integer<->non-integer direct pack/unpack fast paths
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
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 == RGBA_UBYTE.as_uint) {
> - assert(!_mesa_is_format_integer_color(src_format));
> + } else if (dst_array_format.as_uint == RGBA_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 == RGBA_UINT.as_uint) {
> - assert(_mesa_is_format_integer_color(src_format));
> + } else if (dst_array_format.as_uint == RGBA_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 == RGBA_UBYTE.as_uint) {
> - assert(!_mesa_is_format_integer_color(dst_format));
> + } else if (src_array_format.as_uint == RGBA_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 == RGBA_UINT.as_uint) {
> - assert(_mesa_is_format_integer_color(dst_format));
> + } else if (src_array_format.as_uint == RGBA_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@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
