Re: [PATCH v6 10/11] virtio-gpu: Initialize Venus
Le 23/02/2024 à 10:15, Huang Rui a écrit :
On Tue, Jan 02, 2024 at 09:33:11PM +0800, Marc-André Lureau wrote:
Hi
On Tue, Dec 19, 2023 at 11:55 AM Huang Rui wrote:
From: Antonio Caggiano
Request Venus when initializing VirGL.
Signed-off-by: Antonio Caggiano
Signed-off-by: Huang Rui
---
Changes in v6:
- Remove the unstable API flags check because virglrenderer is already 1.0.
- Squash the render server flag support into "Initialize Venus".
hw/display/virtio-gpu-virgl.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index f35a751824..c523a6717a 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -964,6 +964,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
}
#endif
+#ifdef VIRGL_RENDERER_VENUS
+flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
+#endif
+
I wonder if it's a good idea to initialize venus by default. It
doesn't seem to require vulkan during initialization, but this may
evolve. Make it optional?
I am fine. In fact, vulkan is widely used for graphic area such as gaming,
compute, VR/AR, etc.
Actually, making it optional is useful because Venus support is optional in
virglrenderer (= having VIRGL_RENDERER_VENUS defined doesn't mean that
Venus is supported).
Thanks,
Pierre-Eric
Thanks,
Ray
ret = virgl_renderer_init(g, flags, _gpu_3d_cbs);
if (ret != 0) {
error_report("virgl could not be initialized: %d", ret);
--
2.25.1
--
Marc-André Lureau
Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands
Le 21/12/2023 à 09:09, Akihiko Odaki a écrit : On 2023/12/19 16:53, Huang Rui wrote: From: Antonio Caggiano 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 I have another concern about delaying virgl_renderer_resource_unref() until the resource gets unmapped; the guest will expect the resource ID will be available for a new resource immediately after VIRTIO_GPU_CMD_RESOURCE_UNREF, but it will break the assumption and may corrupt things. Yes this is a problem. And another one is virglrenderer is not really thread-safe, so this callstack: #0 virgl_resource_blob_async_unmap () #1 object_finalize () #2 object_unref () #3 memory_region_unref () #4 flatview_destroy () #5 call_rcu_thread () #6 qemu_thread_start () Will call into virgl_renderer_ctx_resource_unmap which in turn uses virgl_resource_lookup without any multithreading considerations. Regards, Pierre-Eric
Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands
Le 09/01/2024 à 17:50, Pierre-Eric Pelloux-Prayer a écrit :
Le 19/12/2023 à 08:53, Huang Rui a écrit :
From: Antonio Caggiano
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
Signed-off-by: Dmitry Osipenko
Signed-off-by: Xenia Ragiadakou
Signed-off-by: Huang Rui
---
Changes in v6:
- Use new struct virgl_gpu_resource.
- Unmap, unref and destroy the resource only after the memory region
has been completely removed.
- In unref check whether the resource is still mapped.
- In unmap_blob check whether the resource has been already unmapped.
- Fix coding style
hw/display/virtio-gpu-virgl.c | 274 +-
hw/display/virtio-gpu.c | 4 +-
meson.build | 4 +
3 files changed, 276 insertions(+), 6 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index faab374336..5a3a292f79 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
#include "trace.h"
#include "hw/virtio/virtio.h"
#include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
#include "ui/egl-helpers.h"
@@ -24,8 +25,62 @@
struct virgl_gpu_resource {
struct virtio_gpu_simple_resource res;
+ uint32_t ref;
+ VirtIOGPU *g;
+
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+ /* only blob resource needs this region to be mapped as guest mmio */
+ MemoryRegion *region;
+#endif
};
+static void vres_get_ref(struct virgl_gpu_resource *vres)
+{
+ uint32_t ref;
+
+ ref = qatomic_fetch_inc(>ref);
+ g_assert(ref < INT_MAX);
+}
+
+static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
+{
+ struct virtio_gpu_simple_resource *res;
+ VirtIOGPU *g;
+
+ if (!vres) {
+ return;
+ }
+
+ g = vres->g;
+ res = >res;
+ QTAILQ_REMOVE(>reslist, res, next);
+ virtio_gpu_cleanup_mapping(g, res);
+ g_free(vres);
+}
+
+static void virgl_resource_unref(struct virgl_gpu_resource *vres)
+{
+ struct virtio_gpu_simple_resource *res;
+
+ if (!vres) {
+ return;
+ }
+
+ res = >res;
+ virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
+ virgl_renderer_resource_unref(res->resource_id);
+}
+
+static void vres_put_ref(struct virgl_gpu_resource *vres)
+{
+ g_assert(vres->ref > 0);
+
+ if (qatomic_fetch_dec(>ref) == 1) {
+ virgl_resource_unref(vres);
+ virgl_resource_destroy(vres);
+ }
+}
+
static struct virgl_gpu_resource *
virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
{
@@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
c2d.width, c2d.height);
vres = g_new0(struct virgl_gpu_resource, 1);
+ vres_get_ref(vres);
+ vres->g = g;
vres->res.width = c2d.width;
vres->res.height = c2d.height;
vres->res.format = c2d.format;
@@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
c3d.width, c3d.height, c3d.depth);
vres = g_new0(struct virgl_gpu_resource, 1);
+ vres_get_ref(vres);
+ vres->g = g;
vres->res.width = c3d.width;
vres->res.height = c3d.height;
vres->res.format = c3d.format;
@@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
return;
}
- virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
- virgl_renderer_resource_unref(unref.resource_id);
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+ if (vres->region) {
+ VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+ MemoryRegion *mr = vres->region;
+
+ warn_report("%s: blob resource %d not unmapped",
+ __func__, unref.resource_id);
+ vres->region = NULL;
Shouldn't there be a call to memory_region_unref(mr)?
+ memory_region_set_enabled(mr, false);
+ memory_region_del_subregion(>hostmem, mr);
+ object_unparent(OBJECT(mr));
+ }
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
- QTAILQ_REMOVE(>reslist, >res, next);
- virtio_gpu_cleanup_mapping(g, >res);
- g_free(vres);
+ vres_put_ref(vres);
}
static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
g_free(resp);
}
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+
+static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
+{
+ if (!vres) {
+ return;
+ }
+
+ virgl_renderer_resource_unmap(vres->res.resource_id);
+
+ vres_put_ref(vres);
+}
+
+static void virgl_resource_blob_async_unmap(void *obj)
+{
+ MemoryRegion *mr = MEMORY_REGION(obj);
+ struct virgl_gpu_reso
Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands
Le 19/12/2023 à 08:53, Huang Rui a écrit :
From: Antonio Caggiano
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
Signed-off-by: Dmitry Osipenko
Signed-off-by: Xenia Ragiadakou
Signed-off-by: Huang Rui
---
Changes in v6:
- Use new struct virgl_gpu_resource.
- Unmap, unref and destroy the resource only after the memory region
has been completely removed.
- In unref check whether the resource is still mapped.
- In unmap_blob check whether the resource has been already unmapped.
- Fix coding style
hw/display/virtio-gpu-virgl.c | 274 +-
hw/display/virtio-gpu.c | 4 +-
meson.build | 4 +
3 files changed, 276 insertions(+), 6 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index faab374336..5a3a292f79 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
#include "trace.h"
#include "hw/virtio/virtio.h"
#include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
#include "ui/egl-helpers.h"
@@ -24,8 +25,62 @@
struct virgl_gpu_resource {
struct virtio_gpu_simple_resource res;
+uint32_t ref;
+VirtIOGPU *g;
+
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+/* only blob resource needs this region to be mapped as guest mmio */
+MemoryRegion *region;
+#endif
};
+static void vres_get_ref(struct virgl_gpu_resource *vres)
+{
+uint32_t ref;
+
+ref = qatomic_fetch_inc(>ref);
+g_assert(ref < INT_MAX);
+}
+
+static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
+{
+struct virtio_gpu_simple_resource *res;
+VirtIOGPU *g;
+
+if (!vres) {
+return;
+}
+
+g = vres->g;
+res = >res;
+QTAILQ_REMOVE(>reslist, res, next);
+virtio_gpu_cleanup_mapping(g, res);
+g_free(vres);
+}
+
+static void virgl_resource_unref(struct virgl_gpu_resource *vres)
+{
+struct virtio_gpu_simple_resource *res;
+
+if (!vres) {
+return;
+}
+
+res = >res;
+virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
+virgl_renderer_resource_unref(res->resource_id);
+}
+
+static void vres_put_ref(struct virgl_gpu_resource *vres)
+{
+g_assert(vres->ref > 0);
+
+if (qatomic_fetch_dec(>ref) == 1) {
+virgl_resource_unref(vres);
+virgl_resource_destroy(vres);
+}
+}
+
static struct virgl_gpu_resource *
virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
{
@@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
c2d.width, c2d.height);
vres = g_new0(struct virgl_gpu_resource, 1);
+vres_get_ref(vres);
+vres->g = g;
vres->res.width = c2d.width;
vres->res.height = c2d.height;
vres->res.format = c2d.format;
@@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
c3d.width, c3d.height, c3d.depth);
vres = g_new0(struct virgl_gpu_resource, 1);
+vres_get_ref(vres);
+vres->g = g;
vres->res.width = c3d.width;
vres->res.height = c3d.height;
vres->res.format = c3d.format;
@@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
return;
}
-virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
-virgl_renderer_resource_unref(unref.resource_id);
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+if (vres->region) {
+VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+MemoryRegion *mr = vres->region;
+
+warn_report("%s: blob resource %d not unmapped",
+__func__, unref.resource_id);
+vres->region = NULL;
Shouldn't there be a call to memory_region_unref(mr)?
+memory_region_set_enabled(mr, false);
+memory_region_del_subregion(>hostmem, mr);
+object_unparent(OBJECT(mr));
+}
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
-QTAILQ_REMOVE(>reslist, >res, next);
-virtio_gpu_cleanup_mapping(g, >res);
-g_free(vres);
+vres_put_ref(vres);
}
static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
g_free(resp);
}
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+
+static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
+{
+if (!vres) {
+return;
+}
+
+virgl_renderer_resource_unmap(vres->res.resource_id);
+
+vres_put_ref(vres);
+}
+
+static void virgl_resource_blob_async_unmap(void *obj)
+{
+MemoryRegion *mr = MEMORY_REGION(obj);
+struct virgl_gpu_resource *vres = mr->opaque;
+
+virgl_resource_unmap(vres);
+
+g_free(obj);
+}
+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+ struct
Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd
Le 21/12/2023 à 07:25, Akihiko Odaki a écrit :
On 2023/12/19 16:53, Huang Rui wrote:
From: Robert Beckett
This relies on a virglrenderer change to include the dmabuf fd when
returning resource info.
Signed-off-by: Robert Beckett
Signed-off-by: Huang Rui
---
Changes in v6:
- Add scanout_blob function for virtio-gpu-virgl.
- Update for new virgl_gpu_resource.
hw/display/virtio-gpu-virgl.c | 104 +
hw/display/virtio-gpu.c | 4 +-
include/hw/virtio/virtio-gpu.h | 6 ++
3 files changed, 112 insertions(+), 2 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index c523a6717a..c384225a98 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -18,6 +18,7 @@
#include "hw/virtio/virtio.h"
#include "hw/virtio/virtio-gpu.h"
#include "hw/virtio/virtio-gpu-bswap.h"
+#include "hw/virtio/virtio-gpu-pixman.h"
#include "ui/egl-helpers.h"
@@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
object_unparent(OBJECT(mr));
}
+static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
+ struct virtio_gpu_ctrl_command *cmd)
+{
+ struct virgl_gpu_resource *vres;
+ struct virtio_gpu_framebuffer fb = { 0 };
+ struct virtio_gpu_set_scanout_blob ss;
+ struct virgl_renderer_resource_info info;
+ uint64_t fbend;
+
+ VIRTIO_GPU_FILL_CMD(ss);
+ virtio_gpu_scanout_blob_bswap();
+ trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
+ ss.r.width, ss.r.height, ss.r.x,
+ ss.r.y);
+
+ if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+ __func__, ss.scanout_id);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
+ return;
+ }
+
+ if (ss.resource_id == 0) {
+ virtio_gpu_disable_scanout(g, ss.scanout_id);
+ return;
+ }
+
+ if (ss.width < 16 ||
+ ss.height < 16 ||
+ ss.r.x + ss.r.width > ss.width ||
+ ss.r.y + ss.r.height > ss.height) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
+ " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
+ __func__, ss.scanout_id, ss.resource_id,
+ ss.r.x, ss.r.y, ss.r.width, ss.r.height,
+ ss.width, ss.height);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+ return;
+ }
+
+ if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {
Shouldn't OpenGL always be available for virgl?
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without
GL!\n", __func__);
+ return;
+ }
+
+ vres = virgl_gpu_find_resource(g, ss.resource_id);
+ if (!vres) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: illegal resource specified %d\n",
+ __func__, ss.resource_id);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+ return;
+ }
+ if (virgl_renderer_resource_get_info(ss.resource_id, )) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: illegal virgl resource specified %d\n",
+ __func__, ss.resource_id);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+ return;
+ }
+ if (!vres->res.dmabuf_fd && info.fd)
+ vres->res.dmabuf_fd = info.fd;
+
+ fb.format = virtio_gpu_get_pixman_format(ss.format);
+ if (!fb.format) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: host couldn't handle guest format %d\n",
+ __func__, ss.format);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+ return;
+ }
+
+ fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
+ fb.width = ss.width;
+ fb.height = ss.height;
+ fb.stride = ss.strides[0];
+ fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
+
+ fbend = fb.offset;
+ fbend += fb.stride * (ss.r.height - 1);
+ fbend += fb.bytes_pp * ss.r.width;
+ if (fbend > vres->res.blob_size) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: fb end out of range\n",
+ __func__);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+ return;
+ }
+
+ g->parent_obj.enable = 1;
+ if (virtio_gpu_update_dmabuf(g, ss.scanout_id, >res,
+ , )) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: failed to update dmabuf\n", __func__);
+ cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+ return;
+ }
+ virtio_gpu_update_scanout(g, ss.scanout_id, >res, );
+}
+
#endif /* HAVE_VIRGL_RESOURCE_BLOB */
void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
@@ -807,6 +908,9 @@ void
Re: [PATCH v6 09/11] virtio-gpu: Support Venus capset
Hi Ray, Antonio,
Le 19/12/2023 à 08:53, Huang Rui a écrit :
From: Antonio Caggiano
Add support for the Venus capset, which enables Vulkan support through
the Venus Vulkan driver for virtio-gpu.
Signed-off-by: Antonio Caggiano
Signed-off-by: Huang Rui
---
No change in v6.
hw/display/virtio-gpu-virgl.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index be9da6e780..f35a751824 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -506,6 +506,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
virgl_renderer_get_cap_set(resp.capset_id,
_max_version,
_max_size);
+} else if (info.capset_index == 2) {
+resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
+virgl_renderer_get_cap_set(resp.capset_id,
+ _max_version,
+ _max_size);
} else {
resp.capset_max_version = 0;
resp.capset_max_size = 0;
@@ -978,10 +983,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
{
-uint32_t capset2_max_ver, capset2_max_size;
+uint32_t capset2_max_ver, capset2_max_size, num_capsets;
+num_capsets = 1;
+
virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
- _max_ver,
- _max_size);
+ _max_ver,
+ _max_size);
+num_capsets += capset2_max_ver ? 1 : 0;
+
+virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
+ _max_ver,
+ _max_size);
+num_capsets += capset2_max_size ? 1 : 0;
IMHO the logic here doesn't work.
The kernel will use num_capset like this:
for (i = 0; i < num_capsets; i++) {
virtio_gpu_cmd_get_capset_info(vgdev, i);
So if num_capset = 2, it will query the capset info of index 0 and 1.
Capset 0 is alway VIRGL so it's fine. But since VIRL2 support is optional,
QEMU has no way to know if index 1 is VIRGL2 (if it's supported)
or VENUS (if VIRGL2 support is missing).
And it'll get worse when we will want to support CAPSET_DRM.
Ray: we have a patch internally for this (virtio-gpu: fix capset query), you
may want to add it to this series, before this patch.
Regards,
Pierre-Eric
-return capset2_max_ver ? 2 : 1;
+return num_capsets;
}
