Message ID | 20210430113547.1816178-1-kraxel@redhat.com |
---|---|
Headers | show |
Series | virtio-gpu: split into two devices. | expand |
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 > > > >
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