| Message ID | 20230915111130.24064-10-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
On 2023/09/15 20:11, Huang Rui wrote: > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > Support BLOB resources creation, mapping and unmapping by calling the > new stable virglrenderer 0.10 interface. Only enabled when available and > via the blob config. E.g. -device virtio-vga-gl,blob=true > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > V4 -> V5: > - Use memory_region_init_ram_ptr() instead of > memory_region_init_ram_device_ptr() (Akihiko) > > hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > hw/display/virtio-gpu.c | 4 +- > include/hw/virtio/virtio-gpu.h | 5 + > meson.build | 4 + > 4 files changed, 225 insertions(+), 1 deletion(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 312953ec16..563a6f2f58 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -17,6 +17,7 @@ > #include "trace.h" > #include "hw/virtio/virtio.h" > #include "hw/virtio/virtio-gpu.h" > +#include "hw/virtio/virtio-gpu-bswap.h" > > #include "ui/egl-helpers.h" > > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > virgl_renderer_resource_create(&args, NULL, 0); > } > > +static void virgl_resource_destroy(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res) > +{ > + if (!res) > + return; > + > + QTAILQ_REMOVE(&g->reslist, res, next); > + > + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > + g_free(res->addrs); > + > + g_free(res); > +} > + > static void virgl_cmd_resource_unref(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > + struct virtio_gpu_simple_resource *res; > struct virtio_gpu_resource_unref unref; > struct iovec *res_iovs = NULL; > int num_iovs = 0; > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > VIRTIO_GPU_FILL_CMD(unref); > trace_virtio_gpu_cmd_res_unref(unref.resource_id); > > + res = virtio_gpu_find_resource(g, unref.resource_id); > + > virgl_renderer_resource_detach_iov(unref.resource_id, > &res_iovs, > &num_iovs); > if (res_iovs != NULL && num_iovs != 0) { > virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > + if (res) { > + res->iov = NULL; > + res->iov_cnt = 0; > + } > } > + > virgl_renderer_resource_unref(unref.resource_id); > + > + virgl_resource_destroy(g, res); > } > > static void virgl_cmd_context_create(VirtIOGPU *g, > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, > g_free(resp); > } > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_create_blob cblob; > + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; > + int ret; > + > + VIRTIO_GPU_FILL_CMD(cblob); > + virtio_gpu_create_blob_bswap(&cblob); > + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); > + > + if (cblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, cblob.resource_id); > + if (res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", > + __func__, cblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = g_new0(struct virtio_gpu_simple_resource, 1); > + if (!res) { > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; > + } > + > + res->resource_id = cblob.resource_id; > + res->blob_size = cblob.size; > + > + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { > + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), > + cmd, &res->addrs, &res->iov, > + &res->iov_cnt); > + if (!ret) { > + g_free(res); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + return; > + } > + } > + > + QTAILQ_INSERT_HEAD(&g->reslist, res, next); > + > + virgl_args.res_handle = cblob.resource_id; > + virgl_args.ctx_id = cblob.hdr.ctx_id; > + virgl_args.blob_mem = cblob.blob_mem; > + virgl_args.blob_id = cblob.blob_id; > + virgl_args.blob_flags = cblob.blob_flags; > + virgl_args.size = cblob.size; > + virgl_args.iovecs = res->iov; > + virgl_args.num_iovs = res->iov_cnt; > + > + ret = virgl_renderer_resource_create_blob(&virgl_args); > + if (ret) { > + virgl_resource_destroy(g, res); > + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + } > +} > + > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_map_blob mblob; > + int ret; > + void *data; > + uint64_t size; > + struct virtio_gpu_resp_map_info resp; > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + VIRTIO_GPU_FILL_CMD(mblob); > + virtio_gpu_map_blob_bswap(&mblob); > + > + if (mblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, mblob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + if (res->region) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); > + if (ret) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res->region = g_new0(MemoryRegion, 1); > + if (!res->region) { > + virgl_renderer_resource_unmap(res->resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; > + } > + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); > + OBJECT(res->region)->free = g_free; > + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); > + memory_region_set_enabled(res->region, true); > + > + memset(&resp, 0, sizeof(resp)); > + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; > + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); > + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); > +} > + > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, > + struct virtio_gpu_simple_resource > + *res) > +{ > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", > + __func__, res->resource_id); > + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; This returns enum virtio_gpu_ctrl_type while virgl_renderer_resource_unmap() returns -errno and the returned error code is ignored. I think you can just inline this function into virgl_cmd_resource_unmap_blob(). It will be more concise and consistent with virgl_cmd_resource_map_blob(). > + } > + > + memory_region_set_enabled(res->region, false); > + memory_region_del_subregion(&b->hostmem, res->region); > + object_unparent(OBJECT(res->region)); > + res->region = NULL; > + > + return virgl_renderer_resource_unmap(res->resource_id); > +} > + > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_unmap_blob ublob; > + VIRTIO_GPU_FILL_CMD(ublob); > + virtio_gpu_unmap_blob_bswap(&ublob); > + > + if (ublob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, ublob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, ublob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + virtio_gpu_virgl_resource_unmap(g, res); > +} > + > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > + > void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > case VIRTIO_GPU_CMD_GET_EDID: > virtio_gpu_get_edid(g, cmd); > break; > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: > + virgl_cmd_resource_create_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: > + virgl_cmd_resource_map_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > + virgl_cmd_resource_unmap_blob(g, cmd); > + break; > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > default: > cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > break; > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 5b7a7eab4f..cc4c1f81bb 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > return; > } > > +#ifndef HAVE_VIRGL_RESOURCE_BLOB > if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { > - error_setg(errp, "blobs and virgl are not compatible (yet)"); > + error_setg(errp, "Linked virglrenderer does not support blob resources"); > return; > } > +#endif > } > > if (!virtio_gpu_base_device_realize(qdev, > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > index 55973e112f..b9adc28071 100644 > --- a/include/hw/virtio/virtio-gpu.h > +++ b/include/hw/virtio/virtio-gpu.h > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { > int dmabuf_fd; > uint8_t *remapped; > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + /* only blob resource needs this region to be mapped as guest mmio */ > + MemoryRegion *region; > +#endif > + Since this is specific to virgl, let's create a struct that embeds virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu common code.
On 2023/09/16 1:04, Akihiko Odaki wrote: > On 2023/09/15 20:11, Huang Rui wrote: >> From: Antonio Caggiano <antonio.caggiano@collabora.com> >> >> Support BLOB resources creation, mapping and unmapping by calling the >> new stable virglrenderer 0.10 interface. Only enabled when available and >> via the blob config. E.g. -device virtio-vga-gl,blob=true >> >> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >> Signed-off-by: Huang Rui <ray.huang@amd.com> >> --- >> >> V4 -> V5: >> - Use memory_region_init_ram_ptr() instead of >> memory_region_init_ram_device_ptr() (Akihiko) >> >> hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ >> hw/display/virtio-gpu.c | 4 +- >> include/hw/virtio/virtio-gpu.h | 5 + >> meson.build | 4 + >> 4 files changed, 225 insertions(+), 1 deletion(-) >> >> diff --git a/hw/display/virtio-gpu-virgl.c >> b/hw/display/virtio-gpu-virgl.c >> index 312953ec16..563a6f2f58 100644 >> --- a/hw/display/virtio-gpu-virgl.c >> +++ b/hw/display/virtio-gpu-virgl.c >> @@ -17,6 +17,7 @@ >> #include "trace.h" >> #include "hw/virtio/virtio.h" >> #include "hw/virtio/virtio-gpu.h" >> +#include "hw/virtio/virtio-gpu-bswap.h" >> #include "ui/egl-helpers.h" >> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, >> virgl_renderer_resource_create(&args, NULL, 0); >> } >> +static void virgl_resource_destroy(VirtIOGPU *g, >> + struct virtio_gpu_simple_resource >> *res) >> +{ >> + if (!res) >> + return; >> + >> + QTAILQ_REMOVE(&g->reslist, res, next); >> + >> + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); >> + g_free(res->addrs); >> + >> + g_free(res); >> +} >> + >> static void virgl_cmd_resource_unref(VirtIOGPU *g, >> struct virtio_gpu_ctrl_command >> *cmd) >> { >> + struct virtio_gpu_simple_resource *res; >> struct virtio_gpu_resource_unref unref; >> struct iovec *res_iovs = NULL; >> int num_iovs = 0; >> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, >> VIRTIO_GPU_FILL_CMD(unref); >> trace_virtio_gpu_cmd_res_unref(unref.resource_id); >> + res = virtio_gpu_find_resource(g, unref.resource_id); >> + >> virgl_renderer_resource_detach_iov(unref.resource_id, >> &res_iovs, >> &num_iovs); >> if (res_iovs != NULL && num_iovs != 0) { >> virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); >> + if (res) { >> + res->iov = NULL; >> + res->iov_cnt = 0; >> + } >> } >> + >> virgl_renderer_resource_unref(unref.resource_id); >> + >> + virgl_resource_destroy(g, res); This may leak memory region.
On Sat, Sep 16, 2023 at 12:04:17AM +0800, Akihiko Odaki wrote: > On 2023/09/15 20:11, Huang Rui wrote: > > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > > > Support BLOB resources creation, mapping and unmapping by calling the > > new stable virglrenderer 0.10 interface. Only enabled when available and > > via the blob config. E.g. -device virtio-vga-gl,blob=true > > > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > > > V4 -> V5: > > - Use memory_region_init_ram_ptr() instead of > > memory_region_init_ram_device_ptr() (Akihiko) > > > > hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > > hw/display/virtio-gpu.c | 4 +- > > include/hw/virtio/virtio-gpu.h | 5 + > > meson.build | 4 + > > 4 files changed, 225 insertions(+), 1 deletion(-) > > > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > > index 312953ec16..563a6f2f58 100644 > > --- a/hw/display/virtio-gpu-virgl.c > > +++ b/hw/display/virtio-gpu-virgl.c > > @@ -17,6 +17,7 @@ > > #include "trace.h" > > #include "hw/virtio/virtio.h" > > #include "hw/virtio/virtio-gpu.h" > > +#include "hw/virtio/virtio-gpu-bswap.h" > > > > #include "ui/egl-helpers.h" > > > > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > > virgl_renderer_resource_create(&args, NULL, 0); > > } > > > > +static void virgl_resource_destroy(VirtIOGPU *g, > > + struct virtio_gpu_simple_resource *res) > > +{ > > + if (!res) > > + return; > > + > > + QTAILQ_REMOVE(&g->reslist, res, next); > > + > > + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > > + g_free(res->addrs); > > + > > + g_free(res); > > +} > > + > > static void virgl_cmd_resource_unref(VirtIOGPU *g, > > struct virtio_gpu_ctrl_command *cmd) > > { > > + struct virtio_gpu_simple_resource *res; > > struct virtio_gpu_resource_unref unref; > > struct iovec *res_iovs = NULL; > > int num_iovs = 0; > > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > > VIRTIO_GPU_FILL_CMD(unref); > > trace_virtio_gpu_cmd_res_unref(unref.resource_id); > > > > + res = virtio_gpu_find_resource(g, unref.resource_id); > > + > > virgl_renderer_resource_detach_iov(unref.resource_id, > > &res_iovs, > > &num_iovs); > > if (res_iovs != NULL && num_iovs != 0) { > > virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > > + if (res) { > > + res->iov = NULL; > > + res->iov_cnt = 0; > > + } > > } > > + > > virgl_renderer_resource_unref(unref.resource_id); > > + > > + virgl_resource_destroy(g, res); > > } > > > > static void virgl_cmd_context_create(VirtIOGPU *g, > > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, > > g_free(resp); > > } > > > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > > + > > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, > > + struct virtio_gpu_ctrl_command *cmd) > > +{ > > + struct virtio_gpu_simple_resource *res; > > + struct virtio_gpu_resource_create_blob cblob; > > + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; > > + int ret; > > + > > + VIRTIO_GPU_FILL_CMD(cblob); > > + virtio_gpu_create_blob_bswap(&cblob); > > + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); > > + > > + if (cblob.resource_id == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > > + __func__); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = virtio_gpu_find_resource(g, cblob.resource_id); > > + if (res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", > > + __func__, cblob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = g_new0(struct virtio_gpu_simple_resource, 1); > > + if (!res) { > > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > > + return; > > + } > > + > > + res->resource_id = cblob.resource_id; > > + res->blob_size = cblob.size; > > + > > + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { > > + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), > > + cmd, &res->addrs, &res->iov, > > + &res->iov_cnt); > > + if (!ret) { > > + g_free(res); > > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > > + return; > > + } > > + } > > + > > + QTAILQ_INSERT_HEAD(&g->reslist, res, next); > > + > > + virgl_args.res_handle = cblob.resource_id; > > + virgl_args.ctx_id = cblob.hdr.ctx_id; > > + virgl_args.blob_mem = cblob.blob_mem; > > + virgl_args.blob_id = cblob.blob_id; > > + virgl_args.blob_flags = cblob.blob_flags; > > + virgl_args.size = cblob.size; > > + virgl_args.iovecs = res->iov; > > + virgl_args.num_iovs = res->iov_cnt; > > + > > + ret = virgl_renderer_resource_create_blob(&virgl_args); > > + if (ret) { > > + virgl_resource_destroy(g, res); > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", > > + __func__, strerror(-ret)); > > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > > + } > > +} > > + > > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, > > + struct virtio_gpu_ctrl_command *cmd) > > +{ > > + struct virtio_gpu_simple_resource *res; > > + struct virtio_gpu_resource_map_blob mblob; > > + int ret; > > + void *data; > > + uint64_t size; > > + struct virtio_gpu_resp_map_info resp; > > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > > + > > + VIRTIO_GPU_FILL_CMD(mblob); > > + virtio_gpu_map_blob_bswap(&mblob); > > + > > + if (mblob.resource_id == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > > + __func__); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = virtio_gpu_find_resource(g, mblob.resource_id); > > + if (!res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > > + __func__, mblob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + if (res->region) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", > > + __func__, mblob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); > > + if (ret) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", > > + __func__, strerror(-ret)); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res->region = g_new0(MemoryRegion, 1); > > + if (!res->region) { > > + virgl_renderer_resource_unmap(res->resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > > + return; > > + } > > + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); > > + OBJECT(res->region)->free = g_free; > > + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); > > + memory_region_set_enabled(res->region, true); > > + > > + memset(&resp, 0, sizeof(resp)); > > + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; > > + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); > > + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); > > +} > > + > > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, > > + struct virtio_gpu_simple_resource > > + *res) > > +{ > > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > > + > > + if (!res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", > > + __func__, res->resource_id); > > + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > This returns enum virtio_gpu_ctrl_type while > virgl_renderer_resource_unmap() returns -errno and the returned error > code is ignored. > > I think you can just inline this function into > virgl_cmd_resource_unmap_blob(). It will be more concise and consistent > with virgl_cmd_resource_map_blob(). OK, will update in V2. > > > + } > > + > > + memory_region_set_enabled(res->region, false); > > + memory_region_del_subregion(&b->hostmem, res->region); > > + object_unparent(OBJECT(res->region)); > > + res->region = NULL; > > + > > + return virgl_renderer_resource_unmap(res->resource_id); > > +} > > + > > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > > + struct virtio_gpu_ctrl_command *cmd) > > +{ > > + struct virtio_gpu_simple_resource *res; > > + struct virtio_gpu_resource_unmap_blob ublob; > > + VIRTIO_GPU_FILL_CMD(ublob); > > + virtio_gpu_unmap_blob_bswap(&ublob); > > + > > + if (ublob.resource_id == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > > + __func__); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = virtio_gpu_find_resource(g, ublob.resource_id); > > + if (!res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > > + __func__, ublob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + virtio_gpu_virgl_resource_unmap(g, res); > > +} > > + > > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > > + > > void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > > struct virtio_gpu_ctrl_command *cmd) > > { > > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > > case VIRTIO_GPU_CMD_GET_EDID: > > virtio_gpu_get_edid(g, cmd); > > break; > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > > + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: > > + virgl_cmd_resource_create_blob(g, cmd); > > + break; > > + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: > > + virgl_cmd_resource_map_blob(g, cmd); > > + break; > > + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > > + virgl_cmd_resource_unmap_blob(g, cmd); > > + break; > > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > > default: > > cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > > break; > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > index 5b7a7eab4f..cc4c1f81bb 100644 > > --- a/hw/display/virtio-gpu.c > > +++ b/hw/display/virtio-gpu.c > > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > return; > > } > > > > +#ifndef HAVE_VIRGL_RESOURCE_BLOB > > if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { > > - error_setg(errp, "blobs and virgl are not compatible (yet)"); > > + error_setg(errp, "Linked virglrenderer does not support blob resources"); > > return; > > } > > +#endif > > } > > > > if (!virtio_gpu_base_device_realize(qdev, > > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > > index 55973e112f..b9adc28071 100644 > > --- a/include/hw/virtio/virtio-gpu.h > > +++ b/include/hw/virtio/virtio-gpu.h > > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { > > int dmabuf_fd; > > uint8_t *remapped; > > > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > > + /* only blob resource needs this region to be mapped as guest mmio */ > > + MemoryRegion *region; > > +#endif > > + > > Since this is specific to virgl, let's create a struct that embeds > virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu > common code. I am thinking should I modify the whole virtio-gpu-virgl.c to use the new struct like virgl_gpu_resource which has virtio_gpu_simple_resource embedded? Then in virgl, we can use this structure to indicate the resource for virglrenderer. Thanks, Ray
On 2023/09/18 17:36, Huang Rui wrote: > On Sat, Sep 16, 2023 at 12:04:17AM +0800, Akihiko Odaki wrote: >> On 2023/09/15 20:11, Huang Rui wrote: >>> From: Antonio Caggiano <antonio.caggiano@collabora.com> >>> >>> Support BLOB resources creation, mapping and unmapping by calling the >>> new stable virglrenderer 0.10 interface. Only enabled when available and >>> via the blob config. E.g. -device virtio-vga-gl,blob=true >>> >>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> --- >>> >>> V4 -> V5: >>> - Use memory_region_init_ram_ptr() instead of >>> memory_region_init_ram_device_ptr() (Akihiko) >>> >>> hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ >>> hw/display/virtio-gpu.c | 4 +- >>> include/hw/virtio/virtio-gpu.h | 5 + >>> meson.build | 4 + >>> 4 files changed, 225 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c >>> index 312953ec16..563a6f2f58 100644 >>> --- a/hw/display/virtio-gpu-virgl.c >>> +++ b/hw/display/virtio-gpu-virgl.c >>> @@ -17,6 +17,7 @@ >>> #include "trace.h" >>> #include "hw/virtio/virtio.h" >>> #include "hw/virtio/virtio-gpu.h" >>> +#include "hw/virtio/virtio-gpu-bswap.h" >>> >>> #include "ui/egl-helpers.h" >>> >>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, >>> virgl_renderer_resource_create(&args, NULL, 0); >>> } >>> >>> +static void virgl_resource_destroy(VirtIOGPU *g, >>> + struct virtio_gpu_simple_resource *res) >>> +{ >>> + if (!res) >>> + return; >>> + >>> + QTAILQ_REMOVE(&g->reslist, res, next); >>> + >>> + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); >>> + g_free(res->addrs); >>> + >>> + g_free(res); >>> +} >>> + >>> static void virgl_cmd_resource_unref(VirtIOGPU *g, >>> struct virtio_gpu_ctrl_command *cmd) >>> { >>> + struct virtio_gpu_simple_resource *res; >>> struct virtio_gpu_resource_unref unref; >>> struct iovec *res_iovs = NULL; >>> int num_iovs = 0; >>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, >>> VIRTIO_GPU_FILL_CMD(unref); >>> trace_virtio_gpu_cmd_res_unref(unref.resource_id); >>> >>> + res = virtio_gpu_find_resource(g, unref.resource_id); >>> + >>> virgl_renderer_resource_detach_iov(unref.resource_id, >>> &res_iovs, >>> &num_iovs); >>> if (res_iovs != NULL && num_iovs != 0) { >>> virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); >>> + if (res) { >>> + res->iov = NULL; >>> + res->iov_cnt = 0; >>> + } >>> } >>> + >>> virgl_renderer_resource_unref(unref.resource_id); >>> + >>> + virgl_resource_destroy(g, res); >>> } >>> >>> static void virgl_cmd_context_create(VirtIOGPU *g, >>> @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, >>> g_free(resp); >>> } >>> >>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB >>> + >>> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, >>> + struct virtio_gpu_ctrl_command *cmd) >>> +{ >>> + struct virtio_gpu_simple_resource *res; >>> + struct virtio_gpu_resource_create_blob cblob; >>> + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; >>> + int ret; >>> + >>> + VIRTIO_GPU_FILL_CMD(cblob); >>> + virtio_gpu_create_blob_bswap(&cblob); >>> + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); >>> + >>> + if (cblob.resource_id == 0) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", >>> + __func__); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + res = virtio_gpu_find_resource(g, cblob.resource_id); >>> + if (res) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", >>> + __func__, cblob.resource_id); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + res = g_new0(struct virtio_gpu_simple_resource, 1); >>> + if (!res) { >>> + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; >>> + return; >>> + } >>> + >>> + res->resource_id = cblob.resource_id; >>> + res->blob_size = cblob.size; >>> + >>> + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { >>> + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), >>> + cmd, &res->addrs, &res->iov, >>> + &res->iov_cnt); >>> + if (!ret) { >>> + g_free(res); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; >>> + return; >>> + } >>> + } >>> + >>> + QTAILQ_INSERT_HEAD(&g->reslist, res, next); >>> + >>> + virgl_args.res_handle = cblob.resource_id; >>> + virgl_args.ctx_id = cblob.hdr.ctx_id; >>> + virgl_args.blob_mem = cblob.blob_mem; >>> + virgl_args.blob_id = cblob.blob_id; >>> + virgl_args.blob_flags = cblob.blob_flags; >>> + virgl_args.size = cblob.size; >>> + virgl_args.iovecs = res->iov; >>> + virgl_args.num_iovs = res->iov_cnt; >>> + >>> + ret = virgl_renderer_resource_create_blob(&virgl_args); >>> + if (ret) { >>> + virgl_resource_destroy(g, res); >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", >>> + __func__, strerror(-ret)); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; >>> + } >>> +} >>> + >>> +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, >>> + struct virtio_gpu_ctrl_command *cmd) >>> +{ >>> + struct virtio_gpu_simple_resource *res; >>> + struct virtio_gpu_resource_map_blob mblob; >>> + int ret; >>> + void *data; >>> + uint64_t size; >>> + struct virtio_gpu_resp_map_info resp; >>> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); >>> + >>> + VIRTIO_GPU_FILL_CMD(mblob); >>> + virtio_gpu_map_blob_bswap(&mblob); >>> + >>> + if (mblob.resource_id == 0) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", >>> + __func__); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + res = virtio_gpu_find_resource(g, mblob.resource_id); >>> + if (!res) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", >>> + __func__, mblob.resource_id); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + if (res->region) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", >>> + __func__, mblob.resource_id); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); >>> + if (ret) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", >>> + __func__, strerror(-ret)); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + res->region = g_new0(MemoryRegion, 1); >>> + if (!res->region) { >>> + virgl_renderer_resource_unmap(res->resource_id); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; >>> + return; >>> + } >>> + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); >>> + OBJECT(res->region)->free = g_free; >>> + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); >>> + memory_region_set_enabled(res->region, true); >>> + >>> + memset(&resp, 0, sizeof(resp)); >>> + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; >>> + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); >>> + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); >>> +} >>> + >>> +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, >>> + struct virtio_gpu_simple_resource >>> + *res) >>> +{ >>> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); >>> + >>> + if (!res) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", >>> + __func__, res->resource_id); >>> + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >> >> This returns enum virtio_gpu_ctrl_type while >> virgl_renderer_resource_unmap() returns -errno and the returned error >> code is ignored. >> >> I think you can just inline this function into >> virgl_cmd_resource_unmap_blob(). It will be more concise and consistent >> with virgl_cmd_resource_map_blob(). > > OK, will update in V2. > >> >>> + } >>> + >>> + memory_region_set_enabled(res->region, false); >>> + memory_region_del_subregion(&b->hostmem, res->region); >>> + object_unparent(OBJECT(res->region)); >>> + res->region = NULL; >>> + >>> + return virgl_renderer_resource_unmap(res->resource_id); >>> +} >>> + >>> +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, >>> + struct virtio_gpu_ctrl_command *cmd) >>> +{ >>> + struct virtio_gpu_simple_resource *res; >>> + struct virtio_gpu_resource_unmap_blob ublob; >>> + VIRTIO_GPU_FILL_CMD(ublob); >>> + virtio_gpu_unmap_blob_bswap(&ublob); >>> + >>> + if (ublob.resource_id == 0) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", >>> + __func__); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + res = virtio_gpu_find_resource(g, ublob.resource_id); >>> + if (!res) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", >>> + __func__, ublob.resource_id); >>> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; >>> + return; >>> + } >>> + >>> + virtio_gpu_virgl_resource_unmap(g, res); >>> +} >>> + >>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ >>> + >>> void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, >>> struct virtio_gpu_ctrl_command *cmd) >>> { >>> @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, >>> case VIRTIO_GPU_CMD_GET_EDID: >>> virtio_gpu_get_edid(g, cmd); >>> break; >>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB >>> + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: >>> + virgl_cmd_resource_create_blob(g, cmd); >>> + break; >>> + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: >>> + virgl_cmd_resource_map_blob(g, cmd); >>> + break; >>> + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: >>> + virgl_cmd_resource_unmap_blob(g, cmd); >>> + break; >>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ >>> default: >>> cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; >>> break; >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c >>> index 5b7a7eab4f..cc4c1f81bb 100644 >>> --- a/hw/display/virtio-gpu.c >>> +++ b/hw/display/virtio-gpu.c >>> @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) >>> return; >>> } >>> >>> +#ifndef HAVE_VIRGL_RESOURCE_BLOB >>> if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { >>> - error_setg(errp, "blobs and virgl are not compatible (yet)"); >>> + error_setg(errp, "Linked virglrenderer does not support blob resources"); >>> return; >>> } >>> +#endif >>> } >>> >>> if (!virtio_gpu_base_device_realize(qdev, >>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h >>> index 55973e112f..b9adc28071 100644 >>> --- a/include/hw/virtio/virtio-gpu.h >>> +++ b/include/hw/virtio/virtio-gpu.h >>> @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { >>> int dmabuf_fd; >>> uint8_t *remapped; >>> >>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB >>> + /* only blob resource needs this region to be mapped as guest mmio */ >>> + MemoryRegion *region; >>> +#endif >>> + >> >> Since this is specific to virgl, let's create a struct that embeds >> virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu >> common code. > > I am thinking should I modify the whole virtio-gpu-virgl.c to use the new > struct like virgl_gpu_resource which has virtio_gpu_simple_resource > embedded? Then in virgl, we can use this structure to indicate the resource > for virglrenderer. Yes, that's what I suggest.
Hi On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote: > > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > Support BLOB resources creation, mapping and unmapping by calling the > new stable virglrenderer 0.10 interface. Only enabled when available and > via the blob config. E.g. -device virtio-vga-gl,blob=true > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > --- > > V4 -> V5: > - Use memory_region_init_ram_ptr() instead of > memory_region_init_ram_device_ptr() (Akihiko) > > hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > hw/display/virtio-gpu.c | 4 +- > include/hw/virtio/virtio-gpu.h | 5 + > meson.build | 4 + > 4 files changed, 225 insertions(+), 1 deletion(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index 312953ec16..563a6f2f58 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -17,6 +17,7 @@ > #include "trace.h" > #include "hw/virtio/virtio.h" > #include "hw/virtio/virtio-gpu.h" > +#include "hw/virtio/virtio-gpu-bswap.h" > > #include "ui/egl-helpers.h" > > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > virgl_renderer_resource_create(&args, NULL, 0); > } > > +static void virgl_resource_destroy(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res) > +{ > + if (!res) > + return; > + QEMU coding style imposes braces > + QTAILQ_REMOVE(&g->reslist, res, next); > + > + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > + g_free(res->addrs); > + > + g_free(res); If you embed virtio_gpu_simple_resource in a new struct, we should instead call the existing virtio_gpu_resource_destroy() I guess. > +} > + > static void virgl_cmd_resource_unref(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > + struct virtio_gpu_simple_resource *res; > struct virtio_gpu_resource_unref unref; > struct iovec *res_iovs = NULL; > int num_iovs = 0; > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > VIRTIO_GPU_FILL_CMD(unref); > trace_virtio_gpu_cmd_res_unref(unref.resource_id); > > + res = virtio_gpu_find_resource(g, unref.resource_id); > + > virgl_renderer_resource_detach_iov(unref.resource_id, > &res_iovs, > &num_iovs); > if (res_iovs != NULL && num_iovs != 0) { > virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > + if (res) { > + res->iov = NULL; > + res->iov_cnt = 0; > + } > } > + > virgl_renderer_resource_unref(unref.resource_id); > + > + virgl_resource_destroy(g, res); > } > > static void virgl_cmd_context_create(VirtIOGPU *g, > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, > g_free(resp); > } > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_create_blob cblob; > + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; > + int ret; > + > + VIRTIO_GPU_FILL_CMD(cblob); > + virtio_gpu_create_blob_bswap(&cblob); > + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); > + > + if (cblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, cblob.resource_id); > + if (res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", > + __func__, cblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = g_new0(struct virtio_gpu_simple_resource, 1); > + if (!res) { > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; needless error handling, glib will abort() on OOM. > + } > + > + res->resource_id = cblob.resource_id; > + res->blob_size = cblob.size; > + > + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { > + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), > + cmd, &res->addrs, &res->iov, > + &res->iov_cnt); > + if (!ret) { > + g_free(res); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + return; > + } > + } > + > + QTAILQ_INSERT_HEAD(&g->reslist, res, next); > + > + virgl_args.res_handle = cblob.resource_id; > + virgl_args.ctx_id = cblob.hdr.ctx_id; > + virgl_args.blob_mem = cblob.blob_mem; > + virgl_args.blob_id = cblob.blob_id; > + virgl_args.blob_flags = cblob.blob_flags; > + virgl_args.size = cblob.size; > + virgl_args.iovecs = res->iov; > + virgl_args.num_iovs = res->iov_cnt; > + > + ret = virgl_renderer_resource_create_blob(&virgl_args); > + if (ret) { > + virgl_resource_destroy(g, res); > + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + } > +} > + > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_map_blob mblob; > + int ret; > + void *data; > + uint64_t size; > + struct virtio_gpu_resp_map_info resp; > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + VIRTIO_GPU_FILL_CMD(mblob); > + virtio_gpu_map_blob_bswap(&mblob); > + > + if (mblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, mblob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + if (res->region) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); > + if (ret) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res->region = g_new0(MemoryRegion, 1); > + if (!res->region) { no need for OOM handling here too > + virgl_renderer_resource_unmap(res->resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; > + } > + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); > + OBJECT(res->region)->free = g_free; > + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); > + memory_region_set_enabled(res->region, true); > + > + memset(&resp, 0, sizeof(resp)); > + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; > + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); > + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); > +} > + > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, > + struct virtio_gpu_simple_resource > + *res) > +{ > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", > + __func__, res->resource_id); > + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + } > + > + memory_region_set_enabled(res->region, false); > + memory_region_del_subregion(&b->hostmem, res->region); > + object_unparent(OBJECT(res->region)); > + res->region = NULL; > + > + return virgl_renderer_resource_unmap(res->resource_id); > +} > + > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resource_unmap_blob ublob; > + VIRTIO_GPU_FILL_CMD(ublob); > + virtio_gpu_unmap_blob_bswap(&ublob); > + > + if (ublob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, ublob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, ublob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + virtio_gpu_virgl_resource_unmap(g, res); > +} > + > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > + > void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > case VIRTIO_GPU_CMD_GET_EDID: > virtio_gpu_get_edid(g, cmd); > break; > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: > + virgl_cmd_resource_create_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: > + virgl_cmd_resource_map_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > + virgl_cmd_resource_unmap_blob(g, cmd); > + break; > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > default: > cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > break; > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 5b7a7eab4f..cc4c1f81bb 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > return; > } > > +#ifndef HAVE_VIRGL_RESOURCE_BLOB > if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { > - error_setg(errp, "blobs and virgl are not compatible (yet)"); > + error_setg(errp, "Linked virglrenderer does not support blob resources"); > return; > } > +#endif > } > > if (!virtio_gpu_base_device_realize(qdev, > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > index 55973e112f..b9adc28071 100644 > --- a/include/hw/virtio/virtio-gpu.h > +++ b/include/hw/virtio/virtio-gpu.h > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { > int dmabuf_fd; > uint8_t *remapped; > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + /* only blob resource needs this region to be mapped as guest mmio */ > + MemoryRegion *region; > +#endif > + > QTAILQ_ENTRY(virtio_gpu_simple_resource) next; > }; > > diff --git a/meson.build b/meson.build > index ff20d3c249..f7b744ab82 100644 > --- a/meson.build > +++ b/meson.build > @@ -1072,6 +1072,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu > cc.has_function('virgl_renderer_context_create_with_flags', > prefix: '#include <virglrenderer.h>', > dependencies: virgl)) > + config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB', > + cc.has_function('virgl_renderer_resource_create_blob', > + prefix: '#include <virglrenderer.h>', > + dependencies: virgl)) better moved under the if virgl.found() block > endif > blkio = not_found > if not get_option('blkio').auto() or have_block > -- > 2.34.1 > >
On Sat, Sep 16, 2023 at 12:37:29AM +0800, Akihiko Odaki wrote: > On 2023/09/16 1:04, Akihiko Odaki wrote: > > On 2023/09/15 20:11, Huang Rui wrote: > >> From: Antonio Caggiano <antonio.caggiano@collabora.com> > >> > >> Support BLOB resources creation, mapping and unmapping by calling the > >> new stable virglrenderer 0.10 interface. Only enabled when available and > >> via the blob config. E.g. -device virtio-vga-gl,blob=true > >> > >> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > >> Signed-off-by: Huang Rui <ray.huang@amd.com> > >> --- > >> > >> V4 -> V5: > >> - Use memory_region_init_ram_ptr() instead of > >> memory_region_init_ram_device_ptr() (Akihiko) > >> > >> hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > >> hw/display/virtio-gpu.c | 4 +- > >> include/hw/virtio/virtio-gpu.h | 5 + > >> meson.build | 4 + > >> 4 files changed, 225 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/display/virtio-gpu-virgl.c > >> b/hw/display/virtio-gpu-virgl.c > >> index 312953ec16..563a6f2f58 100644 > >> --- a/hw/display/virtio-gpu-virgl.c > >> +++ b/hw/display/virtio-gpu-virgl.c > >> @@ -17,6 +17,7 @@ > >> #include "trace.h" > >> #include "hw/virtio/virtio.h" > >> #include "hw/virtio/virtio-gpu.h" > >> +#include "hw/virtio/virtio-gpu-bswap.h" > >> #include "ui/egl-helpers.h" > >> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > >> virgl_renderer_resource_create(&args, NULL, 0); > >> } > >> +static void virgl_resource_destroy(VirtIOGPU *g, > >> + struct virtio_gpu_simple_resource > >> *res) > >> +{ > >> + if (!res) > >> + return; > >> + > >> + QTAILQ_REMOVE(&g->reslist, res, next); > >> + > >> + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > >> + g_free(res->addrs); > >> + > >> + g_free(res); > >> +} > >> + > >> static void virgl_cmd_resource_unref(VirtIOGPU *g, > >> struct virtio_gpu_ctrl_command > >> *cmd) > >> { > >> + struct virtio_gpu_simple_resource *res; > >> struct virtio_gpu_resource_unref unref; > >> struct iovec *res_iovs = NULL; > >> int num_iovs = 0; > >> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > >> VIRTIO_GPU_FILL_CMD(unref); > >> trace_virtio_gpu_cmd_res_unref(unref.resource_id); > >> + res = virtio_gpu_find_resource(g, unref.resource_id); > >> + > >> virgl_renderer_resource_detach_iov(unref.resource_id, > >> &res_iovs, > >> &num_iovs); > >> if (res_iovs != NULL && num_iovs != 0) { > >> virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > >> + if (res) { > >> + res->iov = NULL; > >> + res->iov_cnt = 0; > >> + } > >> } > >> + > >> virgl_renderer_resource_unref(unref.resource_id); > >> + > >> + virgl_resource_destroy(g, res); > > This may leak memory region. The memory region should be freed under virgl_cmd_resource_unmap_blob() which is calling memory_region_del_subregion(&b->hostmem, res->region). Because this region is created by map_blob(). Do we have the case to call virgl_cmd_resource_unref() without calling virgl_cmd_resource_unmap_blob() for blob memory? Thanks, Ray
On 2023/09/20 14:50, Huang Rui wrote: > On Sat, Sep 16, 2023 at 12:37:29AM +0800, Akihiko Odaki wrote: >> On 2023/09/16 1:04, Akihiko Odaki wrote: >>> On 2023/09/15 20:11, Huang Rui wrote: >>>> From: Antonio Caggiano <antonio.caggiano@collabora.com> >>>> >>>> Support BLOB resources creation, mapping and unmapping by calling the >>>> new stable virglrenderer 0.10 interface. Only enabled when available and >>>> via the blob config. E.g. -device virtio-vga-gl,blob=true >>>> >>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> --- >>>> >>>> V4 -> V5: >>>> - Use memory_region_init_ram_ptr() instead of >>>> memory_region_init_ram_device_ptr() (Akihiko) >>>> >>>> hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ >>>> hw/display/virtio-gpu.c | 4 +- >>>> include/hw/virtio/virtio-gpu.h | 5 + >>>> meson.build | 4 + >>>> 4 files changed, 225 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/display/virtio-gpu-virgl.c >>>> b/hw/display/virtio-gpu-virgl.c >>>> index 312953ec16..563a6f2f58 100644 >>>> --- a/hw/display/virtio-gpu-virgl.c >>>> +++ b/hw/display/virtio-gpu-virgl.c >>>> @@ -17,6 +17,7 @@ >>>> #include "trace.h" >>>> #include "hw/virtio/virtio.h" >>>> #include "hw/virtio/virtio-gpu.h" >>>> +#include "hw/virtio/virtio-gpu-bswap.h" >>>> #include "ui/egl-helpers.h" >>>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, >>>> virgl_renderer_resource_create(&args, NULL, 0); >>>> } >>>> +static void virgl_resource_destroy(VirtIOGPU *g, >>>> + struct virtio_gpu_simple_resource >>>> *res) >>>> +{ >>>> + if (!res) >>>> + return; >>>> + >>>> + QTAILQ_REMOVE(&g->reslist, res, next); >>>> + >>>> + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); >>>> + g_free(res->addrs); >>>> + >>>> + g_free(res); >>>> +} >>>> + >>>> static void virgl_cmd_resource_unref(VirtIOGPU *g, >>>> struct virtio_gpu_ctrl_command >>>> *cmd) >>>> { >>>> + struct virtio_gpu_simple_resource *res; >>>> struct virtio_gpu_resource_unref unref; >>>> struct iovec *res_iovs = NULL; >>>> int num_iovs = 0; >>>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, >>>> VIRTIO_GPU_FILL_CMD(unref); >>>> trace_virtio_gpu_cmd_res_unref(unref.resource_id); >>>> + res = virtio_gpu_find_resource(g, unref.resource_id); >>>> + >>>> virgl_renderer_resource_detach_iov(unref.resource_id, >>>> &res_iovs, >>>> &num_iovs); >>>> if (res_iovs != NULL && num_iovs != 0) { >>>> virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); >>>> + if (res) { >>>> + res->iov = NULL; >>>> + res->iov_cnt = 0; >>>> + } >>>> } >>>> + >>>> virgl_renderer_resource_unref(unref.resource_id); >>>> + >>>> + virgl_resource_destroy(g, res); >> >> This may leak memory region. > > The memory region should be freed under virgl_cmd_resource_unmap_blob() > which is calling memory_region_del_subregion(&b->hostmem, res->region). > Because this region is created by map_blob(). Do we have the case to call > virgl_cmd_resource_unref() without calling virgl_cmd_resource_unmap_blob() > for blob memory? Calling virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref() in order is a guest's responsibility, and we are required to prepare for broken guests.
On Wed, Sep 20, 2023 at 01:54:03PM +0800, Akihiko Odaki wrote: > On 2023/09/20 14:50, Huang Rui wrote: > > On Sat, Sep 16, 2023 at 12:37:29AM +0800, Akihiko Odaki wrote: > >> On 2023/09/16 1:04, Akihiko Odaki wrote: > >>> On 2023/09/15 20:11, Huang Rui wrote: > >>>> From: Antonio Caggiano <antonio.caggiano@collabora.com> > >>>> > >>>> Support BLOB resources creation, mapping and unmapping by calling the > >>>> new stable virglrenderer 0.10 interface. Only enabled when available and > >>>> via the blob config. E.g. -device virtio-vga-gl,blob=true > >>>> > >>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> > >>>> --- > >>>> > >>>> V4 -> V5: > >>>> - Use memory_region_init_ram_ptr() instead of > >>>> memory_region_init_ram_device_ptr() (Akihiko) > >>>> > >>>> hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > >>>> hw/display/virtio-gpu.c | 4 +- > >>>> include/hw/virtio/virtio-gpu.h | 5 + > >>>> meson.build | 4 + > >>>> 4 files changed, 225 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/hw/display/virtio-gpu-virgl.c > >>>> b/hw/display/virtio-gpu-virgl.c > >>>> index 312953ec16..563a6f2f58 100644 > >>>> --- a/hw/display/virtio-gpu-virgl.c > >>>> +++ b/hw/display/virtio-gpu-virgl.c > >>>> @@ -17,6 +17,7 @@ > >>>> #include "trace.h" > >>>> #include "hw/virtio/virtio.h" > >>>> #include "hw/virtio/virtio-gpu.h" > >>>> +#include "hw/virtio/virtio-gpu-bswap.h" > >>>> #include "ui/egl-helpers.h" > >>>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > >>>> virgl_renderer_resource_create(&args, NULL, 0); > >>>> } > >>>> +static void virgl_resource_destroy(VirtIOGPU *g, > >>>> + struct virtio_gpu_simple_resource > >>>> *res) > >>>> +{ > >>>> + if (!res) > >>>> + return; > >>>> + > >>>> + QTAILQ_REMOVE(&g->reslist, res, next); > >>>> + > >>>> + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > >>>> + g_free(res->addrs); > >>>> + > >>>> + g_free(res); > >>>> +} > >>>> + > >>>> static void virgl_cmd_resource_unref(VirtIOGPU *g, > >>>> struct virtio_gpu_ctrl_command > >>>> *cmd) > >>>> { > >>>> + struct virtio_gpu_simple_resource *res; > >>>> struct virtio_gpu_resource_unref unref; > >>>> struct iovec *res_iovs = NULL; > >>>> int num_iovs = 0; > >>>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > >>>> VIRTIO_GPU_FILL_CMD(unref); > >>>> trace_virtio_gpu_cmd_res_unref(unref.resource_id); > >>>> + res = virtio_gpu_find_resource(g, unref.resource_id); > >>>> + > >>>> virgl_renderer_resource_detach_iov(unref.resource_id, > >>>> &res_iovs, > >>>> &num_iovs); > >>>> if (res_iovs != NULL && num_iovs != 0) { > >>>> virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > >>>> + if (res) { > >>>> + res->iov = NULL; > >>>> + res->iov_cnt = 0; > >>>> + } > >>>> } > >>>> + > >>>> virgl_renderer_resource_unref(unref.resource_id); > >>>> + > >>>> + virgl_resource_destroy(g, res); > >> > >> This may leak memory region. > > > > The memory region should be freed under virgl_cmd_resource_unmap_blob() > > which is calling memory_region_del_subregion(&b->hostmem, res->region). > > Because this region is created by map_blob(). Do we have the case to call > > virgl_cmd_resource_unref() without calling virgl_cmd_resource_unmap_blob() > > for blob memory? > > Calling virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref() > in order is a guest's responsibility, and we are required to prepare for > broken guests. OK, no problem. I will update this in V6. Thanks, Ray
On Tue, Sep 19, 2023 at 04:44:01PM +0800, Marc-André Lureau wrote: > Hi > > On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote: > > > > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > > > Support BLOB resources creation, mapping and unmapping by calling the > > new stable virglrenderer 0.10 interface. Only enabled when available and > > via the blob config. E.g. -device virtio-vga-gl,blob=true > > > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > --- > > > > V4 -> V5: > > - Use memory_region_init_ram_ptr() instead of > > memory_region_init_ram_device_ptr() (Akihiko) > > > > hw/display/virtio-gpu-virgl.c | 213 +++++++++++++++++++++++++++++++++ > > hw/display/virtio-gpu.c | 4 +- > > include/hw/virtio/virtio-gpu.h | 5 + > > meson.build | 4 + > > 4 files changed, 225 insertions(+), 1 deletion(-) > > > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > > index 312953ec16..563a6f2f58 100644 > > --- a/hw/display/virtio-gpu-virgl.c > > +++ b/hw/display/virtio-gpu-virgl.c > > @@ -17,6 +17,7 @@ > > #include "trace.h" > > #include "hw/virtio/virtio.h" > > #include "hw/virtio/virtio-gpu.h" > > +#include "hw/virtio/virtio-gpu-bswap.h" > > > > #include "ui/egl-helpers.h" > > > > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, > > virgl_renderer_resource_create(&args, NULL, 0); > > } > > > > +static void virgl_resource_destroy(VirtIOGPU *g, > > + struct virtio_gpu_simple_resource *res) > > +{ > > + if (!res) > > + return; > > + > > QEMU coding style imposes braces > Thanks for reminder. > > + QTAILQ_REMOVE(&g->reslist, res, next); > > + > > + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); > > + g_free(res->addrs); > > + > > + g_free(res); > > If you embed virtio_gpu_simple_resource in a new struct, we should > instead call the existing virtio_gpu_resource_destroy() I guess. Yes, but I didn't call virtio_gpu_resource_destroy() here, I am using the virgl_resource_destroy() and the input should be the virgl resource in next version. May I know whether I miss understood your point? > > > +} > > + > > static void virgl_cmd_resource_unref(VirtIOGPU *g, > > struct virtio_gpu_ctrl_command *cmd) > > { > > + struct virtio_gpu_simple_resource *res; > > struct virtio_gpu_resource_unref unref; > > struct iovec *res_iovs = NULL; > > int num_iovs = 0; > > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > > VIRTIO_GPU_FILL_CMD(unref); > > trace_virtio_gpu_cmd_res_unref(unref.resource_id); > > > > + res = virtio_gpu_find_resource(g, unref.resource_id); > > + > > virgl_renderer_resource_detach_iov(unref.resource_id, > > &res_iovs, > > &num_iovs); > > if (res_iovs != NULL && num_iovs != 0) { > > virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); > > + if (res) { > > + res->iov = NULL; > > + res->iov_cnt = 0; > > + } > > } > > + > > virgl_renderer_resource_unref(unref.resource_id); > > + > > + virgl_resource_destroy(g, res); > > } > > > > static void virgl_cmd_context_create(VirtIOGPU *g, > > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, > > g_free(resp); > > } > > > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > > + > > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, > > + struct virtio_gpu_ctrl_command *cmd) > > +{ > > + struct virtio_gpu_simple_resource *res; > > + struct virtio_gpu_resource_create_blob cblob; > > + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; > > + int ret; > > + > > + VIRTIO_GPU_FILL_CMD(cblob); > > + virtio_gpu_create_blob_bswap(&cblob); > > + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); > > + > > + if (cblob.resource_id == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > > + __func__); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = virtio_gpu_find_resource(g, cblob.resource_id); > > + if (res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", > > + __func__, cblob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = g_new0(struct virtio_gpu_simple_resource, 1); > > + if (!res) { > > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > > + return; > > needless error handling, glib will abort() on OOM. OK, I will remove all simliar error handling in next version. > > > + } > > + > > + res->resource_id = cblob.resource_id; > > + res->blob_size = cblob.size; > > + > > + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { > > + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), > > + cmd, &res->addrs, &res->iov, > > + &res->iov_cnt); > > + if (!ret) { > > + g_free(res); > > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > > + return; > > + } > > + } > > + > > + QTAILQ_INSERT_HEAD(&g->reslist, res, next); > > + > > + virgl_args.res_handle = cblob.resource_id; > > + virgl_args.ctx_id = cblob.hdr.ctx_id; > > + virgl_args.blob_mem = cblob.blob_mem; > > + virgl_args.blob_id = cblob.blob_id; > > + virgl_args.blob_flags = cblob.blob_flags; > > + virgl_args.size = cblob.size; > > + virgl_args.iovecs = res->iov; > > + virgl_args.num_iovs = res->iov_cnt; > > + > > + ret = virgl_renderer_resource_create_blob(&virgl_args); > > + if (ret) { > > + virgl_resource_destroy(g, res); > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", > > + __func__, strerror(-ret)); > > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > > + } > > +} > > + > > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, > > + struct virtio_gpu_ctrl_command *cmd) > > +{ > > + struct virtio_gpu_simple_resource *res; > > + struct virtio_gpu_resource_map_blob mblob; > > + int ret; > > + void *data; > > + uint64_t size; > > + struct virtio_gpu_resp_map_info resp; > > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > > + > > + VIRTIO_GPU_FILL_CMD(mblob); > > + virtio_gpu_map_blob_bswap(&mblob); > > + > > + if (mblob.resource_id == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > > + __func__); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = virtio_gpu_find_resource(g, mblob.resource_id); > > + if (!res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > > + __func__, mblob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + if (res->region) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", > > + __func__, mblob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); > > + if (ret) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", > > + __func__, strerror(-ret)); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res->region = g_new0(MemoryRegion, 1); > > + if (!res->region) { > > no need for OOM handling here too > > > + virgl_renderer_resource_unmap(res->resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > > + return; > > + } > > + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); > > + OBJECT(res->region)->free = g_free; > > + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); > > + memory_region_set_enabled(res->region, true); > > + > > + memset(&resp, 0, sizeof(resp)); > > + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; > > + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); > > + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); > > +} > > + > > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, > > + struct virtio_gpu_simple_resource > > + *res) > > +{ > > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > > + > > + if (!res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", > > + __func__, res->resource_id); > > + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + } > > + > > + memory_region_set_enabled(res->region, false); > > + memory_region_del_subregion(&b->hostmem, res->region); > > + object_unparent(OBJECT(res->region)); > > + res->region = NULL; > > + > > + return virgl_renderer_resource_unmap(res->resource_id); > > +} > > + > > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > > + struct virtio_gpu_ctrl_command *cmd) > > +{ > > + struct virtio_gpu_simple_resource *res; > > + struct virtio_gpu_resource_unmap_blob ublob; > > + VIRTIO_GPU_FILL_CMD(ublob); > > + virtio_gpu_unmap_blob_bswap(&ublob); > > + > > + if (ublob.resource_id == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > > + __func__); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + res = virtio_gpu_find_resource(g, ublob.resource_id); > > + if (!res) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > > + __func__, ublob.resource_id); > > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > > + return; > > + } > > + > > + virtio_gpu_virgl_resource_unmap(g, res); > > +} > > + > > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > > + > > void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > > struct virtio_gpu_ctrl_command *cmd) > > { > > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > > case VIRTIO_GPU_CMD_GET_EDID: > > virtio_gpu_get_edid(g, cmd); > > break; > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > > + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: > > + virgl_cmd_resource_create_blob(g, cmd); > > + break; > > + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: > > + virgl_cmd_resource_map_blob(g, cmd); > > + break; > > + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > > + virgl_cmd_resource_unmap_blob(g, cmd); > > + break; > > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > > default: > > cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > > break; > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > index 5b7a7eab4f..cc4c1f81bb 100644 > > --- a/hw/display/virtio-gpu.c > > +++ b/hw/display/virtio-gpu.c > > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > return; > > } > > > > +#ifndef HAVE_VIRGL_RESOURCE_BLOB > > if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { > > - error_setg(errp, "blobs and virgl are not compatible (yet)"); > > + error_setg(errp, "Linked virglrenderer does not support blob resources"); > > return; > > } > > +#endif > > } > > > > if (!virtio_gpu_base_device_realize(qdev, > > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > > index 55973e112f..b9adc28071 100644 > > --- a/include/hw/virtio/virtio-gpu.h > > +++ b/include/hw/virtio/virtio-gpu.h > > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { > > int dmabuf_fd; > > uint8_t *remapped; > > > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > > + /* only blob resource needs this region to be mapped as guest mmio */ > > + MemoryRegion *region; > > +#endif > > + > > QTAILQ_ENTRY(virtio_gpu_simple_resource) next; > > }; > > > > diff --git a/meson.build b/meson.build > > index ff20d3c249..f7b744ab82 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -1072,6 +1072,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu > > cc.has_function('virgl_renderer_context_create_with_flags', > > prefix: '#include <virglrenderer.h>', > > dependencies: virgl)) > > + config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB', > > + cc.has_function('virgl_renderer_resource_create_blob', > > + prefix: '#include <virglrenderer.h>', > > + dependencies: virgl)) > > better moved under the if virgl.found() block Yes, will update. > > > endif > > blkio = not_found > > if not get_option('blkio').auto() or have_block > > -- > > 2.34.1 > > > > > > > -- > Marc-André Lureau
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index 312953ec16..563a6f2f58 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -17,6 +17,7 @@ #include "trace.h" #include "hw/virtio/virtio.h" #include "hw/virtio/virtio-gpu.h" +#include "hw/virtio/virtio-gpu-bswap.h" #include "ui/egl-helpers.h" @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g, virgl_renderer_resource_create(&args, NULL, 0); } +static void virgl_resource_destroy(VirtIOGPU *g, + struct virtio_gpu_simple_resource *res) +{ + if (!res) + return; + + QTAILQ_REMOVE(&g->reslist, res, next); + + virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt); + g_free(res->addrs); + + g_free(res); +} + static void virgl_cmd_resource_unref(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { + struct virtio_gpu_simple_resource *res; struct virtio_gpu_resource_unref unref; struct iovec *res_iovs = NULL; int num_iovs = 0; @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, VIRTIO_GPU_FILL_CMD(unref); trace_virtio_gpu_cmd_res_unref(unref.resource_id); + res = virtio_gpu_find_resource(g, unref.resource_id); + virgl_renderer_resource_detach_iov(unref.resource_id, &res_iovs, &num_iovs); if (res_iovs != NULL && num_iovs != 0) { virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs); + if (res) { + res->iov = NULL; + res->iov_cnt = 0; + } } + virgl_renderer_resource_unref(unref.resource_id); + + virgl_resource_destroy(g, res); } static void virgl_cmd_context_create(VirtIOGPU *g, @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, g_free(resp); } +#ifdef HAVE_VIRGL_RESOURCE_BLOB + +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, + struct virtio_gpu_ctrl_command *cmd) +{ + struct virtio_gpu_simple_resource *res; + struct virtio_gpu_resource_create_blob cblob; + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; + int ret; + + VIRTIO_GPU_FILL_CMD(cblob); + virtio_gpu_create_blob_bswap(&cblob); + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); + + if (cblob.resource_id == 0) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", + __func__); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res = virtio_gpu_find_resource(g, cblob.resource_id); + if (res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", + __func__, cblob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res = g_new0(struct virtio_gpu_simple_resource, 1); + if (!res) { + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; + return; + } + + res->resource_id = cblob.resource_id; + res->blob_size = cblob.size; + + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), + cmd, &res->addrs, &res->iov, + &res->iov_cnt); + if (!ret) { + g_free(res); + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; + return; + } + } + + QTAILQ_INSERT_HEAD(&g->reslist, res, next); + + virgl_args.res_handle = cblob.resource_id; + virgl_args.ctx_id = cblob.hdr.ctx_id; + virgl_args.blob_mem = cblob.blob_mem; + virgl_args.blob_id = cblob.blob_id; + virgl_args.blob_flags = cblob.blob_flags; + virgl_args.size = cblob.size; + virgl_args.iovecs = res->iov; + virgl_args.num_iovs = res->iov_cnt; + + ret = virgl_renderer_resource_create_blob(&virgl_args); + if (ret) { + virgl_resource_destroy(g, res); + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", + __func__, strerror(-ret)); + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; + } +} + +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, + struct virtio_gpu_ctrl_command *cmd) +{ + struct virtio_gpu_simple_resource *res; + struct virtio_gpu_resource_map_blob mblob; + int ret; + void *data; + uint64_t size; + struct virtio_gpu_resp_map_info resp; + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); + + VIRTIO_GPU_FILL_CMD(mblob); + virtio_gpu_map_blob_bswap(&mblob); + + if (mblob.resource_id == 0) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", + __func__); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res = virtio_gpu_find_resource(g, mblob.resource_id); + if (!res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", + __func__, mblob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + if (res->region) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", + __func__, mblob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); + if (ret) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", + __func__, strerror(-ret)); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res->region = g_new0(MemoryRegion, 1); + if (!res->region) { + virgl_renderer_resource_unmap(res->resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; + return; + } + memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data); + OBJECT(res->region)->free = g_free; + memory_region_add_subregion(&b->hostmem, mblob.offset, res->region); + memory_region_set_enabled(res->region, true); + + memset(&resp, 0, sizeof(resp)); + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); +} + +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g, + struct virtio_gpu_simple_resource + *res) +{ + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); + + if (!res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", + __func__, res->resource_id); + return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + } + + memory_region_set_enabled(res->region, false); + memory_region_del_subregion(&b->hostmem, res->region); + object_unparent(OBJECT(res->region)); + res->region = NULL; + + return virgl_renderer_resource_unmap(res->resource_id); +} + +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, + struct virtio_gpu_ctrl_command *cmd) +{ + struct virtio_gpu_simple_resource *res; + struct virtio_gpu_resource_unmap_blob ublob; + VIRTIO_GPU_FILL_CMD(ublob); + virtio_gpu_unmap_blob_bswap(&ublob); + + if (ublob.resource_id == 0) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", + __func__); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res = virtio_gpu_find_resource(g, ublob.resource_id); + if (!res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", + __func__, ublob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + virtio_gpu_virgl_resource_unmap(g, res); +} + +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ + void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, case VIRTIO_GPU_CMD_GET_EDID: virtio_gpu_get_edid(g, cmd); break; +#ifdef HAVE_VIRGL_RESOURCE_BLOB + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: + virgl_cmd_resource_create_blob(g, cmd); + break; + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: + virgl_cmd_resource_map_blob(g, cmd); + break; + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: + virgl_cmd_resource_unmap_blob(g, cmd); + break; +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ default: cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; break; diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 5b7a7eab4f..cc4c1f81bb 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) return; } +#ifndef HAVE_VIRGL_RESOURCE_BLOB if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { - error_setg(errp, "blobs and virgl are not compatible (yet)"); + error_setg(errp, "Linked virglrenderer does not support blob resources"); return; } +#endif } if (!virtio_gpu_base_device_realize(qdev, diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h index 55973e112f..b9adc28071 100644 --- a/include/hw/virtio/virtio-gpu.h +++ b/include/hw/virtio/virtio-gpu.h @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource { int dmabuf_fd; uint8_t *remapped; +#ifdef HAVE_VIRGL_RESOURCE_BLOB + /* only blob resource needs this region to be mapped as guest mmio */ + MemoryRegion *region; +#endif + QTAILQ_ENTRY(virtio_gpu_simple_resource) next; }; diff --git a/meson.build b/meson.build index ff20d3c249..f7b744ab82 100644 --- a/meson.build +++ b/meson.build @@ -1072,6 +1072,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu cc.has_function('virgl_renderer_context_create_with_flags', prefix: '#include <virglrenderer.h>', dependencies: virgl)) + config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB', + cc.has_function('virgl_renderer_resource_create_blob', + prefix: '#include <virglrenderer.h>', + dependencies: virgl)) endif blkio = not_found if not get_option('blkio').auto() or have_block