[PATCH v2 1/5] hw/display/virtio-gpu-virgl: virtio_gpu_gl -> virtio_gpu_virgl

[PATCH v2 1/5] hw/display/virtio-gpu-virgl: virtio_gpu_gl -> virtio_gpu_virgl

[Gurchetan Singh]

From: Gurchetan Singh <gurchetansingh@chromium.org>

The virtio-gpu GL device has a heavy dependence on virgl.
Acknowledge this by naming functions accurately.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1:
 - (Philippe) virtio_gpu_virglrenderer_reset --> virtio_gpu_virgl_reset_renderer
v2:
 - (Akihiko) Fix unnecessary line break

 hw/display/virtio-gpu-gl.c     | 26 +++++++++++++-------------
 hw/display/virtio-gpu-virgl.c  |  2 +-
 include/hw/virtio/virtio-gpu.h |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index e06be60dfb..8573043b85 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -25,9 +25,9 @@
 
 #include <virglrenderer.h>
 
-static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
-                                             struct virtio_gpu_scanout *s,
-                                             uint32_t resource_id)
+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
+                               struct virtio_gpu_scanout *s,
+                               uint32_t resource_id)
 {
     uint32_t width, height;
     uint32_t pixels, *data;
@@ -48,14 +48,14 @@ static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
     free(data);
 }
 
-static void virtio_gpu_gl_flushed(VirtIOGPUBase *b)
+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
 {
     VirtIOGPU *g = VIRTIO_GPU(b);
 
     virtio_gpu_process_cmdq(g);
 }
 
-static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOGPU *g = VIRTIO_GPU(vdev);
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
@@ -71,7 +71,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     }
     if (gl->renderer_reset) {
         gl->renderer_reset = false;
-        virtio_gpu_virgl_reset(g);
+        virtio_gpu_virgl_reset_renderer(g);
     }
 
     cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
@@ -87,7 +87,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     virtio_gpu_virgl_fence_poll(g);
 }
 
-static void virtio_gpu_gl_reset(VirtIODevice *vdev)
+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
 {
     VirtIOGPU *g = VIRTIO_GPU(vdev);
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
@@ -104,7 +104,7 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
     }
 }
 
-static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
+static void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
 {
     VirtIOGPU *g = VIRTIO_GPU(qdev);
 
@@ -143,13 +143,13 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
     VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
     VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
 
-    vbc->gl_flushed = virtio_gpu_gl_flushed;
-    vgc->handle_ctrl = virtio_gpu_gl_handle_ctrl;
+    vbc->gl_flushed = virtio_gpu_virgl_flushed;
+    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
     vgc->process_cmd = virtio_gpu_virgl_process_cmd;
-    vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
+    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
 
-    vdc->realize = virtio_gpu_gl_device_realize;
-    vdc->reset = virtio_gpu_gl_reset;
+    vdc->realize = virtio_gpu_virgl_device_realize;
+    vdc->reset = virtio_gpu_virgl_reset;
     device_class_set_props(dc, virtio_gpu_gl_properties);
 }
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 1c47603d40..ffe4ec7f3d 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -599,7 +599,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
     }
 }
 
-void virtio_gpu_virgl_reset(VirtIOGPU *g)
+void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g)
 {
     virgl_renderer_reset();
 }
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 2e28507efe..21b0f55bc8 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -281,7 +281,7 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                   struct virtio_gpu_ctrl_command *cmd);
 void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
 void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
-void virtio_gpu_virgl_reset(VirtIOGPU *g);
+void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g);
 int virtio_gpu_virgl_init(VirtIOGPU *g);
 int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
 
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 2/5] hw/display/virtio-gpu-virgl: make GL device more library agnostic

[Gurchetan Singh]

From: Gurchetan Singh <gurchetansingh@chromium.org>

Rather than create a virtio-gpu-gfxstream device and it's
associated variants (vga, pci), let's just extend the GL device.

We need to:
    - Move all virgl functions to their own file
    - Only all needed class callbacks in the generic GL device

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
v2:
    - (Akihiko) Fix unnecessary line break

 hw/display/virtio-gpu-gl.c     | 109 ------------------------------
 hw/display/virtio-gpu-virgl.c  | 118 +++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-gpu.h |  11 +--
 3 files changed, 119 insertions(+), 119 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 8573043b85..2d140e8792 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -15,121 +15,12 @@
 #include "qemu/iov.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
-#include "qapi/error.h"
-#include "sysemu/sysemu.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 "hw/qdev-properties.h"
 
-#include <virglrenderer.h>
-
-static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
-                               struct virtio_gpu_scanout *s,
-                               uint32_t resource_id)
-{
-    uint32_t width, height;
-    uint32_t pixels, *data;
-
-    data = virgl_renderer_get_cursor_data(resource_id, &width, &height);
-    if (!data) {
-        return;
-    }
-
-    if (width != s->current_cursor->width ||
-        height != s->current_cursor->height) {
-        free(data);
-        return;
-    }
-
-    pixels = s->current_cursor->width * s->current_cursor->height;
-    memcpy(s->current_cursor->data, data, pixels * sizeof(uint32_t));
-    free(data);
-}
-
-static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
-{
-    VirtIOGPU *g = VIRTIO_GPU(b);
-
-    virtio_gpu_process_cmdq(g);
-}
-
-static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
-{
-    VirtIOGPU *g = VIRTIO_GPU(vdev);
-    VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
-    struct virtio_gpu_ctrl_command *cmd;
-
-    if (!virtio_queue_ready(vq)) {
-        return;
-    }
-
-    if (!gl->renderer_inited) {
-        virtio_gpu_virgl_init(g);
-        gl->renderer_inited = true;
-    }
-    if (gl->renderer_reset) {
-        gl->renderer_reset = false;
-        virtio_gpu_virgl_reset_renderer(g);
-    }
-
-    cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
-    while (cmd) {
-        cmd->vq = vq;
-        cmd->error = 0;
-        cmd->finished = false;
-        QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
-        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
-    }
-
-    virtio_gpu_process_cmdq(g);
-    virtio_gpu_virgl_fence_poll(g);
-}
-
-static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
-{
-    VirtIOGPU *g = VIRTIO_GPU(vdev);
-    VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
-
-    virtio_gpu_reset(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) {
-        virtio_gpu_virgl_reset_scanout(g);
-        gl->renderer_reset = true;
-    }
-}
-
-static void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
-{
-    VirtIOGPU *g = VIRTIO_GPU(qdev);
-
-#if HOST_BIG_ENDIAN
-    error_setg(errp, "virgl is not supported on bigendian platforms");
-    return;
-#endif
-
-    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
-        error_setg(errp, "at most one %s device is permitted", TYPE_VIRTIO_GPU_GL);
-        return;
-    }
-
-    if (!display_opengl) {
-        error_setg(errp, "opengl is not available");
-        return;
-    }
-
-    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);
-
-    virtio_gpu_device_realize(qdev, errp);
-}
-
 static Property virtio_gpu_gl_properties[] = {
     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index ffe4ec7f3d..786351446c 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -14,6 +14,8 @@
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
 #include "qemu/iov.h"
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
 #include "trace.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
@@ -584,12 +586,12 @@ static void virtio_gpu_fence_poll(void *opaque)
     }
 }
 
-void virtio_gpu_virgl_fence_poll(VirtIOGPU *g)
+static void virtio_gpu_virgl_fence_poll(VirtIOGPU *g)
 {
     virtio_gpu_fence_poll(g);
 }
 
