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.caggiano@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.caggiano@collabora.com>
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> > Signed-off-by: Huang Rui <ray.huang@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.