qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PULL 00/13] virtio-gpu vulkan support
@ 2024-10-29 12:10 Alex Bennée
  2024-10-29 12:10 ` [PULL 01/13] virtio-gpu: Use trace events for tracking number of in-flight fences Alex Bennée
                   ` (13 more replies)
  0 siblings, 14 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alex Bennée

The following changes since commit fdf250e5a37830615e324017cb3a503e84b3712c:

  Merge tag 'pull-maintainer-oct-misc-241024-1' of https://gitlab.com/stsquad/qemu into staging (2024-10-25 19:12:06 +0100)

are available in the Git repository at:

  https://gitlab.com/stsquad/qemu.git tags/pull-virtio-gpu-vulkan-291024-1

for you to fetch changes up to 94d0ea1c19289d76ced934711fccd2269e69bb29:

  virtio-gpu: Support Venus context (2024-10-28 16:56:36 +0000)

----------------------------------------------------------------
virtio-gpu: add venus/vulkan capability

We are currently lacking a declared maintainer for the sub-system so
while we look for one I'm merging after testing locally.

  - convert some fprintfs to proper trace events
  - move timers used by GL devices into GL structures
  - handle virtio_gpu_virgl_init() failure better
  - implement unrealize for GL devices
  - use virgl version numbering to gate features
  - support context-init feature
  - don't require udmabuf for virgl only
  - add virgl resource tracker
  - allow command submission to be suspended
  - handle resource blob commands
  - dynamically handle capabilit sets
  - add venus context support for passing vulkan

----------------------------------------------------------------
Antonio Caggiano (1):
      virtio-gpu: Support Venus context

Dmitry Osipenko (8):
      virtio-gpu: Use trace events for tracking number of in-flight fences
      virtio-gpu: Move fence_poll timer to VirtIOGPUGL
      virtio-gpu: Move print_stats timer to VirtIOGPUGL
      virtio-gpu: Handle virtio_gpu_virgl_init() failure
      virtio-gpu: Unrealize GL device
      virtio-gpu: Use pkgconfig version to decide which virgl features are available
      virtio-gpu: Don't require udmabuf when blobs and virgl are enabled
      virtio-gpu: Support suspension of commands processing

Huang Rui (2):
      virtio-gpu: Support context-init feature with virglrenderer
      virtio-gpu: Add virgl resource management

Pierre-Eric Pelloux-Prayer (1):
      virtio-gpu: Register capsets dynamically

Robert Beckett (1):
      virtio-gpu: Handle resource blob commands

 docs/system/devices/virtio-gpu.rst |  11 +
 meson.build                        |   5 +-
 include/hw/virtio/virtio-gpu.h     |  32 +-
 hw/display/virtio-gpu-gl.c         |  62 +++-
 hw/display/virtio-gpu-virgl.c      | 585 +++++++++++++++++++++++++++++++++++--
 hw/display/virtio-gpu.c            |  44 ++-
 hw/display/trace-events            |   3 +
 7 files changed, 685 insertions(+), 57 deletions(-)


-- 
2.39.5



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PULL 01/13] virtio-gpu: Use trace events for tracking number of in-flight fences
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 02/13] virtio-gpu: Move fence_poll timer to VirtIOGPUGL Alex Bennée
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dmitry Osipenko, Marc-André Lureau, Alex Bennée,
	Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Replace printf's used for tracking of in-flight fence inc/dec events
with tracing, for consistency with the rest of virtio-gpu code that
uses tracing.

Suggested-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-2-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 9f34d0e661..14091b191e 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -525,7 +525,7 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
         g_free(cmd);
         g->inflight--;
         if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
-            fprintf(stderr, "inflight: %3d (-)\r", g->inflight);
+            trace_virtio_gpu_dec_inflight_fences(g->inflight);
         }
     }
 }
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 49fd803393..3fcc434732 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1046,7 +1046,7 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
                 if (g->stats.max_inflight < g->inflight) {
                     g->stats.max_inflight = g->inflight;
                 }
-                fprintf(stderr, "inflight: %3d (+)\r", g->inflight);
+                trace_virtio_gpu_inc_inflight_fences(g->inflight);
             }
         } else {
             g_free(cmd);
@@ -1066,7 +1066,7 @@ static void virtio_gpu_process_fenceq(VirtIOGPU *g)
         g_free(cmd);
         g->inflight--;
         if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
-            fprintf(stderr, "inflight: %3d (-)\r", g->inflight);
+            trace_virtio_gpu_dec_inflight_fences(g->inflight);
         }
     }
 }
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 781f8a3320..e212710284 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -53,6 +53,8 @@ virtio_gpu_cmd_ctx_submit(uint32_t ctx, uint32_t size) "ctx 0x%x, size %d"
 virtio_gpu_update_cursor(uint32_t scanout, uint32_t x, uint32_t y, const char *type, uint32_t res) "scanout %d, x %d, y %d, %s, res 0x%x"
 virtio_gpu_fence_ctrl(uint64_t fence, uint32_t type) "fence 0x%" PRIx64 ", type 0x%x"
 virtio_gpu_fence_resp(uint64_t fence) "fence 0x%" PRIx64
