Move device init (realize) and properties.

Drop the virgl property, the virtio-gpu-gl-device has virgl enabled no
matter what.  Just use virtio-gpu-device instead if you don't want
enable virgl and opengl.  This simplifies the logic and reduces the test
matrix.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20210430113547.1816178-1-kraxel@redhat.com
Message-Id: <20210430113547.1816178-4-kraxel@redhat.com>
---
 include/hw/virtio/virtio-gpu.h |  1 +
 hw/display/virtio-gpu-gl.c     | 33 +++++++++++++++++++++++++++++++++
 hw/display/virtio-gpu.c        | 23 +----------------------
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 5b2d011b49d5..7430f3cd9958 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -221,6 +221,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
 void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
                                     struct iovec *iov, uint32_t count);
 void virtio_gpu_process_cmdq(VirtIOGPU *g);
+void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
 
 /* virtio-gpu-3d.c */
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index a477cbe186d3..9b7b5f00d7e6 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -16,14 +16,47 @@
 #include "qemu/module.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "sysemu/sysemu.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
 #include "hw/virtio/virtio-gpu-bswap.h"
 #include "hw/virtio/virtio-gpu-pixman.h"
 #include "hw/qdev-properties.h"
 
+static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
+{
+    VirtIOGPU *g = VIRTIO_GPU(qdev);
+
+#if defined(HOST_WORDS_BIGENDIAN)
+    error_setg(errp, "virgl is not supported on bigendian platforms");
+    return;
+#endif
+
+    if (!display_opengl) {
+        error_setg(errp, "opengl is not available");
+        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_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),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+    vdc->realize = virtio_gpu_gl_device_realize;
+    device_class_set_props(dc, virtio_gpu_gl_properties);
 }
 
 static const TypeInfo virtio_gpu_gl_info = {
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 6f3791deb3ae..461b7769b457 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1121,25 +1121,10 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
     return 0;
 }
 
-static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
+void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
     VirtIOGPU *g = VIRTIO_GPU(qdev);
