Currently we have one virtio-gpu device.  Problem with this approach is
that if you compile a full-featured qemu you'll get a virtio-gpu device
which depends on opengl and virgl, so these dependencies must be
installed and the libraries will be loaded into memory even if you don't
use virgl.  Also the code is cluttered with #ifdefs and a bit messy.

This patch series splits the virtio-gpu device into two:

 (1) virtio-gpu-device becomes the non-virgl device, same as
     virtio-gpu-device,virgl=off today.
 (2) virtio-gpu-gl-device is the new virgl device, same as
     virtio-gpu-device,virgl=on today.

When compiling qemu without virglrenderer support virtio-gpu-device
behavior doesn't change.

v2:
 - rebase to latest master.
 - move pci and vga wrappers to separate modules.
 - fix ci failures.

Gerd Hoffmann (16):
  virtio-gpu: rename virgl source file.
  virtio-gpu: add virtio-gpu-gl-device
  virtio-gpu: move virgl realize + properties
  virtio-gpu: move virgl reset
  virtio-gpu: use class function for ctrl queue handlers
  virtio-gpu: move virgl handle_ctrl
  virtio-gpu: move virgl gl_flushed
  virtio-gpu: move virgl process_cmd
  virtio-gpu: move update_cursor_data
  virtio-gpu: drop VIRGL() macro
  virtio-gpu: move virtio-gpu-gl-device to separate module
  virtio-gpu: drop use_virgl_renderer
  virtio-gpu: move fields to struct VirtIOGPUGL
  virtio-gpu: add virtio-gpu-gl-pci
  modules: add have_vga
  virtio-gpu: add virtio-vga-gl

 include/hw/display/vga.h                      |   6 +
 include/hw/virtio/virtio-gpu.h                |  31 +++-
 hw/display/vga.c                              |   2 +
 hw/display/virtio-gpu-base.c                  |   6 +-
 hw/display/virtio-gpu-gl.c                    | 163 ++++++++++++++++++
 hw/display/virtio-gpu-pci-gl.c                |  55 ++++++
 .../{virtio-gpu-3d.c => virtio-gpu-virgl.c}   |   0
 hw/display/virtio-gpu.c                       | 142 +++------------
 hw/display/virtio-vga-gl.c                    |  47 +++++
 util/module.c                                 |   7 +
 hw/display/meson.build                        |  19 +-
 11 files changed, 344 insertions(+), 134 deletions(-)
 create mode 100644 hw/display/virtio-gpu-gl.c
 create mode 100644 hw/display/virtio-gpu-pci-gl.c
 rename hw/display/{virtio-gpu-3d.c => virtio-gpu-virgl.c} (100%)
 create mode 100644 hw/display/virtio-vga-gl.c

-- 
2.30.2

Patchew URL: https://patchew.org/QEMU/20210430113547.1816178-1-kraxel@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210430113547.1816178-1-kraxel@redhat.com
Subject: [PATCH v2 00/16] virtio-gpu: split into two devices.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   ccdf06c..c3811c0  master     -> master
 - [tag update]      patchew/20210414170352.29927-1-eesposit@redhat.com -> patchew/20210414170352.29927-1-eesposit@redhat.com
 - [tag update]      patchew/20210430101707.245126-1-philmd@redhat.com -> patchew/20210430101707.245126-1-philmd@redhat.com
 - [tag update]      patchew/20210430103437.4140-1-peter.maydell@linaro.org -> patchew/20210430103437.4140-1-peter.maydell@linaro.org
 * [new tag]         patchew/20210430113547.1816178-1-kraxel@redhat.com -> patchew/20210430113547.1816178-1-kraxel@redhat.com
Switched to a new branch 'test'
2964468 virtio-gpu: add virtio-vga-gl
151ae78 modules: add have_vga
0660735 virtio-gpu: add virtio-gpu-gl-pci
6107d93 virtio-gpu: move fields to struct VirtIOGPUGL
811c9e3 virtio-gpu: drop use_virgl_renderer
61dbdb7 virtio-gpu: move virtio-gpu-gl-device to separate module
5c7f7bf virtio-gpu: drop VIRGL() macro
8ce823c virtio-gpu: move update_cursor_data
5a03263 virtio-gpu: move virgl process_cmd
1745a18 virtio-gpu: move virgl gl_flushed
2d23c9b virtio-gpu: move virgl handle_ctrl
31e7684 virtio-gpu: use class function for ctrl queue handlers
927268e virtio-gpu: move virgl reset
4a44d33 virtio-gpu: move virgl realize + properties
07bfe0f virtio-gpu: add virtio-gpu-gl-device
70ac96b virtio-gpu: rename virgl source file.

