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.ol...@amd.com>
Marek
On Thu, Jun 25, 2015 at 2:45 AM, Anuj Phogat <anuj.pho...@gmail.com> wrote:
> On Tue, Jun 23, 2015 at 3:34 AM, Iago Toral Quiroga <ito...@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> Thanks for the patch Iago.
> Reviewed-by: Anuj Phogat <anuj.pho...@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@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
