[QEMU,v4,10/13] virtio-gpu: Resource UUID

Message ID	20230831093252.2461282-11-ray.huang@amd.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org> Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C From: Huang Rui <ray.huang@amd.com> To: Gerd Hoffmann <kraxel@redhat.com>, "Michael S . Tsirkin" <mst@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony.perard@citrix.com>, Antonio Caggiano <antonio.caggiano@collabora.com>, "Dr . David Alan Gilbert" <dgilbert@redhat.com>, Robert Beckett <bob.beckett@collabora.com>, "Dmitry Osipenko" <dmitry.osipenko@collabora.com>, =?utf-8?q?Alex_Benn?= =?utf-8?q?=C3=A9e?= <alex.bennee@linaro.org>, <qemu-devel@nongnu.org> CC: <xen-devel@lists.xenproject.org>, Gurchetan Singh <gurchetansingh@chromium.org>, <ernunes@redhat.com>, =?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= <philmd@linaro.org>, Akihiko Odaki <akihiko.odaki@daynix.com>, Alyssa Ross <hi@alyssa.is>, =?utf-8?q?Roger_Pau_Monn=C3=A9?= <roger.pau@citrix.com>, Alex Deucher <alexander.deucher@amd.com>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, Xenia Ragiadakou <xenia.ragiadakou@amd.com>, Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>, "Honglei Huang" <honglei1.huang@amd.com>, Julia Zhang <julia.zhang@amd.com>, "Chen Jiqian" <Jiqian.Chen@amd.com>, Huang Rui <ray.huang@amd.com> Subject: [QEMU PATCH v4 10/13] virtio-gpu: Resource UUID Date: Thu, 31 Aug 2023 17:32:49 +0800 Message-ID: <20230831093252.2461282-11-ray.huang@amd.com> In-Reply-To: <20230831093252.2461282-1-ray.huang@amd.com> References: <20230831093252.2461282-1-ray.huang@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Received-SPF: softfail client-ip=2a01:111:f400:7e88::621; envelope-from=Ray.Huang@amd.com; helo=NAM10-DM6-obe.outbound.protection.outlook.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action Precedence: list Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org
Series	Support blob memory and venus on qemu \| expand [QEMU,v4,00/13] Support blob memory and venus on qemu [QEMU,v4,01/13] virtio: Add shared memory capability [QEMU,v4,02/13] virtio-gpu: CONTEXT_INIT feature [QEMU,v4,03/13] virtio-gpu: hostmem [QEMU,v4,04/13] virtio-gpu: blob prep [QEMU,v4,05/13] virtio-gpu: Support context init feature with virglrenderer [QEMU,v4,06/13] virtio-gpu: Configure context init for virglrenderer [QEMU,v4,07/13] softmmu/memory: enable automatic deallocation of memory regions [QEMU,v4,08/13] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled [QEMU,v4,09/13] virtio-gpu: Handle resource blob commands [QEMU,v4,10/13] virtio-gpu: Resource UUID [QEMU,v4,11/13] virtio-gpu: Support Venus capset [QEMU,v4,12/13] virtio-gpu: Initialize Venus [QEMU,v4,13/13] virtio-gpu: Enable virglrenderer render server flag for venus

Huang Rui Aug. 31, 2023, 9:32 a.m. UTC

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Enable resource UUID feature and implement command resource assign UUID.
This is done by introducing a hash table to map resource IDs to their
UUIDs.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

v1->v2:
   - Separate declarations from code.

 hw/display/trace-events        |  1 +
 hw/display/virtio-gpu-base.c   |  2 ++
 hw/display/virtio-gpu-virgl.c  | 21 +++++++++++++++++
 hw/display/virtio-gpu.c        | 41 ++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-gpu.h |  4 ++++
 5 files changed, 69 insertions(+)

Akihiko Odaki Aug. 31, 2023, 10:36 a.m. UTC | #1

On 2023/08/31 18:32, Huang Rui wrote:
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> Enable resource UUID feature and implement command resource assign UUID.
> This is done by introducing a hash table to map resource IDs to their
> UUIDs.

The hash table does not seem to be stored during migration.

> 
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> v1->v2:
>     - Separate declarations from code.
> 
>   hw/display/trace-events        |  1 +
>   hw/display/virtio-gpu-base.c   |  2 ++
>   hw/display/virtio-gpu-virgl.c  | 21 +++++++++++++++++
>   hw/display/virtio-gpu.c        | 41 ++++++++++++++++++++++++++++++++++
>   include/hw/virtio/virtio-gpu.h |  4 ++++
>   5 files changed, 69 insertions(+)
> 
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 2336a0ca15..54d6894c59 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
>   virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index 4f2b0ba1f3..f44388715c 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>           features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
>       }
>   
> +    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> +
>       return features;
>   }
>   
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 17b634d4ee..1a996a08fc 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -36,6 +36,7 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>   {
>       struct virtio_gpu_resource_create_2d c2d;
>       struct virgl_renderer_resource_create_args args;
> +    struct virtio_gpu_simple_resource *res;
>   
>       VIRTIO_GPU_FILL_CMD(c2d);
>       trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
> @@ -53,6 +54,14 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>       args.nr_samples = 0;
>       args.flags = VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP;
>       virgl_renderer_resource_create(&args, NULL, 0);
> +
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;

virglrenderer thinks the resource is alive in such a situation.

> +    }
> +    res->resource_id = c2d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>   }
>   
>   static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> @@ -60,6 +69,7 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>   {
>       struct virtio_gpu_resource_create_3d c3d;
>       struct virgl_renderer_resource_create_args args;
> +    struct virtio_gpu_simple_resource *res;
>   
>       VIRTIO_GPU_FILL_CMD(c3d);
>       trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
> @@ -77,6 +87,14 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>       args.nr_samples = c3d.nr_samples;
>       args.flags = c3d.flags;
>       virgl_renderer_resource_create(&args, NULL, 0);
> +
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    res->resource_id = c3d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>   }
>   
>   static void virgl_resource_destroy(VirtIOGPU *g,
> @@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>           /* TODO add security */
>           virgl_cmd_ctx_detach_resource(g, cmd);
>           break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>       case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
>           virgl_cmd_get_capset_info(g, cmd);
>           break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index cc4c1f81bb..770e4747e3 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -966,6 +966,37 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
>       virtio_gpu_cleanup_mapping(g, res);
>   }
>   
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_assign_uuid assign;
> +    struct virtio_gpu_resp_resource_uuid resp;
> +    QemuUUID *uuid = NULL;

This initialization is unnecessary.

> +
> +    VIRTIO_GPU_FILL_CMD(assign);
> +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> +
> +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> +    if (!res) {
> +        return;
> +    }
> +
> +    memset(&resp, 0, sizeof(resp));
> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> +
> +    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
> +    if (!uuid) {
> +        uuid = g_new(QemuUUID, 1);
> +        qemu_uuid_generate(uuid);
> +        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
> +    }
> +
> +    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> +}
> +
>   void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>                                      struct virtio_gpu_ctrl_command *cmd)
>   {
> @@ -1014,6 +1045,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>       case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>           virtio_gpu_resource_detach_backing(g, cmd);
>           break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>       default:
>           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>           break;
> @@ -1393,12 +1427,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>       QTAILQ_INIT(&g->reslist);
>       QTAILQ_INIT(&g->cmdq);
>       QTAILQ_INIT(&g->fenceq);
> +
> +    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>   }
>   
>   static void virtio_gpu_device_unrealize(DeviceState *qdev)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>   
> +    g_hash_table_destroy(g->resource_uuids);
>       g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
>       g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
>       g_clear_pointer(&g->reset_bh, qemu_bh_delete);
> @@ -1452,6 +1489,10 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>           g_free(cmd);
>       }
>   
> +    if (g->resource_uuids) {

Isn't g->resource_uuids always non-NULL?

> +        g_hash_table_remove_all(g->resource_uuids);
> +    }
> +
>       virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
>   }
>   
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index b9adc28071..aa94b1b697 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -208,6 +208,8 @@ struct VirtIOGPU {
>           QTAILQ_HEAD(, VGPUDMABuf) bufs;
>           VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
>       } dmabuf;
> +
> +    GHashTable *resource_uuids;
>   };
>   
>   struct VirtIOGPUClass {
> @@ -285,6 +287,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
>                                       struct iovec *iov, uint32_t count);
>   void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
>                                   struct virtio_gpu_simple_resource *res);
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd);
>   void virtio_gpu_process_cmdq(VirtIOGPU *g);
>   void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
>   void virtio_gpu_reset(VirtIODevice *vdev);

Huang Rui Sept. 9, 2023, 9:09 a.m. UTC | #2

On Thu, Aug 31, 2023 at 06:36:57PM +0800, Akihiko Odaki wrote:
> On 2023/08/31 18:32, Huang Rui wrote:
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> > 
> > Enable resource UUID feature and implement command resource assign UUID.
> > This is done by introducing a hash table to map resource IDs to their
> > UUIDs.
> 
> The hash table does not seem to be stored during migration.

May I know whether you point g->resource_uuids table data in VirtIOGPU
device should be stored in virtio_gpu_save() and resumed in
virtio_gpu_load() for virtio migration?

