On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <shen...@gmail.com> wrote:
>
>
> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
> gurchetansi...@chromium.org>:
> >From: Gurchetan Singh <gurchetansi...@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 <gurchetansi...@chromium.org>
> >Reviewed-by: Philippe Mathieu-Daudé <phi...@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-gurchetansi...@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-7d6baa0d3...@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
>
