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

[Iago Toral]

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.

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.

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