-void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
+static void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
 {
     int i;
 
@@ -599,12 +601,12 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
     }
 }
 
-void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g)
+static void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g)
 {
     virgl_renderer_reset();
 }
 
-int virtio_gpu_virgl_init(VirtIOGPU *g)
+static int virtio_gpu_virgl_init(VirtIOGPU *g)
 {
     int ret;
 
@@ -625,7 +627,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
     return 0;
 }
 
-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
+static int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
 {
     uint32_t capset2_max_ver, capset2_max_size;
     virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
@@ -634,3 +636,109 @@ int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
 
     return capset2_max_ver ? 2 : 1;
 }
+
+void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
+                               struct virtio_gpu_scanout *s,
+                               uint32_t resource_id)
+{
+    uint32_t width, height;
+    uint32_t pixels, *data;
+
+    data = virgl_renderer_get_cursor_data(resource_id, &width, &height);
+    if (!data) {
+        return;
+    }
+
+    if (width != s->current_cursor->width ||
+        height != s->current_cursor->height) {
+        free(data);
+        return;
+    }
+
+    pixels = s->current_cursor->width * s->current_cursor->height;
+    memcpy(s->current_cursor->data, data, pixels * sizeof(uint32_t));
+    free(data);
+}
+
+void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
+{
+    VirtIOGPU *g = VIRTIO_GPU(b);
+
+    virtio_gpu_process_cmdq(g);
+}
+
+void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOGPU *g = VIRTIO_GPU(vdev);
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
+    struct virtio_gpu_ctrl_command *cmd;
+
+    if (!virtio_queue_ready(vq)) {
+        return;
+    }
+
+    if (!gl->renderer_inited) {
+        virtio_gpu_virgl_init(g);
+        gl->renderer_inited = true;
+    }
+    if (gl->renderer_reset) {
+        gl->renderer_reset = false;
+        virtio_gpu_virgl_reset_renderer(g);
+    }
+
+    cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
+    while (cmd) {
+        cmd->vq = vq;
+        cmd->error = 0;
+        cmd->finished = false;
+        QTAILQ_INSERT_TAIL(&g->cmdq, cmd, next);
+        cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
+    }
+
+    virtio_gpu_process_cmdq(g);
+    virtio_gpu_virgl_fence_poll(g);
+}
+
+void virtio_gpu_virgl_reset(VirtIODevice *vdev)
+{
+    VirtIOGPU *g = VIRTIO_GPU(vdev);
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
+
+    virtio_gpu_reset(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) {
+        virtio_gpu_virgl_reset_scanout(g);
+        gl->renderer_reset = true;
+    }
+}
+
+void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
+{
+    VirtIOGPU *g = VIRTIO_GPU(qdev);
+
+#if HOST_BIG_ENDIAN
+    error_setg(errp, "virgl is not supported on bigendian platforms");
+    return;
+#endif
+
+    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
+        error_setg(errp, "at most one %s device is permitted",
+                   TYPE_VIRTIO_GPU_GL);
+        return;
+    }
+
+    if (!display_opengl) {
+        error_setg(errp, "opengl is not available");
+        return;
+    }
+
+    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);
+
+    virtio_gpu_device_realize(qdev, errp);
+}
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 21b0f55bc8..89ee133f07 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -279,10 +279,11 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
 /* virtio-gpu-3d.c */
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                   struct virtio_gpu_ctrl_command *cmd);
-void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
-void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
-void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g);
-int virtio_gpu_virgl_init(VirtIOGPU *g);
-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
+void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct virtio_gpu_scanout *s,
+                                    uint32_t resource_id);
+void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
+void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);
+void virtio_gpu_virgl_reset(VirtIODevice *vdev);
+void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);
 
 #endif
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Gurchetan Singh]

From: Gurchetan Singh <gurchetansingh@chromium.org>

This reduces the amount of renderer backend specific needed to
be exposed to the GL device.  We only need one realize function
per renderer backend.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1: - Remove NULL inits (Philippe)
    - Use VIRTIO_GPU_BASE where possible (Philippe)
v2: - Fix unnecessary line break (Akihiko)

 hw/display/virtio-gpu-gl.c     | 15 ++++++---------
 hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
 include/hw/virtio/virtio-gpu.h |  7 -------
 3 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 2d140e8792..cdc9483e4d 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -21,6 +21,11 @@
 #include "hw/virtio/virtio-gpu-pixman.h"
 #include "hw/qdev-properties.h"
 
+static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
+{
+    virtio_gpu_virgl_device_realize(qdev, errp);
+}
+
 static Property virtio_gpu_gl_properties[] = {
     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
@@ -31,16 +36,8 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
-    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
-    VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
-
-    vbc->gl_flushed = virtio_gpu_virgl_flushed;
-    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
-    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
-    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
 
-    vdc->realize = virtio_gpu_virgl_device_realize;
-    vdc->reset = virtio_gpu_virgl_reset;
+    vdc->realize = virtio_gpu_gl_device_realize;
     device_class_set_props(dc, virtio_gpu_gl_properties);
 }
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 786351446c..d7e01f1c77 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -401,8 +401,9 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
     g_free(resp);
 }
 
-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
-                                      struct virtio_gpu_ctrl_command *cmd)
+static void
+virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
+                             struct virtio_gpu_ctrl_command *cmd)
 {
     VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
 
@@ -637,7 +638,7 @@ static int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
     return capset2_max_ver ? 2 : 1;
 }
 
-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
                                struct virtio_gpu_scanout *s,
                                uint32_t resource_id)
 {
@@ -660,14 +661,14 @@ void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
     free(data);
 }
 
-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
 {
     VirtIOGPU *g = VIRTIO_GPU(b);
 
     virtio_gpu_process_cmdq(g);
 }
 
-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOGPU *g = VIRTIO_GPU(vdev);
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
@@ -699,7 +700,7 @@ void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     virtio_gpu_virgl_fence_poll(g);
 }
 
-void virtio_gpu_virgl_reset(VirtIODevice *vdev)
+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
 {
     VirtIOGPU *g = VIRTIO_GPU(vdev);
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
@@ -718,7 +719,21 @@ void virtio_gpu_virgl_reset(VirtIODevice *vdev)
 
 void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
 {
-    VirtIOGPU *g = VIRTIO_GPU(qdev);
+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
+    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
+
+    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
+    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
+
+    vbc->gl_flushed = virtio_gpu_virgl_flushed;
+    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
+    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
+    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
+
+    vdc->reset = virtio_gpu_virgl_reset;
 
 #if HOST_BIG_ENDIAN
     error_setg(errp, "virgl is not supported on bigendian platforms");
@@ -736,9 +751,9 @@ void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
         return;
     }
 
-    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);
+    VIRTIO_GPU_BASE(gpudev)->conf.flags |= (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);
+    VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets =
+        virtio_gpu_virgl_get_num_capsets(gpudev);
 
     virtio_gpu_device_realize(qdev, errp);
 }
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 89ee133f07..d5808f2ab6 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -277,13 +277,6 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
                              struct virtio_gpu_rect *r);
 
 /* virtio-gpu-3d.c */
-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
-                                  struct virtio_gpu_ctrl_command *cmd);
-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct virtio_gpu_scanout *s,
-                                    uint32_t resource_id);
-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);
-void virtio_gpu_virgl_reset(VirtIODevice *vdev);
 void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);
 
 #endif
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 4/5] virtio: Add shared memory capability