> 
> > 
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > v1->v2:
> >     - Separate declarations from code.
> > 
> >   hw/display/trace-events        |  1 +
> >   hw/display/virtio-gpu-base.c   |  2 ++
> >   hw/display/virtio-gpu-virgl.c  | 21 +++++++++++++++++
> >   hw/display/virtio-gpu.c        | 41 ++++++++++++++++++++++++++++++++++
> >   include/hw/virtio/virtio-gpu.h |  4 ++++
> >   5 files changed, 69 insertions(+)
> > 
> > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > index 2336a0ca15..54d6894c59 100644
> > --- a/hw/display/trace-events
> > +++ b/hw/display/trace-events
> > @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
> >   virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> > +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> > diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> > index 4f2b0ba1f3..f44388715c 100644
> > --- a/hw/display/virtio-gpu-base.c
> > +++ b/hw/display/virtio-gpu-base.c
> > @@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
> >           features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
> >       }
> >   
> > +    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> > +
> >       return features;
> >   }
> >   
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 17b634d4ee..1a996a08fc 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -36,6 +36,7 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >   {
> >       struct virtio_gpu_resource_create_2d c2d;
> >       struct virgl_renderer_resource_create_args args;
> > +    struct virtio_gpu_simple_resource *res;
> >   
> >       VIRTIO_GPU_FILL_CMD(c2d);
> >       trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
> > @@ -53,6 +54,14 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >       args.nr_samples = 0;
> >       args.flags = VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP;
> >       virgl_renderer_resource_create(&args, NULL, 0);
> > +
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> 
> virglrenderer thinks the resource is alive in such a situation.

Yes, so we can move the resource allocation in front of below virglrenderer
resource creation.

virgl_renderer_resource_create(&args, NULL, 0);

> 
> > +    }
> > +    res->resource_id = c2d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> >   }
> >   
> >   static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> > @@ -60,6 +69,7 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >   {
> >       struct virtio_gpu_resource_create_3d c3d;
> >       struct virgl_renderer_resource_create_args args;
> > +    struct virtio_gpu_simple_resource *res;
> >   
> >       VIRTIO_GPU_FILL_CMD(c3d);
> >       trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
> > @@ -77,6 +87,14 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >       args.nr_samples = c3d.nr_samples;
> >       args.flags = c3d.flags;
> >       virgl_renderer_resource_create(&args, NULL, 0);
> > +
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    res->resource_id = c3d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> >   }
> >   
> >   static void virgl_resource_destroy(VirtIOGPU *g,
> > @@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >           /* TODO add security */
> >           virgl_cmd_ctx_detach_resource(g, cmd);
> >           break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >       case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
> >           virgl_cmd_get_capset_info(g, cmd);
> >           break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index cc4c1f81bb..770e4747e3 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -966,6 +966,37 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
> >       virtio_gpu_cleanup_mapping(g, res);
> >   }
> >   
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_assign_uuid assign;
> > +    struct virtio_gpu_resp_resource_uuid resp;
> > +    QemuUUID *uuid = NULL;
> 
> This initialization is unnecessary.

Yes, will remove it.

> 
> > +
> > +    VIRTIO_GPU_FILL_CMD(assign);
> > +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> > +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> > +
> > +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> > +    if (!res) {
> > +        return;
> > +    }
> > +
> > +    memset(&resp, 0, sizeof(resp));
> > +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> > +
> > +    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
> > +    if (!uuid) {
> > +        uuid = g_new(QemuUUID, 1);
> > +        qemu_uuid_generate(uuid);
> > +        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
> > +    }
> > +
> > +    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> > +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> > +}
> > +
> >   void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >                                      struct virtio_gpu_ctrl_command *cmd)
> >   {
> > @@ -1014,6 +1045,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >       case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
> >           virtio_gpu_resource_detach_backing(g, cmd);
> >           break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >       default:
> >           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >           break;
> > @@ -1393,12 +1427,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> >       QTAILQ_INIT(&g->reslist);
> >       QTAILQ_INIT(&g->cmdq);
> >       QTAILQ_INIT(&g->fenceq);
> > +
> > +    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
> >   }
> >   
> >   static void virtio_gpu_device_unrealize(DeviceState *qdev)
> >   {
> >       VirtIOGPU *g = VIRTIO_GPU(qdev);
> >   
> > +    g_hash_table_destroy(g->resource_uuids);
> >       g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
> >       g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
> >       g_clear_pointer(&g->reset_bh, qemu_bh_delete);
> > @@ -1452,6 +1489,10 @@ void virtio_gpu_reset(VirtIODevice *vdev)
> >           g_free(cmd);
> >       }
> >   
> > +    if (g->resource_uuids) {
> 
> Isn't g->resource_uuids always non-NULL?

Yes, right. The if check should be superfluous. Will remove it.

Thanks,
Ray

> 
> > +        g_hash_table_remove_all(g->resource_uuids);
> > +    }
> > +
> >       virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
> >   }
> >   
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index b9adc28071..aa94b1b697 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -208,6 +208,8 @@ struct VirtIOGPU {
> >           QTAILQ_HEAD(, VGPUDMABuf) bufs;
> >           VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
> >       } dmabuf;
> > +
> > +    GHashTable *resource_uuids;
> >   };
> >   
> >   struct VirtIOGPUClass {
> > @@ -285,6 +287,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
> >                                       struct iovec *iov, uint32_t count);
> >   void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
> >                                   struct virtio_gpu_simple_resource *res);
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd);
> >   void virtio_gpu_process_cmdq(VirtIOGPU *g);
> >   void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
> >   void virtio_gpu_reset(VirtIODevice *vdev);

Akihiko Odaki Sept. 10, 2023, 4:21 a.m. UTC | #3

On 2023/09/09 18:09, Huang Rui wrote:
> On Thu, Aug 31, 2023 at 06:36:57PM +0800, Akihiko Odaki wrote:
>> On 2023/08/31 18:32, Huang Rui wrote:
>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>
>>> Enable resource UUID feature and implement command resource assign UUID.
>>> This is done by introducing a hash table to map resource IDs to their
>>> UUIDs.
>>
>> The hash table does not seem to be stored during migration.
> 
> May I know whether you point g->resource_uuids table data in VirtIOGPU
> device should be stored in virtio_gpu_save() and resumed in
> virtio_gpu_load() for virtio migration?

Yes, that's what I meant.

Regards,
Akihiko Odaki

Albert Esteve Sept. 13, 2023, 7:55 a.m. UTC | #4

Hi Antonio,

If I'm not mistaken, this patch is related with:
https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
IMHO, ideally, virtio-gpu and vhost-user-gpu both, would use the
infrastructure from the patch I linked to store the
virtio objects, so that they can be later shared with other devices.

Which, in terms of code, would mean changing:
g_hash_table_insert(g->resource_uuids,
GUINT_TO_POINTER(assign.resource_id), uuid);
by:
virtio_add_dmabuf(uuid, assign.resource_id);

...and simplify part of the infrastructure.

Let me know what you think.

Regard,
Albert

Akihiko Odaki Sept. 13, 2023, 10:34 a.m. UTC | #5

On 2023/09/13 16:55, Albert Esteve wrote:
> Hi Antonio,
> 
> If I'm not mistaken, this patch is related with: 
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html 
> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
> IMHO, ideally, virtio-gpu and vhost-user-gpu both, would use the 
> infrastructure from the patch I linked to store the
> virtio objects, so that they can be later shared with other devices.

I don't think such sharing is possible because the resources are 
identified by IDs that are local to the device. That also complicates 
migration.

Regards,
Akihiko Odaki

Albert Esteve Sept. 13, 2023, 11:34 a.m. UTC | #6

On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2023/09/13 16:55, Albert Esteve wrote:
> > Hi Antonio,
> >
> > If I'm not mistaken, this patch is related with:
> > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> > <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
> > IMHO, ideally, virtio-gpu and vhost-user-gpu both, would use the
> > infrastructure from the patch I linked to store the
> > virtio objects, so that they can be later shared with other devices.
>
> I don't think such sharing is possible because the resources are
> identified by IDs that are local to the device. That also complicates
> migration.
>
> Regards,
> Akihiko Odaki
>
> Hi Akihiko,

As far as I understand, the feature to export dma-bufs from the
virtgpu was introduced as part of the virtio cross-device sharing
proposal [1]. Thus, it shall be posible. When virtgpu ASSING_UUID,
it exports and identifies the dmabuf resource, so that when the dmabuf gets
shared inside the guest (e.g., with virtio-video), we can use the assigned
UUID to find the dmabuf in the host (using the patch that I linked above),
and import it.

[1] - https://lwn.net/Articles/828988/

Akihiko Odaki Sept. 13, 2023, 12:22 p.m. UTC | #7

