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