+virtio_gpu_inc_inflight_fences(uint32_t inflight) "in-flight+ %u"
+virtio_gpu_dec_inflight_fences(uint32_t inflight) "in-flight- %u"
 
 # qxl.c
 disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 02/13] virtio-gpu: Move fence_poll timer to VirtIOGPUGL
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
  2024-10-29 12:10 ` [PULL 01/13] virtio-gpu: Use trace events for tracking number of in-flight fences Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 03/13] virtio-gpu: Move print_stats " Alex Bennée
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Osipenko, Alex Bennée, Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Move fence_poll timer to VirtIOGPUGL for consistency with cmdq_resume_bh
that are used only by GL device.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-3-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index e343110e23..48a67d662d 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -194,7 +194,6 @@ struct VirtIOGPU {
     uint64_t hostmem;
 
     bool processing_cmdq;
-    QEMUTimer *fence_poll;
     QEMUTimer *print_stats;
 
     uint32_t inflight;
@@ -229,6 +228,8 @@ struct VirtIOGPUGL {
 
     bool renderer_inited;
     bool renderer_reset;
+
+    QEMUTimer *fence_poll;
 };
 
 struct VhostUserGPU {
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 14091b191e..91dce90f91 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -594,11 +594,12 @@ static void virtio_gpu_print_stats(void *opaque)
 static void virtio_gpu_fence_poll(void *opaque)
 {
     VirtIOGPU *g = opaque;
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
 
     virgl_renderer_poll();
     virtio_gpu_process_cmdq(g);
     if (!QTAILQ_EMPTY(&g->cmdq) || !QTAILQ_EMPTY(&g->fenceq)) {
-        timer_mod(g->fence_poll, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 10);
+        timer_mod(gl->fence_poll, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 10);
     }
 }
 
@@ -626,6 +627,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
 {
     int ret;
     uint32_t flags = 0;
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
 
 #if VIRGL_RENDERER_CALLBACKS_VERSION >= 4
     if (qemu_egl_display) {
@@ -645,8 +647,8 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
         return ret;
     }
 
-    g->fence_poll = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-                                 virtio_gpu_fence_poll, g);
+    gl->fence_poll = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+                                  virtio_gpu_fence_poll, g);
 
     if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
         g->print_stats = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 03/13] virtio-gpu: Move print_stats timer to VirtIOGPUGL
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
  2024-10-29 12:10 ` [PULL 01/13] virtio-gpu: Use trace events for tracking number of in-flight fences Alex Bennée
  2024-10-29 12:10 ` [PULL 02/13] virtio-gpu: Move fence_poll timer to VirtIOGPUGL Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 04/13] virtio-gpu: Handle virtio_gpu_virgl_init() failure Alex Bennée
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Osipenko, Alex Bennée, Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Move print_stats timer to VirtIOGPUGL for consistency with
cmdq_resume_bh and fence_poll that are used only by GL device.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-4-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 48a67d662d..18b6c3b3a2 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -194,7 +194,6 @@ struct VirtIOGPU {
     uint64_t hostmem;
 
     bool processing_cmdq;
-    QEMUTimer *print_stats;
 
     uint32_t inflight;
     struct {
@@ -230,6 +229,7 @@ struct VirtIOGPUGL {
     bool renderer_reset;
 
     QEMUTimer *fence_poll;
+    QEMUTimer *print_stats;
 };
 
 struct VhostUserGPU {
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 91dce90f91..a63d1f540f 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -574,6 +574,7 @@ static struct virgl_renderer_callbacks virtio_gpu_3d_cbs = {
 static void virtio_gpu_print_stats(void *opaque)
 {
     VirtIOGPU *g = opaque;
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
 
     if (g->stats.requests) {
         fprintf(stderr, "stats: vq req %4d, %3d -- 3D %4d (%5d)\n",
@@ -588,7 +589,7 @@ static void virtio_gpu_print_stats(void *opaque)
     } else {
         fprintf(stderr, "stats: idle\r");
     }
-    timer_mod(g->print_stats, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
+    timer_mod(gl->print_stats, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
 }
 
 static void virtio_gpu_fence_poll(void *opaque)
@@ -651,9 +652,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
                                   virtio_gpu_fence_poll, g);
 
     if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
-        g->print_stats = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-                                      virtio_gpu_print_stats, g);
-        timer_mod(g->print_stats, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
+        gl->print_stats = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+                                       virtio_gpu_print_stats, g);
+        timer_mod(gl->print_stats,
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
     }
     return 0;
 }
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 04/13] virtio-gpu: Handle virtio_gpu_virgl_init() failure
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (2 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 03/13] virtio-gpu: Move print_stats " Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 05/13] virtio-gpu: Unrealize GL device Alex Bennée
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Osipenko, Alex Bennée, Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

virtio_gpu_virgl_init() may fail, leading to a further Qemu crash
because Qemu assumes it never fails. Check virtio_gpu_virgl_init()
return code and don't execute virtio commands on error. Failed
virtio_gpu_virgl_init() will result in a timed out virtio commands
for a guest OS.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-5-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 18b6c3b3a2..71775243e9 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -222,11 +222,18 @@ struct VirtIOGPUClass {
                              Error **errp);
 };
 
+/* VirtIOGPUGL renderer states */
+typedef enum {
+    RS_START,       /* starting state */
+    RS_INIT_FAILED, /* failed initialisation */
+    RS_INITED,      /* initialised and working */
+    RS_RESET,       /* inited and reset pending, moves to start after reset */
+} RenderState;
+
 struct VirtIOGPUGL {
     struct VirtIOGPU parent_obj;
 
-    bool renderer_inited;
-    bool renderer_reset;
+    RenderState renderer_state;
 
     QEMUTimer *fence_poll;
     QEMUTimer *print_stats;
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 29d20b0132..ea3413aa56 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -29,9 +29,14 @@ static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
                                              struct virtio_gpu_scanout *s,
                                              uint32_t resource_id)
 {
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
     uint32_t width, height;
     uint32_t pixels, *data;
 
+    if (gl->renderer_state != RS_INITED) {
+        return;
+    }
+
     data = virgl_renderer_get_cursor_data(resource_id, &width, &height);
     if (!data) {
         return;
@@ -65,13 +70,22 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         return;
     }
 
-    if (!gl->renderer_inited) {
-        virtio_gpu_virgl_init(g);
-        gl->renderer_inited = true;
-    }
-    if (gl->renderer_reset) {
-        gl->renderer_reset = false;
+    switch (gl->renderer_state) {
+    case RS_RESET:
         virtio_gpu_virgl_reset(g);
+        /* fallthrough */
+    case RS_START:
+        if (virtio_gpu_virgl_init(g)) {
+            gl->renderer_state = RS_INIT_FAILED;
+            return;
+        }
+
+        gl->renderer_state = RS_INITED;
+        break;
+    case RS_INIT_FAILED:
+        return;
+    case RS_INITED:
+        break;
     }
 
     cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
@@ -98,9 +112,9 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
      * GL functions must be called with the associated GL context in main
      * thread, and when the renderer is unblocked.
      */
-    if (gl->renderer_inited && !gl->renderer_reset) {
+    if (gl->renderer_state == RS_INITED) {
         virtio_gpu_virgl_reset_scanout(g);
-        gl->renderer_reset = true;
+        gl->renderer_state = RS_RESET;
     }
 }
 
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 05/13] virtio-gpu: Unrealize GL device
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (3 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 04/13] virtio-gpu: Handle virtio_gpu_virgl_init() failure Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 06/13] virtio-gpu: Use pkgconfig version to decide which virgl features are available Alex Bennée
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Osipenko, Alex Bennée, Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Even though GL GPU doesn't support hotplugging today, free virgl
resources when GL device is unrealized. For consistency.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-6-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index ea3413aa56..753b35ed69 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -156,6 +156,22 @@ static Property virtio_gpu_gl_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
+{
+    VirtIOGPU *g = VIRTIO_GPU(qdev);
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
+
+    if (gl->renderer_state >= RS_INITED) {
+        if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
+            timer_free(gl->print_stats);
+        }
+        timer_free(gl->fence_poll);
+        virgl_renderer_cleanup(NULL);
+    }
+
+    gl->renderer_state = RS_START;
+}
+
 static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -169,6 +185,7 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
     vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
 
     vdc->realize = virtio_gpu_gl_device_realize;
+    vdc->unrealize = virtio_gpu_gl_device_unrealize;
     vdc->reset = virtio_gpu_gl_reset;
     device_class_set_props(dc, virtio_gpu_gl_properties);
 }
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 06/13] virtio-gpu: Use pkgconfig version to decide which virgl features are available
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (4 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 05/13] virtio-gpu: Unrealize GL device Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 07/13] virtio-gpu: Support context-init feature with virglrenderer Alex Bennée
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dmitry Osipenko, Alex Bennée, Michael S. Tsirkin,
	Paolo Bonzini, Marc-André Lureau, Daniel P. Berrangé,
	Philippe Mathieu-Daudé

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

New virglrerenderer features were stabilized with release of v1.0.0.
Presence of symbols in virglrenderer.h doesn't guarantee ABI compatibility
with pre-release development versions of libvirglerender. Use virglrenderer
version to decide reliably which virgl features are available.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-7-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/meson.build b/meson.build
index 85594fd3f1..8dc53c12fb 100644
--- a/meson.build
+++ b/meson.build
@@ -2437,10 +2437,7 @@ config_host_data.set('CONFIG_VNC', vnc.found())
 config_host_data.set('CONFIG_VNC_JPEG', jpeg.found())
 config_host_data.set('CONFIG_VNC_SASL', sasl.found())
 if virgl.found()
-  config_host_data.set('HAVE_VIRGL_D3D_INFO_EXT',
-                       cc.has_member('struct virgl_renderer_resource_info_ext', 'd3d_tex2d',
-                                     prefix: '#include <virglrenderer.h>',
-                                     dependencies: virgl))
+  config_host_data.set('VIRGL_VERSION_MAJOR', virgl.version().split('.')[0])
 endif
 config_host_data.set('CONFIG_VIRTFS', have_virtfs)
 config_host_data.set('CONFIG_VTE', vte.found())
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index a63d1f540f..ca6f4d6cbb 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -171,7 +171,7 @@ static void virgl_cmd_set_scanout(VirtIOGPU *g,
         struct virgl_renderer_resource_info info;
         void *d3d_tex2d = NULL;
 
-#ifdef HAVE_VIRGL_D3D_INFO_EXT
+#if VIRGL_VERSION_MAJOR >= 1
         struct virgl_renderer_resource_info_ext ext;
         memset(&ext, 0, sizeof(ext));
         ret = virgl_renderer_resource_get_info_ext(ss.resource_id, &ext);
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 07/13] virtio-gpu: Support context-init feature with virglrenderer
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (5 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 06/13] virtio-gpu: Use pkgconfig version to decide which virgl features are available Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 08/13] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled Alex Bennée
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Huang Rui, Antonio Caggiano, Antonio Caggiano, Dmitry Osipenko,
	Alex Bennée, Michael S. Tsirkin

From: Huang Rui <ray.huang@amd.com>

Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
feature flags. Expose this feature and support creating virglrenderer
context with flags using context_id if libvirglrenderer is new enough.

Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Antonio Caggiano <quic_acaggian@quicinc.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-8-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 753b35ed69..bf87ba4232 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -147,6 +147,10 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
     VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =
         virtio_gpu_virgl_get_num_capsets(g);
 
+#if VIRGL_VERSION_MAJOR >= 1
+    g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED;
+#endif
+
     virtio_gpu_device_realize(qdev, errp);
 }
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index ca6f4d6cbb..b3aa444bcf 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -106,8 +106,24 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
                                     cc.debug_name);
 
-    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
-                                  cc.debug_name);
+    if (cc.context_init) {
+        if (!virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: context_init disabled",
+                          __func__);
+            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+            return;
+        }
+
+#if VIRGL_VERSION_MAJOR >= 1
+        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
+                                                 cc.context_init,
+                                                 cc.nlen,
+                                                 cc.debug_name);
+        return;
+#endif
+    }
+
+    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
 }
 
 static void virgl_cmd_context_destroy(VirtIOGPU *g,
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 08/13] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (6 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 07/13] virtio-gpu: Support context-init feature with virglrenderer Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 09/13] virtio-gpu: Add virgl resource management Alex Bennée
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dmitry Osipenko, Antonio Caggiano, Huang Rui, Antonio Caggiano,
	Marc-André Lureau, Alex Bennée, Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

The udmabuf usage is mandatory when virgl is disabled and blobs feature
enabled in the Qemu machine configuration. If virgl and blobs are enabled,
then udmabuf requirement is optional. Since udmabuf isn't widely supported
by a popular Linux distros today, let's relax the udmabuf requirement for
blobs=on,virgl=on. Now, a full-featured virtio-gpu acceleration is
available to Qemu users without a need to have udmabuf available in the
system.

Reviewed-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Antonio Caggiano <quic_acaggian@quicinc.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-9-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 3fcc434732..3d9679c1ef 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1455,6 +1455,7 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 
     if (virtio_gpu_blob_enabled(g->parent_obj.conf)) {
         if (!virtio_gpu_rutabaga_enabled(g->parent_obj.conf) &&
+            !virtio_gpu_virgl_enabled(g->parent_obj.conf) &&
             !virtio_gpu_have_udmabuf()) {
             error_setg(errp, "need rutabaga or udmabuf for blob resources");
             return;
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 09/13] virtio-gpu: Add virgl resource management
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (7 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 08/13] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 10/13] virtio-gpu: Support suspension of commands processing Alex Bennée
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Huang Rui, Antonio Caggiano, Dmitry Osipenko, Alex Bennée,
	Michael S. Tsirkin

From: Huang Rui <ray.huang@amd.com>

In a preparation to adding host blobs support to virtio-gpu, add virgl
resource management that allows to retrieve resource based on its ID
and virgl resource wrapper on top of simple resource that will be contain
fields specific to virgl.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Antonio Caggiano <quic_acaggian@quicinc.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-10-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index b3aa444bcf..3ffea478e7 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -22,6 +22,23 @@
 
 #include <virglrenderer.h>
 
+struct virtio_gpu_virgl_resource {
+    struct virtio_gpu_simple_resource base;
+};
+
+static struct virtio_gpu_virgl_resource *
+virtio_gpu_virgl_find_resource(VirtIOGPU *g, uint32_t resource_id)
+{
+    struct virtio_gpu_simple_resource *res;
+
+    res = virtio_gpu_find_resource(g, resource_id);
+    if (!res) {
+        return NULL;
+    }
+
+    return container_of(res, struct virtio_gpu_virgl_resource, base);
+}
+
 #if VIRGL_RENDERER_CALLBACKS_VERSION >= 4
 static void *
 virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
@@ -35,11 +52,34 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
 {
     struct virtio_gpu_resource_create_2d c2d;
     struct virgl_renderer_resource_create_args args;
+    struct virtio_gpu_virgl_resource *res;
 
     VIRTIO_GPU_FILL_CMD(c2d);
     trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
                                        c2d.width, c2d.height);
 
+    if (c2d.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, c2d.resource_id);
+    if (res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
+                      __func__, c2d.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res = g_new0(struct virtio_gpu_virgl_resource, 1);
+    res->base.width = c2d.width;
+    res->base.height = c2d.height;
+    res->base.format = c2d.format;
+    res->base.resource_id = c2d.resource_id;
+    QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
+
     args.handle = c2d.resource_id;
     args.target = 2;
     args.format = c2d.format;
@@ -59,11 +99,34 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
 {
     struct virtio_gpu_resource_create_3d c3d;
     struct virgl_renderer_resource_create_args args;
+    struct virtio_gpu_virgl_resource *res;
 
     VIRTIO_GPU_FILL_CMD(c3d);
     trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
                                        c3d.width, c3d.height, c3d.depth);
 
+    if (c3d.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, c3d.resource_id);
+    if (res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
+                      __func__, c3d.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res = g_new0(struct virtio_gpu_virgl_resource, 1);
+    res->base.width = c3d.width;
+    res->base.height = c3d.height;
+    res->base.format = c3d.format;
+    res->base.resource_id = c3d.resource_id;
+    QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
+
     args.handle = c3d.resource_id;
     args.target = c3d.target;
     args.format = c3d.format;
@@ -82,12 +145,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
                                      struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_resource_unref unref;
+    struct virtio_gpu_virgl_resource *res;
     struct iovec *res_iovs = NULL;
     int num_iovs = 0;
 
     VIRTIO_GPU_FILL_CMD(unref);
     trace_virtio_gpu_cmd_res_unref(unref.resource_id);
 
+    res = virtio_gpu_virgl_find_resource(g, unref.resource_id);
+    if (!res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
+                      __func__, unref.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
     virgl_renderer_resource_detach_iov(unref.resource_id,
                                        &res_iovs,
                                        &num_iovs);
@@ -95,6 +167,10 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
         virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
     }
     virgl_renderer_resource_unref(unref.resource_id);
+
+    QTAILQ_REMOVE(&g->reslist, &res->base, next);
+
+    g_free(res);
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 10/13] virtio-gpu: Support suspension of commands processing
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (8 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 09/13] virtio-gpu: Add virgl resource management Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 11/13] virtio-gpu: Handle resource blob commands Alex Bennée
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dmitry Osipenko, Akihiko Odaki, Alex Bennée,
	Michael S. Tsirkin

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Check whether command processing has been finished; otherwise, stop
processing commands and retry the command again next time. This allows
us to support asynchronous execution of non-fenced commands needed for
unmapping host blobs safely.

Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-11-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 3d9679c1ef..180d882f0a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1034,6 +1034,12 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
         /* process command */
         vgc->process_cmd(g, cmd);
 
+        /* command suspended */
+        if (!cmd->finished && !(cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_FENCE)) {
+            trace_virtio_gpu_cmd_suspended(cmd->cmd_hdr.type);
+            break;
+        }
+
         QTAILQ_REMOVE(&g->cmdq, cmd, next);
         if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
             g->stats.requests++;
diff --git a/hw/display/trace-events b/hw/display/trace-events
index e212710284..d26d663f96 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -55,6 +55,7 @@ virtio_gpu_fence_ctrl(uint64_t fence, uint32_t type) "fence 0x%" PRIx64 ", type
 virtio_gpu_fence_resp(uint64_t fence) "fence 0x%" PRIx64
 virtio_gpu_inc_inflight_fences(uint32_t inflight) "in-flight+ %u"
 virtio_gpu_dec_inflight_fences(uint32_t inflight) "in-flight- %u"
+virtio_gpu_cmd_suspended(uint32_t cmd) "cmd 0x%x"
 
 # qxl.c
 disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (9 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 10/13] virtio-gpu: Support suspension of commands processing Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-11-01 15:35   ` Peter Maydell
  2024-10-29 12:10 ` [PULL 12/13] virtio-gpu: Register capsets dynamically Alex Bennée
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Robert Beckett, Antonio Caggiano, Xenia Ragiadakou, Huang Rui,
	Dmitry Osipenko, Alex Bennée, Michael S. Tsirkin

From: Robert Beckett <bob.beckett@collabora.com>

Support BLOB resources creation, mapping, unmapping and set-scanout 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: Robert Beckett <bob.beckett@collabora.com> # added set_scanout_blob
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>
Message-Id: <20241024210311.118220-12-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 71775243e9..f9ed81071f 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -237,6 +237,8 @@ struct VirtIOGPUGL {
 
     QEMUTimer *fence_poll;
     QEMUTimer *print_stats;
+
+    QEMUBH *cmdq_resume_bh;
 };
 
 struct VhostUserGPU {
@@ -336,6 +338,13 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
                              struct virtio_gpu_framebuffer *fb,
                              struct virtio_gpu_rect *r);
 
+void virtio_gpu_update_scanout(VirtIOGPU *g,
+                               uint32_t scanout_id,
+                               struct virtio_gpu_simple_resource *res,
+                               struct virtio_gpu_framebuffer *fb,
+                               struct virtio_gpu_rect *r);
+void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id);
+
 /* virtio-gpu-3d.c */
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                   struct virtio_gpu_ctrl_command *cmd);
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index bf87ba4232..f2555673a1 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -166,6 +166,9 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(qdev);
 
     if (gl->renderer_state >= RS_INITED) {
+#if VIRGL_VERSION_MAJOR >= 1
+        qemu_bh_delete(gl->cmdq_resume_bh);
+#endif
         if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
             timer_free(gl->print_stats);
         }
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 3ffea478e7..b2f4e215a7 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,8 @@
 #include "trace.h"
 #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"
 
@@ -24,6 +26,7 @@
 
 struct virtio_gpu_virgl_resource {
     struct virtio_gpu_simple_resource base;
+    MemoryRegion *mr;
 };
 
 static struct virtio_gpu_virgl_resource *
@@ -47,6 +50,145 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
 }
 #endif
 
+#if VIRGL_VERSION_MAJOR >= 1
+struct virtio_gpu_virgl_hostmem_region {
+    MemoryRegion mr;
+    struct VirtIOGPU *g;
+    bool finish_unmapping;
+};
+
+static struct virtio_gpu_virgl_hostmem_region *
+to_hostmem_region(MemoryRegion *mr)
+{
+    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
+}
+
+static void virtio_gpu_virgl_resume_cmdq_bh(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;
+    VirtIOGPUGL *gl;
+
+    vmr = to_hostmem_region(mr);
+    vmr->finish_unmapping = true;
+
+    b = VIRTIO_GPU_BASE(vmr->g);
+    b->renderer_blocked--;
+
+    /*
+     * memory_region_unref() is executed from RCU thread context, while
+     * virglrenderer works only on the main-loop thread that's holding GL
+     * context.
+     */
+    gl = VIRTIO_GPU_GL(vmr->g);
+    qemu_bh_schedule(gl->cmdq_resume_bh);
+}
+
+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: %s\n",
+                      __func__, strerror(-ret));
+        return ret;
+    }
+
+    vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
+    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);
+
+    /*
+     * 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 freed.
+     */
+    OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
+
+    res->mr = mr;
+
+    return 0;
+}
+
+static int
+virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
+                                     struct virtio_gpu_virgl_resource *res,
+                                     bool *cmd_suspended)
+{
+    struct virtio_gpu_virgl_hostmem_region *vmr;
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+    MemoryRegion *mr = res->mr;
+    int ret;
+
+    if (!mr) {
+        return 0;
+    }
+
+    vmr = to_hostmem_region(res->mr);
+
+    /*
+     * Perform async unmapping in 3 steps:
+     *
+     * 1. Begin async unmapping with memory_region_del_subregion()
+     *    and suspend/block cmd processing.
+     * 2. Wait for res->mr to be freed and cmd processing resumed
+     *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
+     * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
+     */
+    if (vmr->finish_unmapping) {
+        res->mr = NULL;
+        g_free(vmr);
+
+        ret = virgl_renderer_resource_unmap(res->base.resource_id);
+        if (ret) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: failed to unmap virgl resource: %s\n",
+                          __func__, strerror(-ret));
+            return ret;
+        }
+    } else {
+        *cmd_suspended = true;
+
+        /* render will be unblocked once MR is freed */
+        b->renderer_blocked++;
+
+        /* memory region owns self res->mr object and frees it by itself */
+        memory_region_set_enabled(mr, false);
+        memory_region_del_subregion(&b->hostmem, mr);
+        object_unparent(OBJECT(mr));
+    }
+
+    return 0;
+}
+#endif
+
 static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
                                          struct virtio_gpu_ctrl_command *cmd)
 {
@@ -78,6 +220,7 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
     res->base.height = c2d.height;
     res->base.format = c2d.format;
     res->base.resource_id = c2d.resource_id;
+    res->base.dmabuf_fd = -1;
     QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
 
     args.handle = c2d.resource_id;
@@ -125,6 +268,7 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
     res->base.height = c3d.height;
     res->base.format = c3d.format;
     res->base.resource_id = c3d.resource_id;
+    res->base.dmabuf_fd = -1;
     QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
 
     args.handle = c3d.resource_id;
@@ -142,7 +286,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
 }
 
 static void virgl_cmd_resource_unref(VirtIOGPU *g,
-                                     struct virtio_gpu_ctrl_command *cmd)
+                                     struct virtio_gpu_ctrl_command *cmd,
+                                     bool *cmd_suspended)
 {
     struct virtio_gpu_resource_unref unref;
     struct virtio_gpu_virgl_resource *res;
@@ -160,6 +305,16 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
         return;
     }
 
+#if VIRGL_VERSION_MAJOR >= 1
+    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        return;
+    }
+    if (*cmd_suspended) {
+        return;
+    }
+#endif
+
     virgl_renderer_resource_detach_iov(unref.resource_id,
                                        &res_iovs,
                                        &num_iovs);
@@ -509,9 +664,241 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
     g_free(resp);
 }
 
+#if VIRGL_VERSION_MAJOR >= 1
+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 };
+    g_autofree struct virtio_gpu_virgl_resource *res = NULL;
+    struct virtio_gpu_resource_create_blob cblob;
+    struct virgl_renderer_resource_info info;
+    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) {
+            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+            return;
+        }
+    }
+
+    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;
+        virtio_gpu_cleanup_mapping(g, &res->base);
+        return;
+    }
+
+    ret = virgl_renderer_resource_get_info(cblob.resource_id, &info);
+    if (ret) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: resource does not have info %d: %s\n",
+                      __func__, cblob.resource_id, strerror(-ret));
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        virtio_gpu_cleanup_mapping(g, &res->base);
+        virgl_renderer_resource_unref(cblob.resource_id);
+        return;
+    }
+
+    res->base.dmabuf_fd = info.fd;
+
+    QTAILQ_INSERT_HEAD(&g->reslist, &res->base, next);
+    res = NULL;
+}
+
+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;
+    }
+
+    ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
+    if (ret) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        return;
+    }
+
+    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 void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
+                                          struct virtio_gpu_ctrl_command *cmd,
+                                          bool *cmd_suspended)
+{
+    struct virtio_gpu_resource_unmap_blob ublob;
+    struct virtio_gpu_virgl_resource *res;
+    int ret;
+
+    VIRTIO_GPU_FILL_CMD(ublob);
+    virtio_gpu_unmap_blob_bswap(&ublob);
+
+    res = virtio_gpu_virgl_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;
+    }
+
+    ret = virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended);
+    if (ret) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        return;
+    }
+}
+
+static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
+                                       struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_framebuffer fb = { 0 };
+    struct virtio_gpu_virgl_resource *res;
+    struct virtio_gpu_set_scanout_blob ss;
+    uint64_t fbend;
+
+    VIRTIO_GPU_FILL_CMD(ss);
+    virtio_gpu_scanout_blob_bswap(&ss);
+    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;
+    }
+
+    res = virtio_gpu_virgl_find_resource(g, ss.resource_id);
+    if (!res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
+                      __func__, ss.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+    if (res->base.dmabuf_fd < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource not backed by dmabuf %d\n",
+                      __func__, ss.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        return;
+    }
+
+    fb.format = virtio_gpu_get_pixman_format(ss.format);
+    if (!fb.format) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %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 > res->base.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->base, &fb, &ss.r)) {
+        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->base, &fb, &ss.r);
+}
+#endif
+
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                       struct virtio_gpu_ctrl_command *cmd)
 {
+    bool cmd_suspended = false;
+
     VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
 
     virgl_renderer_force_ctx_0();
@@ -553,7 +940,7 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
         virgl_cmd_resource_flush(g, cmd);
         break;
     case VIRTIO_GPU_CMD_RESOURCE_UNREF:
-        virgl_cmd_resource_unref(g, cmd);
+        virgl_cmd_resource_unref(g, cmd, &cmd_suspended);
         break;
     case VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE:
         /* TODO add security */
@@ -575,12 +962,26 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
     case VIRTIO_GPU_CMD_GET_EDID:
         virtio_gpu_get_edid(g, cmd);
         break;
+#if VIRGL_VERSION_MAJOR >= 1
+    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, &cmd_suspended);
+        break;
+    case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB:
+        virgl_cmd_set_scanout_blob(g, cmd);
+        break;
+#endif
     default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         break;
     }
 
-    if (cmd->finished) {
+    if (cmd_suspended || cmd->finished) {
         return;
     }
     if (cmd->error) {
@@ -749,6 +1150,13 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
         timer_mod(gl->print_stats,
                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
     }
+
+#if VIRGL_VERSION_MAJOR >= 1
+    gl->cmdq_resume_bh = aio_bh_new(qemu_get_aio_context(),
+                                    virtio_gpu_virgl_resume_cmdq_bh,
+                                    g);
+#endif
+
     return 0;
 }
 
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 180d882f0a..b6cd9fe567 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -362,7 +362,7 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
     QTAILQ_INSERT_HEAD(&g->reslist, res, next);
 }
 
-static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
+void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
 {
     struct virtio_gpu_scanout *scanout = &g->parent_obj.scanout[scanout_id];
     struct virtio_gpu_simple_resource *res;
@@ -579,11 +579,11 @@ static void virtio_unref_resource(pixman_image_t *image, void *data)
     pixman_image_unref(data);
 }
 
-static void virtio_gpu_update_scanout(VirtIOGPU *g,
-                                      uint32_t scanout_id,
-                                      struct virtio_gpu_simple_resource *res,
-                                      struct virtio_gpu_framebuffer *fb,
-                                      struct virtio_gpu_rect *r)
+void virtio_gpu_update_scanout(VirtIOGPU *g,
+                               uint32_t scanout_id,
+                               struct virtio_gpu_simple_resource *res,
+                               struct virtio_gpu_framebuffer *fb,
+                               struct virtio_gpu_rect *r)
 {
     struct virtio_gpu_simple_resource *ores;
     struct virtio_gpu_scanout *scanout;
@@ -1467,10 +1467,14 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
             return;
         }
 
+#ifdef VIRGL_VERSION_MAJOR
+    #if VIRGL_VERSION_MAJOR < 1
         if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
-            error_setg(errp, "blobs and virgl are not compatible (yet)");
+            error_setg(errp, "old virglrenderer, blob resources unsupported");
             return;
         }
+    #endif
+#endif
     }
 
     if (!virtio_gpu_base_device_realize(qdev,
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 12/13] virtio-gpu: Register capsets dynamically
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (10 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 11/13] virtio-gpu: Handle resource blob commands Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-29 12:10 ` [PULL 13/13] virtio-gpu: Support Venus context Alex Bennée
  2024-10-31 11:29 ` [PULL 00/13] virtio-gpu vulkan support Peter Maydell
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Pierre-Eric Pelloux-Prayer, Manos Pitsidianakis, Dmitry Osipenko,
	Alex Bennée, Michael S. Tsirkin

From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

virtio_gpu_virgl_get_num_capsets will return "num_capsets", but we can't
assume that capset_index 1 is always VIRGL2 once we'll support more capsets,
like Venus and DRM capsets. Register capsets dynamically to avoid that problem.

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-13-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index f9ed81071f..07131d8eb9 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -207,6 +207,8 @@ struct VirtIOGPU {
         QTAILQ_HEAD(, VGPUDMABuf) bufs;
         VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
     } dmabuf;
+
+    GArray *capset_ids;
 };
 
 struct VirtIOGPUClass {
@@ -352,6 +354,6 @@ void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
 void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
 void virtio_gpu_virgl_reset(VirtIOGPU *g);
 int virtio_gpu_virgl_init(VirtIOGPU *g);
-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
+GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g);
 
 #endif
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index f2555673a1..e859c0dff0 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -144,8 +144,8 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
     }
 
     g->parent_obj.conf.flags |= (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);
-    VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =
-        virtio_gpu_virgl_get_num_capsets(g);
+    g->capset_ids = virtio_gpu_virgl_get_capsets(g);
+    VIRTIO_GPU_BASE(g)->virtio_config.num_capsets = g->capset_ids->len;
 
 #if VIRGL_VERSION_MAJOR >= 1
     g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED;
@@ -177,6 +177,8 @@ static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
     }
 
     gl->renderer_state = RS_START;
+
+    g_array_unref(g->capset_ids);
 }
 
 static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index b2f4e215a7..5a881c58a1 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -622,19 +622,13 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
     VIRTIO_GPU_FILL_CMD(info);
 
     memset(&resp, 0, sizeof(resp));
-    if (info.capset_index == 0) {
-        resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
-        virgl_renderer_get_cap_set(resp.capset_id,
-                                   &resp.capset_max_version,
-                                   &resp.capset_max_size);
-    } else if (info.capset_index == 1) {
-        resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL2;
+
+    if (info.capset_index < g->capset_ids->len) {
+        resp.capset_id = g_array_index(g->capset_ids, uint32_t,
+                                       info.capset_index);
         virgl_renderer_get_cap_set(resp.capset_id,
                                    &resp.capset_max_version,
                                    &resp.capset_max_size);
-    } else {
-        resp.capset_max_version = 0;
-        resp.capset_max_size = 0;
     }
     resp.hdr.type = VIRTIO_GPU_RESP_OK_CAPSET_INFO;
     virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
@@ -1160,12 +1154,27 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
     return 0;
 }
 
-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
+static void virtio_gpu_virgl_add_capset(GArray *capset_ids, uint32_t capset_id)
+{
+    g_array_append_val(capset_ids, capset_id);
+}
+
+GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g)
 {
     uint32_t capset2_max_ver, capset2_max_size;
+    GArray *capset_ids;
+
+    capset_ids = g_array_new(false, false, sizeof(uint32_t));
+
+    /* VIRGL is always supported. */
+    virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL);
+
     virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
                               &capset2_max_ver,
                               &capset2_max_size);
+    if (capset2_max_ver) {
+        virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL2);
+    }
 
-    return capset2_max_ver ? 2 : 1;
+    return capset_ids;
 }
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PULL 13/13] virtio-gpu: Support Venus context
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (11 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 12/13] virtio-gpu: Register capsets dynamically Alex Bennée
@ 2024-10-29 12:10 ` Alex Bennée
  2024-10-31 11:29 ` [PULL 00/13] virtio-gpu vulkan support Peter Maydell
  13 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-10-29 12:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Antonio Caggiano, Huang Rui, Dmitry Osipenko, Alex Bennée,
	Michael S. Tsirkin

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

Request Venus when initializing VirGL and if venus=true flag is set for
virtio-gpu-gl device.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20241024210311.118220-14-dmitry.osipenko@collabora.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/docs/system/devices/virtio-gpu.rst b/docs/system/devices/virtio-gpu.rst
index cb73dd7998..b7eb0fc0e7 100644
--- a/docs/system/devices/virtio-gpu.rst
+++ b/docs/system/devices/virtio-gpu.rst
@@ -71,6 +71,17 @@ representation back to OpenGL API calls.
 .. _Gallium3D: https://www.freedesktop.org/wiki/Software/gallium/
 .. _virglrenderer: https://gitlab.freedesktop.org/virgl/virglrenderer/
 
+Translation of Vulkan API calls is supported since release of `virglrenderer`_
+v1.0.0 using `venus`_ protocol. ``Venus`` virtio-gpu capability set ("capset")
+requires host blob support (``hostmem`` and ``blob`` fields) and should
+be enabled using ``venus`` field. The ``hostmem`` field specifies the size
+of virtio-gpu host memory window. This is typically between 256M and 8G.
+
+.. parsed-literal::
+    -device virtio-gpu-gl,hostmem=8G,blob=true,venus=true
+
+.. _venus: https://gitlab.freedesktop.org/virgl/venus-protocol/
+
 virtio-gpu rutabaga
 -------------------
 
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 07131d8eb9..553799b8cc 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -97,6 +97,7 @@ enum virtio_gpu_base_conf_flags {
     VIRTIO_GPU_FLAG_BLOB_ENABLED,
     VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
     VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
+    VIRTIO_GPU_FLAG_VENUS_ENABLED,
 };
 
 #define virtio_gpu_virgl_enabled(_cfg) \
@@ -115,6 +116,8 @@ enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
 #define virtio_gpu_hostmem_enabled(_cfg) \
     (_cfg.hostmem > 0)
+#define virtio_gpu_venus_enabled(_cfg) \
+    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_VENUS_ENABLED))
 
 struct virtio_gpu_base_conf {
     uint32_t max_outputs;
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index e859c0dff0..7c0e448b46 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -157,6 +157,8 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
 static Property virtio_gpu_gl_properties[] = {
     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
+    DEFINE_PROP_BIT("venus", VirtIOGPU, parent_obj.conf.flags,
+                    VIRTIO_GPU_FLAG_VENUS_ENABLED, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 5a881c58a1..eedae7357f 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -1128,6 +1128,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
         flags |= VIRGL_RENDERER_D3D11_SHARE_TEXTURE;
     }
 #endif
+#if VIRGL_VERSION_MAJOR >= 1
+    if (virtio_gpu_venus_enabled(g->parent_obj.conf)) {
+        flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
+    }
+#endif
 
     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
     if (ret != 0) {
@@ -1161,7 +1166,7 @@ static void virtio_gpu_virgl_add_capset(GArray *capset_ids, uint32_t capset_id)
 
 GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g)
 {
-    uint32_t capset2_max_ver, capset2_max_size;
+    uint32_t capset_max_ver, capset_max_size;
     GArray *capset_ids;
 
     capset_ids = g_array_new(false, false, sizeof(uint32_t));
@@ -1170,11 +1175,20 @@ GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g)
     virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL);
 
     virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
-                              &capset2_max_ver,
-                              &capset2_max_size);
-    if (capset2_max_ver) {
+                               &capset_max_ver,
+                               &capset_max_size);
+    if (capset_max_ver) {
         virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL2);
     }
 
+    if (virtio_gpu_venus_enabled(g->parent_obj.conf)) {
+        virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
+                                   &capset_max_ver,
+                                   &capset_max_size);
+        if (capset_max_size) {
+            virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VENUS);
+        }
+    }
+
     return capset_ids;
 }
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index b6cd9fe567..c0570ef856 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1477,6 +1477,21 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 #endif
     }
 
+    if (virtio_gpu_venus_enabled(g->parent_obj.conf)) {
+#ifdef VIRGL_VERSION_MAJOR
+    #if VIRGL_VERSION_MAJOR >= 1
+        if (!virtio_gpu_blob_enabled(g->parent_obj.conf) ||
+            !virtio_gpu_hostmem_enabled(g->parent_obj.conf)) {
+            error_setg(errp, "venus requires enabled blob and hostmem options");
+            return;
+        }
+    #else
+        error_setg(errp, "old virglrenderer, venus unsupported");
+        return;
+    #endif
+#endif
+    }
+
     if (!virtio_gpu_base_device_realize(qdev,
                                         virtio_gpu_handle_ctrl_cb,
                                         virtio_gpu_handle_cursor_cb,
-- 
2.39.5



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PULL 00/13] virtio-gpu vulkan support
  2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
                   ` (12 preceding siblings ...)
  2024-10-29 12:10 ` [PULL 13/13] virtio-gpu: Support Venus context Alex Bennée
@ 2024-10-31 11:29 ` Peter Maydell
  13 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2024-10-31 11:29 UTC (permalink / raw)
  To: Alex Bennée; +Cc: qemu-devel