On 2023/09/13 20:34, Albert Esteve wrote:
> 
> 
> On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2023/09/13 16:55, Albert Esteve wrote:
>      > Hi Antonio,
>      >
>      > If I'm not mistaken, this patch is related with:
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>      >
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      > IMHO, ideally, virtio-gpu and vhost-user-gpu both, would use the
>      > infrastructure from the patch I linked to store the
>      > virtio objects, so that they can be later shared with other devices.
> 
>     I don't think such sharing is possible because the resources are
>     identified by IDs that are local to the device. That also complicates
>     migration.
> 
>     Regards,
>     Akihiko Odaki
> 
> Hi Akihiko,
> 
> As far as I understand, the feature to export dma-bufs from the
> virtgpu was introduced as part of the virtio cross-device sharing
> proposal [1]. Thus, it shall be posible. When virtgpu ASSING_UUID,
> it exports and identifies the dmabuf resource, so that when the dmabuf gets
> shared inside the guest (e.g., with virtio-video), we can use the assigned
> UUID to find the dmabuf in the host (using the patch that I linked above),
> and import it.
> 
> [1] - https://lwn.net/Articles/828988/ <https://lwn.net/Articles/828988/>

The problem is that virtio-gpu can have other kind of resources like 
pixman and OpenGL textures and manage them and DMA-BUFs with unified 
resource ID.

So you cannot change:
g_hash_table_insert(g->resource_uuids, 
GUINT_TO_POINTER(assign.resource_id), uuid);
by:
virtio_add_dmabuf(uuid, assign.resource_id);

assign.resource_id is not DMA-BUF file descriptor, and the underlying 
resource my not be DMA-BUF at first place.

Also, since this lives in the common code that is not used only by 
virtio-gpu-gl but also virtio-gpu, which supports migration, we also 
need to take care of that. It is not a problem for DMA-BUF as DMA-BUF is 
not migratable anyway, but the situation is different in this case.

Implementing cross-device sharing is certainly a possibility, but that 
requires more than dealing with DMA-BUFs.

Albert Esteve Sept. 13, 2023, 12:58 p.m. UTC | #8

On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2023/09/13 20:34, Albert Esteve wrote:
> >
> >
> > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki <akihiko.odaki@daynix.com
> > <mailto:akihiko.odaki@daynix.com>> wrote:
> >
> >     On 2023/09/13 16:55, Albert Esteve wrote:
> >      > Hi Antonio,
> >      >
> >      > If I'm not mistaken, this patch is related with:
> >      >
> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >
> >      >
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >>
> >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both, would use the
> >      > infrastructure from the patch I linked to store the
> >      > virtio objects, so that they can be later shared with other
> devices.
> >
> >     I don't think such sharing is possible because the resources are
> >     identified by IDs that are local to the device. That also complicates
> >     migration.
> >
> >     Regards,
> >     Akihiko Odaki
> >
> > Hi Akihiko,
> >
> > As far as I understand, the feature to export dma-bufs from the
> > virtgpu was introduced as part of the virtio cross-device sharing
> > proposal [1]. Thus, it shall be posible. When virtgpu ASSING_UUID,
> > it exports and identifies the dmabuf resource, so that when the dmabuf
> gets
> > shared inside the guest (e.g., with virtio-video), we can use the
> assigned
> > UUID to find the dmabuf in the host (using the patch that I linked
> above),
> > and import it.
> >
> > [1] - https://lwn.net/Articles/828988/ <https://lwn.net/Articles/828988/
> >
>
> The problem is that virtio-gpu can have other kind of resources like
> pixman and OpenGL textures and manage them and DMA-BUFs with unified
> resource ID.
>

I see.


>
> So you cannot change:
> g_hash_table_insert(g->resource_uuids,
> GUINT_TO_POINTER(assign.resource_id), uuid);
> by:
> virtio_add_dmabuf(uuid, assign.resource_id);
>
> assign.resource_id is not DMA-BUF file descriptor, and the underlying
> resource my not be DMA-BUF at first place.
>

I didn't really look into the patch in-depth, so the code was intended
to give an idea of how the implementation would look like with
the cross-device patch API. Indeed, it is not the resource_id,
(I just took a brief look at the virtio specificacion 1.2), but the
underlying
resource what we want to use here.


>
> Also, since this lives in the common code that is not used only by
> virtio-gpu-gl but also virtio-gpu, which supports migration, we also
> need to take care of that. It is not a problem for DMA-BUF as DMA-BUF is
> not migratable anyway, but the situation is different in this case.
>
> Implementing cross-device sharing is certainly a possibility, but that
> requires more than dealing with DMA-BUFs.
>
>
So, if I understood correctly, dmabufs are just a subset of the resources
that the gpu manages, or can assign UUIDs to. I am not sure why
the virt gpu driver would want to send a ASSIGN_UUID for anything
that is not a dmabuf (are we sure it does?), but I am not super familiarized
with virtgpu to begin with.
But I see that internally, the GPU specs relate a UUID with a resource_id,
so we still need both tables:
- one to relate UUID with resource_id to be able to locate the underlying
resource
- the table that holds the dmabuf with the UUID for cross-device sharing

With that in mind, sounds to me that the support for cross-device sharing
could
be added on top of this patch, once
https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
lands.

I hope that makes sense.
Regards,
Albert

Akihiko Odaki Sept. 13, 2023, 1:43 p.m. UTC | #9

On 2023/09/13 21:58, Albert Esteve wrote:
> 
> 
> On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2023/09/13 20:34, Albert Esteve wrote:
>      >
>      >
>      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
>     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>      > <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>> wrote:
>      >
>      >     On 2023/09/13 16:55, Albert Esteve wrote:
>      >      > Hi Antonio,
>      >      >
>      >      > If I'm not mistaken, this patch is related with:
>      >      >
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      >      >
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
>      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both, would
>     use the
>      >      > infrastructure from the patch I linked to store the
>      >      > virtio objects, so that they can be later shared with
>     other devices.
>      >
>      >     I don't think such sharing is possible because the resources are
>      >     identified by IDs that are local to the device. That also
>     complicates
>      >     migration.
>      >
>      >     Regards,
>      >     Akihiko Odaki
>      >
>      > Hi Akihiko,
>      >
>      > As far as I understand, the feature to export dma-bufs from the
>      > virtgpu was introduced as part of the virtio cross-device sharing
>      > proposal [1]. Thus, it shall be posible. When virtgpu ASSING_UUID,
>      > it exports and identifies the dmabuf resource, so that when the
>     dmabuf gets
>      > shared inside the guest (e.g., with virtio-video), we can use the
>     assigned
>      > UUID to find the dmabuf in the host (using the patch that I
>     linked above),
>      > and import it.
>      >
>      > [1] - https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/> <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>
> 
>     The problem is that virtio-gpu can have other kind of resources like
>     pixman and OpenGL textures and manage them and DMA-BUFs with unified
>     resource ID.
> 
> 
> I see.
> 
> 
>     So you cannot change:
>     g_hash_table_insert(g->resource_uuids,
>     GUINT_TO_POINTER(assign.resource_id), uuid);
>     by:
>     virtio_add_dmabuf(uuid, assign.resource_id);
> 
>     assign.resource_id is not DMA-BUF file descriptor, and the underlying
>     resource my not be DMA-BUF at first place.
> 
> 
> I didn't really look into the patch in-depth, so the code was intended
> to give an idea of how the implementation would look like with
> the cross-device patch API. Indeed, it is not the resource_id,
> (I just took a brief look at the virtio specificacion 1.2), but the 
> underlying
> resource what we want to use here.
> 
> 
>     Also, since this lives in the common code that is not used only by
>     virtio-gpu-gl but also virtio-gpu, which supports migration, we also
>     need to take care of that. It is not a problem for DMA-BUF as
>     DMA-BUF is
>     not migratable anyway, but the situation is different in this case.
> 
>     Implementing cross-device sharing is certainly a possibility, but that
>     requires more than dealing with DMA-BUFs.
> 
> 
> So, if I understood correctly, dmabufs are just a subset of the resources
> that the gpu manages, or can assign UUIDs to. I am not sure why
> the virt gpu driver would want to send a ASSIGN_UUID for anything
> that is not a dmabuf (are we sure it does?), but I am not super familiarized
> with virtgpu to begin with.

In my understanding, an resource will be first created as OpenGL or 
Vulkan textures and then exported as a DMA-BUF file descriptor. For 
these resource types exporting/importing code is mandatory.

For pixman buffers (i.e., non-virgl device), I don't see a compelling 
reason to have cross-device sharing. It is possible to omit resource 
UUID feature from non-virgl device to avoid implementing complicated 
migration.

> But I see that internally, the GPU specs relate a UUID with a resource_id,
> so we still need both tables:
> - one to relate UUID with resource_id to be able to locate the 
> underlying resource
> - the table that holds the dmabuf with the UUID for cross-device sharing
> 
> With that in mind, sounds to me that the support for cross-device 
> sharing could
> be added on top of this patch, once 
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html 
> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
> lands.

That is possible, but I think it's better to implement cross-device 
sharing at the same time introducing virtio-dmabuf.

The current design of virtio-dmabuf looks somewhat inconsistent; it's 
named "dmabuf", but internally the UUIDs are stored into something named 
"resource_uuids" and it has SharedObjectType so it's more like a generic 
resource sharing mechanism. If you actually have an implementation of 
cross-device sharing using virtio-dmabuf, it will be clear what kind of 
feature is truly necessary.

