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

[Gert Wollny]

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.  
 
> > +            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). 
> 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 passwithout 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 */
> > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180618/4ff6665f/attachment.html>