[Mesa-dev] [PATCH] mesa: remove unnecessary checks in _mesa_readpixels_needs_slow_path

[Marek Olšák]

Gallium should be alright. We'll let you know if we find a regression,
but I don't think there will be any.

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek

On Thu, Jun 25, 2015 at 2:45 AM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> On Tue, Jun 23, 2015 at 3:34 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
>> readpixels_can_use_memcpy will later call _mesa_format_matches_format_and_type
>> which does much tighter checks than these to decide if we can use
>> memcpy for readpixels.
>>
>> Also, the checks do not seem to be extensive enough anyway, since we are
>> checking for signed/unsigned conversion only when the framebuffer has integers,
>> but the same checks could be done for other types anyway, since as long as
>> there is a signed/unsigned conversion we can't memcpy.
>>
>> No regressions observed on i965/llvmpipe.
>> ---
>> The way gallium uses _mesa_format_matches_format_and_type and
>> _mesa_readpixels_needs_slow_path is a bit different, they call these
>> functions to decide if they want to fallback to Mesa's implementation
>> of ReadPixels, not exactly to check if we can memcpy... so it is unclear
>> to me if some combinations may still need these checks to make the right
>> decision. I did not see any regressions with llvmpipe at least, so I am
>> assuming that they are not needed, but maybe someone wants to give this
>> patch a test run on nouveau or radeon, just in case. If they are needed
>> I guess the right thing would be to move the checks to st_cb_readpixels.c.
>>
>>  src/mesa/main/readpix.c | 16 ----------------
>>  1 file changed, 16 deletions(-)
>>
>> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
>> index a3357cd..e256695 100644
>> --- a/src/mesa/main/readpix.c
>> +++ b/src/mesa/main/readpix.c
>> @@ -128,7 +128,6 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>>  {
>>     struct gl_renderbuffer *rb =
>>           _mesa_get_read_renderbuffer_for_format(ctx, format);
>> -   GLenum srcType;
>>
>>     assert(rb);
>>
>> @@ -153,21 +152,6 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>>           return GL_TRUE;
>>        }
>>
>> -      /* Conversion between signed and unsigned integers needs masking
>> -       * (it isn't just memcpy). */
>> -      srcType = _mesa_get_format_datatype(rb->Format);
>> -
>> -      if ((srcType == GL_INT &&
>> -           (type == GL_UNSIGNED_INT ||
>> -            type == GL_UNSIGNED_SHORT ||
>> -            type == GL_UNSIGNED_BYTE)) ||
>> -          (srcType == GL_UNSIGNED_INT &&
>> -           (type == GL_INT ||
>> -            type == GL_SHORT ||
>> -            type == GL_BYTE))) {
>> -         return GL_TRUE;
>> -      }
>> -
>>        /* And finally, see if there are any transfer ops. */
>>        return get_readpixels_transfer_ops(ctx, rb->Format, format, type,
>>                                           uses_blit) != 0;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> Thanks for the patch Iago.
> Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>
>
> You might want to wait few days to hear any comments on the gallium
> usage of the function. If no one objects you can push it upstream.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev