| Message ID | 20210331031001.1564125-1-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for Blob resources feature | expand |
Patchew URL: https://patchew.org/QEMU/20210331031001.1564125-1-vivek.kasireddy@intel.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210331031001.1564125-1-vivek.kasireddy@intel.com
Subject: [PATCH 00/11] Add support for Blob resources feature
=== 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
* [new tag] patchew/20210331031001.1564125-1-vivek.kasireddy@intel.com -> patchew/20210331031001.1564125-1-vivek.kasireddy@intel.com
Switched to a new branch 'test'
b446fd9 virtio-gpu: Update cursor data using blob
3239ca7 virtio-gpu: Add virtio_gpu_set_scanout_blob
f52199e virtio-gpu: Add helpers to create and destroy dmabuf objects
1dcd341 virtio-gpu: Add virtio_gpu_resource_create_blob
6550dc7 virtio-gpu: Add initial definitions for blob resources
d394612 virtio-gpu: Refactor virtio_gpu_create_mapping_iov
372d98c virtio-gpu: Refactor virtio_gpu_set_scanout
43aa8d5 virtio-gpu: Add virtio_gpu_find_check_resource
4ebddba virtio-gpu: Add udmabuf helpers
969da42 ui/pixman: Add qemu_pixman_to_drm_format()
d33ad11 ui: Get the fd associated with udmabuf driver
=== OUTPUT BEGIN ===
1/11 Checking commit d33ad11ce69f (ui: Get the fd associated with udmabuf driver)
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?
#44:
new file mode 100644
total: 0 errors, 1 warnings, 54 lines checked
Patch 1/11 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/11 Checking commit 969da423dd95 (ui/pixman: Add qemu_pixman_to_drm_format())
3/11 Checking commit 4ebddba7f74b (virtio-gpu: Add udmabuf helpers)
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?
#39:
new file mode 100644
ERROR: use qemu_real_host_page_size instead of getpagesize()
#119: FILE: hw/display/virtio-gpu-udmabuf.c:76:
+ res->remapsz = QEMU_ALIGN_UP(res->remapsz, getpagesize());
ERROR: braces {} are necessary for all arms of this statement
#169: FILE: hw/display/virtio-gpu-udmabuf.c:126:
+ if (udmabuf < 0)
[...]
total: 2 errors, 1 warnings, 265 lines checked
Patch 3/11 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/11 Checking commit 43aa8d5eb5dd (virtio-gpu: Add virtio_gpu_find_check_resource)
5/11 Checking commit 372d98ca604a (virtio-gpu: Refactor virtio_gpu_set_scanout)
6/11 Checking commit d3946125fd9b (virtio-gpu: Refactor virtio_gpu_create_mapping_iov)
7/11 Checking commit 6550dc790eef (virtio-gpu: Add initial definitions for blob resources)
8/11 Checking commit 1dcd3417ed0f (virtio-gpu: Add virtio_gpu_resource_create_blob)
9/11 Checking commit f52199e64d16 (virtio-gpu: Add helpers to create and destroy dmabuf objects)
WARNING: line over 80 characters
#51: FILE: hw/display/virtio-gpu-udmabuf.c:195:
+ struct virtio_gpu_simple_resource *res,
total: 0 errors, 1 warnings, 131 lines checked
Patch 9/11 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/11 Checking commit 3239ca755628 (virtio-gpu: Add virtio_gpu_set_scanout_blob)
WARNING: line over 80 characters
#104: FILE: hw/display/virtio-gpu.c:744:
+ ss.r.width, ss.r.height, ss.r.x, ss.r.y);
total: 0 errors, 1 warnings, 155 lines checked
Patch 10/11 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/11 Checking commit b446fd91fd53 (virtio-gpu: Update cursor data using blob)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210331031001.1564125-1-vivek.kasireddy@intel.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Hi Gerd, Since the udmabuf and blob supports are on their way, can we reconsider the display-drm ui support which was list in https://git.kraxel.org/cgit/qemu/refs/heads as a branch before. I did some rebasing work and can help to send them out for review if possible. BR, Tina > -----Original Message----- > From: Kasireddy, Vivek <vivek.kasireddy@intel.com> > Sent: Wednesday, March 31, 2021 11:10 AM > To: qemu-devel@nongnu.org > Cc: Kasireddy, Vivek <vivek.kasireddy@intel.com>; Gerd Hoffmann > <kraxel@redhat.com>; Marc-André Lureau > <marcandre.lureau@redhat.com>; Kim, Dongwon > <dongwon.kim@intel.com>; Zhang, Tina <tina.zhang@intel.com> > Subject: [PATCH 00/11] Add support for Blob resources feature > > Enabling this feature would eliminate data copies from the resource object in > the Guest to the shadow resource in Qemu. This patch series however adds > support only for Blobs of type VIRTIO_GPU_BLOB_MEM_GUEST with > property VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE. > > Most of the patches in this series are a rebased, refactored and bugfixed > versions of Gerd Hoffmann's patches located here: > https://gitlab.freedesktop.org/virgl/qemu/-/commits/virtio-gpu-next > > TODO: > - Enable the combination virgl + blob resources > - Add support for VIRTGPU_BLOB_MEM_HOST3D blobs > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Cc: Dongwon Kim <dongwon.kim@intel.com> > Cc: Tina Zhang <tina.zhang@intel.com> > > Vivek Kasireddy (11): > ui: Get the fd associated with udmabuf driver > ui/pixman: Add qemu_pixman_to_drm_format() > virtio-gpu: Add udmabuf helpers > virtio-gpu: Add virtio_gpu_find_check_resource > virtio-gpu: Refactor virtio_gpu_set_scanout > virtio-gpu: Refactor virtio_gpu_create_mapping_iov > virtio-gpu: Add initial definitions for blob resources > virtio-gpu: Add virtio_gpu_resource_create_blob > virtio-gpu: Add helpers to create and destroy dmabuf objects > virtio-gpu: Add virtio_gpu_set_scanout_blob > virtio-gpu: Update cursor data using blob > > hw/display/meson.build | 2 +- > hw/display/trace-events | 2 + > hw/display/virtio-gpu-3d.c | 3 +- > hw/display/virtio-gpu-base.c | 3 + > hw/display/virtio-gpu-udmabuf.c | 276 +++++++++++++ > hw/display/virtio-gpu.c | 423 +++++++++++++++----- > include/hw/virtio/virtio-gpu-bswap.h | 16 + > include/hw/virtio/virtio-gpu.h | 41 +- > include/standard-headers/linux/udmabuf.h | 32 ++ > include/standard-headers/linux/virtio_gpu.h | 1 + > include/ui/console.h | 3 + > include/ui/qemu-pixman.h | 1 + > scripts/update-linux-headers.sh | 3 + > ui/meson.build | 1 + > ui/qemu-pixman.c | 35 +- > ui/udmabuf.c | 40 ++ > 16 files changed, 772 insertions(+), 110 deletions(-) create mode 100644 > hw/display/virtio-gpu-udmabuf.c create mode 100644 include/standard- > headers/linux/udmabuf.h > create mode 100644 ui/udmabuf.c > > -- > 2.26.2
Hi Gerd, While looking at the Qemu UI code, I noticed that there is a Blit operation performed to copy the Guest FB (texture) into a Host buffer before it is presented to the Host compositor. I was wondering if there are any elegant ways to eliminate this Blit to further the goal of absolute zero-copy. One idea that I am familiar with involves the usage of this extension: https://www.khronos.org/registry/EGL/extensions/WL/EGL_WL_create_wayland_buffer_from_image.txt However, I do realize that this is very Wayland specific but we are also going to need to add explicit sync support which is currently only available in upstream Weston. I did try adding explicit sync support to GTK but the maintainers are not particularly thrilled about it: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3410 Any other ideas as to how to eliminate that Blit cleanly? Thanks, Vivek > -----Original Message----- > From: Kasireddy, Vivek <vivek.kasireddy@intel.com> > Sent: Tuesday, March 30, 2021 8:10 PM > To: qemu-devel@nongnu.org > Cc: Kasireddy, Vivek <vivek.kasireddy@intel.com>; Gerd Hoffmann > <kraxel@redhat.com>; Marc-André Lureau <marcandre.lureau@redhat.com>; Kim, > Dongwon <dongwon.kim@intel.com>; Zhang, Tina <tina.zhang@intel.com> > Subject: [PATCH 00/11] Add support for Blob resources feature > > Enabling this feature would eliminate data copies from the resource object in the Guest to > the shadow resource in Qemu. This patch series however adds support only for Blobs of > type VIRTIO_GPU_BLOB_MEM_GUEST with property > VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE. > > Most of the patches in this series are a rebased, refactored and bugfixed versions of Gerd > Hoffmann's patches located here: > https://gitlab.freedesktop.org/virgl/qemu/-/commits/virtio-gpu-next > > TODO: > - Enable the combination virgl + blob resources > - Add support for VIRTGPU_BLOB_MEM_HOST3D blobs > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Cc: Dongwon Kim <dongwon.kim@intel.com> > Cc: Tina Zhang <tina.zhang@intel.com> > > Vivek Kasireddy (11): > ui: Get the fd associated with udmabuf driver > ui/pixman: Add qemu_pixman_to_drm_format() > virtio-gpu: Add udmabuf helpers > virtio-gpu: Add virtio_gpu_find_check_resource > virtio-gpu: Refactor virtio_gpu_set_scanout > virtio-gpu: Refactor virtio_gpu_create_mapping_iov > virtio-gpu: Add initial definitions for blob resources > virtio-gpu: Add virtio_gpu_resource_create_blob > virtio-gpu: Add helpers to create and destroy dmabuf objects > virtio-gpu: Add virtio_gpu_set_scanout_blob > virtio-gpu: Update cursor data using blob > > hw/display/meson.build | 2 +- > hw/display/trace-events | 2 + > hw/display/virtio-gpu-3d.c | 3 +- > hw/display/virtio-gpu-base.c | 3 + > hw/display/virtio-gpu-udmabuf.c | 276 +++++++++++++ > hw/display/virtio-gpu.c | 423 +++++++++++++++----- > include/hw/virtio/virtio-gpu-bswap.h | 16 + > include/hw/virtio/virtio-gpu.h | 41 +- > include/standard-headers/linux/udmabuf.h | 32 ++ > include/standard-headers/linux/virtio_gpu.h | 1 + > include/ui/console.h | 3 + > include/ui/qemu-pixman.h | 1 + > scripts/update-linux-headers.sh | 3 + > ui/meson.build | 1 + > ui/qemu-pixman.c | 35 +- > ui/udmabuf.c | 40 ++ > 16 files changed, 772 insertions(+), 110 deletions(-) create mode 100644 > hw/display/virtio-gpu-udmabuf.c create mode 100644 include/standard- > headers/linux/udmabuf.h > create mode 100644 ui/udmabuf.c > > -- > 2.26.2
Hi,
> Any other ideas as to how to eliminate that Blit cleanly?
Well, "cleanly" pretty much implies "supported by toolkit".
gtk glarea for example sets up a framebuffer and expects the application
render to that framebuffer. So qemu glarea code does a fb-to-fb blit.
Other reasons are scaling and cursor rendering. Not all reasons apply
to all UIs. I think when using spice qemu doesn't blit (not fully sure
what happens inside spice-server), but it could very well be that the
spice-client does the blit instead, i.e. we just shift the issue to
another place ...
take care,
Gerd
Hi Gerd, > > > Any other ideas as to how to eliminate that Blit cleanly? > > Well, "cleanly" pretty much implies "supported by toolkit". [Kasireddy, Vivek] I was kind of hoping you'd not draw that implication :) > > gtk glarea for example sets up a framebuffer and expects the application render to that > framebuffer. So qemu glarea code does a fb-to-fb blit. [Kasireddy, Vivek] Right, that is how it works today but we'd like to not have that blit and instead submit the Qemu application buffer (i.e Guest FB) directly to the compositor -- so that it can be placed on a hardware plane should the compositor decide to do so. Only then it'd be absolute zero-copy but the compositor may decide to composite it which would mean another blit that we cannot avoid. I am trying to make a concerted effort to accomplish this using GTK/GLArea: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3410 But I get a feeling that it is inadequate as GTK/GLArea does not manage the wl_buffers submitted to the compositor -- EGL does. I suspect we either need to use a new GTK mechanism -- that perhaps does not exist yet -- or not use GTK at all for this. I do understand that adding a new purely Wayland backend would make it redundant given that GTK, SDL, Spice, etc already support Wayland; however, I do not see any good options available for eliminating that blit. Thanks, Vivek > > Other reasons are scaling and cursor rendering. Not all reasons apply to all UIs. I think > when using spice qemu doesn't blit (not fully sure what happens inside spice-server), but it > could very well be that the spice-client does the blit instead, i.e. we just shift the issue to > another place ... > > take care, > Gerd
Hi, > But I get a feeling that it is inadequate as GTK/GLArea does not manage the wl_buffers > submitted to the compositor -- EGL does. I suspect we either need to use a new GTK > mechanism -- that perhaps does not exist yet -- or not use GTK at all for this. > > I do understand that adding a new purely Wayland backend would make it redundant given > that GTK, SDL, Spice, etc already support Wayland; however, I do not see any good options > available for eliminating that blit. Well, one idea is using dbus (discovery/setup) and pipewire (data streams) to allow other applications access the guest display (also audio, input, ...). Simliar to gnome exporting the wayland display that way for remote access and screen sharing. pipewire supports using dmabufs, so that should allow to decouple user interface code from qemu without making compromises on performance, which is probably quite useful for a number of use cases, like a zero-copy wayland client, or a client using drm directly. Right now pipewire support is at "idea" level, there isn't even a proof-of-concept for that. So it surely doesn't help short-term, but IMHO this makes alot of sense medium/long-term. Marc-André has experimental code for a dbus-based UI for qemu. It doesn't use pipewire as data transport though. At least the first version posted a while ago @ qemu-devel doesn't. take care, Gerd
Hi Gerd, > > I do understand that adding a new purely Wayland backend would make it > > redundant given that GTK, SDL, Spice, etc already support Wayland; > > however, I do not see any good options available for eliminating that blit. > > Well, one idea is using dbus (discovery/setup) and pipewire (data > streams) to allow other applications access the guest display (also audio, input, ...). > Simliar to gnome exporting the wayland display that way for remote access and screen > sharing. > > pipewire supports using dmabufs, so that should allow to decouple user interface code > from qemu without making compromises on performance, which is probably quite useful > for a number of use cases, like a zero-copy wayland client, or a client using drm directly. [Kasireddy, Vivek] We considered having a separate client but decided that adding the relevant pieces to Qemu UI would be sufficient. We also felt that the interaction between the client and Qemu for sharing the dmabuf (guest FB) would add to the overhead and exacerbate synchronization issues. And, maintaining and distributing such a client would probably not be prudent for us right now. > > Right now pipewire support is at "idea" level, there isn't even a proof-of-concept for that. > So it surely doesn't help short-term, but IMHO this makes alot of sense medium/long- > term. [Kasireddy, Vivek] Ok, we'll explore this idea further to see if it would work for our use-case. > > Marc-André has experimental code for a dbus-based UI for qemu. It doesn't use pipewire > as data transport though. At least the first version posted a while ago @ qemu-devel > doesn't. [Kasireddy, Vivek] What is the main motivation for a new dbus-based UI for Qemu? Also, would you be able to review the patches in this series soon? Thanks, Vivek > > take care, > Gerd
Hi, > > Marc-André has experimental code for a dbus-based UI for qemu. It doesn't use pipewire > > as data transport though. At least the first version posted a while ago @ qemu-devel > > doesn't. > [Kasireddy, Vivek] What is the main motivation for a new dbus-based UI for Qemu? Having UI and qemu run in separate processes has serveral advantages, for starters the VM is independent from the desktop session. Today you can use vnc or spice, the dbus UI should allow better performance and integration than that. If it works out well we maybe we can even drop the sdl/gtk UIs. > Also, would you be able to review the patches in this series soon? I expect there will be a v2 anyway due to the kernel-side changes? take care, Gerd