Regards,
Akihiko Odaki

Albert Esteve Sept. 13, 2023, 2:18 p.m. UTC | #10

On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2023/09/13 21:58, Albert Esteve wrote:
> >
> >
> > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki <akihiko.odaki@daynix.com
> > <mailto:akihiko.odaki@daynix.com>> wrote:
> >
> >     On 2023/09/13 20:34, Albert Esteve wrote:
> >      >
> >      >
> >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
> >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
> >      > <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>> wrote:
> >      >
> >      >     On 2023/09/13 16:55, Albert Esteve wrote:
> >      >      > Hi Antonio,
> >      >      >
> >      >      > If I'm not mistaken, this patch is related with:
> >      >      >
> >      >
> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
> >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both, would
> >     use the
> >      >      > infrastructure from the patch I linked to store the
> >      >      > virtio objects, so that they can be later shared with
> >     other devices.
> >      >
> >      >     I don't think such sharing is possible because the resources
> are
> >      >     identified by IDs that are local to the device. That also
> >     complicates
> >      >     migration.
> >      >
> >      >     Regards,
> >      >     Akihiko Odaki
> >      >
> >      > Hi Akihiko,
> >      >
> >      > As far as I understand, the feature to export dma-bufs from the
> >      > virtgpu was introduced as part of the virtio cross-device sharing
> >      > proposal [1]. Thus, it shall be posible. When virtgpu ASSING_UUID,
> >      > it exports and identifies the dmabuf resource, so that when the
> >     dmabuf gets
> >      > shared inside the guest (e.g., with virtio-video), we can use the
> >     assigned
> >      > UUID to find the dmabuf in the host (using the patch that I
> >     linked above),
> >      > and import it.
> >      >
> >      > [1] - https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/> <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>
> >
> >     The problem is that virtio-gpu can have other kind of resources like
> >     pixman and OpenGL textures and manage them and DMA-BUFs with unified
> >     resource ID.
> >
> >
> > I see.
> >
> >
> >     So you cannot change:
> >     g_hash_table_insert(g->resource_uuids,
> >     GUINT_TO_POINTER(assign.resource_id), uuid);
> >     by:
> >     virtio_add_dmabuf(uuid, assign.resource_id);
> >
> >     assign.resource_id is not DMA-BUF file descriptor, and the underlying
> >     resource my not be DMA-BUF at first place.
> >
> >
> > I didn't really look into the patch in-depth, so the code was intended
> > to give an idea of how the implementation would look like with
> > the cross-device patch API. Indeed, it is not the resource_id,
> > (I just took a brief look at the virtio specificacion 1.2), but the
> > underlying
> > resource what we want to use here.
> >
> >
> >     Also, since this lives in the common code that is not used only by
> >     virtio-gpu-gl but also virtio-gpu, which supports migration, we also
> >     need to take care of that. It is not a problem for DMA-BUF as
> >     DMA-BUF is
> >     not migratable anyway, but the situation is different in this case.
> >
> >     Implementing cross-device sharing is certainly a possibility, but
> that
> >     requires more than dealing with DMA-BUFs.
> >
> >
> > So, if I understood correctly, dmabufs are just a subset of the resources
> > that the gpu manages, or can assign UUIDs to. I am not sure why
> > the virt gpu driver would want to send a ASSIGN_UUID for anything
> > that is not a dmabuf (are we sure it does?), but I am not super
> familiarized
> > with virtgpu to begin with.
>
> In my understanding, an resource will be first created as OpenGL or
> Vulkan textures and then exported as a DMA-BUF file descriptor. For
> these resource types exporting/importing code is mandatory.
>
> For pixman buffers (i.e., non-virgl device), I don't see a compelling
> reason to have cross-device sharing. It is possible to omit resource
> UUID feature from non-virgl device to avoid implementing complicated
> migration.
>

I see, thanks for the clarification.
I would assume you could avoid the UUID feature for those resources, but
I will need to check the driver implementation. It is worth checking
though, if
that would simplify the implementation.


>
> > But I see that internally, the GPU specs relate a UUID with a
> resource_id,
> > so we still need both tables:
> > - one to relate UUID with resource_id to be able to locate the
> > underlying resource
> > - the table that holds the dmabuf with the UUID for cross-device sharing
> >
> > With that in mind, sounds to me that the support for cross-device
> > sharing could
> > be added on top of this patch, once
> > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> > <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
> > lands.
>
> That is possible, but I think it's better to implement cross-device
> sharing at the same time introducing virtio-dmabuf.
>
> The current design of virtio-dmabuf looks somewhat inconsistent; it's
> named "dmabuf", but internally the UUIDs are stored into something named
> "resource_uuids" and it has SharedObjectType so it's more like a generic
> resource sharing mechanism. If you actually have an implementation of
> cross-device sharing using virtio-dmabuf, it will be clear what kind of
> feature is truly necessary.
>
>
Yeah, the file was named as virtio-dmabuf following the kernel
implementation. Also, because for the moment it only aims to share
dmabufs. However, virtio specs leave the virtio object defintion vague [1]
(I guess purposely). It is up to the specific devices to define what an
object
means for them. So the implementation tries to follow that, and
leave the contents of the table generic. The table can hold any kind of
object,
and the API exposes type-specific functions (for dmabufs, or others).

[1] -
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-10500011


> Regards,
> Akihiko Odaki
>
>

Albert Esteve Sept. 13, 2023, 2:27 p.m. UTC | #11

On Wed, Sep 13, 2023 at 4:18 PM Albert Esteve <aesteve@redhat.com> wrote:

>
>
> On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki <akihiko.odaki@daynix.com>
> wrote:
>
>> On 2023/09/13 21:58, Albert Esteve wrote:
>> >
>> >
>> > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki <akihiko.odaki@daynix.com
>> > <mailto:akihiko.odaki@daynix.com>> wrote:
>> >
>> >     On 2023/09/13 20:34, Albert Esteve wrote:
>> >      >
>> >      >
>> >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
>> >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>> >      > <mailto:akihiko.odaki@daynix.com
>> >     <mailto:akihiko.odaki@daynix.com>>> wrote:
>> >      >
>> >      >     On 2023/09/13 16:55, Albert Esteve wrote:
>> >      >      > Hi Antonio,
>> >      >      >
>> >      >      > If I'm not mistaken, this patch is related with:
>> >      >      >
>> >      >
>> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>> >     <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>> >      >
>> >       <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>> >      >      >
>> >      >
>> >       <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>> >      >
>> >       <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
>> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
>> >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both, would
>> >     use the
>> >      >      > infrastructure from the patch I linked to store the
>> >      >      > virtio objects, so that they can be later shared with
>> >     other devices.
>> >      >
>> >      >     I don't think such sharing is possible because the resources
>> are
>> >      >     identified by IDs that are local to the device. That also
>> >     complicates
>> >      >     migration.
>> >      >
>> >      >     Regards,
>> >      >     Akihiko Odaki
>> >      >
>> >      > Hi Akihiko,
>> >      >
>> >      > As far as I understand, the feature to export dma-bufs from the
>> >      > virtgpu was introduced as part of the virtio cross-device sharing
>> >      > proposal [1]. Thus, it shall be posible. When
>> virtgpu ASSING_UUID,
>> >      > it exports and identifies the dmabuf resource, so that when the
>> >     dmabuf gets
>> >      > shared inside the guest (e.g., with virtio-video), we can use the
>> >     assigned
>> >      > UUID to find the dmabuf in the host (using the patch that I
>> >     linked above),
>> >      > and import it.
>> >      >
>> >      > [1] - https://lwn.net/Articles/828988/
>> >     <https://lwn.net/Articles/828988/> <
>> https://lwn.net/Articles/828988/
>> >     <https://lwn.net/Articles/828988/>>
>> >
>> >     The problem is that virtio-gpu can have other kind of resources like
>> >     pixman and OpenGL textures and manage them and DMA-BUFs with unified
>> >     resource ID.
>> >
>> >
>> > I see.
>> >
>> >
>> >     So you cannot change:
>> >     g_hash_table_insert(g->resource_uuids,
>> >     GUINT_TO_POINTER(assign.resource_id), uuid);
>> >     by:
>> >     virtio_add_dmabuf(uuid, assign.resource_id);
>> >
>> >     assign.resource_id is not DMA-BUF file descriptor, and the
>> underlying
>> >     resource my not be DMA-BUF at first place.
>> >
>> >
>> > I didn't really look into the patch in-depth, so the code was intended
>> > to give an idea of how the implementation would look like with
>> > the cross-device patch API. Indeed, it is not the resource_id,
>> > (I just took a brief look at the virtio specificacion 1.2), but the
>> > underlying
>> > resource what we want to use here.
>> >
>> >
>> >     Also, since this lives in the common code that is not used only by
>> >     virtio-gpu-gl but also virtio-gpu, which supports migration, we also
>> >     need to take care of that. It is not a problem for DMA-BUF as
>> >     DMA-BUF is
>> >     not migratable anyway, but the situation is different in this case.
>> >
>> >     Implementing cross-device sharing is certainly a possibility, but
>> that
>> >     requires more than dealing with DMA-BUFs.
>> >
>> >
>> > So, if I understood correctly, dmabufs are just a subset of the
>> resources
>> > that the gpu manages, or can assign UUIDs to. I am not sure why
>> > the virt gpu driver would want to send a ASSIGN_UUID for anything
>> > that is not a dmabuf (are we sure it does?), but I am not super
>> familiarized
>> > with virtgpu to begin with.
>>
>> In my understanding, an resource will be first created as OpenGL or
>> Vulkan textures and then exported as a DMA-BUF file descriptor. For
>> these resource types exporting/importing code is mandatory.
>>
>> For pixman buffers (i.e., non-virgl device), I don't see a compelling
>> reason to have cross-device sharing. It is possible to omit resource
>> UUID feature from non-virgl device to avoid implementing complicated
>> migration.
>>
>
> I see, thanks for the clarification.
> I would assume you could avoid the UUID feature for those resources, but
> I will need to check the driver implementation. It is worth checking
> though, if
> that would simplify the implementation.
>
>
>>
>> > But I see that internally, the GPU specs relate a UUID with a
>> resource_id,
>> > so we still need both tables:
>> > - one to relate UUID with resource_id to be able to locate the
>> > underlying resource
>> > - the table that holds the dmabuf with the UUID for cross-device sharing
>> >
>> > With that in mind, sounds to me that the support for cross-device
>> > sharing could
>> > be added on top of this patch, once
>> > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
>> > <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
>> > lands.
>>
>>
Also, to clarify what I stated here:
I am not trying to get your patch blocked until the other one lands.
I think both could be integrated in parallel, and then have virtgpu
use the cross-device sharing API on top of your patch.

