[Mesa-dev] [PATCH 05/14] meta: Abort meta pbo path if readpixels need signed-unsigned conversion

[Iago Toral]

On Tue, 2015-07-21 at 17:05 -0700, Anuj Phogat wrote:
> On Tue, Jul 21, 2015 at 1:36 AM, Iago Toral <itoral at igalia.com> wrote:
> > On Tue, 2015-07-21 at 08:13 +0200, Iago Toral wrote:
> >> On Mon, 2015-07-20 at 10:56 -0700, Anuj Phogat wrote:
> >> > On Mon, Jul 20, 2015 at 5:10 AM, Iago Toral <itoral at igalia.com> wrote:
> >> > > On Fri, 2015-06-19 at 13:40 -0700, Anuj Phogat wrote:
> >> > >> On Tue, Jun 16, 2015 at 9:21 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> >> > >> >
> >> > >> > On Jun 16, 2015 11:15, "Anuj Phogat" <anuj.phogat at gmail.com> wrote:
> >> > >> >>
> >> > >> >> Without this patch, piglit test fbo_integer_readpixels_sint_uint fails,
> >> > >> >> when
> >> > >> >> forced to use the meta pbo path.
> >> > >> >>
> >> > >> >> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> >> > >> >> Cc: <mesa-stable at lists.freedesktop.org>
> >> > >> >> ---
> >> > >> >>  src/mesa/drivers/common/meta_tex_subimage.c | 3 +++
> >> > >> >>  1 file changed, 3 insertions(+)
> >> > >> >>
> >> > >> >> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
> >> > >> >> b/src/mesa/drivers/common/meta_tex_subimage.c
> >> > >> >> index 00364f8..84cbc50 100644
> >> > >> >> --- a/src/mesa/drivers/common/meta_tex_subimage.c
> >> > >> >> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> >> > >> >> @@ -283,6 +283,9 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
> >> > >> >> GLuint dims,
> >> > >> >>
> >> > >> >>        if (_mesa_need_rgb_to_luminance_conversion(rb->Format, format))
> >> > >> >>           return false;
> >> > >> >> +
> >> > >> >> +      if (_mesa_need_signed_unsigned_int_conversion(rb->Format, format,
> >> > >> >> type))
> >> > >> >> +         return false;
> >> > >> >
> >> > >> > Hrm... This seems fishy.  Isn't glBlitFramebuffers supposed to handle format
> >> > >> > conversion with integers?  If so we should probably fix it rather than just
> >> > >> > skip it for the meta pbo path.
> >> > >> >
> >> > >> As discussed offline, here is relevant text for glBlitFrameBuffer() from
> >> > >> OpenGL 4.5 spec, section 18.3.1:
> >> > >> "An INVALID_OPERATION error is generated if format conversions are not
> >> > >> supported, which occurs under any of the following conditions:
> >> > >> -The read buffer contains fixed-point or floating-point values and any draw
> >> > >>   buffer contains neither fixed-point nor floating-point values.
> >> > >> -The read buffer contains unsigned integer values and any draw buffer does
> >> > >>   not contain unsigned integer values.
> >> > >> - The read buffer contains signed integer values and any draw buffer does
> >> > >>   not contain signed integer values."
> >> > >>
> >> > >> I'll add a comment here explaining the reason to avoid meta path.
> >> > >
> >> > > Is this code going to run only for glBlitFramebuffer? I see this
> >> > > function being called from code paths that implement glReadPixels and
> >> > > glGetTexImage too.
> >> > >
> >> > _mesa_meta_pbo_GetTexSubImage() is used only for glReadPixels and
> >> > glGetTexImage. I quoted the glBliFrameBuffer restriction above because
> >> > the function is later using _mesa_meta_BlitFramebuffer(), which doesn't
> >> > support some format conversions.
> >>
> >> If this function can be used to resolve ReadPixels and GetTexImage but
> >> the checks you add are *specific* to BlitFramebuffer, it does not look
> >> like this is the right place for them. Shouldn't you put them inside
> >> _mesa_meta_BlitFramebuffer instead? Otherwise they would affect to
> >> ReadPixels and GetTexImage too and I don't see the same restrictions
> >> applying to ReadPixels for example.
> We already have error checks in place for glBlitFrameBuffer(). Take a
> look at compatible_color_datatypes() in _mesa_blit_framebuffer().
> 
> > Specifically for ReadPixels I only see this in the spec:
> >
> > "An INVALID_OPERATION error is generated 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."
> >
> > So, unlike BlitFramebuffer, it seems that ReadPixels is fine as long as
> > both formats are integer, no matter if the types have the same sign or
> > not.
> Right. That's the reason this patch doesn't generate any GL error for
> signed-unsigned int mismatch. It just decides not to use meta pbo path
> because of  unsupported format conversions in _mesa_meta_BlitFrameBuffer(),
> and fallback to using other paths.

Ah, I see now, thanks for clarifying this.

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

> >
> > Iago
> >
> >>
> >> > >> >>     }
> >> > >> >>
> >> > >> >>     /* For arrays, use a tall (height * depth) 2D texture but taking into
> >> > >> >> --
> >> > >> >> 1.9.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
> >> > >
> >> > >
> >> >
> >>
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
>