From: Akihiko Odaki <akihiko.odaki@daynix.com>
To: "Dmitry Osipenko" <dmitry.osipenko@collabora.com>,
"Huang Rui" <ray.huang@amd.com>,
"Marc-André Lureau" <marcandre.lureau@gmail.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Gerd Hoffmann" <kraxel@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Anthony PERARD" <anthony.perard@citrix.com>,
"Antonio Caggiano" <quic_acaggian@quicinc.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
"Robert Beckett" <bob.beckett@collabora.com>,
"Gert Wollny" <gert.wollny@collabora.com>,
"Alex Bennée" <alex.bennee@linaro.org>
Cc: qemu-devel@nongnu.org,
"Gurchetan Singh" <gurchetansingh@chromium.org>,
ernunes@redhat.com, "Alyssa Ross" <hi@alyssa.is>,
"Roger Pau Monné" <roger.pau@citrix.com>,
"Alex Deucher" <alexander.deucher@amd.com>,
"Stefano Stabellini" <stefano.stabellini@amd.com>,
"Christian König" <christian.koenig@amd.com>,
"Xenia Ragiadakou" <xenia.ragiadakou@amd.com>,
"Pierre-Eric Pelloux-Prayer" <pierre-eric.pelloux-prayer@amd.com>,
"Honglei Huang" <honglei1.huang@amd.com>,
"Julia Zhang" <julia.zhang@amd.com>,
"Chen Jiqian" <Jiqian.Chen@amd.com>,
"Yiwei Zhang" <zzyiwei@chromium.org>
Subject: Re: [PATCH v9 07/11] virtio-gpu: Handle resource blob commands
Date: Sat, 27 Apr 2024 15:57:06 +0900 [thread overview]
Message-ID: <9b155a47-87ac-4395-84ba-2a31b16a2e56@daynix.com> (raw)
In-Reply-To: <20240425154539.2680550-8-dmitry.osipenko@collabora.com>
On 2024/04/26 0:45, Dmitry Osipenko 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: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
> hw/display/virtio-gpu-virgl.c | 268 ++++++++++++++++++++++++++++++++++
> hw/display/virtio-gpu.c | 4 +-
> 2 files changed, 271 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 0feaa9f2c52e..73d4acbf1777 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -26,6 +26,8 @@
>
> struct virtio_gpu_virgl_resource {
> struct virtio_gpu_simple_resource base;
> + bool async_unmap_in_progress;
Why is this flag needed?
> + MemoryRegion *mr;
> };
>
> static struct virtio_gpu_virgl_resource *
> @@ -49,6 +51,120 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
> }
> #endif
>
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +struct virtio_gpu_virgl_hostmem_region {
> + MemoryRegion mr;
> + struct VirtIOGPU *g;
> + struct virtio_gpu_virgl_resource *res;
> +};
> +
> +static void virtio_gpu_virgl_resume_cmdq(void *opaque)
> +{
> + VirtIOGPU *g = opaque;
> +
> + virtio_gpu_process_cmdq(g);
> +}
> +
> +static void virtio_gpu_virgl_hostmem_region_free(void *obj)
> +{
> + MemoryRegion *mr = MEMORY_REGION(obj);
> + struct virtio_gpu_virgl_hostmem_region *vmr;
> + VirtIOGPUBase *b;
> +
> + vmr = container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
> + vmr->res->async_unmap_in_progress = false;
> +
> + b = VIRTIO_GPU_BASE(vmr->g);
> + b->renderer_blocked--;
> +
> + /*
> + * memory_region_unref() may be executed from RCU thread context, while
> + * virglrenderer works only on the main-loop thread that's holding GL
> + * context.
> + */
> + aio_bh_schedule_oneshot(qemu_get_aio_context(),
> + virtio_gpu_virgl_resume_cmdq, vmr->g);
Use aio_bh_new() and qemu_bh_schedule() instead to save one-time bottom
half allocation.
> + g_free(vmr);
> +}
> +
> +static int
> +virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
> + struct virtio_gpu_virgl_resource *res,
> + uint64_t offset)
> +{
> + struct virtio_gpu_virgl_hostmem_region *vmr;
> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> + MemoryRegion *mr;
> + uint64_t size;
> + void *data;
> + int ret;
> +
> + if (!virtio_gpu_hostmem_enabled(b->conf)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: hostmem disabled\n", __func__);
> + return -EOPNOTSUPP; > + }
> +
> + ret = virgl_renderer_resource_map(res->base.resource_id, &data, &size);
> + if (ret) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource\n",
> + __func__);
Print strerror(-ret) here instead as printing strerror(EOPNOTSUPP) helps
little when !virtio_gpu_hostmem_enabled(b->conf).
> + return -ret;
> + }
> +
> + vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
> + vmr->res = res;
> + vmr->g = g;
> +
> + mr = &vmr->mr;
> + memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
> + memory_region_add_subregion(&b->hostmem, offset, mr);
> + memory_region_set_enabled(mr, true);
> +
> + /*
> + * Potentially, MR could outlive the resource if MR's reference is held
> + * outside of virtio-gpu. In order to prevent unmapping resource while
> + * MR is alive, and thus, making the data pointer invalid, we will block
> + * virtio-gpu command processing until MR is fully unreferenced and
> + * released.
> + */
> + OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
> +
> + res->mr = mr;
> +
> + return 0;
> +}
> +
> +static bool
> +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
> + struct virtio_gpu_virgl_resource *res)
> +{
> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> + if (!res->async_unmap_in_progress && res->mr) {
> + /* memory region owns self res->mr object and frees it by itself */
> + MemoryRegion *mr = res->mr;
> + res->mr = NULL;
> +
> + res->async_unmap_in_progress = true;
> +
> + /* render will be unblocked when MR is freed */
> + b->renderer_blocked++;
> +
> + memory_region_set_enabled(mr, false);
> + memory_region_del_subregion(&b->hostmem, mr);
> + object_unparent(OBJECT(mr));
> + }
> +
> + if (res->async_unmap_in_progress) {
> + return false;
> + }
> +
> + virgl_renderer_resource_unmap(res->base.resource_id);
> +
> + return true;
> +}
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> +
> static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> struct virtio_gpu_ctrl_command *cmd)
> {
> @@ -162,6 +278,14 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> return;
> }
>
> + if (res->mr || cmd->suspended) {
> + bool unmapped = virtio_gpu_virgl_unmap_resource_blob(g, res);
> + cmd->suspended = !unmapped;
> + if (cmd->suspended) {
> + return;
> + }
> + }
> +
> virgl_renderer_resource_detach_iov(unref.resource_id,
> &res_iovs,
> &num_iovs);
> @@ -512,6 +636,141 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> }
>
> #ifdef HAVE_VIRGL_RESOURCE_BLOB
> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> + struct virtio_gpu_ctrl_command *cmd)
> +{
> + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> + struct virtio_gpu_resource_create_blob cblob;
> + struct virtio_gpu_virgl_resource *res;
> + int ret;
> +
> + if (!virtio_gpu_blob_enabled(g->parent_obj.conf)) {
> + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> + return;
> + }
> +
> + 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_virgl_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_virgl_resource, 1);
> + res->base.resource_id = cblob.resource_id;
> + res->base.blob_size = cblob.size;
> + res->base.dmabuf_fd = -1;
> +
> + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> + cmd, &res->base.addrs,
> + &res->base.iov, &res->base.iov_cnt);
> + if (!ret) {
> + g_free(res);
> + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> + return;
> + }
> + }
> +
> + QTAILQ_INSERT_HEAD(&g->reslist, &res->base, 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->base.iov;
> + virgl_args.num_iovs = res->base.iov_cnt;
> +
> + ret = virgl_renderer_resource_create_blob(&virgl_args);
> + if (ret) {
> + 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_resource_map_blob mblob;
> + struct virtio_gpu_virgl_resource *res;
> + struct virtio_gpu_resp_map_info resp;
> + int ret;
> +
> + VIRTIO_GPU_FILL_CMD(mblob);
> + virtio_gpu_map_blob_bswap(&mblob);
> +
> + res = virtio_gpu_virgl_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->mr) {
> + 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 = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
> + if (ret) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> + __func__, strerror(ret));
> + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
I think it's better to use VIRTIO_GPU_RESP_ERR_UNSPEC here; we don't
know if the error is out-of-memory or something else.
next prev parent reply other threads:[~2024-04-27 6:58 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 15:45 [PATCH v9 00/11] Support blob memory and venus on qemu Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 01/11] virtio-gpu: Use pkgconfig version to decide which virgl features are available Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 02/11] virtio-gpu: Support context-init feature with virglrenderer Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 03/11] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 04/11] virtio-gpu: Add virgl resource management Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 05/11] virtio-gpu: Support blob scanout using dmabuf fd Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 06/11] virtio-gpu: Support suspension of commands processing Dmitry Osipenko
2024-04-25 15:45 ` [PATCH v9 07/11] virtio-gpu: Handle resource blob commands Dmitry Osipenko
2024-04-27 6:57 ` Akihiko Odaki [this message]
2024-04-25 15:45 ` [PATCH v9 08/11] virtio-gpu: Resource UUID Dmitry Osipenko
2024-04-27 7:01 ` Akihiko Odaki
2024-04-25 15:45 ` [PATCH v9 09/11] virtio-gpu: Register capsets dynamically Dmitry Osipenko
2024-04-27 7:12 ` Akihiko Odaki
2024-05-01 19:31 ` Dmitry Osipenko
2024-05-01 19:38 ` Dmitry Osipenko
2024-05-01 19:52 ` Dmitry Osipenko
2024-05-03 7:32 ` Akihiko Odaki
2024-04-25 15:45 ` [PATCH v9 10/11] virtio-gpu: Support Venus context Dmitry Osipenko
2024-04-27 7:29 ` Akihiko Odaki
2024-04-25 15:45 ` [PATCH v9 11/11] migration/virtio: Add virtio-gpu section Dmitry Osipenko
2024-04-27 7:27 ` Akihiko Odaki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9b155a47-87ac-4395-84ba-2a31b16a2e56@daynix.com \
--to=akihiko.odaki@daynix.com \
--cc=Jiqian.Chen@amd.com \
--cc=alex.bennee@linaro.org \
--cc=alexander.deucher@amd.com \
--cc=anthony.perard@citrix.com \
--cc=bob.beckett@collabora.com \
--cc=christian.koenig@amd.com \
--cc=dgilbert@redhat.com \
--cc=dmitry.osipenko@collabora.com \
--cc=ernunes@redhat.com \
--cc=gert.wollny@collabora.com \
--cc=gurchetansingh@chromium.org \
--cc=hi@alyssa.is \
--cc=honglei1.huang@amd.com \
--cc=julia.zhang@amd.com \
--cc=kraxel@redhat.com \
--cc=marcandre.lureau@gmail.com \
--cc=mst@redhat.com \
--cc=philmd@linaro.org \
--cc=pierre-eric.pelloux-prayer@amd.com \
--cc=qemu-devel@nongnu.org \
--cc=quic_acaggian@quicinc.com \
--cc=ray.huang@amd.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=stefano.stabellini@amd.com \
--cc=xenia.ragiadakou@amd.com \
--cc=zzyiwei@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.