[Gurchetan Singh]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Define a new capability type 'VIRTIO_PCI_CAP_SHARED_MEMORY_CFG' to allow
defining shared memory regions with sizes and offsets of 2^32 and more.
Multiple instances of the capability are allowed and distinguished
by a device-specific 'id'.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 hw/virtio/virtio-pci.c         | 18 ++++++++++++++++++
 include/hw/virtio/virtio-pci.h |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 02fb84a8fa..40a798d794 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1399,6 +1399,24 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
     return offset;
 }
 
+int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy,
+                           uint8_t bar, uint64_t offset, uint64_t length,
+                           uint8_t id)
+{
+    struct virtio_pci_cap64 cap = {
+        .cap.cap_len = sizeof cap,
+        .cap.cfg_type = VIRTIO_PCI_CAP_SHARED_MEMORY_CFG,
+    };
+
+    cap.cap.bar = bar;
+    cap.cap.length = cpu_to_le32(length);
+    cap.length_hi = cpu_to_le32(length >> 32);
+    cap.cap.offset = cpu_to_le32(offset);
+    cap.offset_hi = cpu_to_le32(offset >> 32);
+    cap.cap.id = id;
+    return virtio_pci_add_mem_cap(proxy, &cap.cap);
+}
+
 static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
                                        unsigned size)
 {
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index ab2051b64b..5a3f182f99 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -264,4 +264,8 @@ unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues);
 void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev, VirtQueue *vq,
                                               int n, bool assign,
                                               bool with_irqfd);
+
+int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, uint8_t bar, uint64_t offset,
+                           uint64_t length, uint8_t id);
+
 #endif
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 5/5] virtio-gpu: CONTEXT_INIT feature

[Gurchetan Singh]

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

The feature can be enabled when a backend wants it.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 hw/display/virtio-gpu-base.c   | 3 +++
 include/hw/virtio/virtio-gpu.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index a29f191aa8..6c5f1f327f 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -215,6 +215,9 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
     if (virtio_gpu_blob_enabled(g->conf)) {
         features |= (1 << VIRTIO_GPU_F_RESOURCE_BLOB);
     }
+    if (virtio_gpu_context_init_enabled(g->conf)) {
+        features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
+    }
 
     return features;
 }
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index d5808f2ab6..cf24d2e21b 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -90,6 +90,7 @@ enum virtio_gpu_base_conf_flags {
     VIRTIO_GPU_FLAG_EDID_ENABLED,
     VIRTIO_GPU_FLAG_DMABUF_ENABLED,
     VIRTIO_GPU_FLAG_BLOB_ENABLED,
+    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
 };
 
 #define virtio_gpu_virgl_enabled(_cfg) \
@@ -102,6 +103,8 @@ enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_DMABUF_ENABLED))
 #define virtio_gpu_blob_enabled(_cfg) \
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
+#define virtio_gpu_context_init_enabled(_cfg) \
+    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
 
 struct virtio_gpu_base_conf {
     uint32_t max_outputs;
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH v2 2/5] hw/display/virtio-gpu-virgl: make GL device more library agnostic

[Philippe Mathieu-Daudé]

On 28/4/23 18:48, Gurchetan Singh wrote:
> From: Gurchetan Singh <gurchetansingh@chromium.org>
> 
> Rather than create a virtio-gpu-gfxstream device and it's
> associated variants (vga, pci), let's just extend the GL device.
> 
> We need to:
>      - Move all virgl functions to their own file
>      - Only all needed class callbacks in the generic GL device
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
> v2:
>      - (Akihiko) Fix unnecessary line break
> 
>   hw/display/virtio-gpu-gl.c     | 109 ------------------------------
>   hw/display/virtio-gpu-virgl.c  | 118 +++++++++++++++++++++++++++++++--
>   include/hw/virtio/virtio-gpu.h |  11 +--
>   3 files changed, 119 insertions(+), 119 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 5/5] virtio-gpu: CONTEXT_INIT feature

[Philippe Mathieu-Daudé]

On 28/4/23 18:48, Gurchetan Singh wrote:
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> The feature can be enabled when a backend wants it.
> 
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>   hw/display/virtio-gpu-base.c   | 3 +++
>   include/hw/virtio/virtio-gpu.h | 3 +++
>   2 files changed, 6 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/5] hw/display/virtio-gpu-virgl: virtio_gpu_gl -> virtio_gpu_virgl

[Akihiko Odaki]

On 2023/04/29 1:48, Gurchetan Singh wrote:
> From: Gurchetan Singh <gurchetansingh@chromium.org>
> 
> The virtio-gpu GL device has a heavy dependence on virgl.
> Acknowledge this by naming functions accurately.
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v1:
>   - (Philippe) virtio_gpu_virglrenderer_reset --> virtio_gpu_virgl_reset_renderer
> v2:
>   - (Akihiko) Fix unnecessary line break
> 
>   hw/display/virtio-gpu-gl.c     | 26 +++++++++++++-------------
>   hw/display/virtio-gpu-virgl.c  |  2 +-
>   include/hw/virtio/virtio-gpu.h |  2 +-
>   3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index e06be60dfb..8573043b85 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -25,9 +25,9 @@
>   
>   #include <virglrenderer.h>
>   
> -static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
> -                                             struct virtio_gpu_scanout *s,
> -                                             uint32_t resource_id)
> +static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> +                               struct virtio_gpu_scanout *s,
> +                               uint32_t resource_id)

Now the last two parameters are misaligned.

