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:
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:
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:
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