=== OUTPUT BEGIN ===
1/16 Checking commit 70ac96bb0ec3 (virtio-gpu: rename virgl source file.)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#14: 
 hw/display/{virtio-gpu-3d.c => virtio-gpu-virgl.c} | 0

total: 0 errors, 1 warnings, 8 lines checked

Patch 1/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/16 Checking commit 07bfe0f6ddb3 (virtio-gpu: add virtio-gpu-gl-device)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#32: 
new file mode 100644

total: 0 errors, 1 warnings, 75 lines checked

Patch 2/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/16 Checking commit 4a44d33bf474 (virtio-gpu: move virgl realize + properties)
4/16 Checking commit 927268e776bb (virtio-gpu: move virgl reset)
5/16 Checking commit 31e768487193 (virtio-gpu: use class function for ctrl queue handlers)
6/16 Checking commit 2d23c9b29cdd (virtio-gpu: move virgl handle_ctrl)
7/16 Checking commit 1745a18eb6bf (virtio-gpu: move virgl gl_flushed)
8/16 Checking commit 5a0326303241 (virtio-gpu: move virgl process_cmd)
WARNING: line over 80 characters
#101: FILE: include/hw/virtio/virtio-gpu.h:232:
+void virtio_gpu_simple_process_cmd(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd);

total: 0 errors, 1 warnings, 70 lines checked

Patch 8/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/16 Checking commit 8ce823cd9712 (virtio-gpu: move update_cursor_data)
10/16 Checking commit 5c7f7bfb6eeb (virtio-gpu: drop VIRGL() macro)
11/16 Checking commit 61dbdb735c96 (virtio-gpu: move virtio-gpu-gl-device to separate module)
12/16 Checking commit 811c9e3428b5 (virtio-gpu: drop use_virgl_renderer)
ERROR: "foo * bar" should be "foo *bar"
#96: FILE: hw/display/virtio-gpu-gl.c:47:
+    memcpy(s->current_cursor->data, data, pixels * sizeof(uint32_t));

total: 1 errors, 0 warnings, 116 lines checked

Patch 12/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

13/16 Checking commit 6107d9347287 (virtio-gpu: move fields to struct VirtIOGPUGL)
14/16 Checking commit 066073520377 (virtio-gpu: add virtio-gpu-gl-pci)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#34: 
new file mode 100644

total: 0 errors, 1 warnings, 80 lines checked

Patch 14/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
15/16 Checking commit 151ae7847aec (modules: add have_vga)
16/16 Checking commit 2964468a36aa (virtio-gpu: add virtio-vga-gl)
Use of uninitialized value $acpi_testexpected in string eq at ./scripts/checkpatch.pl line 1529.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#34: 
new file mode 100644

total: 0 errors, 1 warnings, 72 lines checked

Patch 16/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210430113547.1816178-1-kraxel@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Hi

On Fri, Apr 30, 2021 at 4:23 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Currently we have one virtio-gpu device.  Problem with this approach is
> that if you compile a full-featured qemu you'll get a virtio-gpu device
> which depends on opengl and virgl, so these dependencies must be
> installed and the libraries will be loaded into memory even if you don't
> use virgl.  Also the code is cluttered with #ifdefs and a bit messy.
>
> This patch series splits the virtio-gpu device into two:
>
>  (1) virtio-gpu-device becomes the non-virgl device, same as
>      virtio-gpu-device,virgl=off today.
>  (2) virtio-gpu-gl-device is the new virgl device, same as
>      virtio-gpu-device,virgl=on today.
>
> When compiling qemu without virglrenderer support virtio-gpu-device
> behavior doesn't change.
>
> v2:
>  - rebase to latest master.
>  - move pci and vga wrappers to separate modules.
>  - fix ci failures.
>

The patch series looks good.