On Tue, 29 Oct 2024 at 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The following changes since commit fdf250e5a37830615e324017cb3a503e84b3712c:
>
>   Merge tag 'pull-maintainer-oct-misc-241024-1' of https://gitlab.com/stsquad/qemu into staging (2024-10-25 19:12:06 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/stsquad/qemu.git tags/pull-virtio-gpu-vulkan-291024-1
>
> for you to fetch changes up to 94d0ea1c19289d76ced934711fccd2269e69bb29:
>
>   virtio-gpu: Support Venus context (2024-10-28 16:56:36 +0000)
>
> ----------------------------------------------------------------
> virtio-gpu: add venus/vulkan capability
>
> We are currently lacking a declared maintainer for the sub-system so
> while we look for one I'm merging after testing locally.
>
>   - convert some fprintfs to proper trace events
>   - move timers used by GL devices into GL structures
>   - handle virtio_gpu_virgl_init() failure better
>   - implement unrealize for GL devices
>   - use virgl version numbering to gate features
>   - support context-init feature
>   - don't require udmabuf for virgl only
>   - add virgl resource tracker
>   - allow command submission to be suspended
>   - handle resource blob commands
>   - dynamically handle capabilit sets
>   - add venus context support for passing vulkan


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.2
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-10-29 12:10 ` [PULL 11/13] virtio-gpu: Handle resource blob commands Alex Bennée
@ 2024-11-01 15:35   ` Peter Maydell
  2024-11-01 16:04     ` Dmitry Osipenko
  2024-11-01 17:16     ` Alex Bennée
  0 siblings, 2 replies; 22+ messages in thread
