[virglrenderer-devel] [PATCH 3/5] vrend: store offsets into backing ivo for mipmaps and use it when reading data back (v2)

[Gurchetan Singh]

On Sun, Jun 17, 2018 at 11:14 PM Gert Wollny <gert.wollny at collabora.com> wrote:
>
> Am Freitag, den 15.06.2018, 15:50 -0700 schrieb Gurchetan Singh:
>
> Patches 1, 2, 4, and 5 are:
>
>
> Thanks for the review.
>
>
> +         if (info->level> 0 && info->level <= MAX_READBACK_MIPMAP_LEVELS) {
>
>
> Why don't you just have a case for level 0?  Right now it's a little confusing to follow.  It's also consistent with EGL_EXT_image_dma_buf_import offsets.
>
>
> The size of the offset array is limied, so I have to check the upper bound, otherwise we might later try to read from unallocated memory. I can think of how to make it more clear but in the end it's the same.
>

Essentially, you're trying to re-create the level_offset array from
the guest side on the host.  Since that has case for level 0, I think
we should have one too.  Using the the same naming convention
(res->level_offsets[level], VR_MAX_TEXTURE_2D_LEVELS) would make the
code a little clearer has well.

> +            int64_t level_height = u_minify(res->base.height0, info->level);
> +            res->mipmap_offsets[info->level-1] = info->offset -
> +                                               ((info->box->z * level_height + y) * stride + x * elsize);
>
>
> I don't fully understand this calculation.  From  virgl_texture_transfer_map it seems info->offset represents the start of the transfer from with given {x, y, z} coordination in the texture.  So:
> info->offset == info->box->z * level_height + y * level_stride + x * elsize
>
>
> level_height is the numbers of rows in the mip-map level, you still have to multiply with
> the stride to get the number of bytes for a full slice, hence for info->box->z slices
> you have an offset of ( info->box->z * level_height * level_stride) bytes, adding the bytes
> per y rows you get
>
> ( info->box->z * level_height * level_stride) + y * level_stride =
> ( info->box->z * level_height + y) * level_stride (reduce four operations to three)
> and to this you add the number of bytes to get to x (= x* elsize).

Got it.  So that's essentially:

image_stride * box->z + stride * y + x * elsize

That's the offset into transfer at a particular mipmap level (level =
0, level = 1 etc.).  But we just want the start of the mipmap level,
so we subtract this value.


>
>
> So won't mipmap_offsets always be zero?
>
>
> The offset for the mipmap data is only zero for the 0-level, if it would alway be zero the tests would pass
> without this patch.
>
>
> Also nit: parens would make more sense like this -- info->offset - (info->box->z * level_height + y * stride + x * elsize);
>
> Same as above, you forgot that for a slice the number of bytes is (level_height*stride).
>
>
> Best,
> Gert
>
>
>
> +         }
> +
>           if (res->base.format == (enum pipe_format)VIRGL_FORMAT_Z24X8_UNORM) {
>              /* we get values from the guest as 24-bit scaled integers
>                 but we give them to the host GL and it interprets them
> @@ -6088,8 +6102,6 @@ static void vrend_resource_copy_fallback(struct vrend_resource *src_res,
>
>     box = *src_box;
>     box.depth = vrend_get_texture_depth(src_res, src_level);
> -
> -   src_stride = util_format_get_stride(src_res->base.format, src_res->base.width0);
>     dst_stride = util_format_get_stride(dst_res->base.format, dst_res->base.width0);
>
>     /* this is ugly need to do a full GetTexImage */
> @@ -6111,12 +6123,21 @@ static void vrend_resource_copy_fallback(struct vrend_resource *src_res,
>      * iovec to have the data we need, otherwise we can use glGetTexture
>      */
>     if (vrend_state.use_gles) {
> -      read_transfer_data(&src_res->base, src_res->iov, src_res->num_iovs,
> -                         tptr, src_stride, &box, src_level, 0, false);
> +      uint64_t src_offset = 0;
> +      uint64_t dst_offset = 0;
> +      if (src_level > 0 && src_level <= MAX_READBACK_MIPMAP_LEVELS) {
> +         src_offset = src_res->mipmap_offsets[src_level-1];
> +         dst_offset = dst_res->mipmap_offsets[src_level-1];
> +      }
> +
> +      src_stride = util_format_get_nblocksx(src_res->base.format,
> +                                            u_minify(src_res->base.width0, src_level)) * elsize;
> +      read_transfer_data(&src_res->base, src_res->iov, src_res->num_iovs, tptr,
> +                         src_stride, &box, src_level, src_offset, false);
>        /* In on GLES Sync the iov that backs the dst resource because
>         * we might need it in a chain copy A->B, B->C */
>        write_transfer_data(&dst_res->base, dst_res->iov, dst_res->num_iovs, tptr,
> -                          dst_stride, &box, src_level, 0, false);
> +                          dst_stride, &box, src_level, dst_offset, false);
>        /* we get values from the guest as 24-bit scaled integers
>           but we give them to the host GL and it interprets them
>           as 32-bit scaled integers, so we need to scale them here */
> diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h
> index 042d439..258c439 100644
> --- a/src/vrend_renderer.h
> +++ b/src/vrend_renderer.h
> @@ -44,6 +44,9 @@ struct virgl_gl_ctx_param {
>  extern int vrend_dump_shaders;
>  struct vrend_context;
>
> +/* Number of mipmap levels for which to keep the backing iov offsets */
> +#define MAX_READBACK_MIPMAP_LEVELS 16
> +
>  struct vrend_resource {
>     struct pipe_resource base;
>     GLuint id;
> @@ -62,6 +65,7 @@ struct vrend_resource {
>     char *ptr;
>     struct iovec *iov;
>     uint32_t num_iovs;
> +   uint64_t mipmap_offsets[MAX_READBACK_MIPMAP_LEVELS];
>  };
>
>  /* assume every format is sampler friendly */