From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8146BC77B73 for ; Mon, 1 May 2023 16:54:16 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ptWmB-0005bi-Uw; Mon, 01 May 2023 12:53:23 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ptWmA-0005bZ-UH for qemu-devel@nongnu.org; Mon, 01 May 2023 12:53:22 -0400 Received: from mail-ed1-x536.google.com ([2a00:1450:4864:20::536]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1ptWm7-0004iu-ON for qemu-devel@nongnu.org; Mon, 01 May 2023 12:53:22 -0400 Received: by mail-ed1-x536.google.com with SMTP id 4fb4d7f45d1cf-50bc4d96e14so10609163a12.1 for ; Mon, 01 May 2023 09:53:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1682959997; x=1685551997; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=15L/smWCQwYGjQBnUMZs1FwBDjxfs9Q/IvVQtMrF64Y=; b=DgfvaPCsKAhHlZCIJSFNA64y8E1iWa+Vu6GPvhG9qSCNp4TYn2e2BiaA6VIY+ve43F yHK+zWsVegEdZN7fs784AvpPb1g547Y4ZYRxC6YYoQ/jBCYSqu6V9Z6iroxaGbIjK4Il Qp9iVv7YPTx5niBzDquCQai962LkQ/eejGEqk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682959997; x=1685551997; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=15L/smWCQwYGjQBnUMZs1FwBDjxfs9Q/IvVQtMrF64Y=; b=Ap6ZGWa4Y69jpcxO1TfMBXwLfhrMdffQ0XIllgyHAWJSymChmNpDZb3VAKiRikbV7r mn5ZLeLLqtg8MYDAs1RiZj6aBE7jb4AVcFBGZseLoDYXYYUAU2l4Cuz6T6jIyrdnjPuH W+xAY1BrWNERg7YEoPHWJIe53VJz7RqlJCaY5aSE/iCZT4RvqXxyrwWma9P7lbN0SWzp xkVIwAnVh5Rgwj4EQ3mqbIX5u5sbrnDEbQxA2k0FL9XlTB3pfVpCnvEA9AYBFs5oL0VJ 3EAf3j9yRawZMkVyW8Lio3Dstc+ifS7XYQLFfnKZ0r+YUsV83nx/JeFOLzSJrhFyfyg5 WsxQ== X-Gm-Message-State: AC+VfDziZgt4LiunA0n/pfSGEI+PdJdKA6DZrd5D085Mkd5yUrbWZPl8 7AwIMJiDPKor2dpJw0E2Ndhd4OJJ/ZKev5QMu52OAA== X-Google-Smtp-Source: ACHHUZ6Ci3jqsaQ1IIqej0e5f0dQ8s8zdkcb8Mham6HYkApC+xq9x20zrvJ+VYGlXra/YldKlU8M9A== X-Received: by 2002:a17:906:4e1a:b0:946:c1d2:8b5d with SMTP id z26-20020a1709064e1a00b00946c1d28b5dmr17974569eju.17.1682959997217; Mon, 01 May 2023 09:53:17 -0700 (PDT) Received: from mail-ed1-f52.google.com (mail-ed1-f52.google.com. [209.85.208.52]) by smtp.gmail.com with ESMTPSA id k9-20020a170906054900b00923f05b2931sm14949703eja.118.2023.05.01.09.53.16 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 May 2023 09:53:16 -0700 (PDT) Received: by mail-ed1-f52.google.com with SMTP id 4fb4d7f45d1cf-50bc3a5f462so3227a12.1 for ; Mon, 01 May 2023 09:53:16 -0700 (PDT) X-Received: by 2002:a05:6402:2682:b0:505:bdac:5590 with SMTP id w2-20020a056402268200b00505bdac5590mr111edd.0.1682959996231; Mon, 01 May 2023 09:53:16 -0700 (PDT) MIME-Version: 1.0 References: <20230428164823.789-1-gurchetansingh@google.com> <20230428164823.789-3-gurchetansingh@google.com> <0DAAC63B-0C0F-44C4-B7EB-ACD6C9A36BF1@gmail.com> In-Reply-To: <0DAAC63B-0C0F-44C4-B7EB-ACD6C9A36BF1@gmail.com> From: Gurchetan Singh Date: Mon, 1 May 2023 09:53:03 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 3/5] hw/display/virtio-gpu-virgl: define callbacks in realize function To: Bernhard Beschow Cc: qemu-devel@nongnu.org, philmd@linaro.org, kraxel@redhat.com, marcandre.lureau@redhat.com, akihiko.odaki@gmail.com, dmitry.osipenko@collabora.com, ray.huang@amd.com, alex.bennee@linaro.org Content-Type: multipart/alternative; boundary="000000000000bfe97005faa4a8dc" Received-SPF: pass client-ip=2a00:1450:4864:20::536; envelope-from=gurchetansingh@chromium.org; helo=mail-ed1-x536.google.com X-Spam_score_int: -22 X-Spam_score: -2.3 X-Spam_bar: -- X-Spam_report: (-2.3 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.171, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --000000000000bfe97005faa4a8dc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, Apr 30, 2023 at 2:48=E2=80=AFPM Bernhard Beschow wrote: > > > Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh < > gurchetansingh@chromium.org>: > >From: Gurchetan Singh > > > >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 > >Reviewed-by: Philippe Mathieu-Daud=C3=A9 > >--- > >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 **err= p) > >+{ > >+ virtio_gpu_virgl_device_realize(qdev, errp); > >+} > >+ > > static Property virtio_gpu_gl_properties[] =3D { > > 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 =3D DEVICE_CLASS(klass); > > VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_CLASS(klass); > >- VirtIOGPUBaseClass *vbc =3D VIRTIO_GPU_BASE_CLASS(klass); > >- VirtIOGPUClass *vgc =3D VIRTIO_GPU_CLASS(klass); > >- > >- vbc->gl_flushed =3D virtio_gpu_virgl_flushed; > >- vgc->handle_ctrl =3D virtio_gpu_virgl_handle_ctrl; > >- vgc->process_cmd =3D virtio_gpu_virgl_process_cmd; > >- vgc->update_cursor_data =3D virtio_gpu_virgl_update_cursor; > > > >- vdc->realize =3D virtio_gpu_virgl_device_realize; > >- vdc->reset =3D virtio_gpu_virgl_reset; > >+ vdc->realize =3D 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(VirtIOGP= U > *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 =3D 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 =3D VIRTIO_GPU(vdev); > > VirtIOGPUGL *gl =3D 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 =3D VIRTIO_GPU(vdev); > > VirtIOGPUGL *gl =3D 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 =3D VIRTIO_GPU(qdev); > >+ VirtIODevice *vdev =3D VIRTIO_DEVICE(qdev); > >+ VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_GET_CLASS(vdev); > >+ > >+ VirtIOGPUBase *bdev =3D VIRTIO_GPU_BASE(qdev); > >+ VirtIOGPUBaseClass *vbc =3D VIRTIO_GPU_BASE_GET_CLASS(bdev); > >+ > >+ VirtIOGPU *gpudev =3D VIRTIO_GPU(qdev); > >+ VirtIOGPUClass *vgc =3D VIRTIO_GPU_GET_CLASS(gpudev); > >+ > >+ vbc->gl_flushed =3D virtio_gpu_virgl_flushed; > >+ vgc->handle_ctrl =3D virtio_gpu_virgl_handle_ctrl; > >+ vgc->process_cmd =3D virtio_gpu_virgl_process_cmd; > >+ vgc->update_cursor_data =3D virtio_gpu_virgl_update_cursor; > >+ > >+ vdc->reset =3D 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/mast= er 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 |=3D (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED); > >- VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =3D > >- virtio_gpu_virgl_get_num_capsets(g); > >+ VIRTIO_GPU_BASE(gpudev)->conf.flags |=3D (1 << > VIRTIO_GPU_FLAG_VIRGL_ENABLED); > >+ VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets =3D > >+ 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 > --000000000000bfe97005faa4a8dc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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


Am 28. April 2023 16:48:21 UTC schrieb Gurchetan Singh <gurchetansingh@chromium.or= g>:
>From: Gurchetan Singh <gurchetansingh@chromium.org>
>
>This reduces the amount of renderer backend specific needed to
>be exposed to the GL device.=C2=A0 We only need one realize function >per renderer backend.
>
>Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>Reviewed-by: Philippe Mathieu-Daud=C3=A9 <philmd@linaro.org>
>---
>v1: - Remove NULL inits (Philippe)
>=C2=A0 =C2=A0 - Use VIRTIO_GPU_BASE where possible (Philippe)
>v2: - Fix unnecessary line break (Akihiko)
>
> hw/display/virtio-gpu-gl.c=C2=A0 =C2=A0 =C2=A0| 15 ++++++---------
> hw/display/virtio-gpu-virgl.c=C2=A0 | 35 ++++++++++++++++++++++++-----= -----
> include/hw/virtio/virtio-gpu.h |=C2=A0 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 **er= rp)
>+{
>+=C2=A0 =C2=A0 virtio_gpu_virgl_device_realize(qdev, errp);
>+}
>+
> static Property virtio_gpu_gl_properties[] =3D {
>=C2=A0 =C2=A0 =C2=A0DEFINE_PROP_BIT("stats", VirtIOGPU, paren= t_obj.conf.flags,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0VIRTIO_GPU_FLAG_STATS_ENABLED, false),
>@@ -31,16 +36,8 @@ static void virtio_gpu_gl_class_init(ObjectClass *kl= ass, void *data)
> {
>=C2=A0 =C2=A0 =C2=A0DeviceClass *dc =3D DEVICE_CLASS(klass);
>=C2=A0 =C2=A0 =C2=A0VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_CLASS(klas= s);
>-=C2=A0 =C2=A0 VirtIOGPUBaseClass *vbc =3D VIRTIO_GPU_BASE_CLASS(klass)= ;
>-=C2=A0 =C2=A0 VirtIOGPUClass *vgc =3D VIRTIO_GPU_CLASS(klass);
>-
>-=C2=A0 =C2=A0 vbc->gl_flushed =3D virtio_gpu_virgl_flushed;
>-=C2=A0 =C2=A0 vgc->handle_ctrl =3D virtio_gpu_virgl_handle_ctrl; >-=C2=A0 =C2=A0 vgc->process_cmd =3D virtio_gpu_virgl_process_cmd; >-=C2=A0 =C2=A0 vgc->update_cursor_data =3D virtio_gpu_virgl_update_c= ursor;
>
>-=C2=A0 =C2=A0 vdc->realize =3D virtio_gpu_virgl_device_realize;
>-=C2=A0 =C2=A0 vdc->reset =3D virtio_gpu_virgl_reset;
>+=C2=A0 =C2=A0 vdc->realize =3D virtio_gpu_gl_device_realize;
>=C2=A0 =C2=A0 =C2=A0device_class_set_props(dc, virtio_gpu_gl_properties= );
> }
>
>diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virg= l.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,
>=C2=A0 =C2=A0 =C2=A0g_free(resp);
> }
>
>-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct virti= o_gpu_ctrl_command *cmd)
>+static void
>+virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct virtio_gpu_ctrl_command *cmd)
> {
>=C2=A0 =C2=A0 =C2=A0VIRTIO_GPU_FILL_CMD(cmd->cmd_hdr);
>
>@@ -637,7 +638,7 @@ static int virtio_gpu_virgl_get_num_capsets(VirtIOG= PU *g)
>=C2=A0 =C2=A0 =C2=A0return capset2_max_ver ? 2 : 1;
> }
>
>-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>+static void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct virtio_gpu_scanout *s,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 uint32_t resource_id)
> {
>@@ -660,14 +661,14 @@ void virtio_gpu_virgl_update_cursor(VirtIOGPU *g,=
>=C2=A0 =C2=A0 =C2=A0free(data);
> }
>
>-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
>+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
> {
>=C2=A0 =C2=A0 =C2=A0VirtIOGPU *g =3D VIRTIO_GPU(b);
>
>=C2=A0 =C2=A0 =C2=A0virtio_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)
> {
>=C2=A0 =C2=A0 =C2=A0VirtIOGPU *g =3D VIRTIO_GPU(vdev);
>=C2=A0 =C2=A0 =C2=A0VirtIOGPUGL *gl =3D VIRTIO_GPU_GL(vdev);
>@@ -699,7 +700,7 @@ void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vde= v, VirtQueue *vq)
>=C2=A0 =C2=A0 =C2=A0virtio_gpu_virgl_fence_poll(g);
> }
>
>-void virtio_gpu_virgl_reset(VirtIODevice *vdev)
>+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
> {
>=C2=A0 =C2=A0 =C2=A0VirtIOGPU *g =3D VIRTIO_GPU(vdev);
>=C2=A0 =C2=A0 =C2=A0VirtIOGPUGL *gl =3D 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)<= br> > {
>-=C2=A0 =C2=A0 VirtIOGPU *g =3D VIRTIO_GPU(qdev);
>+=C2=A0 =C2=A0 VirtIODevice *vdev =3D VIRTIO_DEVICE(qdev);
>+=C2=A0 =C2=A0 VirtioDeviceClass *vdc =3D VIRTIO_DEVICE_GET_CLASS(vdev)= ;
>+
>+=C2=A0 =C2=A0 VirtIOGPUBase *bdev =3D VIRTIO_GPU_BASE(qdev);
>+=C2=A0 =C2=A0 VirtIOGPUBaseClass *vbc =3D VIRTIO_GPU_BASE_GET_CLASS(bd= ev);
>+
>+=C2=A0 =C2=A0 VirtIOGPU *gpudev =3D VIRTIO_GPU(qdev);
>+=C2=A0 =C2=A0 VirtIOGPUClass *vgc =3D VIRTIO_GPU_GET_CLASS(gpudev); >+
>+=C2=A0 =C2=A0 vbc->gl_flushed =3D virtio_gpu_virgl_flushed;
>+=C2=A0 =C2=A0 vgc->handle_ctrl =3D virtio_gpu_virgl_handle_ctrl; >+=C2=A0 =C2=A0 vgc->process_cmd =3D virtio_gpu_virgl_process_cmd; >+=C2=A0 =C2=A0 vgc->update_cursor_data =3D virtio_gpu_virgl_update_c= ursor;
>+
>+=C2=A0 =C2=A0 vdc->reset =3D 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 ins= tance of these classes.=C2=A0
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 pre= paration for the gfxstream/rutabaga support:


I explored cr= eating 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).=C2=A0 Please see here:


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

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 access= ible after class_init(..).