From: Peter Maydell @ 2024-11-01 15:35 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, Robert Beckett, Antonio Caggiano, Xenia Ragiadakou,
	Huang Rui, Dmitry Osipenko, Michael S. Tsirkin

On Tue, 29 Oct 2024 at 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From: Robert Beckett <bob.beckett@collabora.com>
>
> Support BLOB resources creation, mapping, unmapping and set-scanout 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: Robert Beckett <bob.beckett@collabora.com> # added set_scanout_blob
> 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>
> Message-Id: <20241024210311.118220-12-dmitry.osipenko@collabora.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Hi; Coverity points out some possible issues with this commit:


> +    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;

Here 'fbend' is a uint64_t, but fb.stride, fb.bytes_pp,
ss.r.height and ss.r.width are all uint32_t. So these
multiplications will be done as 32x32->32 before being
added to fbend, potentially overflowing. This probably
isn't what was intended.

(This is Coverity CID 1564769, 1564770.)

The calculation of fb.offset also might overflow, but
Coverity doesn't spot that because fb.offset is uint32_t
and so the whole calculation is 32-bit all the way through
without a late-32-to-64 extension.

> +    if (fbend > res->base.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;
> +    }

thanks
-- PMM


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-11-01 15:35   ` Peter Maydell
@ 2024-11-01 16:04     ` Dmitry Osipenko
  2024-11-01 17:17       ` Alex Bennée
  2024-11-01 17:16     ` Alex Bennée
  1 sibling, 1 reply; 22+ messages in thread