But isn't that a breaking change? Any existing user of
virtio-gpu/vga,virgl=on will no longer get a working setup. Right? Imho, in
this case (virgl VM being not very common) the benefit is worth it.
However, I suggest to keep the 'virgl=' property, and print a deprecation /
replaced-by warning, falling back to non-virgl/2d mode. Or perhaps we could
have more cleverness to have virgl=on aliasing to the new devices.


> Gerd Hoffmann (16):
>   virtio-gpu: rename virgl source file.
>   virtio-gpu: add virtio-gpu-gl-device
>   virtio-gpu: move virgl realize + properties
>   virtio-gpu: move virgl reset
>   virtio-gpu: use class function for ctrl queue handlers
>   virtio-gpu: move virgl handle_ctrl
>   virtio-gpu: move virgl gl_flushed
>   virtio-gpu: move virgl process_cmd
>   virtio-gpu: move update_cursor_data
>   virtio-gpu: drop VIRGL() macro
>   virtio-gpu: move virtio-gpu-gl-device to separate module
>   virtio-gpu: drop use_virgl_renderer
>   virtio-gpu: move fields to struct VirtIOGPUGL
>   virtio-gpu: add virtio-gpu-gl-pci
>   modules: add have_vga
>   virtio-gpu: add virtio-vga-gl
>
>  include/hw/display/vga.h                      |   6 +
>  include/hw/virtio/virtio-gpu.h                |  31 +++-
>  hw/display/vga.c                              |   2 +
>  hw/display/virtio-gpu-base.c                  |   6 +-
>  hw/display/virtio-gpu-gl.c                    | 163 ++++++++++++++++++
>  hw/display/virtio-gpu-pci-gl.c                |  55 ++++++
>  .../{virtio-gpu-3d.c => virtio-gpu-virgl.c}   |   0
>  hw/display/virtio-gpu.c                       | 142 +++------------
>  hw/display/virtio-vga-gl.c                    |  47 +++++
>  util/module.c                                 |   7 +
>  hw/display/meson.build                        |  19 +-
>  11 files changed, 344 insertions(+), 134 deletions(-)
>  create mode 100644 hw/display/virtio-gpu-gl.c
>  create mode 100644 hw/display/virtio-gpu-pci-gl.c
>  rename hw/display/{virtio-gpu-3d.c => virtio-gpu-virgl.c} (100%)
>  create mode 100644 hw/display/virtio-vga-gl.c
>
> --
> 2.30.2
>
>
>
>

-- 
Marc-André Lureau

On Fri, Apr 30, 2021 at 07:32:58PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Apr 30, 2021 at 4:23 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > Currently we have one virtio-gpu device.  Problem with this approach is
> > that if you compile a full-featured qemu you'll get a virtio-gpu device
> > which depends on opengl and virgl, so these dependencies must be
> > installed and the libraries will be loaded into memory even if you don't
> > use virgl.  Also the code is cluttered with #ifdefs and a bit messy.
> >
> > This patch series splits the virtio-gpu device into two:
> >
> >  (1) virtio-gpu-device becomes the non-virgl device, same as
> >      virtio-gpu-device,virgl=off today.
> >  (2) virtio-gpu-gl-device is the new virgl device, same as
> >      virtio-gpu-device,virgl=on today.
> >
> > When compiling qemu without virglrenderer support virtio-gpu-device
> > behavior doesn't change.
> >
> > v2:
> >  - rebase to latest master.
> >  - move pci and vga wrappers to separate modules.
> >  - fix ci failures.
> >
> 
> The patch series looks good.
> 
> But isn't that a breaking change? Any existing user of
> virtio-gpu/vga,virgl=on will no longer get a working setup. Right?

Correct.

> Imho, in
> this case (virgl VM being not very common) the benefit is worth it.
> However, I suggest to keep the 'virgl=' property, and print a deprecation /
> replaced-by warning, falling back to non-virgl/2d mode.

Problem with that is that libvirt uses the virgl property to figure
whenever virtio-gpu-pci / virtio-vga supports virgl or not.  So a dummy
virgl property just for printing a warning message doesn't look like a
good idea to me.

> Or perhaps we could
> have more cleverness to have virgl=on aliasing to the new devices.

If that is doable without ugly hacks I'm open to it.
Suggestions how to do that anyone?

take care,
  Gerd