The difference be= tween instance_init() and the realize() has also come up before here:
=

=C2=A0
=
I think the code should b= e cleaned up in a different way if really needed.

=
Sure, if there's a cleaner way, we should definite= ly explore it.=C2=A0 Given the goal of adding another backend for virtio-gp= u, how do you suggest refactoring the code?
=C2=A0

Best regards,
Bernhard

>
> #if HOST_BIG_ENDIAN
>=C2=A0 =C2=A0 =C2=A0error_setg(errp, "virgl is not supported on bi= gendian platforms");
>@@ -736,9 +751,9 @@ void virtio_gpu_virgl_device_realize(DeviceState *q= dev, Error **errp)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
>=C2=A0 =C2=A0 =C2=A0}
>
>-=C2=A0 =C2=A0 g->parent_obj.conf.flags |=3D (1 << VIRTIO_GPU_= FLAG_VIRGL_ENABLED);
>-=C2=A0 =C2=A0 VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =3D
>-=C2=A0 =C2=A0 =C2=A0 =C2=A0 virtio_gpu_virgl_get_num_capsets(g);
>+=C2=A0 =C2=A0 VIRTIO_GPU_BASE(gpudev)->conf.flags |=3D (1 << = VIRTIO_GPU_FLAG_VIRGL_ENABLED);
>+=C2=A0 =C2=A0 VIRTIO_GPU_BASE(gpudev)->virtio_config.num_capsets = =3D
>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 virtio_gpu_virgl_get_num_capsets(gpudev);<= br> >
>=C2=A0 =C2=A0 =C2=A0virtio_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,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct virtio_gpu_rect *r);
>
> /* virtio-gpu-3d.c */
>-void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct virtio_gpu_ctrl_com= mand *cmd);
>-void virtio_gpu_virgl_update_cursor(VirtIOGPU *g, struct virtio_gpu_sc= anout *s,
>-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 uint32_t resource_i= d);
>-void virtio_gpu_virgl_flushed(VirtIOGPUBase *b);
>-void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq);<= br> >-void virtio_gpu_virgl_reset(VirtIODevice *vdev);
> void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp);=
>
> #endif
--000000000000bfe97005faa4a8dc--