From: Dmitry Osipenko @ 2024-11-01 16:04 UTC (permalink / raw)
  To: Peter Maydell, Alex Bennée
  Cc: qemu-devel, Robert Beckett, Antonio Caggiano, Xenia Ragiadakou,
	Huang Rui, Michael S. Tsirkin

On 11/1/24 18:35, Peter Maydell wrote:
> On Tue, 29 Oct 2024 at 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> Support BLOB resources creation, mapping, unmapping and set-scanout 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: Robert Beckett <bob.beckett@collabora.com> # added set_scanout_blob
>> 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>
>> Message-Id: <20241024210311.118220-12-dmitry.osipenko@collabora.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> Hi; Coverity points out some possible issues with this commit:
> 
> 
>> +    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;
> 
> Here 'fbend' is a uint64_t, but fb.stride, fb.bytes_pp,
> ss.r.height and ss.r.width are all uint32_t. So these
> multiplications will be done as 32x32->32 before being
> added to fbend, potentially overflowing. This probably
> isn't what was intended.
> 
> (This is Coverity CID 1564769, 1564770.)
> 
> The calculation of fb.offset also might overflow, but
> Coverity doesn't spot that because fb.offset is uint32_t
> and so the whole calculation is 32-bit all the way through
> without a late-32-to-64 extension.