Regards,
Albert


> That is possible, but I think it's better to implement cross-device
>> sharing at the same time introducing virtio-dmabuf.
>>
>> The current design of virtio-dmabuf looks somewhat inconsistent; it's
>> named "dmabuf", but internally the UUIDs are stored into something named
>> "resource_uuids" and it has SharedObjectType so it's more like a generic
>> resource sharing mechanism. If you actually have an implementation of
>> cross-device sharing using virtio-dmabuf, it will be clear what kind of
>> feature is truly necessary.
>>
>>
> Yeah, the file was named as virtio-dmabuf following the kernel
> implementation. Also, because for the moment it only aims to share
> dmabufs. However, virtio specs leave the virtio object defintion vague [1]
> (I guess purposely). It is up to the specific devices to define what an
> object
> means for them. So the implementation tries to follow that, and
> leave the contents of the table generic. The table can hold any kind of
> object,
> and the API exposes type-specific functions (for dmabufs, or others).
>
> [1] -
> https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-10500011
>
>
>> Regards,
>> Akihiko Odaki
>>
>>

Akihiko Odaki Sept. 14, 2023, 7:17 a.m. UTC | #12

On 2023/09/13 23:18, Albert Esteve wrote:
> 
> 
> On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2023/09/13 21:58, Albert Esteve wrote:
>      >
>      >
>      > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki
>     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>      > <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>> wrote:
>      >
>      >     On 2023/09/13 20:34, Albert Esteve wrote:
>      >      >
>      >      >
>      >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
>      >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>     <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>>
>      >      > <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>
>      >     <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>>> wrote:
>      >      >
>      >      >     On 2023/09/13 16:55, Albert Esteve wrote:
>      >      >      > Hi Antonio,
>      >      >      >
>      >      >      > If I'm not mistaken, this patch is related with:
>      >      >      >
>      >      >
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
>      >      >      >
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>
>      >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both,
>     would
>      >     use the
>      >      >      > infrastructure from the patch I linked to store the
>      >      >      > virtio objects, so that they can be later shared with
>      >     other devices.
>      >      >
>      >      >     I don't think such sharing is possible because the
>     resources are
>      >      >     identified by IDs that are local to the device. That also
>      >     complicates
>      >      >     migration.
>      >      >
>      >      >     Regards,
>      >      >     Akihiko Odaki
>      >      >
>      >      > Hi Akihiko,
>      >      >
>      >      > As far as I understand, the feature to export
>     dma-bufs from the
>      >      > virtgpu was introduced as part of the virtio cross-device
>     sharing
>      >      > proposal [1]. Thus, it shall be posible. When
>     virtgpu ASSING_UUID,
>      >      > it exports and identifies the dmabuf resource, so that
>     when the
>      >     dmabuf gets
>      >      > shared inside the guest (e.g., with virtio-video), we can
>     use the
>      >     assigned
>      >      > UUID to find the dmabuf in the host (using the patch that I
>      >     linked above),
>      >      > and import it.
>      >      >
>      >      > [1] - https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>
>     <https://lwn.net/Articles/828988/ <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>>
>      >
>      >     The problem is that virtio-gpu can have other kind of
>     resources like
>      >     pixman and OpenGL textures and manage them and DMA-BUFs with
>     unified
>      >     resource ID.
>      >
>      >
>      > I see.
>      >
>      >
>      >     So you cannot change:
>      >     g_hash_table_insert(g->resource_uuids,
>      >     GUINT_TO_POINTER(assign.resource_id), uuid);
>      >     by:
>      >     virtio_add_dmabuf(uuid, assign.resource_id);
>      >
>      >     assign.resource_id is not DMA-BUF file descriptor, and the
>     underlying
>      >     resource my not be DMA-BUF at first place.
>      >
>      >
>      > I didn't really look into the patch in-depth, so the code was
>     intended
>      > to give an idea of how the implementation would look like with
>      > the cross-device patch API. Indeed, it is not the resource_id,
>      > (I just took a brief look at the virtio specificacion 1.2), but the
>      > underlying
>      > resource what we want to use here.
>      >
>      >
>      >     Also, since this lives in the common code that is not used
>     only by
>      >     virtio-gpu-gl but also virtio-gpu, which supports migration,
>     we also
>      >     need to take care of that. It is not a problem for DMA-BUF as
>      >     DMA-BUF is
>      >     not migratable anyway, but the situation is different in this
>     case.
>      >
>      >     Implementing cross-device sharing is certainly a possibility,
>     but that
>      >     requires more than dealing with DMA-BUFs.
>      >
>      >
>      > So, if I understood correctly, dmabufs are just a subset of the
>     resources
>      > that the gpu manages, or can assign UUIDs to. I am not sure why
>      > the virt gpu driver would want to send a ASSIGN_UUID for anything
>      > that is not a dmabuf (are we sure it does?), but I am not super
>     familiarized
>      > with virtgpu to begin with.
> 
>     In my understanding, an resource will be first created as OpenGL or
>     Vulkan textures and then exported as a DMA-BUF file descriptor. For
>     these resource types exporting/importing code is mandatory.
> 
>     For pixman buffers (i.e., non-virgl device), I don't see a compelling
>     reason to have cross-device sharing. It is possible to omit resource
>     UUID feature from non-virgl device to avoid implementing complicated
>     migration.
> 
> 
> I see, thanks for the clarification.
> I would assume you could avoid the UUID feature for those resources, but
> I will need to check the driver implementation. It is worth checking 
> though, if
> that would simplify the implementation.
> 
> 
>      > But I see that internally, the GPU specs relate a UUID with a
>     resource_id,
>      > so we still need both tables:
>      > - one to relate UUID with resource_id to be able to locate the
>      > underlying resource
>      > - the table that holds the dmabuf with the UUID for cross-device
>     sharing
>      >
>      > With that in mind, sounds to me that the support for cross-device
>      > sharing could
>      > be added on top of this patch, once
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
>      >
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>>
>      > lands.
> 
>     That is possible, but I think it's better to implement cross-device
>     sharing at the same time introducing virtio-dmabuf.
> 
>     The current design of virtio-dmabuf looks somewhat inconsistent; it's
>     named "dmabuf", but internally the UUIDs are stored into something
>     named
>     "resource_uuids" and it has SharedObjectType so it's more like a
>     generic
>     resource sharing mechanism. If you actually have an implementation of
>     cross-device sharing using virtio-dmabuf, it will be clear what kind of
>     feature is truly necessary.
> 
> 
> Yeah, the file was named as virtio-dmabuf following the kernel
> implementation. Also, because for the moment it only aims to share
> dmabufs. However, virtio specs leave the virtio object defintion vague [1]
> (I guess purposely). It is up to the specific devices to define what an 
> object
> means for them. So the implementation tries to follow that, and
> leave the contents of the table generic. The table can hold any kind of 
> object,
> and the API exposes type-specific functions (for dmabufs, or others).
In the guest kernel, the name "virtio_dma_buf" represents the interface 
between the *guest* kernel and *guest* user-space. It makes sense since 
the cross-device resource sharing is managed by the userspace and 
DMA-BUF is the only interface between them for this purpose.

The situation is different for QEMU; QEMU interacts with backends using 
backend-specific interfaces (OpenGL/pixman) and virgl is capable to 
export textures as DMA-BUF. DMA-BUF is not universal in this sense. As 
such, we cannot just borrow the kernel-side naming but invent one.

