virgl: fix a sync issue in virgl_buffer_transfer_extend

In virgl_buffer_transfer_extend, when no flush is needed, it tries
to extend a previously queued transfer instead if it can find one.
Comparing to virgl_resource_transfer_prepare, it fails to check if
the resource is busy.

The existence of a previously queued transfer normally implies that
the resource is not busy, maybe except for when the transfer is
PIPE_TRANSFER_UNSYNCHRONIZED.  Rather than burdening us with a
lengthy comment, and potential concerns over breaking it as the
transfer code evolves, this commit makes the valid_buffer_range
check the only condition to take the fast path.

In real world, we hit the fast path almost only because of the
valid_buffer_range check.  In micro benchmarks, the condition should
always be true, otherwise the benchmarks are not very representative
of meaningful workloads.  I think this fix is justified.

The recent change to PIPE_TRANSFER_MAP_DIRECTLY usage disables the
fast path.  This commit re-enables it as well.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>

diff --git a/src/gallium/drivers/virgl/virgl_resource.c b/src/gallium/drivers/virgl/virgl_resource.c
index dcc0bce717cbeccb067905a7a1d33a64aa0935e2..74da5ef968cc7b330350563d1ae066b665a8fc3a 100644 (file)
--- a/src/gallium/drivers/virgl/virgl_resource.c
+++ b/src/gallium/drivers/virgl/virgl_resource.c
@@ -536,76 +536,29 @@ void virgl_init_screen_resource_functions(struct pipe_screen *screen)
     screen->resource_destroy = u_resource_destroy_vtbl;
 }
 
-static bool virgl_buffer_transfer_extend(struct pipe_context *ctx,
-                                         struct pipe_resource *resource,
-                                         unsigned usage,
-                                         const struct pipe_box *box,
-                                         const void *data)
-{
-   struct virgl_context *vctx = virgl_context(ctx);
-   struct virgl_resource *vbuf = virgl_resource(resource);
-   struct virgl_transfer dummy_trans = { 0 };
-   bool flush;
-
-   /*
-    * Attempts to short circuit the entire process of mapping and unmapping
-    * a resource if there is an existing transfer that can be extended.
-    * Pessimestically falls back if a flush is required.
-    */
-   dummy_trans.base.resource = resource;
-   dummy_trans.hw_res = vbuf->hw_res;
-   dummy_trans.base.usage = usage;
-   dummy_trans.base.box = *box;
-   dummy_trans.base.stride = vbuf->metadata.stride[0];
-   dummy_trans.base.layer_stride = vbuf->metadata.layer_stride[0];
-   dummy_trans.offset = box->x;
-
-   flush = virgl_res_needs_flush(vctx, &dummy_trans);
-   if (flush && util_ranges_intersect(&vbuf->valid_buffer_range,
-                                      box->x, box->x + box->width))
-      return false;
-
-   if (!virgl_transfer_queue_extend_buffer(&vctx->queue,
-            vbuf->hw_res, box->x, box->width, data))
-      return false;
-
-   util_range_add(&vbuf->valid_buffer_range, box->x, box->x + box->width);
-
-   return true;
-}
-
 static void virgl_buffer_subdata(struct pipe_context *pipe,
                                  struct pipe_resource *resource,
                                  unsigned usage, unsigned offset,
                                  unsigned size, const void *data)
 {
-   struct pipe_transfer *transfer;
-   uint8_t *map;
-   struct pipe_box box;
-
-   assert(!(usage & PIPE_TRANSFER_READ));
-
-   /* the write flag is implicit by the nature of buffer_subdata */
-   usage |= PIPE_TRANSFER_WRITE;
-
-   if (!(usage & PIPE_TRANSFER_MAP_DIRECTLY)) {
-      if (offset == 0 && size == resource->width0)
-         usage |= PIPE_TRANSFER_DISCARD_WHOLE_RESOURCE;
-      else
-         usage |= PIPE_TRANSFER_DISCARD_RANGE;
-   }
-
-   u_box_1d(offset, size, &box);
+   struct virgl_context *vctx = virgl_context(pipe);
+   struct virgl_resource *vbuf = virgl_resource(resource);
 
-   if (usage & PIPE_TRANSFER_DISCARD_RANGE &&
-       virgl_buffer_transfer_extend(pipe, resource, usage, &box, data))
+   /* We can try virgl_transfer_queue_extend_buffer when there is no
+    * flush/readback/wait required.  Based on virgl_resource_transfer_prepare,
+    * the simplest way to make sure that is the case is to check the valid
+    * buffer range.
+    */
+   if (!util_ranges_intersect(&vbuf->valid_buffer_range,
+                              offset, offset + size) &&
+       likely(!(virgl_debug & VIRGL_DEBUG_XFER)) &&
+       virgl_transfer_queue_extend_buffer(&vctx->queue,
+                                          vbuf->hw_res, offset, size, data)) {
+      util_range_add(&vbuf->valid_buffer_range, offset, offset + size);
       return;
-
-   map = pipe->transfer_map(pipe, resource, 0, usage, &box, &transfer);
-   if (map) {
-      memcpy(map, data, size);
-      pipe_transfer_unmap(pipe, transfer);
    }
+
+   u_default_buffer_subdata(pipe, resource, usage, offset, size, data);
 }
 
 void virgl_init_context_resource_functions(struct pipe_context *ctx)