Will make patch to silence this Coverity warning. AFAICT, there are
other ways to cause integer overflows in that code, though I assume it
all should be harmless. Thanks!

-- 
Best regards,
Dmitry


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-11-01 15:35   ` Peter Maydell
  2024-11-01 16:04     ` Dmitry Osipenko
@ 2024-11-01 17:16     ` Alex Bennée
  2024-11-01 18:23       ` Dmitry Osipenko
  1 sibling, 1 reply; 22+ messages in thread
From: Alex Bennée @ 2024-11-01 17:16 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-devel, Robert Beckett, Antonio Caggiano, Xenia Ragiadakou,
	Huang Rui, Dmitry Osipenko, Michael S. Tsirkin

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 29 Oct 2024 at 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> Support BLOB resources creation, mapping, unmapping and set-scanout 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: Robert Beckett <bob.beckett@collabora.com> # added set_scanout_blob
>> 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>
>> Message-Id: <20241024210311.118220-12-dmitry.osipenko@collabora.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> Hi; Coverity points out some possible issues with this commit:
>
>
>> +    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;
>
> Here 'fbend' is a uint64_t, but fb.stride, fb.bytes_pp,
> ss.r.height and ss.r.width are all uint32_t. So these
> multiplications will be done as 32x32->32 before being
> added to fbend, potentially overflowing. This probably
> isn't what was intended.