Albert Esteve Sept. 14, 2023, 8:29 a.m. UTC | #13

On Thu, Sep 14, 2023 at 9:17 AM Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2023/09/13 23:18, Albert Esteve wrote:
> >
> >
> > On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki <akihiko.odaki@daynix.com
> > <mailto:akihiko.odaki@daynix.com>> wrote:
> >
> >     On 2023/09/13 21:58, Albert Esteve wrote:
> >      >
> >      >
> >      > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki
> >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
> >      > <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>> wrote:
> >      >
> >      >     On 2023/09/13 20:34, Albert Esteve wrote:
> >      >      >
> >      >      >
> >      >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
> >      >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
> >     <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>>
> >      >      > <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>
> >      >     <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>>> wrote:
> >      >      >
> >      >      >     On 2023/09/13 16:55, Albert Esteve wrote:
> >      >      >      > Hi Antonio,
> >      >      >      >
> >      >      >      > If I'm not mistaken, this patch is related with:
> >      >      >      >
> >      >      >
> >      >
> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
> >      >      >      >
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>
> >      >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu both,
> >     would
> >      >     use the
> >      >      >      > infrastructure from the patch I linked to store the
> >      >      >      > virtio objects, so that they can be later shared
> with
> >      >     other devices.
> >      >      >
> >      >      >     I don't think such sharing is possible because the
> >     resources are
> >      >      >     identified by IDs that are local to the device. That
> also
> >      >     complicates
> >      >      >     migration.
> >      >      >
> >      >      >     Regards,
> >      >      >     Akihiko Odaki
> >      >      >
> >      >      > Hi Akihiko,
> >      >      >
> >      >      > As far as I understand, the feature to export
> >     dma-bufs from the
> >      >      > virtgpu was introduced as part of the virtio cross-device
> >     sharing
> >      >      > proposal [1]. Thus, it shall be posible. When
> >     virtgpu ASSING_UUID,
> >      >      > it exports and identifies the dmabuf resource, so that
> >     when the
> >      >     dmabuf gets
> >      >      > shared inside the guest (e.g., with virtio-video), we can
> >     use the
> >      >     assigned
> >      >      > UUID to find the dmabuf in the host (using the patch that I
> >      >     linked above),
> >      >      > and import it.
> >      >      >
> >      >      > [1] - https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>
> >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>
> >     <https://lwn.net/Articles/828988/ <https://lwn.net/Articles/828988/>
> >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>>
> >      >
> >      >     The problem is that virtio-gpu can have other kind of
> >     resources like
> >      >     pixman and OpenGL textures and manage them and DMA-BUFs with
> >     unified
> >      >     resource ID.
> >      >
> >      >
> >      > I see.
> >      >
> >      >
> >      >     So you cannot change:
> >      >     g_hash_table_insert(g->resource_uuids,
> >      >     GUINT_TO_POINTER(assign.resource_id), uuid);
> >      >     by:
> >      >     virtio_add_dmabuf(uuid, assign.resource_id);
> >      >
> >      >     assign.resource_id is not DMA-BUF file descriptor, and the
> >     underlying
> >      >     resource my not be DMA-BUF at first place.
> >      >
> >      >
> >      > I didn't really look into the patch in-depth, so the code was
> >     intended
> >      > to give an idea of how the implementation would look like with
> >      > the cross-device patch API. Indeed, it is not the resource_id,
> >      > (I just took a brief look at the virtio specificacion 1.2), but
> the
> >      > underlying
> >      > resource what we want to use here.
> >      >
> >      >
> >      >     Also, since this lives in the common code that is not used
> >     only by
> >      >     virtio-gpu-gl but also virtio-gpu, which supports migration,
> >     we also
> >      >     need to take care of that. It is not a problem for DMA-BUF as
> >      >     DMA-BUF is
> >      >     not migratable anyway, but the situation is different in this
> >     case.
> >      >
> >      >     Implementing cross-device sharing is certainly a possibility,
> >     but that
> >      >     requires more than dealing with DMA-BUFs.
> >      >
> >      >
> >      > So, if I understood correctly, dmabufs are just a subset of the
> >     resources
> >      > that the gpu manages, or can assign UUIDs to. I am not sure why
> >      > the virt gpu driver would want to send a ASSIGN_UUID for anything
> >      > that is not a dmabuf (are we sure it does?), but I am not super
> >     familiarized
> >      > with virtgpu to begin with.
> >
> >     In my understanding, an resource will be first created as OpenGL or
> >     Vulkan textures and then exported as a DMA-BUF file descriptor. For
> >     these resource types exporting/importing code is mandatory.
> >
> >     For pixman buffers (i.e., non-virgl device), I don't see a compelling
> >     reason to have cross-device sharing. It is possible to omit resource
> >     UUID feature from non-virgl device to avoid implementing complicated
> >     migration.
> >
> >
> > I see, thanks for the clarification.
> > I would assume you could avoid the UUID feature for those resources, but
> > I will need to check the driver implementation. It is worth checking
> > though, if
> > that would simplify the implementation.
> >
> >
> >      > But I see that internally, the GPU specs relate a UUID with a
> >     resource_id,
> >      > so we still need both tables:
> >      > - one to relate UUID with resource_id to be able to locate the
> >      > underlying resource
> >      > - the table that holds the dmabuf with the UUID for cross-device
> >     sharing
> >      >
> >      > With that in mind, sounds to me that the support for cross-device
> >      > sharing could
> >      > be added on top of this patch, once
> >      >
> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> >
> >      >
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> >>
> >      > lands.
> >
> >     That is possible, but I think it's better to implement cross-device
> >     sharing at the same time introducing virtio-dmabuf.
> >
> >     The current design of virtio-dmabuf looks somewhat inconsistent; it's
> >     named "dmabuf", but internally the UUIDs are stored into something
> >     named
> >     "resource_uuids" and it has SharedObjectType so it's more like a
> >     generic
> >     resource sharing mechanism. If you actually have an implementation of
> >     cross-device sharing using virtio-dmabuf, it will be clear what kind
> of
> >     feature is truly necessary.
> >
> >
> > Yeah, the file was named as virtio-dmabuf following the kernel
> > implementation. Also, because for the moment it only aims to share
> > dmabufs. However, virtio specs leave the virtio object defintion vague
> [1]
> > (I guess purposely). It is up to the specific devices to define what an
> > object
> > means for them. So the implementation tries to follow that, and
> > leave the contents of the table generic. The table can hold any kind of
> > object,
> > and the API exposes type-specific functions (for dmabufs, or others).
> In the guest kernel, the name "virtio_dma_buf" represents the interface
> between the *guest* kernel and *guest* user-space. It makes sense since
> the cross-device resource sharing is managed by the userspace and
> DMA-BUF is the only interface between them for this purpose.
>
> The situation is different for QEMU; QEMU interacts with backends using
> backend-specific interfaces (OpenGL/pixman) and virgl is capable to
> export textures as DMA-BUF. DMA-BUF is not universal in this sense. As
> such, we cannot just borrow the kernel-side naming but invent one.
>
> It is not a gpu-specific feature. It is a generic cross-device sharing
mechanism for virtio objects. In this case, virtio objects happen to be
dmabufs in this first iteration. Hence, the name.

virtio-gpu (and vhost-user-gpu) will use this feature only with virgl, that
is
fine, and transversal to the object-sharing mechanism. It allows
to share dmabufs in the host following how they are shared in the guest.
The virtgpu driver may call ASSIGN_UUID for other types of resources (not
sure,
but could be), but they will never be shared with other virtio devices.
So they are not too relevant. Also, the shared objects table could
potentially
be accessed from any virtio device, not only virtio-gpu or virtio-video.

What I am trying to say, is that the name focuses solely in its current
usage,
i.e., sharing dmabufs between virtio-gpu (as exporter), and virtio-video
(as importer).
If it grows to something more, imo it can be renamed later.

Regards,
Albert

Akihiko Odaki Sept. 14, 2023, 4:55 p.m. UTC | #14

