On Tue, Jul 21, 2015 at 1:36 AM, Iago Toral <ito...@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 <ito...@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 <ja...@jlekstrand.net> >> > >> wrote: >> > >> > >> > >> > On Jun 16, 2015 11:15, "Anuj Phogat" <anuj.pho...@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.pho...@gmail.com> >> > >> >> Cc: <mesa-sta...@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. > > Iago > >> >> > >> >> } >> > >> >> >> > >> >> /* For arrays, use a tall (height * depth) 2D texture but taking >> > >> >> into >> > >> >> -- >> > >> >> 1.9.3 >> > >> >> >> > >> >> _______________________________________________ >> > >> >> 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 >> > > >> > > >> > >> >> >> _______________________________________________ >> 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