>   {
>       uint32_t width, height;
>       uint32_t pixels, *data;
> @@ -48,14 +48,14 @@ static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
>       free(data);
>   }
>   
> -static void virtio_gpu_gl_flushed(VirtIOGPUBase *b)
> +static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(b);
>   
>       virtio_gpu_process_cmdq(g);
>   }
>   
> -static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> +static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(vdev);
>       VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
> @@ -71,7 +71,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>       }
>       if (gl->renderer_reset) {
>           gl->renderer_reset = false;
> -        virtio_gpu_virgl_reset(g);
> +        virtio_gpu_virgl_reset_renderer(g);
>       }
>   
>       cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
> @@ -87,7 +87,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>       virtio_gpu_virgl_fence_poll(g);
>   }
>   
> -static void virtio_gpu_gl_reset(VirtIODevice *vdev)
> +static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(vdev);
>       VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
> @@ -104,7 +104,7 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>       }
>   }
>   
> -static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
> +static void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
>   {
>       VirtIOGPU *g = VIRTIO_GPU(qdev);
>   
> @@ -143,13 +143,13 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
>       VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
>       VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
>   
> -    vbc->gl_flushed = virtio_gpu_gl_flushed;
> -    vgc->handle_ctrl = virtio_gpu_gl_handle_ctrl;
> +    vbc->gl_flushed = virtio_gpu_virgl_flushed;
> +    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>       vgc->process_cmd = virtio_gpu_virgl_process_cmd;
> -    vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
> +    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
>   
> -    vdc->realize = virtio_gpu_gl_device_realize;
> -    vdc->reset = virtio_gpu_gl_reset;
> +    vdc->realize = virtio_gpu_virgl_device_realize;
> +    vdc->reset = virtio_gpu_virgl_reset;
>       device_class_set_props(dc, virtio_gpu_gl_properties);
>   }
>   
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 1c47603d40..ffe4ec7f3d 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -599,7 +599,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>       }
>   }
>   
> -void virtio_gpu_virgl_reset(VirtIOGPU *g)
> +void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g)
>   {
>       virgl_renderer_reset();
>   }
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 2e28507efe..21b0f55bc8 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -281,7 +281,7 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>                                     struct virtio_gpu_ctrl_command *cmd);
>   void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
>   void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
> -void virtio_gpu_virgl_reset(VirtIOGPU *g);
> +void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g);
>   int virtio_gpu_virgl_init(VirtIOGPU *g);
>   int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
>   

Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Bernhard Beschow]

Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <gurchetansingh@chromium.org>:
>From: Gurchetan Singh <gurchetansingh@chromium.org>
>
>This reduces the amount of renderer backend specific needed to
>be exposed to the GL device.  We only need one realize function
>per renderer backend.
>
>Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
>v1: - Remove NULL inits (Philippe)
>    - Use VIRTIO_GPU_BASE where possible (Philippe)
>v2: - Fix unnecessary line break (Akihiko)
>
> hw/display/virtio-gpu-gl.c     | 15 ++++++---------
> hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
> include/hw/virtio/virtio-gpu.h |  7 -------
> 3 files changed, 31 insertions(+), 26 deletions(-)
>
>diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>index 2d140e8792..cdc9483e4d 100644
>--- a/hw/display/virtio-gpu-gl.c
>+++ b/hw/display/virtio-gpu-gl.c
>@@ -21,6 +21,11 @@
> #include "hw/virtio/virtio-gpu-pixman.h"
> #include "hw/qdev-properties.h"
> 
>+static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>+{
>+    virtio_gpu_virgl_device_realize(qdev, errp);
>+}
>+
> static Property virtio_gpu_gl_properties[] = {
>     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
>                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
>@@ -31,16 +36,8 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
>-    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
>-    VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
>-
>-    vbc->gl_flushed = virtio_gpu_virgl_flushed;
>-    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>-    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>-    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
> 
>-    vdc->realize = virtio_gpu_virgl_device_realize;
>-    vdc->reset = virtio_gpu_virgl_reset;
>+    vdc->realize = virtio_gpu_gl_device_realize;
>     device_class_set_props(dc, virtio_gpu_gl_properties);
> }
> 
>diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>index 786351446c..d7e01f1c77 100644
>--- a/hw/display/virtio-gpu-virgl.c
>+++ b/hw/display/virtio-gpu-virgl.c
>@@ -401,8 +401,9 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>     g_free(resp);
> }
> 
>-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>-                                      struct virtio_gpu_ctrl_command *cmd)
>+static void
>+virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>+                             struct virtio_gpu_ctrl_command *cmd)
> {
>     VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
> 
>@@ -637,7 +638,7 @@ static int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>     return capset2_max_ver ? 2 : 1;
> }
> 
>-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>                                struct virtio_gpu_scanout *s,
>                                uint32_t resource_id)
> {
>@@ -660,14 +661,14 @@ void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>     free(data);
> }
> 
>-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
>+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
> {
>     VirtIOGPU *g = VIRTIO_GPU(b);
> 
>     virtio_gpu_process_cmdq(g);
> }
> 
>-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> {
>     VirtIOGPU *g = VIRTIO_GPU(vdev);
>     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
>@@ -699,7 +700,7 @@ void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>     virtio_gpu_virgl_fence_poll(g);
> }
> 
>-void virtio_gpu_virgl_reset(VirtIODevice *vdev)
>+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> {
>     VirtIOGPU *g = VIRTIO_GPU(vdev);
>     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
>@@ -718,7 +719,21 @@ void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> 
> void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
> {
>-    VirtIOGPU *g = VIRTIO_GPU(qdev);
>+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>+
>+    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
>+    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
>+
>+    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
>+    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
>+
>+    vbc->gl_flushed = virtio_gpu_virgl_flushed;
>+    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>+    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>+    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
>+
>+    vdc->reset = virtio_gpu_virgl_reset;

A realize method is supposed to modify a single instance only while we're modifying the behavior of whole classes here, i.e. will affect every instance of these classes. This goes against QOM design principles and will therefore be confusing for people who are familiar with QOM in particular and OOP in general. I think the code should be cleaned up in a different way if really needed.

Best regards,
Bernhard

> 
> #if HOST_BIG_ENDIAN
>     error_setg(errp, "virgl is not supported on bigendian platforms");
>@@ -736,9 +751,9 @@ void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
>         return;
>     }
> 
>-    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);
>+    VIRTIO_GPU_BASE(gpudev)->conf.flags |= (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);
>+    VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets =
>+        virtio_gpu_virgl_get_num_capsets(gpudev);
> 
>     virtio_gpu_device_realize(qdev, errp);
> }
>diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
>index 89ee133f07..d5808f2ab6 100644
>--- a/include/hw/virtio/virtio-gpu.h
>+++ b/include/hw/virtio/virtio-gpu.h
>@@ -277,13 +277,6 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
>                              struct virtio_gpu_rect *r);
> 
> /* virtio-gpu-3d.c */
>-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>-                                  struct virtio_gpu_ctrl_command *cmd);
>-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct virtio_gpu_scanout *s,
>-                                    uint32_t resource_id);
>-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
>-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);
>-void virtio_gpu_virgl_reset(VirtIODevice *vdev);
> void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);
> 
> #endif

Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Gurchetan Singh]

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

On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <shentey@gmail.com> wrote:

