[Mesa-dev] [PATCH] st/mesa: don't assume that the whole surface gets mapped

[Nicolai Hähnle]

On 24.06.2016 04:32, Ilia Mirkin wrote:
> Under some circumstances, the driver may choose to return a temporary
> surface instead of a pointer to the original. Make sure to pass the
> actual view volume to be mapped to the transfer function rather than
> adjusting the map pointer after-the-fact.

I'm curious, why does nouveau decide to do that?

>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>
> Perhaps this should use PIPE_TRANSFER_MAP_DIRECTLY when mapping? Nouveau will bail on the map if it can't do the direct map.

Interestingly, we don't handle that flag at all for r600g/radeonsi.

By the way, looking at this again I think the real bug is that the 
width/height passed into pipe_transfer_map_3d was incorrect in the cache 
case: the box doesn't actually cover the whole cache texture. But your 
fix seems fine to me as well -- it'll still do the right thing for 
radeon, and I expect you checked that it does the right thing on nouveau.

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


>
>   src/mesa/state_tracker/st_cb_readpixels.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_readpixels.c b/src/mesa/state_tracker/st_cb_readpixels.c
> index 39d2274..b8372cd 100644
> --- a/src/mesa/state_tracker/st_cb_readpixels.c
> +++ b/src/mesa/state_tracker/st_cb_readpixels.c
> @@ -406,7 +406,7 @@ st_ReadPixels(struct gl_context *ctx, GLint x, GLint y,
>      unsigned bind = PIPE_BIND_TRANSFER_READ;
>      struct pipe_transfer *tex_xfer;
>      ubyte *map = NULL;
> -   bool window;
> +   int dst_x, dst_y;
>
>      /* Validate state (to be sure we have up-to-date framebuffer surfaces)
>       * and flush the bitmap cache prior to reading. */
> @@ -483,7 +483,8 @@ st_ReadPixels(struct gl_context *ctx, GLint x, GLint y,
>                                  st_fb_orientation(ctx->ReadBuffer) == Y_0_TOP,
>                                  width, height, format, src_format, dst_format);
>      if (dst) {
> -      window = false;
> +      dst_x = x;
> +      dst_y = y;
>      } else {
>         /* See if the texture format already matches the format and type,
>          * in which case the memcpy-based fast path will likely be used and
> @@ -500,23 +501,21 @@ st_ReadPixels(struct gl_context *ctx, GLint x, GLint y,
>         if (!dst)
>            goto fallback;
>
> -      window = true;
> +      dst_x = 0;
> +      dst_y = 0;
>      }
>
>      /* map resources */
>      pixels = _mesa_map_pbo_dest(ctx, pack, pixels);
>
>      map = pipe_transfer_map_3d(pipe, dst, 0, PIPE_TRANSFER_READ,
> -                              0, 0, 0, width, height, 1, &tex_xfer);
> +                              dst_x, dst_y, 0, width, height, 1, &tex_xfer);
>      if (!map) {
>         _mesa_unmap_pbo_dest(ctx, pack);
>         pipe_resource_reference(&dst, NULL);
>         goto fallback;
>      }
>
> -   if (!window)
> -      map += y * tex_xfer->stride + x * util_format_get_blocksize(dst_format);
> -
>      /* memcpy data into a user buffer */
>      {
>         const uint bytesPerRow = width * util_format_get_blocksize(dst_format);
>