On 2023/09/14 17:29, Albert Esteve wrote:
> 
> 
> On Thu, Sep 14, 2023 at 9:17 AM Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2023/09/13 23:18, Albert Esteve wrote:
>      >
>      >
>      > On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki
>     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>      > <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>> wrote:
>      >
>      >     On 2023/09/13 21:58, Albert Esteve wrote:
>      >      >
>      >      >
>      >      > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki
>      >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>     <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>>
>      >      > <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>
>      >     <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>>> wrote:
>      >      >
>      >      >     On 2023/09/13 20:34, Albert Esteve wrote:
>      >      >      >
>      >      >      >
>      >      >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
>      >      >     <akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>
>      >     <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>>
>      >      >      > <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>
>      >     <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>
>      >      >     <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>
>      >     <mailto:akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>>>> wrote:
>      >      >      >
>      >      >      >     On 2023/09/13 16:55, Albert Esteve wrote:
>      >      >      >      > Hi Antonio,
>      >      >      >      >
>      >      >      >      > If I'm not mistaken, this patch is related with:
>      >      >      >      >
>      >      >      >
>      >      >
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
>      >      >      >
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>
>      >      >      >      >
>      >      >      >
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
>      >      >      >
>      >      >
>      >     
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>>
>      >      >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu
>     both,
>      >     would
>      >      >     use the
>      >      >      >      > infrastructure from the patch I linked to
>     store the
>      >      >      >      > virtio objects, so that they can be later
>     shared with
>      >      >     other devices.
>      >      >      >
>      >      >      >     I don't think such sharing is possible because the
>      >     resources are
>      >      >      >     identified by IDs that are local to the device.
>     That also
>      >      >     complicates
>      >      >      >     migration.
>      >      >      >
>      >      >      >     Regards,
>      >      >      >     Akihiko Odaki
>      >      >      >
>      >      >      > Hi Akihiko,
>      >      >      >
>      >      >      > As far as I understand, the feature to export
>      >     dma-bufs from the
>      >      >      > virtgpu was introduced as part of the virtio
>     cross-device
>      >     sharing
>      >      >      > proposal [1]. Thus, it shall be posible. When
>      >     virtgpu ASSING_UUID,
>      >      >      > it exports and identifies the dmabuf resource, so that
>      >     when the
>      >      >     dmabuf gets
>      >      >      > shared inside the guest (e.g., with virtio-video),
>     we can
>      >     use the
>      >      >     assigned
>      >      >      > UUID to find the dmabuf in the host (using the
>     patch that I
>      >      >     linked above),
>      >      >      > and import it.
>      >      >      >
>      >      >      > [1] - https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>
>      >      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/> <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>
>      >      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>
>      >     <https://lwn.net/Articles/828988/
>     <https://lwn.net/Articles/828988/>>>>
>      >      >
>      >      >     The problem is that virtio-gpu can have other kind of
>      >     resources like
>      >      >     pixman and OpenGL textures and manage them and
>     DMA-BUFs with
>      >     unified
>      >      >     resource ID.
>      >      >
>      >      >
>      >      > I see.
>      >      >
>      >      >
>      >      >     So you cannot change:
>      >      >     g_hash_table_insert(g->resource_uuids,
>      >      >     GUINT_TO_POINTER(assign.resource_id), uuid);
>      >      >     by:
>      >      >     virtio_add_dmabuf(uuid, assign.resource_id);
>      >      >
>      >      >     assign.resource_id is not DMA-BUF file descriptor, and the
>      >     underlying
>      >      >     resource my not be DMA-BUF at first place.
>      >      >
>      >      >
>      >      > I didn't really look into the patch in-depth, so the code was
>      >     intended
>      >      > to give an idea of how the implementation would look like with
>      >      > the cross-device patch API. Indeed, it is not the resource_id,
>      >      > (I just took a brief look at the virtio
>     specificacion 1.2), but the
>      >      > underlying
>      >      > resource what we want to use here.
>      >      >
>      >      >
>      >      >     Also, since this lives in the common code that is not used
>      >     only by
>      >      >     virtio-gpu-gl but also virtio-gpu, which supports
>     migration,
>      >     we also
>      >      >     need to take care of that. It is not a problem for
>     DMA-BUF as
>      >      >     DMA-BUF is
>      >      >     not migratable anyway, but the situation is different
>     in this
>      >     case.
>      >      >
>      >      >     Implementing cross-device sharing is certainly a
>     possibility,
>      >     but that
>      >      >     requires more than dealing with DMA-BUFs.
>      >      >
>      >      >
>      >      > So, if I understood correctly, dmabufs are just a subset
>     of the
>      >     resources
>      >      > that the gpu manages, or can assign UUIDs to. I am not
>     sure why
>      >      > the virt gpu driver would want to send a ASSIGN_UUID for
>     anything
>      >      > that is not a dmabuf (are we sure it does?), but I am not
>     super
>      >     familiarized
>      >      > with virtgpu to begin with.
>      >
>      >     In my understanding, an resource will be first created as
>     OpenGL or
>      >     Vulkan textures and then exported as a DMA-BUF file
>     descriptor. For
>      >     these resource types exporting/importing code is mandatory.
>      >
>      >     For pixman buffers (i.e., non-virgl device), I don't see a
>     compelling
>      >     reason to have cross-device sharing. It is possible to omit
>     resource
>      >     UUID feature from non-virgl device to avoid implementing
>     complicated
>      >     migration.
>      >
>      >
>      > I see, thanks for the clarification.
>      > I would assume you could avoid the UUID feature for those
>     resources, but
>      > I will need to check the driver implementation. It is worth checking
>      > though, if
>      > that would simplify the implementation.
>      >
>      >
>      >      > But I see that internally, the GPU specs relate a UUID with a
>      >     resource_id,
>      >      > so we still need both tables:
>      >      > - one to relate UUID with resource_id to be able to locate the
>      >      > underlying resource
>      >      > - the table that holds the dmabuf with the UUID for
>     cross-device
>      >     sharing
>      >      >
>      >      > With that in mind, sounds to me that the support for
>     cross-device
>      >      > sharing could
>      >      > be added on top of this patch, once
>      >      >
>      >
>     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>>
>      >      >
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
>      >   
>       <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>>>
>      >      > lands.
>      >
>      >     That is possible, but I think it's better to implement
>     cross-device
>      >     sharing at the same time introducing virtio-dmabuf.
>      >
>      >     The current design of virtio-dmabuf looks somewhat
>     inconsistent; it's
>      >     named "dmabuf", but internally the UUIDs are stored into
>     something
>      >     named
>      >     "resource_uuids" and it has SharedObjectType so it's more like a
>      >     generic
>      >     resource sharing mechanism. If you actually have an
>     implementation of
>      >     cross-device sharing using virtio-dmabuf, it will be clear
>     what kind of
>      >     feature is truly necessary.
>      >
>      >
>      > Yeah, the file was named as virtio-dmabuf following the kernel
>      > implementation. Also, because for the moment it only aims to share
>      > dmabufs. However, virtio specs leave the virtio object
>     defintion vague [1]
>      > (I guess purposely). It is up to the specific devices to define
>     what an
>      > object
>      > means for them. So the implementation tries to follow that, and
>      > leave the contents of the table generic. The table can hold any
>     kind of
>      > object,
>      > and the API exposes type-specific functions (for dmabufs, or others).
>     In the guest kernel, the name "virtio_dma_buf" represents the interface
>     between the *guest* kernel and *guest* user-space. It makes sense since
>     the cross-device resource sharing is managed by the userspace and
>     DMA-BUF is the only interface between them for this purpose.
> 
>     The situation is different for QEMU; QEMU interacts with backends using
>     backend-specific interfaces (OpenGL/pixman) and virgl is capable to
>     export textures as DMA-BUF. DMA-BUF is not universal in this sense. As
>     such, we cannot just borrow the kernel-side naming but invent one.
> 
> It is not a gpu-specific feature. It is a generic cross-device sharing
> mechanism for virtio objects. In this case, virtio objects happen to be
> dmabufs in this first iteration. Hence, the name.
> 
> virtio-gpu (and vhost-user-gpu) will use this feature only with virgl, 
> that is
> fine, and transversal to the object-sharing mechanism. It allows
> to share dmabufs in the host following how they are shared in the guest.
> The virtgpu driver may call ASSIGN_UUID for other types of resources 
> (not sure,
> but could be), but they will never be shared with other virtio devices.
> So they are not too relevant. Also, the shared objects table could 
> potentially
> be accessed from any virtio device, not only virtio-gpu or virtio-video.

The virtgpu driver will call ASSIGN_UUID for resources that are backed 
with pixman buffer. What is used as the backing store for resources is 
an implementation detail of VMM and the guest cannot know it. For the 
guest, they are same kind of resources (2D images).

Albert Esteve Sept. 15, 2023, 9:56 a.m. UTC | #15