>
>
> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
> gurchetansingh@chromium.org>:
> >From: Gurchetan Singh <gurchetansingh@chromium.org>
> >
> >This reduces the amount of renderer backend specific needed to
> >be exposed to the GL device.  We only need one realize function
> >per renderer backend.
> >
> >Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >---
> >v1: - Remove NULL inits (Philippe)
> >    - Use VIRTIO_GPU_BASE where possible (Philippe)
> >v2: - Fix unnecessary line break (Akihiko)
> >
> > hw/display/virtio-gpu-gl.c     | 15 ++++++---------
> > hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
> > include/hw/virtio/virtio-gpu.h |  7 -------
> > 3 files changed, 31 insertions(+), 26 deletions(-)
> >
> >diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> >index 2d140e8792..cdc9483e4d 100644
> >--- a/hw/display/virtio-gpu-gl.c
> >+++ b/hw/display/virtio-gpu-gl.c
> >@@ -21,6 +21,11 @@
> > #include "hw/virtio/virtio-gpu-pixman.h"
> > #include "hw/qdev-properties.h"
> >
> >+static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
> >+{
> >+    virtio_gpu_virgl_device_realize(qdev, errp);
> >+}
> >+
> > static Property virtio_gpu_gl_properties[] = {
> >     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
> >                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
> >@@ -31,16 +36,8 @@ static void virtio_gpu_gl_class_init(ObjectClass
> *klass, void *data)
> > {
> >     DeviceClass *dc = DEVICE_CLASS(klass);
> >     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> >-    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
> >-    VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
> >-
> >-    vbc->gl_flushed = virtio_gpu_virgl_flushed;
> >-    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
> >-    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
> >-    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
> >
> >-    vdc->realize = virtio_gpu_virgl_device_realize;
> >-    vdc->reset = virtio_gpu_virgl_reset;
> >+    vdc->realize = virtio_gpu_gl_device_realize;
> >     device_class_set_props(dc, virtio_gpu_gl_properties);
> > }
> >
> >diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >index 786351446c..d7e01f1c77 100644
> >--- a/hw/display/virtio-gpu-virgl.c
> >+++ b/hw/display/virtio-gpu-virgl.c
> >@@ -401,8 +401,9 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >     g_free(resp);
> > }
> >
> >-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >-                                      struct virtio_gpu_ctrl_command
> *cmd)
> >+static void
> >+virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >+                             struct virtio_gpu_ctrl_command *cmd)
> > {
> >     VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
> >
> >@@ -637,7 +638,7 @@ static int virtio_gpu_virgl_get_num_capsets(VirtIOGPU
> *g)
> >     return capset2_max_ver ? 2 : 1;
> > }
> >
> >-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> >+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> >                                struct virtio_gpu_scanout *s,
> >                                uint32_t resource_id)
> > {
> >@@ -660,14 +661,14 @@ void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> >     free(data);
> > }
> >
> >-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
> >+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
> > {
> >     VirtIOGPU *g = VIRTIO_GPU(b);
> >
> >     virtio_gpu_process_cmdq(g);
> > }
> >
> >-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> >+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue
> *vq)
> > {
> >     VirtIOGPU *g = VIRTIO_GPU(vdev);
> >     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
> >@@ -699,7 +700,7 @@ void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev,
> VirtQueue *vq)
> >     virtio_gpu_virgl_fence_poll(g);
> > }
> >
> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> >+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> > {
> >     VirtIOGPU *g = VIRTIO_GPU(vdev);
> >     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
> >@@ -718,7 +719,21 @@ void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> >
> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
> > {
> >-    VirtIOGPU *g = VIRTIO_GPU(qdev);
> >+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> >+
> >+    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
> >+    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
> >+
> >+    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
> >+    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
> >+
> >+    vbc->gl_flushed = virtio_gpu_virgl_flushed;
> >+    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
> >+    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
> >+    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
> >+
> >+    vdc->reset = virtio_gpu_virgl_reset;
>
> A realize method is supposed to modify a single instance only while we're
> modifying the behavior of whole classes here, i.e. will affect every
> instance of these classes.

This goes against QOM design principles and will therefore be confusing for
> people who are familiar with QOM in particular and OOP in general.


Context: this is a cleanup in preparation for the gfxstream/rutabaga
support:

 https://patchew.org/QEMU/20230421011223.718-1-gurchetansingh@chromium.org/

I explored creating a separate "virtio-gpu-rutabaga" device, but felt it
added too much duplicate code (such as virtio-gpu-pci-rutabaga.c and
virtio-vga-rutabaga.c).  Please see here:

https://gitlab.freedesktop.org/gurchetansingh/qemu-gfxstream/-/commits/master

for that approach (current approach is in "qemu-gfxstream2" branch).

In the current approach, function pointers are modified in realize(..)
instead of class_init(..) since "capset_names" can choose the appropriate
backend, but that variable is only accessible after class_init(..).

The difference between instance_init() and the realize() has also come up
before here:

https://lore.kernel.org/all/268082DD-5FBB-41CC-8718-7D6BAA0D323A@livius.net/T/#m52be60860e2bf598816ed162f7b6dd070b52cd1d


> I think the code should be cleaned up in a different way if really needed.
>

Sure, if there's a cleaner way, we should definitely explore it.  Given the
goal of adding another backend for virtio-gpu, how do you suggest
refactoring the code?


>
> Best regards,
> Bernhard
>
> >
> > #if HOST_BIG_ENDIAN
> >     error_setg(errp, "virgl is not supported on bigendian platforms");
> >@@ -736,9 +751,9 @@ void virtio_gpu_virgl_device_realize(DeviceState
> *qdev, Error **errp)
> >         return;
> >     }
> >
> >-    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);
> >+    VIRTIO_GPU_BASE(gpudev)->conf.flags |= (1 <<
> VIRTIO_GPU_FLAG_VIRGL_ENABLED);
> >+    VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets =
> >+        virtio_gpu_virgl_get_num_capsets(gpudev);
> >
> >     virtio_gpu_device_realize(qdev, errp);
> > }
> >diff --git a/include/hw/virtio/virtio-gpu.h
> b/include/hw/virtio/virtio-gpu.h
> >index 89ee133f07..d5808f2ab6 100644
> >--- a/include/hw/virtio/virtio-gpu.h
> >+++ b/include/hw/virtio/virtio-gpu.h
> >@@ -277,13 +277,6 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
> >                              struct virtio_gpu_rect *r);
> >
> > /* virtio-gpu-3d.c */
> >-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >-                                  struct virtio_gpu_ctrl_command *cmd);
> >-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct
> virtio_gpu_scanout *s,
> >-                                    uint32_t resource_id);
> >-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
> >-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);
> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev);
> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);
> >
> > #endif
>

[-- Attachment #2: Type: text/html, Size: 11120 bytes --]

Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Bernhard Beschow]

