On Tue, 2015-07-21 at 17:05 -0700, Anuj Phogat wrote: > On Tue, Jul 21, 2015 at 1:36 AM, Iago Toral <[email protected]> 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 <[email protected]> wrote: > >> > > On Fri, 2015-06-19 at 13:40 -0700, Anuj Phogat wrote: > >> > >> On Tue, Jun 16, 2015 at 9:21 PM, Jason Ekstrand > >> > >> <[email protected]> wrote: > >> > >> > > >> > >> > On Jun 16, 2015 11:15, "Anuj Phogat" <[email protected]> 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 <[email protected]> > >> > >> >> Cc: <[email protected]> > >> > >> >> --- > >> > >> >> 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 <[email protected]> > > > > Iago > > > >> > >> > >> >> } > >> > >> >> > >> > >> >> /* For arrays, use a tall (height * depth) 2D texture but > >> > >> >> taking into > >> > >> >> -- > >> > >> >> 1.9.3 > >> > >> >> > >> > >> >> _______________________________________________ > >> > >> >> mesa-dev mailing list > >> > >> >> [email protected] > >> > >> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > >> > >> _______________________________________________ > >> > >> mesa-dev mailing list > >> > >> [email protected] > >> > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > >> > > > >> > > > >> > > >> > >> > >> _______________________________________________ > >> mesa-dev mailing list > >> [email protected] > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