Also what is the subtlety behind using both stride and bytes_pp in the
calculation. My naive thought would be:

  fb.bytes_pp * ss.r.width == fb.stride

Can anyone enlighten me?

> (This is Coverity CID 1564769, 1564770.)
>
> The calculation of fb.offset also might overflow, but
> Coverity doesn't spot that because fb.offset is uint32_t
> and so the whole calculation is 32-bit all the way through
> without a late-32-to-64 extension.

I'll roll a patch once I understand this (and fixup the other case as
well).

>
>> +    if (fbend > res->base.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;
>> +    }
>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-11-01 16:04     ` Dmitry Osipenko
@ 2024-11-01 17:17       ` Alex Bennée
  0 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2024-11-01 17:17 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Peter Maydell, qemu-devel, Robert Beckett, Antonio Caggiano,
	Xenia Ragiadakou, Huang Rui, Michael S. Tsirkin

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 11/1/24 18:35, Peter Maydell wrote:
>> On Tue, 29 Oct 2024 at 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> From: Robert Beckett <bob.beckett@collabora.com>
>>>
>>> Support BLOB resources creation, mapping, unmapping and set-scanout 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: Robert Beckett <bob.beckett@collabora.com> # added set_scanout_blob
>>> 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>
>>> Message-Id: <20241024210311.118220-12-dmitry.osipenko@collabora.com>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> Hi; Coverity points out some possible issues with this commit:
>> 
>> 
>>> +    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;
>> 
>> Here 'fbend' is a uint64_t, but fb.stride, fb.bytes_pp,
>> ss.r.height and ss.r.width are all uint32_t. So these
>> multiplications will be done as 32x32->32 before being
>> added to fbend, potentially overflowing. This probably
>> isn't what was intended.
>> 
>> (This is Coverity CID 1564769, 1564770.)
>> 
>> The calculation of fb.offset also might overflow, but
>> Coverity doesn't spot that because fb.offset is uint32_t
>> and so the whole calculation is 32-bit all the way through
>> without a late-32-to-64 extension.
>
> Will make patch to silence this Coverity warning. AFAICT, there are
> other ways to cause integer overflows in that code, though I assume it
> all should be harmless. Thanks!