Am 1. Mai 2023 16:53:03 UTC schrieb Gurchetan Singh <gurchetansingh@chromium.org>:
>On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <shentey@gmail.com> wrote:
>
>>
>>
>> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
>> gurchetansingh@chromium.org>:
>> >From: Gurchetan Singh <gurchetansingh@chromium.org>
>> >
>> >This reduces the amount of renderer backend specific needed to
>> >be exposed to the GL device.  We only need one realize function
>> >per renderer backend.
>> >
>> >Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>> >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> >---
>> >v1: - Remove NULL inits (Philippe)
>> >    - Use VIRTIO_GPU_BASE where possible (Philippe)
>> >v2: - Fix unnecessary line break (Akihiko)
>> >
>> > hw/display/virtio-gpu-gl.c     | 15 ++++++---------
>> > hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
>> > include/hw/virtio/virtio-gpu.h |  7 -------
>> > 3 files changed, 31 insertions(+), 26 deletions(-)
>> >
>> >diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> >index 2d140e8792..cdc9483e4d 100644
>> >--- a/hw/display/virtio-gpu-gl.c
>> >+++ b/hw/display/virtio-gpu-gl.c
>> >@@ -21,6 +21,11 @@
>> > #include "hw/virtio/virtio-gpu-pixman.h"
>> > #include "hw/qdev-properties.h"
>> >
>> >+static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>> >+{
>> >+    virtio_gpu_virgl_device_realize(qdev, errp);
>> >+}
>> >+
>> > static Property virtio_gpu_gl_properties[] = {
>> >     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
>> >                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
>> >@@ -31,16 +36,8 @@ static void virtio_gpu_gl_class_init(ObjectClass
>> *klass, void *data)
>> > {
>> >     DeviceClass *dc = DEVICE_CLASS(klass);
>> >     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
>> >-    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
>> >-    VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
>> >-
>> >-    vbc->gl_flushed = virtio_gpu_virgl_flushed;
>> >-    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>> >-    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>> >-    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
>> >
>> >-    vdc->realize = virtio_gpu_virgl_device_realize;
>> >-    vdc->reset = virtio_gpu_virgl_reset;
>> >+    vdc->realize = virtio_gpu_gl_device_realize;
>> >     device_class_set_props(dc, virtio_gpu_gl_properties);
>> > }
>> >
>> >diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> >index 786351446c..d7e01f1c77 100644
>> >--- a/hw/display/virtio-gpu-virgl.c
>> >+++ b/hw/display/virtio-gpu-virgl.c
>> >@@ -401,8 +401,9 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>> >     g_free(resp);
>> > }
>> >
>> >-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>> >-                                      struct virtio_gpu_ctrl_command
>> *cmd)
>> >+static void
>> >+virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>> >+                             struct virtio_gpu_ctrl_command *cmd)
>> > {
>> >     VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
>> >
>> >@@ -637,7 +638,7 @@ static int virtio_gpu_virgl_get_num_capsets(VirtIOGPU
>> *g)
>> >     return capset2_max_ver ? 2 : 1;
>> > }
>> >
>> >-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>> >+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>> >                                struct virtio_gpu_scanout *s,
>> >                                uint32_t resource_id)
>> > {
>> >@@ -660,14 +661,14 @@ void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>> >     free(data);
>> > }
>> >
>> >-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
>> >+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
>> > {
>> >     VirtIOGPU *g = VIRTIO_GPU(b);
>> >
>> >     virtio_gpu_process_cmdq(g);
>> > }
>> >
>> >-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>> >+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue
>> *vq)
>> > {
>> >     VirtIOGPU *g = VIRTIO_GPU(vdev);
>> >     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
>> >@@ -699,7 +700,7 @@ void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev,
>> VirtQueue *vq)
>> >     virtio_gpu_virgl_fence_poll(g);
>> > }
>> >
>> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev)
>> >+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
>> > {
>> >     VirtIOGPU *g = VIRTIO_GPU(vdev);
>> >     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
>> >@@ -718,7 +719,21 @@ void virtio_gpu_virgl_reset(VirtIODevice *vdev)
>> >
>> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
>> > {
>> >-    VirtIOGPU *g = VIRTIO_GPU(qdev);
>> >+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>> >+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>> >+
>> >+    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
>> >+    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
>> >+
>> >+    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
>> >+    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
>> >+
>> >+    vbc->gl_flushed = virtio_gpu_virgl_flushed;
>> >+    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>> >+    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>> >+    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
>> >+
>> >+    vdc->reset = virtio_gpu_virgl_reset;
>>
>> A realize method is supposed to modify a single instance only while we're
>> modifying the behavior of whole classes here, i.e. will affect every
>> instance of these classes.
>
>This goes against QOM design principles and will therefore be confusing for
>> people who are familiar with QOM in particular and OOP in general.
>
>
>Context: this is a cleanup in preparation for the gfxstream/rutabaga
>support:
>
> https://patchew.org/QEMU/20230421011223.718-1-gurchetansingh@chromium.org/

Judging from this series the benefit of having a common -gl class isn't that big: AFAICS the only synergy effect is sharing a few properties which IMO don't warrant sharing. IOW the almost non-existant benefit rather confirms the current design. The last word needs to be spoken by the maintainers though.

>
>I explored creating a separate "virtio-gpu-rutabaga" device,

Although this approach requires another -pci class it seems cleaner to me due to how the QEMU object model works. See below.

> but felt it
>added too much duplicate code (such as virtio-gpu-pci-rutabaga.c and
>virtio-vga-rutabaga.c).  Please see here:
>
>https://gitlab.freedesktop.org/gurchetansingh/qemu-gfxstream/-/commits/master
>
>for that approach (current approach is in "qemu-gfxstream2" branch).
>
>In the current approach, function pointers are modified in realize(..)
>instead of class_init(..) since "capset_names" can choose the appropriate
>backend, but that variable is only accessible after class_init(..).

Yeah, your're selecting a backend at runtime by changing a whole class' behavior inside an *instance* of that class. This is not how the QEMU object model is supposed to work...

>
>The difference between instance_init() and the realize() has also come up
>before here:
>
>https://lore.kernel.org/all/268082DD-5FBB-41CC-8718-7D6BAA0D323A@livius.net/T/#m52be60860e2bf598816ed162f7b6dd070b52cd1d

The link is about a related but different topic. It discusses .instance_init vs. .realize. This patch is essentially confusing .instance_init/.realize and .class_init. You can read more about the QEMU object model in general and class initialization in particular here: https://www.qemu.org/docs/master/devel/qom.html#class-initialization

>
>
>> I think the code should be cleaned up in a different way if really needed.
>>
>
>Sure, if there's a cleaner way, we should definitely explore it.  Given the
>goal of adding another backend for virtio-gpu, how do you suggest
>refactoring the code?

I'm no maintainer but my suggestion would be this: Use your first approach with dedicated classes. This would also allow to force the new backend via the command line. If you really need detection at runtime you could either delegate this to Android Studio (by having it select the best (tm) backend via command line) or you might be able to add a so called "sugar property" and have QEMU make the choice (sugar properties exceed my knowledge though).

Regarding rutabaga I have the following comments:

Given that rutabaga seems to be an abstraction layer over virgl and gfxstream it seems redundant to me. QEMU already has an abstraction layer for various graphics backends so IMO using another just introduces a maintenance burden. Therefore I suggest introducing a dedicated class wich uses gfxstream directly. The class name could end with -gfxstream to match the technology.

Furthermore, rutabaga abstracts two C APIs and is used as C API. So the benefit of using Rust seems to be low -- not even mentioning the packaging issues this causes for Linux distributions.

Last but not least rutabaga seems to be a personal pet project rather than something official. I guess that QEMU would't want to rely on a personal pet project.

In any case I'd leave the last word to the maintainers.

Best regards,
Bernhard
>
>
>>
>> Best regards,
>> Bernhard
>>
>> >
>> > #if HOST_BIG_ENDIAN
>> >     error_setg(errp, "virgl is not supported on bigendian platforms");
>> >@@ -736,9 +751,9 @@ void virtio_gpu_virgl_device_realize(DeviceState
>> *qdev, Error **errp)
>> >         return;
>> >     }
>> >
>> >-    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);
>> >+    VIRTIO_GPU_BASE(gpudev)->conf.flags |= (1 <<
>> VIRTIO_GPU_FLAG_VIRGL_ENABLED);
>> >+    VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets =
>> >+        virtio_gpu_virgl_get_num_capsets(gpudev);
>> >
>> >     virtio_gpu_device_realize(qdev, errp);
>> > }
>> >diff --git a/include/hw/virtio/virtio-gpu.h
>> b/include/hw/virtio/virtio-gpu.h
>> >index 89ee133f07..d5808f2ab6 100644
>> >--- a/include/hw/virtio/virtio-gpu.h
>> >+++ b/include/hw/virtio/virtio-gpu.h
>> >@@ -277,13 +277,6 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
>> >                              struct virtio_gpu_rect *r);
>> >
>> > /* virtio-gpu-3d.c */
>> >-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>> >-                                  struct virtio_gpu_ctrl_command *cmd);
>> >-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct
>> virtio_gpu_scanout *s,
>> >-                                    uint32_t resource_id);
>> >-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
>> >-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);
>> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev);
>> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);
>> >
>> > #endif
>>

Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Gurchetan Singh]

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

On Fri, May 5, 2023 at 10:48 AM Bernhard Beschow <shentey@gmail.com> wrote:

