Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Xenia Ragiadakou]

On 21/12/23 07:57, Akihiko Odaki wrote:
On 2023/12/19 16:53, Huang Rui wrote:
From: Antonio Caggiano <antonio.caggi...@collabora.com>

Support BLOB resources creation, mapping and unmapping by calling the
new stable virglrenderer 0.10 interface. Only enabled when available and
via the blob config. E.g. -device virtio-vga-gl,blob=true

Signed-off-by: Antonio Caggiano <antonio.caggi...@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipe...@collabora.com>
Signed-off-by: Xenia Ragiadakou <xenia.ragiada...@amd.com>
Signed-off-by: Huang Rui <ray.hu...@amd.com>
---

Changes in v6:
- Use new struct virgl_gpu_resource.
- Unmap, unref and destroy the resource only after the memory region
   has been completely removed.
- In unref check whether the resource is still mapped.
- In unmap_blob check whether the resource has been already unmapped.
- Fix coding style

  hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
  hw/display/virtio-gpu.c       |   4 +-
  meson.build                   |   4 +
  3 files changed, 276 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c 
b/hw/display/virtio-gpu-virgl.c
index faab374336..5a3a292f79 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
  #include "trace.h"
  #include "hw/virtio/virtio.h"
  #include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
  #include "ui/egl-helpers.h"
@@ -24,8 +25,62 @@
  struct virgl_gpu_resource {
      struct virtio_gpu_simple_resource res;
+    uint32_t ref;
+    VirtIOGPU *g;
+
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    /* only blob resource needs this region to be mapped as guest 
mmio */
+    MemoryRegion *region;

Why not just embed MemoryRegion into struct virgl_gpu_resource instead 
of having a pointer?

To decouple memory region from the resource. Since there are different 
commands for creating and mapping maybe there is the intention to map 
the same resource multiple times. If the lifecycle of the resource is 
tightly coupled to the lifecycle of the memory region then the memory 
region could be embedded into the struct. Ray could give more insight.


+#endif
  };
+static void vres_get_ref(struct virgl_gpu_resource *vres)
+{
+    uint32_t ref;
+
+    ref = qatomic_fetch_inc(&vres->ref);
+    g_assert(ref < INT_MAX);
+}
+
+static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
+{
+    struct virtio_gpu_simple_resource *res;
+    VirtIOGPU *g;
+
+    if (!vres) {
+        return;
+    }
+
+    g = vres->g;
+    res = &vres->res;
+    QTAILQ_REMOVE(&g->reslist, res, next);
+    virtio_gpu_cleanup_mapping(g, res);
+    g_free(vres);
+}
+
+static void virgl_resource_unref(struct virgl_gpu_resource *vres)
+{
+    struct virtio_gpu_simple_resource *res;
+
+    if (!vres) {
+        return;
+    }
+
+    res = &vres->res;
+    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
+    virgl_renderer_resource_unref(res->resource_id);
+}
+
+static void vres_put_ref(struct virgl_gpu_resource *vres)
+{
+    g_assert(vres->ref > 0);
+
+    if (qatomic_fetch_dec(&vres->ref) == 1) {
+        virgl_resource_unref(vres);
+        virgl_resource_destroy(vres);
+    }
+}
+
  static struct virgl_gpu_resource *
  virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
  {
@@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
                                         c2d.width, c2d.height);
      vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
      vres->res.width = c2d.width;
      vres->res.height = c2d.height;
      vres->res.format = c2d.format;
@@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
                                         c3d.width, c3d.height, 
c3d.depth);
      vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
      vres->res.width = c3d.width;
      vres->res.height = c3d.height;
      vres->res.format = c3d.format;
@@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
          return;
      }
-    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
-    virgl_renderer_resource_unref(unref.resource_id);
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    if (vres->region) {
+        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+        MemoryRegion *mr = vres->region;
+
+        warn_report("%s: blob resource %d not unmapped",
+                    __func__, unref.resource_id);
+        vres->region = NULL;
+        memory_region_set_enabled(mr, false);
+        memory_region_del_subregion(&b->hostmem, mr);
+        object_unparent(OBJECT(mr));
+    }
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
-    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
-    virtio_gpu_cleanup_mapping(g, &vres->res);
-    g_free(vres);
+    vres_put_ref(vres);

What will happen if the guest consecutively requests 
VIRTIO_GPU_CMD_RESOURCE_UNREF twice for a mapped resource?

You are right. I think the resource needs to be removed from the list 
synchronously on first unref.