<div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Dec 11, 2018 at 2:49 PM Elie Tournier <<a href="mailto:tournier.elie@gmail.com">tournier.elie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Dec 10, 2018 at 10:20:36AM -0800, Gurchetan Singh wrote:<br>
> Previously, we ignored the the glUnmap(..) operation and<br>
> flushed before we flush the cbuf. Now, let's just flush<br>
> the data when we unmap.<br>
> <br>
> Neither method is optimal, for example:<br>
> <br>
> glMapBufferRange(.., 0, 100, GL_MAP_FLUSH_EXPLICIT_BIT)<br>
> glFlushMappedBufferRange(.., 25, 30)<br>
> glFlushMappedBufferRange(.., 65, 70)<br>
> <br>
> We'll end up flushing 25 --> 70. Maybe we can fix this later.<br>
> <br>
> v2: Add fixme comment in the code (Elie)<br>
Thanks.<br>
I still have to run some regressions tests. They are a bit slow on my system.<br>
So for now, the series is:<br>
Acked-by: Elie Tournier <<a href="mailto:elie.tournier@collabora.com" target="_blank">elie.tournier@collabora.com</a>><br></blockquote><div><br></div><div>Thanks! When your tests are complete, I posted a slightly updated series on Gitlab:</div><div><br></div><div><a href="https://gitlab.freedesktop.org/mesa/mesa/merge_requests/25">https://gitlab.freedesktop.org/mesa/mesa/merge_requests/25</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ---<br>
> src/gallium/drivers/virgl/virgl_buffer.c  | 46 +++++++++++-----------<br>
> src/gallium/drivers/virgl/virgl_context.c | 34 +---------------<br>
> src/gallium/drivers/virgl/virgl_context.h | 1 -<br>
>Â src/gallium/drivers/virgl/virgl_resource.h | 13 ------<br>
>Â 4 files changed, 25 insertions(+), 69 deletions(-)<br>
> <br>
> diff --git a/src/gallium/drivers/virgl/virgl_buffer.c b/src/gallium/drivers/virgl/virgl_buffer.c<br>
> index d5d728735e..a20deab549 100644<br>
> --- a/src/gallium/drivers/virgl/virgl_buffer.c<br>
> +++ b/src/gallium/drivers/virgl/virgl_buffer.c<br>
> @@ -33,7 +33,6 @@ static void virgl_buffer_destroy(struct pipe_screen *screen,<br>
>Â Â Â struct virgl_screen *vs = virgl_screen(screen);<br>
>Â Â Â struct virgl_buffer *vbuf = virgl_buffer(buf);<br>
>Â <br>
> -Â Â util_range_destroy(&vbuf->valid_buffer_range);<br>
>Â Â Â vs->vws->resource_unref(vs->vws, vbuf->base.hw_res);<br>
>Â Â Â FREE(vbuf);<br>
>Â }<br>
> @@ -53,7 +52,7 @@ static void *virgl_buffer_transfer_map(struct pipe_context *ctx,<br>
>Â Â Â bool readback;<br>
>Â Â Â bool doflushwait = false;<br>
>Â <br>
> -Â Â if ((usage & PIPE_TRANSFER_READ) && (vbuf->on_list == TRUE))<br>
> +Â Â if (usage & PIPE_TRANSFER_READ)<br>
>Â Â Â Â doflushwait = true;<br>
>Â Â Â else<br>
>Â Â Â Â doflushwait = virgl_res_needs_flush_wait(vctx, &vbuf->base, usage);<br>
> @@ -92,13 +91,19 @@ static void virgl_buffer_transfer_unmap(struct pipe_context *ctx,<br>
>Â Â Â struct virgl_buffer *vbuf = virgl_buffer(transfer->resource);<br>
>Â <br>
>Â Â Â if (trans->base.usage & PIPE_TRANSFER_WRITE) {<br>
> -Â Â Â if (!(transfer->usage & PIPE_TRANSFER_FLUSH_EXPLICIT)) {<br>
> -Â Â Â Â Â struct virgl_screen *vs = virgl_screen(ctx->screen);<br>
> -Â Â Â Â Â vctx->num_transfers++;<br>
> -Â Â Â Â Â vs->vws->transfer_put(vs->vws, vbuf->base.hw_res,<br>
> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &transfer->box, trans->base.stride, trans->base.layer_stride, trans->offset, transfer->level);<br>
> -<br>
> +Â Â Â struct virgl_screen *vs = virgl_screen(ctx->screen);<br>
> +Â Â Â if (transfer->usage & PIPE_TRANSFER_FLUSH_EXPLICIT) {<br>
> +Â Â Â Â Â transfer->box.x += trans->range.start;<br>
> +Â Â Â Â Â transfer->box.width = trans->range.end - trans->range.start;<br>
> +Â Â Â Â Â trans->offset = transfer->box.x;<br>
>Â Â Â Â }<br>
> +<br>
> +Â Â Â vctx->num_transfers++;<br>
> +Â Â Â vs->vws->transfer_put(vs->vws, vbuf->base.hw_res,<br>
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â &transfer->box, trans->base.stride,<br>
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â trans->base.layer_stride, trans->offset,<br>
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â transfer->level);<br>
> +<br>
>Â Â Â }<br>
>Â <br>
>Â Â Â virgl_resource_destroy_transfer(vctx, trans);<br>
> @@ -108,20 +113,19 @@ static void virgl_buffer_transfer_flush_region(struct pipe_context *ctx,<br>
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct pipe_transfer *transfer,<br>
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â const struct pipe_box *box)<br>
>Â {<br>
> -Â Â struct virgl_context *vctx = virgl_context(ctx);<br>
>Â Â Â struct virgl_buffer *vbuf = virgl_buffer(transfer->resource);<br>
> +Â Â struct virgl_transfer *trans = virgl_transfer(transfer);<br>
>Â <br>
> -Â Â if (!vbuf->on_list) {<br>
> -Â Â Â Â struct pipe_resource *res = NULL;<br>
> -<br>
> -Â Â Â Â list_addtail(&vbuf->flush_list, &vctx->to_flush_bufs);<br>
> -Â Â Â Â vbuf->on_list = TRUE;<br>
> -Â Â Â Â pipe_resource_reference(&res, &vbuf->base.u.b);<br>
> -Â Â }<br>
> -<br>
> -Â Â util_range_add(&vbuf->valid_buffer_range, transfer->box.x + box->x,<br>
> -Â Â Â Â Â Â Â Â Â transfer->box.x + box->x + box->width);<br>
> -<br>
> +Â Â /*<br>
> +  * FIXME: This is not optimal. For example,<br>
> +Â Â *<br>
> +Â Â * glMapBufferRange(.., 0, 100, GL_MAP_FLUSH_EXPLICIT_BIT)<br>
> +Â Â * glFlushMappedBufferRange(.., 25, 30)<br>
> +Â Â * glFlushMappedBufferRange(.., 65, 70)<br>
> +Â Â *<br>
> +Â Â * We'll end up flushing 25 --> 70.<br>
> +Â Â */<br>
> +Â Â util_range_add(&trans->range, box->x, box->x + box->width);<br>
>Â Â Â vbuf->base.clean = FALSE;<br>
>Â }<br>
>Â <br>
> @@ -145,7 +149,6 @@ struct pipe_resource *virgl_buffer_create(struct virgl_screen *vs,<br>
>Â Â Â buf->base.u.b.screen = &vs->base;<br>
>Â Â Â buf->base.u.vtbl = &virgl_buffer_vtbl;<br>
>Â Â Â pipe_reference_init(&buf->base.u.b.reference, 1);<br>
> -Â Â util_range_init(&buf->valid_buffer_range);<br>
>Â Â Â virgl_resource_layout(&buf->base.u.b, &buf->metadata);<br>
>Â <br>
>Â Â Â vbind = pipe_to_virgl_bind(template->bind);<br>
> @@ -155,6 +158,5 @@ struct pipe_resource *virgl_buffer_create(struct virgl_screen *vs,<br>
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â template->width0, 1, 1, 1, 0, 0,<br>
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â buf->metadata.total_size);<br>
>Â <br>
> -Â Â util_range_set_empty(&buf->valid_buffer_range);<br>
>Â Â Â return &buf->base.u.b;<br>
>Â }<br>
> diff --git a/src/gallium/drivers/virgl/virgl_context.c b/src/gallium/drivers/virgl/virgl_context.c<br>
> index 90a9429fa5..a92fd6115a 100644<br>
> --- a/src/gallium/drivers/virgl/virgl_context.c<br>
> +++ b/src/gallium/drivers/virgl/virgl_context.c<br>
> @@ -54,29 +54,6 @@ uint32_t virgl_object_assign_handle(void)<br>
>Â Â Â return ++next_handle;<br>
>Â }<br>
>Â <br>
> -static void virgl_buffer_flush(struct virgl_context *vctx,<br>
> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct virgl_buffer *vbuf)<br>
> -{<br>
> -Â Â struct virgl_screen *rs = virgl_screen(vctx->base.screen);<br>
> -Â Â struct pipe_box box;<br>
> -<br>
> -Â Â assert(vbuf->on_list);<br>
> -<br>
> -Â Â box.height = 1;<br>
> -Â Â box.depth = 1;<br>
> -Â Â box.y = 0;<br>
> -Â Â box.z = 0;<br>
> -<br>
> -Â Â box.x = vbuf->valid_buffer_range.start;<br>
> -Â Â box.width = MIN2(vbuf->valid_buffer_range.end - vbuf->valid_buffer_range.start, vbuf->base.u.b.width0);<br>
> -<br>
> -Â Â vctx->num_transfers++;<br>
> -Â Â rs->vws->transfer_put(rs->vws, vbuf->base.hw_res,<br>
> -Â Â Â Â Â Â Â Â Â Â Â Â Â &box, 0, 0, box.x, 0);<br>
> -<br>
> -Â Â util_range_set_empty(&vbuf->valid_buffer_range);<br>
> -}<br>
> -<br>
>Â static void virgl_attach_res_framebuffer(struct virgl_context *vctx)<br>
>Â {<br>
>Â Â Â struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws;<br>
> @@ -736,19 +713,11 @@ static void virgl_flush_from_st(struct pipe_context *ctx,<br>
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â enum pipe_flush_flags flags)<br>
>Â {<br>
>Â Â Â struct virgl_context *vctx = virgl_context(ctx);<br>
> -Â Â struct virgl_buffer *buf, *tmp;<br>
> +Â Â struct virgl_screen *rs = virgl_screen(ctx->screen);<br>
>Â <br>
>Â Â Â if (flags & PIPE_FLUSH_FENCE_FD)<br>
>Â Â Â Â Â vctx->cbuf->needs_out_fence_fd = true;<br>
>Â <br>
> -Â Â LIST_FOR_EACH_ENTRY_SAFE(buf, tmp, &vctx->to_flush_bufs, flush_list) {<br>
> -Â Â Â struct pipe_resource *res = &buf->base.u.b;<br>
> -Â Â Â virgl_buffer_flush(vctx, buf);<br>
> -Â Â Â list_del(&buf->flush_list);<br>
> -Â Â Â buf->on_list = FALSE;<br>
> -Â Â Â pipe_resource_reference(&res, NULL);<br>
> -<br>
> -Â Â }<br>
>Â Â Â virgl_flush_eq(vctx, vctx, fence);<br>
>Â <br>
>Â Â Â if (vctx->cbuf->in_fence_fd != -1) {<br>
> @@ -1291,7 +1260,6 @@ struct pipe_context *virgl_context_create(struct pipe_screen *pscreen,<br>
>Â Â Â virgl_init_query_functions(vctx);<br>
>Â Â Â virgl_init_so_functions(vctx);<br>
>Â <br>
> -Â Â list_inithead(&vctx->to_flush_bufs);<br>
>Â Â Â slab_create_child(&vctx->transfer_pool, &rs->transfer_pool);<br>
>Â <br>
>Â Â Â vctx->primconvert = util_primconvert_create(&vctx->base, rs->caps.caps.v1.prim_mask);<br>
> diff --git a/src/gallium/drivers/virgl/virgl_context.h b/src/gallium/drivers/virgl/virgl_context.h<br>
> index 4a6cc09ee5..65166154e4 100644<br>
> --- a/src/gallium/drivers/virgl/virgl_context.h<br>
> +++ b/src/gallium/drivers/virgl/virgl_context.h<br>
> @@ -73,7 +73,6 @@ struct virgl_context {<br>
>Â Â Â struct pipe_resource *images[PIPE_SHADER_TYPES][PIPE_MAX_SHADER_BUFFERS];<br>
>Â Â Â int num_transfers;<br>
>Â Â Â int num_draws;<br>
> -Â Â struct list_head to_flush_bufs;<br>
>Â <br>
>Â Â Â struct pipe_resource *atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS];<br>
>Â <br>
> diff --git a/src/gallium/drivers/virgl/virgl_resource.h b/src/gallium/drivers/virgl/virgl_resource.h<br>
> index fa985494a7..2e2fa186d1 100644<br>
> --- a/src/gallium/drivers/virgl/virgl_resource.h<br>
> +++ b/src/gallium/drivers/virgl/virgl_resource.h<br>
> @@ -52,19 +52,6 @@ struct virgl_resource {<br>
>Â <br>
>Â struct virgl_buffer {<br>
>Â Â Â struct virgl_resource base;<br>
> -<br>
> -Â Â struct list_head flush_list;<br>
> -Â Â boolean on_list;<br>
> -<br>
> -Â Â /* The buffer range which is initialized (with a write transfer,<br>
> -Â Â * streamout, DMA, or as a random access target). The rest of<br>
> -Â Â * the buffer is considered invalid and can be mapped unsynchronized.<br>
> -Â Â *<br>
> -Â Â * This allows unsychronized mapping of a buffer range which hasn't<br>
> -Â Â * been used yet. It's for applications which forget to use<br>
> -Â Â * the unsynchronized map flag and expect the driver to figure it out.<br>
> -Â Â */<br>
> -Â Â struct util_range valid_buffer_range;<br>
>Â Â Â struct virgl_resource_metadata metadata;<br>
>Â };<br>
>Â <br>
> -- <br>
> 2.18.1<br>
> <br>
> _______________________________________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div></div></div>