Ahh I'm happy for you to do it as you are more familiar with the code.
Note the duplicated code.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-11-01 17:16     ` Alex Bennée
@ 2024-11-01 18:23       ` Dmitry Osipenko
  2024-11-04 11:48         ` Alex Bennée
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Osipenko @ 2024-11-01 18:23 UTC (permalink / raw)
  To: Alex Bennée, Peter Maydell
  Cc: qemu-devel, Robert Beckett, Antonio Caggiano, Xenia Ragiadakou,
	Huang Rui, Michael S. Tsirkin

On 11/1/24 20:16, Alex Bennée wrote:
> Also what is the subtlety behind using both stride and bytes_pp in the
> calculation. My naive thought would be:
> 
>   fb.bytes_pp * ss.r.width == fb.stride
> 
> Can anyone enlighten me?

GPUs want image line size to be aligned to a power of 2 value, like 64
bytes for example. This aligned size of the line is called stride.

GPU's DMA engine operates with a predefined granularity when it accesses
memory, it reads/writes memory chunks that are multiple of a stride.
GPUs almost never support memory accesses at a granularity of one byte,
like CPUs do it.

-- 
Best regards,
Dmitry


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-11-01 18:23       ` Dmitry Osipenko
@ 2024-11-04 11:48         ` Alex Bennée
  2024-11-04 13:50           ` BALATON Zoltan
  0 siblings, 1 reply; 22+ messages in thread
From: Alex Bennée @ 2024-11-04 11:48 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Peter Maydell, qemu-devel, Robert Beckett, Antonio Caggiano,
	Xenia Ragiadakou, Huang Rui, Michael S. Tsirkin

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 11/1/24 20:16, Alex Bennée wrote:
>> Also what is the subtlety behind using both stride and bytes_pp in the
>> calculation. My naive thought would be:
>> 
>>   fb.bytes_pp * ss.r.width == fb.stride
>> 
>> Can anyone enlighten me?
>
> GPUs want image line size to be aligned to a power of 2 value, like 64
> bytes for example. This aligned size of the line is called stride.
>
> GPU's DMA engine operates with a predefined granularity when it accesses
> memory, it reads/writes memory chunks that are multiple of a stride.
> GPUs almost never support memory accesses at a granularity of one byte,
> like CPUs do it.

Ok that seems worth covering in a comment. Also what is going on with:

    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;

and then calculating fbend from offset + the calculation? Is this
because we need 2 full frames of storage? Can that ever change?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PULL 11/13] virtio-gpu: Handle resource blob commands
  2024-11-04 11:48         ` Alex Bennée
@ 2024-11-04 13:50           ` BALATON Zoltan
  0 siblings, 0 replies; 22+ messages in thread
From: BALATON Zoltan @ 2024-11-04 13:50 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Dmitry Osipenko, Peter Maydell, qemu-devel, Robert Beckett,
	Antonio Caggiano, Xenia Ragiadakou, Huang Rui, Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Mon, 4 Nov 2024, Alex Bennée wrote:
> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>
>> On 11/1/24 20:16, Alex Bennée wrote:
>>> Also what is the subtlety behind using both stride and bytes_pp in the
>>> calculation. My naive thought would be:
>>>
>>>   fb.bytes_pp * ss.r.width == fb.stride
>>>
>>> Can anyone enlighten me?
>>
>> GPUs want image line size to be aligned to a power of 2 value, like 64
>> bytes for example. This aligned size of the line is called stride.
>>
>> GPU's DMA engine operates with a predefined granularity when it accesses
>> memory, it reads/writes memory chunks that are multiple of a stride.
>> GPUs almost never support memory accesses at a granularity of one byte,
>> like CPUs do it.
>
> Ok that seems worth covering in a comment. Also what is going on with:
>
>    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
>
> and then calculating fbend from offset + the calculation? Is this
> because we need 2 full frames of storage? Can that ever change?

Stride is commonly used for storing data describing 2D blocks such as 
bitmaps so maybe not comment worthy but could be mentioned if you think it 
makes it clearer. Here's some documentation with a figure that may answer 
some of your questions:

https://learn.microsoft.com/en-us/windows/win32/medfound/image-stride

(I don't know what this thread is about, just stumbled upon this message 
and thought this may help. If not then sorry for the noise.)

Regards,
BALATON Zoltan

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2024-11-04 13:51 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-29 12:10 [PULL 00/13] virtio-gpu vulkan support Alex Bennée
2024-10-29 12:10 ` [PULL 01/13] virtio-gpu: Use trace events for tracking number of in-flight fences Alex Bennée
2024-10-29 12:10 ` [PULL 02/13] virtio-gpu: Move fence_poll timer to VirtIOGPUGL Alex Bennée
2024-10-29 12:10 ` [PULL 03/13] virtio-gpu: Move print_stats " Alex Bennée
2024-10-29 12:10 ` [PULL 04/13] virtio-gpu: Handle virtio_gpu_virgl_init() failure Alex Bennée
2024-10-29 12:10 ` [PULL 05/13] virtio-gpu: Unrealize GL device Alex Bennée
2024-10-29 12:10 ` [PULL 06/13] virtio-gpu: Use pkgconfig version to decide which virgl features are available Alex Bennée
2024-10-29 12:10 ` [PULL 07/13] virtio-gpu: Support context-init feature with virglrenderer Alex Bennée
2024-10-29 12:10 ` [PULL 08/13] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled Alex Bennée
2024-10-29 12:10 ` [PULL 09/13] virtio-gpu: Add virgl resource management Alex Bennée
2024-10-29 12:10 ` [PULL 10/13] virtio-gpu: Support suspension of commands processing Alex Bennée
2024-10-29 12:10 ` [PULL 11/13] virtio-gpu: Handle resource blob commands Alex Bennée
2024-11-01 15:35   ` Peter Maydell
2024-11-01 16:04     ` Dmitry Osipenko
2024-11-01 17:17       ` Alex Bennée
2024-11-01 17:16     ` Alex Bennée
2024-11-01 18:23       ` Dmitry Osipenko
2024-11-04 11:48         ` Alex Bennée
2024-11-04 13:50           ` BALATON Zoltan
2024-10-29 12:10 ` [PULL 12/13] virtio-gpu: Register capsets dynamically Alex Bennée
2024-10-29 12:10 ` [PULL 13/13] virtio-gpu: Support Venus context Alex Bennée
2024-10-31 11:29 ` [PULL 00/13] virtio-gpu vulkan support Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).