[Mesa-dev] [PATCH 03/14] mesa: Fix conditions to test signed, unsigned integer format

[Iago Toral]

On Tue, 2015-06-23 at 08:54 +0200, Iago Toral wrote:
> On Mon, 2015-06-22 at 12:35 -0700, Anuj Phogat wrote:
> > On Sun, Jun 21, 2015 at 11:25 PM, Iago Toral <itoral at igalia.com> wrote:
> > > On Fri, 2015-06-19 at 13:32 -0700, Anuj Phogat wrote:
> > >> On Thu, Jun 18, 2015 at 11:41 PM, Iago Toral <itoral at igalia.com> wrote:
> > >> > On Thu, 2015-06-18 at 09:19 -0700, Anuj Phogat wrote:
> > >> >> On Thu, Jun 18, 2015 at 7:09 AM, Iago Toral <itoral at igalia.com> wrote:
> > >> >> > On Tue, 2015-06-16 at 11:15 -0700, Anuj Phogat wrote:
> > >> >> >> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> > >> >> >> Cc: <mesa-stable at lists.freedesktop.org>
> > >> >> >> ---
> > >> >> >>  src/mesa/main/readpix.c | 2 ++
> > >> >> >>  1 file changed, 2 insertions(+)
> > >> >> >>
> > >> >> >> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> > >> >> >> index caa2648..a9416ef 100644
> > >> >> >> --- a/src/mesa/main/readpix.c
> > >> >> >> +++ b/src/mesa/main/readpix.c
> > >> >> >> @@ -160,10 +160,12 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
> > >> >> >>        srcType = _mesa_get_format_datatype(rb->Format);
> > >> >> >>
> > >> >> >>        if ((srcType == GL_INT &&
> > >> >> >> +           _mesa_is_enum_format_integer(format) &&
> > >> >> >>             (type == GL_UNSIGNED_INT ||
> > >> >> >>              type == GL_UNSIGNED_SHORT ||
> > >> >> >>              type == GL_UNSIGNED_BYTE)) ||
> > >> >> >>            (srcType == GL_UNSIGNED_INT &&
> > >> >> >> +           _mesa_is_enum_format_integer(format) &&
> > >> >> >>             (type == GL_INT ||
> > >> >> >>              type == GL_SHORT ||
> > >> >> >>              type == GL_BYTE))) {
> > >> >> >
> > >> >> > As far as I understand this code we are trying to see if we can use
> > >> >> > memcpy to directly copy the contents of the framebuffer to the
> > >> >> > destination buffer. In that case, as long as the src/dst types have
> > >> >> > different sign we can't just use memcpy, right? In fact it looks like we
> > >> >> > might need to expand the checks to include the cases where srcType is
> > >> >> > GL_(UNSIGNED_)SHORT and GL_(UNSIGNED_)BYTE as well.
> > >> >> >
> > >> >> srcType returned by _mesa_get_format_datatype() is one of:
> > >> >> GL_UNSIGNED_NORMALIZED
> > >> >> GL_SIGNED_NORMALIZED
> > >> >> GL_UNSIGNED_INT
> > >> >> GL_INT
> > >> >> GL_FLOAT
> > >> >> So, the suggested checks for srcType are not required.
> > >> >
> > >> > Oh, right, although I think that does not invalidate my point: can we
> > >> > memcpy from a GL_UNSIGNED_NORMALIZED to a format with type GL_FLOAT or
> > >> > GL_SIGNED_NORMALIZED? It does not look like these checks here are
> > >> > thorough.
> > >> >
> > >> Helper function _mesa_need_signed_unsigned_int_conversion() is
> > >> meant to do the checks only for integer formats. May be add another
> > >> function to do the missing checks for other formats?
> > >
> > > I have no concerns about the _mesa_need_signed_unsigned_int_conversion
> > > function that you add in a later patch for your PBO work, my concern is
> > > related to the fact that you are assuming that the checks that you need
> > > in the PBO path are the same that we have in
> > > _mesa_readpixels_needs_slow_path, so you make both the same when I think
> > > they are trying to address different things.
> > >
> > > In your PBO code, you can't handle signed/unsigned integer conversions,
> > > so you need to detect that and fall back to another path. That should be
> > > fine I guess and the function _mesa_need_signed_unsigned_int_conversion
> > > does what you need, so no problems there.
> > >
> > > However, in _mesa_readpixels_needs_slow_path I think we don't want to
> > > just do integer checking. The purpose of the function is to tell whether
> > > we can use memcpy to copy pixels from the framebuffer to the dst, and if
> > > we have types with different signs, *whether they are integer or not*,
> > > we can't, so limiting the check only to integer types does not look
> > > right to me. The key aspect here is that what this function needs to
> > > check is not specific to integer types, even if the current code only
> > > seems to check things when the framebuffer has an integer format.
> > >
> > >> > In any case, that's beyond the point of your patch. Talking specifically
> > >> > about your patch: can we memcpy, for example, from a _signed_ integer
> > >> > format like MESA_FORMAT_R_SINT8 to an _unsigned_ format (integer or
> > >> > not)? I don't think we can, in which case your patch would not look
> > >> > correct to me.
> > >> >
> > >> Reading integer format to a non integer format is not allowed in
> > >> glReadPixels. That's why those cases are not relevant here and
> > >> we just check for integer formats. From ext_texture_integer:
> > >> "INVALID_OPERATON is generated by ReadPixels if <format> is
> > >> an integer format and the color buffer is not an integer format, or
> > >>  if <format> is not an integer format and the color buffer is an
> > >>  integer format."
> > >
> > > Right, that was not a good example, but forget about integer types, what
> > > if the framebuffer is something like MESA_FORMAT_R8G8B8A8_UNORM and our
> > > dst format/type is GL_RGBA/GL_BYTE? These are not integer types but we
> > > can't memcpy anyway because the framebuffer is unsigned and the dst is
> > > signed so a conversion is needed.
> > >
> > > Of course, the current code in this function only cares about the
> > > framebuffer being an integer format, but for the reasons I explain
> > > above, I think that is wrong in this case, I think this code should be
> > > either eliminated (and rely in the much tighter checks in
> > > _mesa_format_matches_format_and_type) or expanded to not be specific to
> > > integer formats, and your patch to this function conflicts with both
> > > scenarios.
> > >
> > Thanks for explaining the issue. With much tighter checks available in
> > _mesa_format_matches_format_and_type(), I agree that this code can
> > be deleted. I got no piglit failures when this code is deleted and the driver
> > is forced to use _mesa_readpixels().
> > 
> > >> > Also, as I said in my previous e-mail, I feel like these checks here do
> > >> > not add anything, at least when this function is called from
> > >> > readpixels_can_use_memcpy, since even if we return true here, we will
> > >> > later call _mesa_format_matches_format_and_type and that would check
> > >> > that the formats match anyway before going through the memcpy path. Even
> > >> > the other use of this function in Gallium would call
> > >> > _mesa_format_matches_format_and_type before it calls this... That's why
> > >> > I think we probably want to remove these checks from this function and
> > >> > rely on _mesa_format_matches_format_and_type exclusively to check
> > >> > allowed formats and types.
> > >> >
> > >> It does seem like calling _mesa_need_signed_unsigned_int_conversion()
> > >> is redundant in case of readpixels_can_use_memcpy(). Thanks for noticing
> > >> it. Feel free to submit a patch to remove the redundant check.
> > >>
> > >> But,  I still need _mesa_need_signed_unsigned_int_conversion() and changes
> > >> in this patch for one of my later patches fixing meta PBO path for glReadPixels.
> > >> Do you still have any concerns about this patch?
> > >
> > > Yes, hopefully I explained them more clearly this time around, but
> > > notice that the concerns I have with this patch do not affect your PBO
> > > changes, you can still have _mesa_need_signed_unsigned_int_conversion
> > > and you can still use that function from your PBO path, my concerns are
> > > solely with your changes to _mesa_readpixels_needs_slow_path.
> > >
> > After deleting the redundant checks in _mesa_readpixels_needs_slow_path(),
> > _mesa_need_signed_unsigned_int_conversion() need not to be global.
> > I'll make it a local function and squash with my meta PBO changes.
> > 
> > Do you want to send out the patch deleting the checks or I'll send it out later?
> 
> I can take care of that, the function is also called from one place in
> gallium with a slightly different usage pattern, so I want to make sure
> that these checks are not relevant to the gallium path either. I'll do
> some tests and send the patch if I don't see any problems.

Actually, llvmpipe shows one regression if we remove these checks (the
same regression shows up if I apply your change).

It looks like Gallium relies on this function to check specifically for
signed/unsigned conversions when the framebuffer is integer so they can
fallback to Mesa's implementation of readpixels in that case, so for now
we can't remove that code.

It would probably make sense to move the checks to the gallium state
tracker instead I guess, so I'll write a patch for that.

Iago