> Am 1. Mai 2023 16:53:03 UTC schrieb Gurchetan Singh <
> gurchetansingh@chromium.org>:
> >On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <shentey@gmail.com>
> wrote:
> >
> >>
> >>
> >> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
> >> gurchetansingh@chromium.org>:
> >> >From: Gurchetan Singh <gurchetansingh@chromium.org>
> >> >
> >> >This reduces the amount of renderer backend specific needed to
> >> >be exposed to the GL device.  We only need one realize function
> >> >per renderer backend.
> >> >
> >> >Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> >> >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> >---
> >> >v1: - Remove NULL inits (Philippe)
> >> >    - Use VIRTIO_GPU_BASE where possible (Philippe)
> >> >v2: - Fix unnecessary line break (Akihiko)
> >> >
> >> > hw/display/virtio-gpu-gl.c     | 15 ++++++---------
> >> > hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
> >> > include/hw/virtio/virtio-gpu.h |  7 -------
> >> > 3 files changed, 31 insertions(+), 26 deletions(-)
> >> >
> >> >diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> >> >index 2d140e8792..cdc9483e4d 100644
> >> >--- a/hw/display/virtio-gpu-gl.c
> >> >+++ b/hw/display/virtio-gpu-gl.c
> >> >@@ -21,6 +21,11 @@
> >> > #include "hw/virtio/virtio-gpu-pixman.h"
> >> > #include "hw/qdev-properties.h"
> >> >
> >> >+static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error
> **errp)
> >> >+{
> >> >+    virtio_gpu_virgl_device_realize(qdev, errp);
> >> >+}
> >> >+
> >> > static Property virtio_gpu_gl_properties[] = {
> >> >     DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
> >> >                     VIRTIO_GPU_FLAG_STATS_ENABLED, false),
> >> >@@ -31,16 +36,8 @@ static void virtio_gpu_gl_class_init(ObjectClass
> >> *klass, void *data)
> >> > {
> >> >     DeviceClass *dc = DEVICE_CLASS(klass);
> >> >     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> >> >-    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
> >> >-    VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
> >> >-
> >> >-    vbc->gl_flushed = virtio_gpu_virgl_flushed;
> >> >-    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
> >> >-    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
> >> >-    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
> >> >
> >> >-    vdc->realize = virtio_gpu_virgl_device_realize;
> >> >-    vdc->reset = virtio_gpu_virgl_reset;
> >> >+    vdc->realize = virtio_gpu_gl_device_realize;
> >> >     device_class_set_props(dc, virtio_gpu_gl_properties);
> >> > }
> >> >
> >> >diff --git a/hw/display/virtio-gpu-virgl.c
> b/hw/display/virtio-gpu-virgl.c
> >> >index 786351446c..d7e01f1c77 100644
> >> >--- a/hw/display/virtio-gpu-virgl.c
> >> >+++ b/hw/display/virtio-gpu-virgl.c
> >> >@@ -401,8 +401,9 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >> >     g_free(resp);
> >> > }
> >> >
> >> >-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >> >-                                      struct virtio_gpu_ctrl_command
> >> *cmd)
> >> >+static void
> >> >+virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >> >+                             struct virtio_gpu_ctrl_command *cmd)
> >> > {
> >> >     VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
> >> >
> >> >@@ -637,7 +638,7 @@ static int
> virtio_gpu_virgl_get_num_capsets(VirtIOGPU
> >> *g)
> >> >     return capset2_max_ver ? 2 : 1;
> >> > }
> >> >
> >> >-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> >> >+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> >> >                                struct virtio_gpu_scanout *s,
> >> >                                uint32_t resource_id)
> >> > {
> >> >@@ -660,14 +661,14 @@ void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
> >> >     free(data);
> >> > }
> >> >
> >> >-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
> >> >+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
> >> > {
> >> >     VirtIOGPU *g = VIRTIO_GPU(b);
> >> >
> >> >     virtio_gpu_process_cmdq(g);
> >> > }
> >> >
> >> >-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> >> >+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue
> >> *vq)
> >> > {
> >> >     VirtIOGPU *g = VIRTIO_GPU(vdev);
> >> >     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
> >> >@@ -699,7 +700,7 @@ void virtio_gpu_virgl_handle_ctrl(VirtIODevice
> *vdev,
> >> VirtQueue *vq)
> >> >     virtio_gpu_virgl_fence_poll(g);
> >> > }
> >> >
> >> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> >> >+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> >> > {
> >> >     VirtIOGPU *g = VIRTIO_GPU(vdev);
> >> >     VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
> >> >@@ -718,7 +719,21 @@ void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> >> >
> >> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
> >> > {
> >> >-    VirtIOGPU *g = VIRTIO_GPU(qdev);
> >> >+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >> >+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> >> >+
> >> >+    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
> >> >+    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
> >> >+
> >> >+    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
> >> >+    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
> >> >+
> >> >+    vbc->gl_flushed = virtio_gpu_virgl_flushed;
> >> >+    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
> >> >+    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
> >> >+    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
> >> >+
> >> >+    vdc->reset = virtio_gpu_virgl_reset;
> >>
> >> A realize method is supposed to modify a single instance only while
> we're
> >> modifying the behavior of whole classes here, i.e. will affect every
> >> instance of these classes.
> >
> >This goes against QOM design principles and will therefore be confusing
> for
> >> people who are familiar with QOM in particular and OOP in general.
> >
> >
> >Context: this is a cleanup in preparation for the gfxstream/rutabaga
> >support:
> >
> >
> https://patchew.org/QEMU/20230421011223.718-1-gurchetansingh@chromium.org/
>
> Judging from this series the benefit of having a common -gl class isn't
> that big: AFAICS the only synergy effect is sharing a few properties which
> IMO don't warrant sharing. IOW the almost non-existant benefit rather
> confirms the current design. The last word needs to be spoken by the
> maintainers though.




> >
> >I explored creating a separate "virtio-gpu-rutabaga" device,
>
> Although this approach requires another -pci class it seems cleaner to me
> due to how the QEMU object model works. See below.
>
> > but felt it
> >added too much duplicate code (such as virtio-gpu-pci-rutabaga.c and
> >virtio-vga-rutabaga.c).  Please see here:
> >
> >
> https://gitlab.freedesktop.org/gurchetansingh/qemu-gfxstream/-/commits/master
> >
> >for that approach (current approach is in "qemu-gfxstream2" branch).
> >
> >In the current approach, function pointers are modified in realize(..)
> >instead of class_init(..) since "capset_names" can choose the appropriate
> >backend, but that variable is only accessible after class_init(..).
>
> Yeah, your're selecting a backend at runtime by changing a whole class'
> behavior inside an *instance* of that class. This is not how the QEMU
> object model is supposed to work...
>
> >
> >The difference between instance_init() and the realize() has also come up
> >before here:
> >
> >
> https://lore.kernel.org/all/268082DD-5FBB-41CC-8718-7D6BAA0D323A@livius.net/T/#m52be60860e2bf598816ed162f7b6dd070b52cd1d
>
> The link is about a related but different topic. It discusses
> .instance_init vs. .realize. This patch is essentially confusing
> .instance_init/.realize and .class_init. You can read more about the QEMU
> object model in general and class initialization in particular here:
> https://www.qemu.org/docs/master/devel/qom.html#class-initialization
>
> >
> >
> >> I think the code should be cleaned up in a different way if really
> needed.
> >>
> >
> >Sure, if there's a cleaner way, we should definitely explore it.  Given
> the
> >goal of adding another backend for virtio-gpu, how do you suggest
> >refactoring the code?
>
> I'm no maintainer but my suggestion would be this: Use your first approach
> with dedicated classes. This would also allow to force the new backend via
> the command line. If you really need detection at runtime you could either
> delegate this to Android Studio (by having it select the best (tm) backend
> via command line) or you might be able to add a so called "sugar property"
> and have QEMU make the choice (sugar properties exceed my knowledge though).


Sure, it's not too much trouble to go back to the original approach as you
suggest (unless maintainers have any opinions -- it's essentially
overriding class pointers versus additional cookie cutter
virtio-gpu-pci-rutabaga.c/virtio-vga-gl.c files).


>
> Regarding rutabaga I have the following comments:
>
> Given that rutabaga seems to be an abstraction layer over virgl and
> gfxstream it seems redundant to me. QEMU already has an abstraction layer
> for various graphics backends so IMO using another just introduces a
> maintenance burden. Therefore I suggest introducing a dedicated class wich
> uses gfxstream directly. The class name could end with -gfxstream to match
> the technology.
>
> Furthermore, rutabaga abstracts two C APIs and is used as C API. So the
> benefit of using Rust seems to be low -- not even mentioning the packaging
> issues this causes for Linux distributions.



Last but not least rutabaga seems to be a personal pet project rather than
> something official. I guess that QEMU would't want to rely on a personal
> pet project.


True, my main goal is to get gfxstream support for Android Studio .  But
the secondary goal is also to get Wayland passthrough in QEMU [a] -- which
is currently implemented in Rust owing to its crosvm roots.  We're
targeting a 100% Rust paravirtualization host stack in some cases [b], so
rutabaga_gfx does fulfill a need.

I've also taken a look at Ubuntu/Debian packaging, and it doesn't seem like
a problem since Rust support is very good [c].  As such, using rutabaga
APIs rather than gfxstream APIs in QEMU seems like the way to go (absent
any maintainer opinions).

[a]
https://roscidus.com/blog/blog/2021/03/07/qubes-lite-with-kvm-and-wayland/
[b]  https://fuchsia-review.googlesource.com/c/fuchsia/+/778764
[c] https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg05555.html


>
> In any case I'd leave the last word to the maintainers.


> Best regards,
> Bernhard
> >
> >
> >>
> >> Best regards,
> >> Bernhard
> >>
> >> >
> >> > #if HOST_BIG_ENDIAN
> >> >     error_setg(errp, "virgl is not supported on bigendian platforms");
> >> >@@ -736,9 +751,9 @@ void virtio_gpu_virgl_device_realize(DeviceState
> >> *qdev, Error **errp)
> >> >         return;
> >> >     }
> >> >
> >> >-    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);
> >> >+    VIRTIO_GPU_BASE(gpudev)->conf.flags |= (1 <<
> >> VIRTIO_GPU_FLAG_VIRGL_ENABLED);
> >> >+    VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets =
> >> >+        virtio_gpu_virgl_get_num_capsets(gpudev);
> >> >
> >> >     virtio_gpu_device_realize(qdev, errp);
> >> > }
> >> >diff --git a/include/hw/virtio/virtio-gpu.h
> >> b/include/hw/virtio/virtio-gpu.h
> >> >index 89ee133f07..d5808f2ab6 100644
> >> >--- a/include/hw/virtio/virtio-gpu.h
> >> >+++ b/include/hw/virtio/virtio-gpu.h
> >> >@@ -277,13 +277,6 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
> >> >                              struct virtio_gpu_rect *r);
> >> >
> >> > /* virtio-gpu-3d.c */
> >> >-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >> >-                                  struct virtio_gpu_ctrl_command
> *cmd);
> >> >-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct
> >> virtio_gpu_scanout *s,
> >> >-                                    uint32_t resource_id);
> >> >-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
> >> >-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);
> >> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev);
> >> > void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);
> >> >
> >> > #endif
> >>
>

