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

[Ilia Mirkin]

On Fri, Jun 24, 2016 at 3:27 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> 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?

It doesn't (usually, in this case). It was just a theory I had while
debugging, since I saw small strides coming back.

>
>>
>> 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.

Well, the *real* issue was that nouveau was setting totally bogus
values on the returned stride - they were based on the requested
width, not on the width of the mapped surface. So if the width had
been the full thing, it would have worked. But IMO what I've done is
the proper solution to this issue, and a separate fix in nouveau will
return the correct values for stride/offset.

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

Thanks!

>
>
>
>>
>>   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);
>>
>