On Thu, Sep 14, 2023 at 6:56 PM Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2023/09/14 17:29, Albert Esteve wrote:
> >
> >
> > On Thu, Sep 14, 2023 at 9:17 AM Akihiko Odaki <akihiko.odaki@daynix.com
> > <mailto:akihiko.odaki@daynix.com>> wrote:
> >
> >     On 2023/09/13 23:18, Albert Esteve wrote:
> >      >
> >      >
> >      > On Wed, Sep 13, 2023 at 3:43 PM Akihiko Odaki
> >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
> >      > <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>> wrote:
> >      >
> >      >     On 2023/09/13 21:58, Albert Esteve wrote:
> >      >      >
> >      >      >
> >      >      > On Wed, Sep 13, 2023 at 2:22 PM Akihiko Odaki
> >      >     <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
> >     <mailto:akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>>
> >      >      > <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>
> >      >     <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>>> wrote:
> >      >      >
> >      >      >     On 2023/09/13 20:34, Albert Esteve wrote:
> >      >      >      >
> >      >      >      >
> >      >      >      > On Wed, Sep 13, 2023 at 12:34 PM Akihiko Odaki
> >      >      >     <akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>
> >      >     <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com> <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>>
> >      >      >      > <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>
> >      >     <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>
> >      >      >     <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>
> >      >     <mailto:akihiko.odaki@daynix.com
> >     <mailto:akihiko.odaki@daynix.com>>>>> wrote:
> >      >      >      >
> >      >      >      >     On 2023/09/13 16:55, Albert Esteve wrote:
> >      >      >      >      > Hi Antonio,
> >      >      >      >      >
> >      >      >      >      > If I'm not mistaken, this patch is related
> with:
> >      >      >      >      >
> >      >      >      >
> >      >      >
> >      >
> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html
> >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
> >      >      >      >
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>
> >      >      >      >      >
> >      >      >      >
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>
> >      >      >      >
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html> <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01853.html>>>>>
> >      >      >      >      > IMHO, ideally, virtio-gpu and vhost-user-gpu
> >     both,
> >      >     would
> >      >      >     use the
> >      >      >      >      > infrastructure from the patch I linked to
> >     store the
> >      >      >      >      > virtio objects, so that they can be later
> >     shared with
> >      >      >     other devices.
> >      >      >      >
> >      >      >      >     I don't think such sharing is possible because
> the
> >      >     resources are
> >      >      >      >     identified by IDs that are local to the device.
> >     That also
> >      >      >     complicates
> >      >      >      >     migration.
> >      >      >      >
> >      >      >      >     Regards,
> >      >      >      >     Akihiko Odaki
> >      >      >      >
> >      >      >      > Hi Akihiko,
> >      >      >      >
> >      >      >      > As far as I understand, the feature to export
> >      >     dma-bufs from the
> >      >      >      > virtgpu was introduced as part of the virtio
> >     cross-device
> >      >     sharing
> >      >      >      > proposal [1]. Thus, it shall be posible. When
> >      >     virtgpu ASSING_UUID,
> >      >      >      > it exports and identifies the dmabuf resource, so
> that
> >      >     when the
> >      >      >     dmabuf gets
> >      >      >      > shared inside the guest (e.g., with virtio-video),
> >     we can
> >      >     use the
> >      >      >     assigned
> >      >      >      > UUID to find the dmabuf in the host (using the
> >     patch that I
> >      >      >     linked above),
> >      >      >      > and import it.
> >      >      >      >
> >      >      >      > [1] - https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>
> >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>
> >      >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>
> >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>>
> >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/> <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>
> >      >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>
> >      >     <https://lwn.net/Articles/828988/
> >     <https://lwn.net/Articles/828988/>>>>
> >      >      >
> >      >      >     The problem is that virtio-gpu can have other kind of
> >      >     resources like
> >      >      >     pixman and OpenGL textures and manage them and
> >     DMA-BUFs with
> >      >     unified
> >      >      >     resource ID.
> >      >      >
> >      >      >
> >      >      > I see.
> >      >      >
> >      >      >
> >      >      >     So you cannot change:
> >      >      >     g_hash_table_insert(g->resource_uuids,
> >      >      >     GUINT_TO_POINTER(assign.resource_id), uuid);
> >      >      >     by:
> >      >      >     virtio_add_dmabuf(uuid, assign.resource_id);
> >      >      >
> >      >      >     assign.resource_id is not DMA-BUF file descriptor, and
> the
> >      >     underlying
> >      >      >     resource my not be DMA-BUF at first place.
> >      >      >
> >      >      >
> >      >      > I didn't really look into the patch in-depth, so the code
> was
> >      >     intended
> >      >      > to give an idea of how the implementation would look like
> with
> >      >      > the cross-device patch API. Indeed, it is not the
> resource_id,
> >      >      > (I just took a brief look at the virtio
> >     specificacion 1.2), but the
> >      >      > underlying
> >      >      > resource what we want to use here.
> >      >      >
> >      >      >
> >      >      >     Also, since this lives in the common code that is not
> used
> >      >     only by
> >      >      >     virtio-gpu-gl but also virtio-gpu, which supports
> >     migration,
> >      >     we also
> >      >      >     need to take care of that. It is not a problem for
> >     DMA-BUF as
> >      >      >     DMA-BUF is
> >      >      >     not migratable anyway, but the situation is different
> >     in this
> >      >     case.
> >      >      >
> >      >      >     Implementing cross-device sharing is certainly a
> >     possibility,
> >      >     but that
> >      >      >     requires more than dealing with DMA-BUFs.
> >      >      >
> >      >      >
> >      >      > So, if I understood correctly, dmabufs are just a subset
> >     of the
> >      >     resources
> >      >      > that the gpu manages, or can assign UUIDs to. I am not
> >     sure why
> >      >      > the virt gpu driver would want to send a ASSIGN_UUID for
> >     anything
> >      >      > that is not a dmabuf (are we sure it does?), but I am not
> >     super
> >      >     familiarized
> >      >      > with virtgpu to begin with.
> >      >
> >      >     In my understanding, an resource will be first created as
> >     OpenGL or
> >      >     Vulkan textures and then exported as a DMA-BUF file
> >     descriptor. For
> >      >     these resource types exporting/importing code is mandatory.
> >      >
> >      >     For pixman buffers (i.e., non-virgl device), I don't see a
> >     compelling
> >      >     reason to have cross-device sharing. It is possible to omit
> >     resource
> >      >     UUID feature from non-virgl device to avoid implementing
> >     complicated
> >      >     migration.
> >      >
> >      >
> >      > I see, thanks for the clarification.
> >      > I would assume you could avoid the UUID feature for those
> >     resources, but
> >      > I will need to check the driver implementation. It is worth
> checking
> >      > though, if
> >      > that would simplify the implementation.
> >      >
> >      >
> >      >      > But I see that internally, the GPU specs relate a UUID
> with a
> >      >     resource_id,
> >      >      > so we still need both tables:
> >      >      > - one to relate UUID with resource_id to be able to locate
> the
> >      >      > underlying resource
> >      >      > - the table that holds the dmabuf with the UUID for
> >     cross-device
> >      >     sharing
> >      >      >
> >      >      > With that in mind, sounds to me that the support for
> >     cross-device
> >      >      > sharing could
> >      >      > be added on top of this patch, once
> >      >      >
> >      >
> >     https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> >     <https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html
> >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>>
> >      >      >
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>
> >      >
> >       <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html <
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg01850.html>>>
> >      >      > lands.
> >      >
> >      >     That is possible, but I think it's better to implement
> >     cross-device
> >      >     sharing at the same time introducing virtio-dmabuf.
> >      >
> >      >     The current design of virtio-dmabuf looks somewhat
> >     inconsistent; it's
> >      >     named "dmabuf", but internally the UUIDs are stored into
> >     something
> >      >     named
> >      >     "resource_uuids" and it has SharedObjectType so it's more
> like a
> >      >     generic
> >      >     resource sharing mechanism. If you actually have an
> >     implementation of
> >      >     cross-device sharing using virtio-dmabuf, it will be clear
> >     what kind of
> >      >     feature is truly necessary.
> >      >
> >      >
> >      > Yeah, the file was named as virtio-dmabuf following the kernel
> >      > implementation. Also, because for the moment it only aims to share
> >      > dmabufs. However, virtio specs leave the virtio object
> >     defintion vague [1]
> >      > (I guess purposely). It is up to the specific devices to define
> >     what an
> >      > object
> >      > means for them. So the implementation tries to follow that, and
> >      > leave the contents of the table generic. The table can hold any
> >     kind of
> >      > object,
> >      > and the API exposes type-specific functions (for dmabufs, or
> others).
> >     In the guest kernel, the name "virtio_dma_buf" represents the
> interface
> >     between the *guest* kernel and *guest* user-space. It makes sense
> since
> >     the cross-device resource sharing is managed by the userspace and
> >     DMA-BUF is the only interface between them for this purpose.
> >
> >     The situation is different for QEMU; QEMU interacts with backends
> using
> >     backend-specific interfaces (OpenGL/pixman) and virgl is capable to
> >     export textures as DMA-BUF. DMA-BUF is not universal in this sense.
> As
> >     such, we cannot just borrow the kernel-side naming but invent one.
> >
> > It is not a gpu-specific feature. It is a generic cross-device sharing
> > mechanism for virtio objects. In this case, virtio objects happen to be
> > dmabufs in this first iteration. Hence, the name.
> >
> > virtio-gpu (and vhost-user-gpu) will use this feature only with virgl,
> > that is
> > fine, and transversal to the object-sharing mechanism. It allows
> > to share dmabufs in the host following how they are shared in the guest.
> > The virtgpu driver may call ASSIGN_UUID for other types of resources
> > (not sure,
> > but could be), but they will never be shared with other virtio devices.
> > So they are not too relevant. Also, the shared objects table could
> > potentially
> > be accessed from any virtio device, not only virtio-gpu or virtio-video.
>
> The virtgpu driver will call ASSIGN_UUID for resources that are backed
> with pixman buffer. What is used as the backing store for resources is
> an implementation detail of VMM and the guest cannot know it. For the
> guest, they are same kind of resources (2D images).
>
> Ok, got it. In any case, those resources that are actually backed by a
dmabuf
ought to be shared using virtio_dmabuf. I think that is the key point of
this discussion.

That support can be added once both this patch and the virtio_dmabuf land.

[QEMU,v4,10/13] virtio-gpu: Resource UUID

Commit Message

Comments

Patch