[-- Attachment #2: Type: text/html, Size: 17833 bytes --]

Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Philippe Mathieu-Daudé]

On 30/4/23 23:48, Bernhard Beschow wrote:
> 
> 
> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <gurchetansingh@chromium.org>:
>> From: Gurchetan Singh <gurchetansingh@chromium.org>
>>
>> This reduces the amount of renderer backend specific needed to
>> be exposed to the GL device.  We only need one realize function
>> per renderer backend.
>>
>> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> v1: - Remove NULL inits (Philippe)
>>     - Use VIRTIO_GPU_BASE where possible (Philippe)
>> v2: - Fix unnecessary line break (Akihiko)
>>
>> hw/display/virtio-gpu-gl.c     | 15 ++++++---------
>> hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
>> include/hw/virtio/virtio-gpu.h |  7 -------
>> 3 files changed, 31 insertions(+), 26 deletions(-)


>> void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
>> {
>> -    VirtIOGPU *g = VIRTIO_GPU(qdev);
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +
>> +    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
>> +    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
>> +
>> +    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
>> +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
>> +
>> +    vbc->gl_flushed = virtio_gpu_virgl_flushed;
>> +    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>> +    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>> +    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
>> +
>> +    vdc->reset = virtio_gpu_virgl_reset;
> 
> A realize method is supposed to modify a single instance only while we're modifying the behavior of whole classes here, i.e. will affect every instance of these classes. This goes against QOM design principles and will therefore be confusing for people who are familiar with QOM in particular and OOP in general. I think the code should be cleaned up in a different way if really needed.

Doh I was too quick and totally missed this was an instance,
thanks for being careful Bernhard!

Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function

[Bernhard Beschow]

Am 10. Mai 2023 07:56:15 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 30/4/23 23:48, Bernhard Beschow wrote:
>> 
>> 
>> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <gurchetansingh@chromium.org>:
>>> From: Gurchetan Singh <gurchetansingh@chromium.org>
>>> 
>>> This reduces the amount of renderer backend specific needed to
>>> be exposed to the GL device.  We only need one realize function
>>> per renderer backend.
>>> 
>>> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> v1: - Remove NULL inits (Philippe)
>>>     - Use VIRTIO_GPU_BASE where possible (Philippe)
>>> v2: - Fix unnecessary line break (Akihiko)
>>> 
>>> hw/display/virtio-gpu-gl.c     | 15 ++++++---------
>>> hw/display/virtio-gpu-virgl.c  | 35 ++++++++++++++++++++++++----------
>>> include/hw/virtio/virtio-gpu.h |  7 -------
>>> 3 files changed, 31 insertions(+), 26 deletions(-)
>
>
>>> void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
>>> {
>>> -    VirtIOGPU *g = VIRTIO_GPU(qdev);
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>>> +
>>> +    VirtIOGPUBase *bdev = VIRTIO_GPU_BASE(qdev);
>>> +    VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_GET_CLASS(bdev);
>>> +
>>> +    VirtIOGPU *gpudev = VIRTIO_GPU(qdev);
>>> +    VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(gpudev);
>>> +
>>> +    vbc->gl_flushed = virtio_gpu_virgl_flushed;
>>> +    vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
>>> +    vgc->process_cmd = virtio_gpu_virgl_process_cmd;
>>> +    vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
>>> +
>>> +    vdc->reset = virtio_gpu_virgl_reset;
>> 
>> A realize method is supposed to modify a single instance only while we're modifying the behavior of whole classes here, i.e. will affect every instance of these classes. This goes against QOM design principles and will therefore be confusing for people who are familiar with QOM in particular and OOP in general. I think the code should be cleaned up in a different way if really needed.
>
>Doh I was too quick and totally missed this was an instance,
>thanks for being careful Bernhard!

My obligation ;)

I wonder if *_GET_CLASS() macros could return const pointers in order for the compiler to catch such uses? Are there use cases at all in retrieving non-const class pointers from instances?

Best regards,
Bernhard