| Message ID | 20230831093252.2461282-11-ray.huang@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
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);
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);
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
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
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
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/
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.
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
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
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 > >
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 >> >>
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.
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
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).
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.
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; + } + 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; + + 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) { + 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);