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

Commit Message

Gurchetan Singh April 28, 2023, 4:48 p.m. UTC

From: Gurchetan Singh <[e-mail]>

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 <[e-mail]>
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
---
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(-)

Comments

Bernhard Beschow April 30, 2023, 9:48 p.m. UTC | #1

Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <[e-mail]>:
>From: Gurchetan Singh <[e-mail]>
>
>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 <[e-mail]>
>Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>---
>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

Gurchetan Singh May 1, 2023, 4:53 p.m. UTC | #2

On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <[e-mail]> wrote:

>
>
> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
> [e-mail]>:
> >From: Gurchetan Singh <[e-mail]>
> >
> >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 <[e-mail]>
> >Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> >---
> >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/[e-mail]/

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/[e-mail]/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
>

Bernhard Beschow May 5, 2023, 5:47 p.m. UTC | #3

Am 1. Mai 2023 16:53:03 UTC schrieb Gurchetan Singh <[e-mail]>:
>On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <[e-mail]> wrote:
>
>>
>>
>> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
>> [e-mail]>:
>> >From: Gurchetan Singh <[e-mail]>
>> >
>> >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 <[e-mail]>
>> >Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>> >---
>> >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/[e-mail]/

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/[e-mail]/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
>>

Gurchetan Singh May 5, 2023, 8:53 p.m. UTC | #4

On Fri, May 5, 2023 at 10:48 AM Bernhard Beschow <[e-mail]> wrote:

> Am 1. Mai 2023 16:53:03 UTC schrieb Gurchetan Singh <
> [e-mail]>:
> >On Sun, Apr 30, 2023 at 2:48 PM Bernhard Beschow <[e-mail]>
> wrote:
> >
> >>
> >>
> >> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <
> >> [e-mail]>:
> >> >From: Gurchetan Singh <[e-mail]>
> >> >
> >> >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 <[e-mail]>
> >> >Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> >> >---
> >> >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/[e-mail]/
>
> 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/[e-mail]/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
> >>
>

Philippe Mathieu-Daudé May 10, 2023, 7:56 a.m. UTC | #5

On 30/4/23 23:48, Bernhard Beschow wrote:
> 
> 
> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <[e-mail]>:
>> From: Gurchetan Singh <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>> ---
>> 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!

Bernhard Beschow May 10, 2023, 5:55 p.m. UTC | #6

Am 10. Mai 2023 07:56:15 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>On 30/4/23 23:48, Bernhard Beschow wrote:
>> 
>> 
>> Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <[e-mail]>:
>>> From: Gurchetan Singh <[e-mail]>
>>> 
>>> 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 <[e-mail]>
>>> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>>> ---
>>> 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

Patch

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