<html><head></head><body><div>Am Freitag, den 15.06.2018, 15:50 -0700 schrieb Gurchetan Singh:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr">Patches 1, 2, 4, and 5 are:</div></blockquote><div><br></div><div>Thanks for the review. </div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><br>
+ if (info->level> 0 && info->level <= MAX_READBACK_MIPMAP_LEVELS) {<br></blockquote><div><br></div><div><div>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.</div></div></div></div></div></blockquote><div><br></div><div>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. </div><div><br></div><div> </div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+ int64_t level_height = u_minify(res->base.height0, info->level);<br>
+ res->mipmap_offsets[info->level-1] = info->offset -<br>
+ ((info->box->z * level_height + y) * stride + x * elsize);<br>
<br></blockquote><div><br></div><div>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:</div><div>info->offset == info->box->z * level_height + y * level_stride + x * elsize</div></div></div></div></blockquote><div><br></div><div>level_height is the numbers of rows in the mip-map level, you still have to multiply with </div><div>the stride to get the number of bytes for a full slice, hence for info->box->z slices </div><div>you have an offset of ( info->box->z * level_height * level_stride) bytes, adding the bytes </div><div>per y rows you get</div><div><br></div><div>( info->box->z * level_height * level_stride) + y * level_stride = </div><div> ( info->box->z * level_height + y) * level_stride (reduce four operations to three)</div><div>and to this you add the number of bytes to get to x (= x* elsize). </div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div><br></div><div>So won't mipmap_offsets always be zero?</div></div></div></div></blockquote><div><br></div><div>The offset for the mipmap data is only zero for the 0-level, if it would alway be zero the tests would pass</div><div>without this patch. </div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div><br></div><div>Also nit: parens would make more sense like this -- info->offset - (info->box->z * level_height + y * stride + x * elsize);</div></div></div></div></blockquote><div>Same as above, you forgot that for a slice the number of bytes is (level_height*stride). </div><div><br></div><div><br></div><div>Best, </div><div>Gert</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div> </div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">+ }<br>
+<br>
if (res->base.format == (enum pipe_format)VIRGL_FORMAT_Z24X8_UNORM) {<br>
/* we get values from the guest as 24-bit scaled integers<br>
but we give them to the host GL and it interprets them<br>
@@ -6088,8 +6102,6 @@ static void vrend_resource_copy_fallback(struct vrend_resource *src_res,<br>
<br>
box = *src_box;<br>
box.depth = vrend_get_texture_depth(src_res, src_level);<br>
-<br>
- src_stride = util_format_get_stride(src_res->base.format, src_res->base.width0);<br>
dst_stride = util_format_get_stride(dst_res->base.format, dst_res->base.width0);<br>
<br>
/* this is ugly need to do a full GetTexImage */<br>
@@ -6111,12 +6123,21 @@ static void vrend_resource_copy_fallback(struct vrend_resource *src_res,<br>
* iovec to have the data we need, otherwise we can use glGetTexture<br>
*/<br>
if (vrend_state.use_gles) {<br>
- read_transfer_data(&src_res->base, src_res->iov, src_res->num_iovs,<br>
- tptr, src_stride, &box, src_level, 0, false);<br>
+ uint64_t src_offset = 0;<br>
+ uint64_t dst_offset = 0;<br>
+ if (src_level > 0 && src_level <= MAX_READBACK_MIPMAP_LEVELS) {<br>
+ src_offset = src_res->mipmap_offsets[src_level-1];<br>
+ dst_offset = dst_res->mipmap_offsets[src_level-1];<br>
+ }<br>
+<br>
+ src_stride = util_format_get_nblocksx(src_res->base.format,<br>
+ u_minify(src_res->base.width0, src_level)) * elsize;<br>
+ read_transfer_data(&src_res->base, src_res->iov, src_res->num_iovs, tptr,<br>
+ src_stride, &box, src_level, src_offset, false);<br>
/* In on GLES Sync the iov that backs the dst resource because<br>
* we might need it in a chain copy A->B, B->C */<br>
write_transfer_data(&dst_res->base, dst_res->iov, dst_res->num_iovs, tptr,<br>
- dst_stride, &box, src_level, 0, false);<br>
+ dst_stride, &box, src_level, dst_offset, false);<br>
/* we get values from the guest as 24-bit scaled integers<br>
but we give them to the host GL and it interprets them<br>
as 32-bit scaled integers, so we need to scale them here */<br>
diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h<br>
index 042d439..258c439 100644<br>
--- a/src/vrend_renderer.h<br>
+++ b/src/vrend_renderer.h<br>
@@ -44,6 +44,9 @@ struct virgl_gl_ctx_param {<br>
extern int vrend_dump_shaders;<br>
struct vrend_context;<br>
<br>
+/* Number of mipmap levels for which to keep the backing iov offsets */<br>
+#define MAX_READBACK_MIPMAP_LEVELS 16<br>
+<br>
struct vrend_resource {<br>
struct pipe_resource base;<br>
GLuint id;<br>
@@ -62,6 +65,7 @@ struct vrend_resource {<br>
char *ptr;<br>
struct iovec *iov;<br>
uint32_t num_iovs;<br>
+ uint64_t mipmap_offsets[MAX_READBACK_MIPMAP_LEVELS];<br>
};<br>
<br>
/* assume every format is sampler friendly */<br>
</blockquote></div></div></div></blockquote></body></html>