[virglrenderer-devel] [PATCH] vrend: Copy from iovec to avoid needing glGetTexImage
Gurchetan Singh
gurchetansingh at chromium.org
Fri Jun 8 00:26:01 UTC 2018
On Thu, May 24, 2018 at 11:58 PM Tomeu Vizoso
<tomeu.vizoso at collabora.com> wrote:
>
> GLES doesn't have glGetTexImage, so this is a way to get around that.
Definitely need some variation of this patch, I get
"vrend_resource_copy_fallback: gles violation reported 6 "deqp-gles3"
glGetTexImage is missing" when I run
dEQP-GLES3.functional.texture.units.2_units.mixed.5 with a GLES host.
> We rely on the existing assumption that a resource's backing iovec is
> always in sync with the texture data if it isn't renderable.
This is a more of general question and not related to this specific patch:
What's should be the lifetime of a resource's backing iovec?
I see vrend_renderer_resource_attach_iov and
vrend_renderer_resource_detach_iov seem to control it. This goes back
to virgl_cmd_resource_unref in QEMU and virtio_gpu_ttm_bo_destroy in
the kernel. So the lifetime of the iovecs is essentially the lifetime
of the GEM buffer, and therefore the lifetime of the texture.
Considering 95% on textures in games just do one upload, would be
possible to not keep the iovecs around for the simple, general cases
(we'll probably need them for non-renderable formats)?
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> ---
> src/vrend_renderer.c | 92 +++++++++++++++++++++++++++-----------------
> 1 file changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index 862a873cd812..1fccccbc28de 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -5809,13 +5809,15 @@ static void vrend_resource_copy_fallback(struct vrend_context *ctx,
> const struct pipe_box *src_box)
> {
> char *tptr;
> - uint32_t transfer_size;
> + uint32_t transfer_size, src_stride, dst_stride;
> GLenum glformat, gltype;
> int elsize = util_format_get_blocksize(dst_res->base.format);
> int compressed = util_format_is_compressed(dst_res->base.format);
> int cube_slice = 1;
> uint32_t slice_size, slice_offset;
> int i;
> + struct pipe_box box;
> +
> if (src_res->target == GL_TEXTURE_CUBE_MAP)
> cube_slice = 6;
>
> @@ -5824,6 +5826,12 @@ static void vrend_resource_copy_fallback(struct vrend_context *ctx,
> return;
> }
>
> + 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 */
> slice_size = util_format_get_nblocks(src_res->base.format, u_minify(src_res->base.width0, src_level), u_minify(src_res->base.height0, src_level)) *
> util_format_get_blocksize(src_res->base.format);
> @@ -5839,42 +5847,51 @@ static void vrend_resource_copy_fallback(struct vrend_context *ctx,
> if (compressed)
> glformat = tex_conv_table[src_res->base.format].internalformat;
>
> - switch (elsize) {
> - case 1:
> - glPixelStorei(GL_PACK_ALIGNMENT, 1);
> - break;
> - case 2:
> - glPixelStorei(GL_PACK_ALIGNMENT, 2);
> - break;
> - case 4:
> - default:
> - glPixelStorei(GL_PACK_ALIGNMENT, 4);
> - break;
> - case 8:
> - glPixelStorei(GL_PACK_ALIGNMENT, 8);
> - break;
> - }
> - glBindTexture(src_res->target, src_res->id);
> + if (!vrend_format_can_render(src_res->base.format)) {
> + /*
> + * If the src resource isn't renderable, we can rely on its backing
> + * iovec having the data we need.
> + */
> + read_transfer_data(&src_res->base, src_res->iov, src_res->num_iovs,
> + tptr, src_stride, &box, src_level, 0, false);
> + } else {
Should we always use this or only on GLES? Is copying from the
backing iovecs more efficient than glGetTexImage when we have a GL
host?
> + switch (elsize) {
> + case 1:
> + glPixelStorei(GL_PACK_ALIGNMENT, 1);
> + break;
> + case 2:
> + glPixelStorei(GL_PACK_ALIGNMENT, 2);
> + break;
> + case 4:
> + default:
> + glPixelStorei(GL_PACK_ALIGNMENT, 4);
> + break;
> + case 8:
> + glPixelStorei(GL_PACK_ALIGNMENT, 8);
> + break;
> + }
> + glBindTexture(src_res->target, src_res->id);
>
> - slice_offset = 0;
> - for (i = 0; i < cube_slice; i++) {
> - GLenum ctarget = src_res->target == GL_TEXTURE_CUBE_MAP ? GL_TEXTURE_CUBE_MAP_POSITIVE_X + i : src_res->target;
> - if (compressed) {
> - if (vrend_state.have_arb_robustness)
> - glGetnCompressedTexImageARB(ctarget, src_level, transfer_size, tptr + slice_offset);
> - else if (vrend_state.use_gles)
> - report_gles_missing_func(ctx, "glGetCompressedTexImage");
> - else
> - glGetCompressedTexImage(ctarget, src_level, tptr + slice_offset);
> - } else {
> - if (vrend_state.have_arb_robustness)
> - glGetnTexImageARB(ctarget, src_level, glformat, gltype, transfer_size, tptr + slice_offset);
> - else if (vrend_state.use_gles)
> - report_gles_missing_func(ctx, "glGetTexImage");
> - else
> - glGetTexImage(ctarget, src_level, glformat, gltype, tptr + slice_offset);
> + slice_offset = 0;
> + for (i = 0; i < cube_slice; i++) {
> + GLenum ctarget = src_res->target == GL_TEXTURE_CUBE_MAP ? GL_TEXTURE_CUBE_MAP_POSITIVE_X + i : src_res->target;
> + if (compressed) {
> + if (vrend_state.have_arb_robustness)
> + glGetnCompressedTexImageARB(ctarget, src_level, transfer_size, tptr + slice_offset);
> + else if (vrend_state.use_gles)
> + report_gles_missing_func(ctx, "glGetCompressedTexImage");
> + else
> + glGetCompressedTexImage(ctarget, src_level, tptr + slice_offset);
> + } else {
> + if (vrend_state.have_arb_robustness)
> + glGetnTexImageARB(ctarget, src_level, glformat, gltype, transfer_size, tptr + slice_offset);
> + else if (vrend_state.use_gles)
> + report_gles_missing_func(ctx, "glGetTexImage");
> + else
> + glGetTexImage(ctarget, src_level, glformat, gltype, tptr + slice_offset);
> + }
> + slice_offset += slice_size;
> }
> - slice_offset += slice_size;
> }
>
> glPixelStorei(GL_PACK_ALIGNMENT, 4);
> @@ -5925,6 +5942,11 @@ static void vrend_resource_copy_fallback(struct vrend_context *ctx,
> }
>
> glPixelStorei(GL_UNPACK_ALIGNMENT, 4);
> +
> + /* Sync the iov that backs the dst resource */
> + write_transfer_data(&dst_res->base, dst_res->iov, dst_res->num_iovs, tptr,
> + dst_stride, &box, src_level, 0, false);
> +
I assume we need this because of this scenario:
1) Resources A, B, C are not rendereable
2) Gallium issues a command to copy A to B
3) Gallium issues a command to copy B to C
Thus we need to update (B) in (2). Still, we could probably avoid
this copy with a host GL driver.
> free(tptr);
> }
>
> --
> 2.17.0
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
More information about the virglrenderer-devel
mailing list