-    bool have_virgl;
-
-#if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN)
-    have_virgl = false;
-#else
-    have_virgl = display_opengl;
-#endif
-    if (!have_virgl) {
-        g->parent_obj.conf.flags &= ~(1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);
-    } else {
-#if defined(CONFIG_VIRGL)
-        VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =
-            virtio_gpu_virgl_get_num_capsets(g);
-#endif
-    }
 
     if (!virtio_gpu_base_device_realize(qdev,
                                         virtio_gpu_handle_ctrl_cb,
@@ -1251,12 +1236,6 @@ static Property virtio_gpu_properties[] = {
     VIRTIO_GPU_BASE_PROPERTIES(VirtIOGPU, parent_obj.conf),
     DEFINE_PROP_SIZE("max_hostmem", VirtIOGPU, conf_max_hostmem,
                      256 * MiB),
-#ifdef CONFIG_VIRGL
-    DEFINE_PROP_BIT("virgl", VirtIOGPU, parent_obj.conf.flags,
-                    VIRTIO_GPU_FLAG_VIRGL_ENABLED, true),
-    DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
-                    VIRTIO_GPU_FLAG_STATS_ENABLED, false),
-#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.31.1

On 5/10/21 3:20 PM, Gerd Hoffmann wrote:
> Move device init (realize) and properties.
> 
> Drop the virgl property, the virtio-gpu-gl-device has virgl enabled no
> matter what.  Just use virtio-gpu-device instead if you don't want
> enable virgl and opengl.  This simplifies the logic and reduces the test
> matrix.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Message-id: 20210430113547.1816178-1-kraxel@redhat.com
> Message-Id: <20210430113547.1816178-4-kraxel@redhat.com>
> ---
>  include/hw/virtio/virtio-gpu.h |  1 +
>  hw/display/virtio-gpu-gl.c     | 33 +++++++++++++++++++++++++++++++++
>  hw/display/virtio-gpu.c        | 23 +----------------------
>  3 files changed, 35 insertions(+), 22 deletions(-)
> 

> @@ -1251,12 +1236,6 @@ static Property virtio_gpu_properties[] = {
>      VIRTIO_GPU_BASE_PROPERTIES(VirtIOGPU, parent_obj.conf),
>      DEFINE_PROP_SIZE("max_hostmem", VirtIOGPU, conf_max_hostmem,
>                       256 * MiB),
> -#ifdef CONFIG_VIRGL
> -    DEFINE_PROP_BIT("virgl", VirtIOGPU, parent_obj.conf.flags,
> -                    VIRTIO_GPU_FLAG_VIRGL_ENABLED, true),
> -    DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
> -                    VIRTIO_GPU_FLAG_STATS_ENABLED, false),
> -#endif
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

Sorry for catching this a bit late, but libvirt is looking for "virgl"
property when guest XML has 3D acceleration enabled:

    <video>
      <model type='virtio' heads='1' primary='yes'>
        <acceleration accel3d='yes'/>
      </model>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
function='0x0'/>
    </video>

This is the corresponding part of cmd line:

  -device virtio-vga,id=video0,virgl=on,max_outputs=1,bus=pci.0,addr=0x2

The commit message suggests that virtio-gpu-gl-device should be used
instead. Fair enough, so IIUC the cmd line should be changed to:

  -device virtio-gpu-gl-device,id=video0,max_outputs=1,bus=pci.0,addr=0x2


Michal

Hi

On Fri, May 21, 2021 at 1:34 PM Michal Prívozník <mprivozn@redhat.com>
wrote:

> On 5/10/21 3:20 PM, Gerd Hoffmann wrote:
> > Move device init (realize) and properties.
> >
> > Drop the virgl property, the virtio-gpu-gl-device has virgl enabled no
> > matter what.  Just use virtio-gpu-device instead if you don't want
> > enable virgl and opengl.  This simplifies the logic and reduces the test
> > matrix.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Message-id: 20210430113547.1816178-1-kraxel@redhat.com
> > Message-Id: <20210430113547.1816178-4-kraxel@redhat.com>
> > ---
> >  include/hw/virtio/virtio-gpu.h |  1 +
> >  hw/display/virtio-gpu-gl.c     | 33 +++++++++++++++++++++++++++++++++
> >  hw/display/virtio-gpu.c        | 23 +----------------------
> >  3 files changed, 35 insertions(+), 22 deletions(-)
> >
>
> > @@ -1251,12 +1236,6 @@ static Property virtio_gpu_properties[] = {
> >      VIRTIO_GPU_BASE_PROPERTIES(VirtIOGPU, parent_obj.conf),
> >      DEFINE_PROP_SIZE("max_hostmem", VirtIOGPU, conf_max_hostmem,
> >                       256 * MiB),
> > -#ifdef CONFIG_VIRGL
> > -    DEFINE_PROP_BIT("virgl", VirtIOGPU, parent_obj.conf.flags,
> > -                    VIRTIO_GPU_FLAG_VIRGL_ENABLED, true),
> > -    DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
> > -                    VIRTIO_GPU_FLAG_STATS_ENABLED, false),
> > -#endif
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> >
>
> Sorry for catching this a bit late, but libvirt is looking for "virgl"
> property when guest XML has 3D acceleration enabled:
>
>     <video>
>       <model type='virtio' heads='1' primary='yes'>
>         <acceleration accel3d='yes'/>
>       </model>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> function='0x0'/>
>     </video>
>
> This is the corresponding part of cmd line:
>
>   -device virtio-vga,id=video0,virgl=on,max_outputs=1,bus=pci.0,addr=0x2
>
> The commit message suggests that virtio-gpu-gl-device should be used
> instead. Fair enough, so IIUC the cmd line should be changed to:
>
>   -device virtio-gpu-gl-device,id=video0,max_outputs=1,bus=pci.0,addr=0x2
>

Should be with virtio-vga-gl instead. And I think virtio-gpu-gl-pci for
secondary devices.

(it's not clear to me if virtio-gpu*-device should be user_creatable on x86
at least)

-- 
Marc-André Lureau

  Hi,

> (it's not clear to me if virtio-gpu*-device should be user_creatable on x86
> at least)

Yes (microvm uses virtio-mmio).

take care,
  Gerd

  Hi,

> Sorry for catching this a bit late, but libvirt is looking for "virgl"
> property when guest XML has 3D acceleration enabled:

Yes, libvirt must be adapted to this.

https://gitlab.com/libvirt/libvirt/-/issues/167

As far I know libvirt checks whenever the virgl property exists to
figure whenever virgl support is available (as you can compile qemu
without virgl support).  So without changes libvirt will simply
think there is no 3d support and configurations without virgl enables
should continue to work fine.

Configurations with virgl enabled will break though, and unfortunaly
there is no easy way to avoid that.

> The commit message suggests that virtio-gpu-gl-device should be used
> instead. Fair enough, so IIUC the cmd line should be changed to:
> 
>   -device virtio-gpu-gl-device,id=video0,max_outputs=1,bus=pci.0,addr=0x2

virtio-gpu-gl-device is the mmio variant, for pci you need
virtio-gpu-gl-pci.  But otherwise yes, this is what libvirt should use
in case it figures qemu supports the virtio-gpu-gl-pci device (again,
when compiling with virgl disabled the device will not be there).

take care,
  Gerd