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
