RE: [PATCH v2] target/i386/host-cpu: Use iommu phys_bits with VFIO assigned devices on Intel h/w
Hi Alex, Eric,
> > > Recent updates in OVMF and Seabios have resulted in MMIO regions
> > > being placed at the upper end of the physical address space. As a
> > > result, when a Host device is assigned to the Guest via VFIO, the
> > > following mapping failures occur when VFIO tries to map the MMIO
> > > regions of the device:
> > > VFIO_MAP_DMA failed: Invalid argument
> > > vfio_dma_map(0x557b2f2736d0, 0x3800, 0x100,
> 0x7f98ac40) = -22 (Invalid argument)
> > >
> > > The above failures are mainly seen on some Intel platforms where
> > > the physical address width is larger than the Host's IOMMU
> > > address width. In these cases, VFIO fails to map the MMIO regions
> > > because the IOVAs would be larger than the IOMMU aperture regions.
> > >
> > > Therefore, one way to solve this problem would be to ensure that
> > > cpu->phys_bits =
> > > This can be done by parsing the IOMMU caps value from sysfs and
> > > extracting the address width and using it to override the
> > > phys_bits value as shown in this patch.
> > >
> > > Previous attempt at solving this issue in OVMF:
> > > https://edk2.groups.io/g/devel/topic/102359124
> > >
> > > Cc: Gerd Hoffmann
> > > Cc: Philippe Mathieu-Daudé
> > > Cc: Alex Williamson
> > > Cc: Cédric Le Goater
> > > Cc: Laszlo Ersek
> > > Cc: Dongwon Kim
> > > Acked-by: Gerd Hoffmann
> > > Tested-by: Yanghang Liu
> > > Signed-off-by: Vivek Kasireddy
> > >
> > > ---
> > > v2:
> > > - Replace the term passthrough with assigned (Laszlo)
> > > - Update the commit message to note that both OVMF and Seabios
> > > guests are affected (Cédric)
> > > - Update the subject to indicate what is done in the patch
> > > ---
> > > target/i386/host-cpu.c | 61
> +-
> > > 1 file changed, 60 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> > > index 92ecb7254b..5c9fcd7dc2 100644
> > > --- a/target/i386/host-cpu.c
> > > +++ b/target/i386/host-cpu.c
> > > @@ -12,6 +12,8 @@
> > > #include "host-cpu.h"
> > > #include "qapi/error.h"
> > > #include "qemu/error-report.h"
> > > +#include "qemu/config-file.h"
> > > +#include "qemu/option.h"
> > > #include "sysemu/sysemu.h"
> > >
> > > /* Note: Only safe for use on x86(-64) hosts */
> > > @@ -51,11 +53,58 @@ static void host_cpu_enable_cpu_pm(X86CPU
> *cpu)
> > > env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> > > }
> > >
> > > +static int intel_iommu_check(void *opaque, QemuOpts *opts, Error
> **errp)
> > > +{
> > > +g_autofree char *dev_path = NULL, *iommu_path = NULL, *caps =
> NULL;
> > > +const char *driver = qemu_opt_get(opts, "driver");
> > > +const char *device = qemu_opt_get(opts, "host");
> > > +uint32_t *iommu_phys_bits = opaque;
> > > +struct stat st;
> > > +uint64_t iommu_caps;
> > > +
> > > +/*
> > > + * Check if the user requested VFIO device assignment. We don't have
> > > + * to limit phys_bits if there are no valid assigned devices.
> > > + */
> > > +if (g_strcmp0(driver, "vfio-pci") || !device) {
> > > +return 0;
> > > +}
> > > +
> > > +dev_path = g_strdup_printf("/sys/bus/pci/devices/%s", device);
> > > +if (stat(dev_path, ) < 0) {
> > > +return 0;
> > > +}
> > > +
> > > +iommu_path = g_strdup_printf("%s/iommu/intel-iommu/cap",
> dev_path);
> > > +if (stat(iommu_path, ) < 0) {
> > > +return 0;
> > > +}
> > > +
> > > +if (g_file_get_contents(iommu_path, , NULL, NULL)) {
> > > +if (sscanf(caps, "%lx", _caps) != 1) {
> > > +return 0;
> > > +}
> > > +*iommu_phys_bits = ((iommu_caps >> 16) & 0x3f) + 1;
> > > +}
> > > +
> > > +return 0;
> > > +}
> > > +
> > > +static uint32_t host_iommu_phys_bits(void)
> > > +{
> > > +uint32_t iommu_phys_bits = 0;
> > > +
> > > +qemu_opts_foreach(qemu_find_opts("device"),
> > > + intel_iommu_check, _phys_bits, NULL);
> > > +return iommu_phys_bits;
> > > +}
> > > +
> > > static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
> > > {
> > > uint32_t host_phys_bits = host_cpu_phys_bits();
> > > +uint32_t iommu_phys_bits = host_iommu_phys_bits();
> > > uint32_t phys_bits = cpu->phys_bits;
> > > -static bool warned;
> > > +static bool warned, warned2;
> > >
> > > /*
> > > * Print a warning if the user set it to a value that's not the
> > > @@ -78,6 +127,16 @@ static uint32_t
> host_cpu_adjust_phys_bits(X86CPU *cpu)
> > > }
> > > }
> > >
> > > +if (iommu_phys_bits && phys_bits > iommu_phys_bits) {
> > > +phys_bits = iommu_phys_bits;
> > are you allowed to change the host cpu characteristics without taking
> > care of compats for migration?
>
> Not only is migration an issue, but so is hotplug. Anything that
> aligns the VM configuration to the host is going to have migration
> issues and anything that does so conditionally based on the current
RE: [PATCH v1 1/7] ui/spice: Add an option for users to provide a preferred codec
Hi Marc-Andre,
>
> Hi
>
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> wrote:
> >
> > Giving users an option to choose a particular codec will enable
> > them to make an appropriate decision based on their hardware and
> > use-case.
> >
> > Cc: Gerd Hoffmann
> > Cc: Marc-André Lureau
> > Cc: Frediano Ziglio
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> >
> > ---
> > v2:
> > - Don't override the default Spice codec if preferred-codec is not
> > provided (Frediano)
> > ---
> > qemu-options.hx | 5 +
> > ui/spice-core.c | 12
> > 2 files changed, 17 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index b66570ae00..caaafe01d5 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2260,6 +2260,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
> > "
> > [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
> > " [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
> > " [,playback-compression=[on|off]][,seamless-
> migration=[on|off]]\n"
> > +" [,preferred-codec=:\n"
>
> The SPICE API is "spice_server_set_video_codecs()", let's name the
> option: "video-codecs" to avoid confusions.
I am ok with "video-codes" instead of "preferred-codec".
>
> > " [,gl=[on|off]][,rendernode=]\n"
> > "enable spice\n"
> > "at least one of {port, tls-port} is mandatory\n",
> > @@ -2348,6 +2349,10 @@ SRST
> > ``seamless-migration=[on|off]``
> > Enable/disable spice seamless migration. Default is off.
> >
> > +``preferred-codec=:``
> > +Provide the preferred codec the Spice server should use.
> > +Default would be spice:mjpeg.
>
> The SPICE API says:
> * @codecs: a codec string in the following format:
> encoder:codec;encoder:codec
>
> But the doc doesn't say whether the order is important, and doesn't
> give more details on the "encoder:codec" format.
>
> Also reading the code, it seems "auto" has a special meaning for
> default video codecs.
Although, I am using spice_server_set_video_codecs() API, my initial goal
with this option is for the user to specify one : entry
only. I guess it might be OK to have the user specify a list as long as Spice
picks
the first entry (aka preferred) and falls back to the next if it cannot create
or
use the encoder associated with the first entry.
>
> > +
> > ``gl=[on|off]``
> > Enable/disable OpenGL context. Default is off.
> >
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index db21db2c94..13bfbe4e89 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -488,6 +488,9 @@ static QemuOptsList qemu_spice_opts = {
> > },{
> > .name = "streaming-video",
> > .type = QEMU_OPT_STRING,
> > +},{
> > +.name = "preferred-codec",
> > +.type = QEMU_OPT_STRING,
> > },{
> > .name = "agent-mouse",
> > .type = QEMU_OPT_BOOL,
> > @@ -663,6 +666,7 @@ static void qemu_spice_init(void)
> > char *x509_key_file = NULL,
> > *x509_cert_file = NULL,
> > *x509_cacert_file = NULL;
> > +const char *preferred_codec = NULL;
> > int port, tls_port, addr_flags;
> > spice_image_compression_t compression;
> > spice_wan_compression_t wan_compr;
> > @@ -802,6 +806,14 @@ static void qemu_spice_init(void)
> > spice_server_set_streaming_video(spice_server,
> SPICE_STREAM_VIDEO_OFF);
> > }
> >
> > +preferred_codec = qemu_opt_get(opts, "preferred-codec");
> > +if (preferred_codec) {
> > +if (spice_server_set_video_codecs(spice_server, preferred_codec)) {
>
> Sadly, the API just returns 0 if one of the codec was accepted, not
> great if you want a specific set of codecs.
IIUC, although a user can specify a set of codecs, only one can by actively
used at
any given time (at-least with Gstreamer ones, not Spice codecs) with my
use-case (gl=on).
Thanks,
Vivek
>
> otherwise, lgtm
>
>
> > +error_report("Preferred codec name is not valid");
> > +exit(1);
> > +}
> > +}
> > +
> > spice_server_set_agent_mouse
> > (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
> > spice_server_set_playback_compression
> > --
> > 2.39.2
> >
> >
>
>
> --
> Marc-André Lureau
RE: [PATCH v1 3/7] ui/spice: Submit the gl_draw requests at 60 FPS for remote clients
Hi Marc-Andre,
>
> Hi
>
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> wrote:
> >
> > In the specific case where the display layer (virtio-gpu) is using
> > dmabuf, and if remote clients are enabled (-spice gl=on,port=),
> > it makes sense to limit the maximum (streaming) rate to 60 FPS
> > using the GUI timer. This matches the behavior of GTK UI where the
> > display updates are submitted at 60 FPS (assuming the underlying
> > mode is WxY@60).
>
> One of the issues with this approach is that the DMABUF isn't owned by
> the consumer. By delaying the usage of it, we risk having
> damaged/invalid updates.
This patch is only relevant with blob=true option. And, in this case, the
Guest (virtio-gpu kernel driver) is blocked (on a fence) until the Host
has consumed the buffer, which in this case happens after the encoder
signals the async to indicate that encoding is completed. Therefore,
AFAIU, there is no risk of missing or invalid updates. Ideally, the rate
should be driven by the consumer which in this case is the Spice client,
and given that it doesn't make sense to submit new frames faster than
the refresh rate, I figured it is ok to limit the producer to 60 FPS as well.
Note that Spice also does rate limiting based on network latencies and
dropped frames.
>
> It would be great if we could have a mechanism for double/triple
> buffering with virtio-gpu, as far as I know this is not possible yet.
Given that virtio-gpu is a drm/kms driver, there can only be one buffer
in flight at any given time. And, it doesn't make sense to submit frames
faster than the refresh rate as they would simply get dropped. However,
I tried to address this issue few years ago but it did not go anywhere:
https://lore.kernel.org/all/20211014124402.46f95ebc@eldfell/T/
>
> I wonder if setting dpy_set_ui_info() with the fixed refresh_rate is
> enough for the guest to have a fixed FPS.
I am not sure. Let me try to experiment with it and see how things work.
Thanks,
Vivek
> It will only work with gfx hw that support ui_info() though.
>
>
>
> >
> > Cc: Gerd Hoffmann
> > Cc: Marc-André Lureau
> > Cc: Frediano Ziglio
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > ---
> > ui/spice-display.c | 38 --
> > 1 file changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 384b8508d4..90c04623ec 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -841,12 +841,31 @@ static void qemu_spice_gl_block_timer(void
> *opaque)
> > warn_report("spice: no gl-draw-done within one second");
> > }
> >
> > +static void spice_gl_draw(SimpleSpiceDisplay *ssd,
> > + uint32_t x, uint32_t y, uint32_t w, uint32_t h)
> > +{
> > +uint64_t cookie;
> > +
> > +cookie =
> (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> > +spice_qxl_gl_draw_async(>qxl, x, y, w, h, cookie);
> > +}
> > +
> > static void spice_gl_refresh(DisplayChangeListener *dcl)
> > {
> > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > -uint64_t cookie;
> > +QemuDmaBuf *dmabuf = ssd->guest_dmabuf;
> >
> > -if (!ssd->ds || qemu_console_is_gl_blocked(ssd->dcl.con)) {
> > +if (!ssd->ds) {
> > +return;
> > +}
> > +
> > +if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
> > +if (remote_client && ssd->gl_updates && dmabuf) {
> > +spice_gl_draw(ssd, 0, 0, dmabuf->width, dmabuf->height);
> > +ssd->gl_updates = 0;
> > +/* To stream at 60 FPS, the (GUI) timer delay needs to be ~17
> > ms */
> > +dcl->update_interval = 1000 / (2 *
> GUI_REFRESH_INTERVAL_DEFAULT) + 1;
> > +}
> > return;
> > }
> >
> > @@ -854,11 +873,8 @@ static void spice_gl_refresh(DisplayChangeListener
> *dcl)
> > if (ssd->gl_updates && ssd->have_surface) {
> > qemu_spice_gl_block(ssd, true);
> > glFlush();
> > -cookie =
> (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> > -spice_qxl_gl_draw_async(>qxl, 0, 0,
> > -surface_width(ssd->ds),
> > -surface_height(ssd->ds),
> > -cookie);
> > +spice_gl_draw(ssd, 0, 0,
> > + surface_width(ssd->ds), surface_height(ssd->ds));
> > ssd->gl_updates = 0;
> > }
> > }
> > @@ -1025,7 +1041,6 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> > EGLint stride = 0, fourcc = 0;
> > bool render_cursor = false;
> > bool y_0_top = false; /* FIXME */
> > -uint64_t cookie;
> > int fd;
> >
> > if (!ssd->have_scanout) {
> > @@ -1098,8 +1113,11 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> > trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
> > qemu_spice_gl_block(ssd, true);
> > glFlush();
> > -cookie =
>
RE: [PATCH v1 2/7] ui/spice: Enable gl=on option for non-local or remote clients
Hi Marc-Andre,
>
> Hi
>
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> wrote:
> >
> > Newer versions of Spice server should be able to accept dmabuf
> > fds from Qemu for clients that are connected via the network.
> > In other words, when this option is enabled, Qemu would share
> > a dmabuf fd with Spice which would encode and send the data
> > associated with the fd to a client that could be located on
> > a different machine.
> >
> > Cc: Gerd Hoffmann
> > Cc: Marc-André Lureau
> > Cc: Frediano Ziglio
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > ---
> > include/ui/spice-display.h | 1 +
> > ui/spice-core.c| 4
> > ui/spice-display.c | 1 +
> > 3 files changed, 6 insertions(+)
> >
> > diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> > index e1a9b36185..f4922dd74b 100644
> > --- a/include/ui/spice-display.h
> > +++ b/include/ui/spice-display.h
> > @@ -151,6 +151,7 @@ struct SimpleSpiceCursor {
> > };
> >
> > extern bool spice_opengl;
> > +extern bool remote_client;
> >
> > int qemu_spice_rect_is_empty(const QXLRect* r);
> > void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 13bfbe4e89..3b9a54685f 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -849,9 +849,13 @@ static void qemu_spice_init(void)
> > #ifdef HAVE_SPICE_GL
> > if (qemu_opt_get_bool(opts, "gl", 0)) {
> > if ((port != 0) || (tls_port != 0)) {
> > +#if SPICE_SERVER_VERSION >= 0x000f03 /* release 0.15.3 */
>
> (ok, we should wait for the Spice series to be merged)
Sure, I'll post the v2 after the Spice series gets merged.
Thanks,
Vivek
>
>
> > +remote_client = 1;
> > +#else
> > error_report("SPICE GL support is local-only for now and "
> > "incompatible with -spice port/tls-port");
> > exit(1);
> > +#endif
> > }
> > egl_init(qemu_opt_get(opts, "rendernode"), DISPLAYGL_MODE_ON,
> _fatal);
> > spice_opengl = 1;
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 6eb98a5a5c..384b8508d4 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -29,6 +29,7 @@
> > #include "ui/spice-display.h"
> >
> > bool spice_opengl;
> > +bool remote_client;
> >
> > int qemu_spice_rect_is_empty(const QXLRect* r)
> > {
> > --
> > 2.39.2
> >
> >
>
>
> --
> Marc-André Lureau
RE: [PATCH v1 7/7] ui/spice: Create another texture with linear layout when gl=on is enabled
Hi Marc-Andre,
>
> Hi
>
> On Sat, Jan 20, 2024 at 4:54 AM Vivek Kasireddy
> wrote:
> >
> > Since most encoders (invoked by Spice) may not work with tiled memory
> > associated with a texture, we need to create another texture that has
> > linear memory layout and use that instead.
> >
> > Note that, there does not seem to be a direct way to indicate to the
> > GL implementation that a texture's backing memory needs to be linear.
> > Instead, we have to do it in a roundabout way where we first need to
> > create a tiled texture and obtain a dmabuf fd associated with it via
> > EGL. Next, we have to create a memory object by importing the dmabuf
> > fd created earlier and finally create a linear texture by using the
> > memory object as the texture storage mechanism.
> >
> > Cc: Gerd Hoffmann
> > Cc: Marc-André Lureau
> > Cc: Frediano Ziglio
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > ---
> > ui/spice-display.c | 33 +
> > 1 file changed, 33 insertions(+)
> >
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 08b4aec921..94cb378dbe 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -893,6 +893,7 @@ static void spice_gl_switch(DisplayChangeListener
> *dcl,
> > {
> > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > EGLint stride, fourcc;
> > +GLuint texture = 0;
> > int fd;
> >
> > if (ssd->ds) {
> > @@ -912,6 +913,38 @@ static void spice_gl_switch(DisplayChangeListener
> *dcl,
> > return;
> > }
> >
> > +if (remote_client && surface_format(ssd->ds) != PIXMAN_r5g6b5) {
>
> hmm
>
> > +/*
> > + * We really want to ensure that the memory layout of the
> > texture
> > + * is linear; otherwise, the encoder's output may show
> > corruption.
> > + */
> > +surface_gl_create_texture_from_fd(ssd->ds, fd, );
>
> What if the encoder actually supports tiled layout?
Right, that would be true in most cases as the Render and Encode hardware are
mostly compatible. And, my patches are not required in this case if Spice
chooses
a hardware encoder. However, the choice of which encoder to use (hardware vs
software) is decided dynamically and is internal to Spice at this point. And,
since
there is no easy way to know from Qemu whether an encoder chosen by Spice
would support any given tiling format, I chose to always use linear layout
since it
is guaranteed to work with all types of encoders.
>
> Shouldn't this conversion be done at the encoder level as necessary?
Yeah, that is probably the right place but a software encoder like x264enc is
not
going to know how to do the conversion from various tiled formats. This is the
specific case I am trying to address given that it is not a problem with
hardware
encoders. I guess we could add a Qemu ui/spice option to tweak this behavior
while launching the VM.
>
> It's also strange to reuse an FD associated with a tiled texture for a
> linear layout (I am uncomfortable with all this tbh)
I have looked around but there doesn't seem to be a way for requesting the GL
implementation to create a texture with linear layout other than via importing
memory objects. As noted in the comments below, the fd's ownership is taken
over by the GL implementation as part of the import.
Also, I have started a conversation to figure out if there is any other way to
create linear textures more efficiently:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27067
>
> > +
> > +/*
> > + * A successful return after glImportMemoryFdEXT() means that
> > + * the ownership of fd has been passed to GL. In other words,
> > + * the fd we got above should not be used anymore.
> > + */
> > +if (texture > 0) {
> > +fd = egl_get_fd_for_texture(texture,
> > +, ,
> > +NULL);
> > +if (fd < 0) {
>
> I suggest adding warnings or tracing, to help debug issues...
Sure, will do that in v2.
>
> > +glDeleteTextures(1, );
> > +fd = egl_get_fd_for_texture(ssd->ds->texture,
> > +, ,
> > +NULL);
> > +if (fd < 0) {
> > +surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > +return;
> > +}
> > +} else {
> > +surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > +ssd->ds->texture = texture;
>
> Have you tried this series with virgl? (I doubt the renderer accepts
> that the scanout texture is changed)
I have tried with virgl enabled and it mostly works because my patches
don't affect virgl codepaths such as
RE: [PATCH 1/3] ui/gtk: flush display pipeline before saving vmstate when blob=true
Hi,
>
> On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim
> wrote:
> >
> > If the guest state is paused before it gets a response for the current
> > scanout frame submission (resource-flush), it won't flush new frames
> > after being restored as it still waits for the old response, which is
> > accepted as a scanout render done signal. So it's needed to unblock
> > the current scanout render pipeline before the run state is changed
> > to make sure the guest receives the response for the current frame
> > submission.
> >
> > v2: Giving some time for the fence to be signaled before flushing
> > the pipeline
> >
> > Cc: Marc-André Lureau
> > Cc: Vivek Kasireddy
> > Signed-off-by: Dongwon Kim
> > ---
> > ui/gtk.c | 19 +++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..ea8d07833e 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -678,6 +678,25 @@ static const DisplayGLCtxOps egl_ctx_ops = {
> > static void gd_change_runstate(void *opaque, bool running, RunState
> state)
> > {
> > GtkDisplayState *s = opaque;
> > +int i;
> > +
> > +if (state == RUN_STATE_SAVE_VM) {
> > +for (i = 0; i < s->nb_vcs; i++) {
> > +VirtualConsole *vc = >vc[i];
> > +
> > +if (vc->gfx.guest_fb.dmabuf &&
> > +vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
> > +eglClientWaitSync(qemu_egl_display,
> > + vc->gfx.guest_fb.dmabuf->sync,
> > + EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
> > + 1);
>
> This won't work. dmabuf->sync is NULL after egl_dmabuf_create_sync.
Right, we destroy the sync object after we create the fence from it. If you
want to use eglClientWaitSync() here, you either need to recreate the sync
object using fence_fd or don't destroy it in egl_dmabuf_create_fence().
Either way should be ok but make sure you destroy it when the fence_fd
is closed.
Thanks,
Vivek
>
> I will let Vivek, who wrote the sync code, comment.
>
> thanks
>
>
>
> --
> Marc-André Lureau
RE: [PATCH v1] target/i386/host-cpu: Use IOMMU addr width for passthrough devices on Intel platforms
Hi Laszlo,
>
> On 11/13/23 08:32, Vivek Kasireddy wrote:
> > A recent OVMF update has resulted in MMIO regions being placed at
> > the upper end of the physical address space. As a result, when a
> > Host device is passthrough'd to the Guest via VFIO, the following
> > mapping failures occur when VFIO tries to map the MMIO regions of
> > the device:
> > VFIO_MAP_DMA failed: Invalid argument
> > vfio_dma_map(0x557b2f2736d0, 0x3800, 0x100,
> 0x7f98ac40) = -22 (Invalid argument)
> >
> > The above failures are mainly seen on some Intel platforms where
> > the physical address width is larger than the Host's IOMMU
> > address width. In these cases, VFIO fails to map the MMIO regions
> > because the IOVAs would be larger than the IOMMU aperture regions.
> >
> > Therefore, one way to solve this problem would be to ensure that
> > cpu->phys_bits =
> > This can be done by parsing the IOMMU caps value from sysfs and
> > extracting the address width and using it to override the
> > phys_bits value as shown in this patch.
> >
> > Previous attempt at solving this issue in OVMF:
> > https://edk2.groups.io/g/devel/topic/102359124
> >
> > Cc: Gerd Hoffmann
> > Cc: Philippe Mathieu-Daudé
> > Cc: Alex Williamson
> > Cc: Laszlo Ersek
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > ---
> > target/i386/host-cpu.c | 61
> +-
> > 1 file changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> > index 92ecb7254b..8326ec95bc 100644
> > --- a/target/i386/host-cpu.c
> > +++ b/target/i386/host-cpu.c
> > @@ -12,6 +12,8 @@
> > #include "host-cpu.h"
> > #include "qapi/error.h"
> > #include "qemu/error-report.h"
> > +#include "qemu/config-file.h"
> > +#include "qemu/option.h"
> > #include "sysemu/sysemu.h"
> >
> > /* Note: Only safe for use on x86(-64) hosts */
> > @@ -51,11 +53,58 @@ static void host_cpu_enable_cpu_pm(X86CPU
> *cpu)
> > env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> > }
> >
> > +static int intel_iommu_check(void *opaque, QemuOpts *opts, Error
> **errp)
> > +{
> > +g_autofree char *dev_path = NULL, *iommu_path = NULL, *caps = NULL;
> > +const char *driver = qemu_opt_get(opts, "driver");
> > +const char *device = qemu_opt_get(opts, "host");
> > +uint32_t *iommu_phys_bits = opaque;
> > +struct stat st;
> > +uint64_t iommu_caps;
> > +
> > +/*
> > + * Check if the user is passthroughing any devices via VFIO. We don't
> > + * have to limit phys_bits if there are no valid passthrough devices.
> > + */
> > +if (g_strcmp0(driver, "vfio-pci") || !device) {
> > +return 0;
> > +}
> > +
> > +dev_path = g_strdup_printf("/sys/bus/pci/devices/%s", device);
> > +if (stat(dev_path, ) < 0) {
> > +return 0;
> > +}
> > +
> > +iommu_path = g_strdup_printf("%s/iommu/intel-iommu/cap",
> dev_path);
> > +if (stat(iommu_path, ) < 0) {
> > +return 0;
> > +}
> > +
> > +if (g_file_get_contents(iommu_path, , NULL, NULL)) {
> > +if (sscanf(caps, "%lx", _caps) != 1) {
> > +return 0;
> > +}
> > +*iommu_phys_bits = ((iommu_caps >> 16) & 0x3f) + 1;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static uint32_t host_iommu_phys_bits(void)
> > +{
> > +uint32_t iommu_phys_bits = 0;
> > +
> > +qemu_opts_foreach(qemu_find_opts("device"),
> > + intel_iommu_check, _phys_bits, NULL);
> > +return iommu_phys_bits;
> > +}
> > +
> > static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
> > {
> > uint32_t host_phys_bits = host_cpu_phys_bits();
> > +uint32_t iommu_phys_bits = host_iommu_phys_bits();
> > uint32_t phys_bits = cpu->phys_bits;
> > -static bool warned;
> > +static bool warned, warned2;
> >
> > /*
> > * Print a warning if the user set it to a value that's not the
> > @@ -78,6 +127,16 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU
> *cpu)
> > }
> > }
> >
> > +if (iommu_phys_bits && phys_bits > iommu_phys_bits) {
> > +phys_bits = iommu_phys_bits;
> > +if (!warned2) {
> > +warn_report("Using physical bits (%u)"
> > +" to prevent VFIO mapping failures",
> > +iommu_phys_bits);
> > +warned2 = true;
> > +}
> > +}
> > +
> > return phys_bits;
> > }
> >
>
> I only have very superficial comments here (sorry about that -- I find
> it too bad that this QEMU source file seems to have no designated
> reviewer or maintainer in QEMU, so I don't want to ignore it).
>
> - Terminology: I think we like to call these devices "assigned", and not
> "passed through". Also, in noun form, "device assignment" and not
> "device passthrough". Sorry about being pedantic.
No problem; I'll try to start using the right terminology.
>
> - As I (may have) mentioned in my OVMF comments, I'm unsure if narrowing
>
RE: [PATCH] udmabuf: revert 'Add support for mapping hugepages (v4)'
Hi David, > > On 13.06.23 10:26, Kasireddy, Vivek wrote: > > Hi David, > > > >> > >> On 12.06.23 09:10, Kasireddy, Vivek wrote: > >>> Hi Mike, > >> > >> Hi Vivek, > >> > >>> > >>> Sorry for the late reply; I just got back from vacation. > >>> If it is unsafe to directly use the subpages of a hugetlb page, then > reverting > >>> this patch seems like the only option for addressing this issue > immediately. > >>> So, this patch is > >>> Acked-by: Vivek Kasireddy > >>> > >>> As far as the use-case is concerned, there are two main users of the > >> udmabuf > >>> driver: Qemu and CrosVM VMMs. However, it appears Qemu is the only > >> one > >>> that uses hugetlb pages (when hugetlb=on is set) as the backing store for > >>> Guest (Linux, Android and Windows) system memory. The main goal is > to > >>> share the pages associated with the Guest allocated framebuffer (FB) > with > >>> the Host GPU driver and other components in a zero-copy way. To that > >> end, > >>> the guest GPU driver (virtio-gpu) allocates 4k size pages (associated with > >>> the FB) and pins them before sharing the (guest) physical (or dma) > >> addresses > >>> (and lengths) with Qemu. Qemu then translates the addresses into file > >>> offsets and shares these offsets with udmabuf. > >> > >> Is my understanding correct, that we can effectively long-term pin > >> (worse than mlock) 64 MiB per UDMABUF_CREATE, allowing eventually > !root > > The 64 MiB limit is the theoretical upper bound that we have not seen hit in > > practice. Typically, for a 1920x1080 resolution (commonly used in Guests), > > the size of the FB is ~8 MB (1920x1080x4). And, most modern Graphics > > compositors flip between two FBs. > > > > Okay, but users with privileges to open that file can just create as > many as they want? I think I'll have to play with it. Yeah, unfortunately, we are not restricting the total number of FBs or other buffers that are mapped by udambuf per user. We'll definitely try to add a patch to align it with mlock limits. > > >> users > >> > >> ll /dev/udmabuf > >> crw-rw 1 root kvm 10, 125 12. Jun 08:12 /dev/udmabuf > >> > >> to bypass there effective MEMLOCK limit, fragmenting physical memory > and > >> breaking swap? > > Right, it does not look like the mlock limits are honored. > > > > That should be added. > > >> > >> Regarding the udmabuf_vm_fault(), I assume we're mapping pages we > >> obtained from the memfd ourselves into a special VMA (mmap() of the > > mmap operation is really needed only if any component on the Host needs > > CPU access to the buffer. But in most scenarios, we try to ensure direct GPU > > access (h/w acceleration via gl) to these pages. > > > >> udmabuf). I'm not sure how well shmem pages are prepared for getting > >> mapped by someone else into an arbitrary VMA (page->index?). > > Most drm/gpu drivers use shmem pages as the backing store for FBs and > > other buffers and also provide mmap capability. What concerns do you see > > with this approach? > > Are these mmaping the pages the way udmabuf maps these pages (IOW, > on-demand fault where we core-mm will adjust the mapcount etc)? > > Skimming over at shmem_read_mapping_page() users, I assume most of > them > use a VM_PFNMAP mapping (or don't mmap them at all), where we won't be > messing with the struct page at all. > > (That might even allow you to mmap hugetlb sub-pages, because the struct > page -- and mapcount -- will be ignored completely and not touched.) Oh, are you suggesting that if we do vma->vm_flags |= VM_PFNMAP in the mmap handler (mmap_udmabuf) and also do vmf_insert_pfn(vma, vmf->address, page_to_pfn(page)) instead of vmf->page = ubuf->pages[pgoff]; get_page(vmf->page); in the vma fault handler (udmabuf_vm_fault), we can avoid most of the pitfalls you have identified -- including with the usage of hugetlb subpages? > > > > >> > >> ... also, just imagine someone doing FALLOC_FL_PUNCH_HOLE / > ftruncate() > >> on the memfd. What's mapped into the memfd no longer corresponds to > >> what's pinned / mapped into the VMA. > > IIUC, making use of the DMA_BUF_IOCTL_SYNC ioctl would help with any > > coherency issues: > > https://www.kernel.org/doc/html/v6.2/driver-api/dma- > buf.html#c.dma_buf_sync > > > > Would it as of now? udmabuf_create() pul
RE: [PATCH] udmabuf: revert 'Add support for mapping hugepages (v4)'
Hi David, > > On 12.06.23 09:10, Kasireddy, Vivek wrote: > > Hi Mike, > > Hi Vivek, > > > > > Sorry for the late reply; I just got back from vacation. > > If it is unsafe to directly use the subpages of a hugetlb page, then > > reverting > > this patch seems like the only option for addressing this issue immediately. > > So, this patch is > > Acked-by: Vivek Kasireddy > > > > As far as the use-case is concerned, there are two main users of the > udmabuf > > driver: Qemu and CrosVM VMMs. However, it appears Qemu is the only > one > > that uses hugetlb pages (when hugetlb=on is set) as the backing store for > > Guest (Linux, Android and Windows) system memory. The main goal is to > > share the pages associated with the Guest allocated framebuffer (FB) with > > the Host GPU driver and other components in a zero-copy way. To that > end, > > the guest GPU driver (virtio-gpu) allocates 4k size pages (associated with > > the FB) and pins them before sharing the (guest) physical (or dma) > addresses > > (and lengths) with Qemu. Qemu then translates the addresses into file > > offsets and shares these offsets with udmabuf. > > Is my understanding correct, that we can effectively long-term pin > (worse than mlock) 64 MiB per UDMABUF_CREATE, allowing eventually !root The 64 MiB limit is the theoretical upper bound that we have not seen hit in practice. Typically, for a 1920x1080 resolution (commonly used in Guests), the size of the FB is ~8 MB (1920x1080x4). And, most modern Graphics compositors flip between two FBs. > users > > ll /dev/udmabuf > crw-rw 1 root kvm 10, 125 12. Jun 08:12 /dev/udmabuf > > to bypass there effective MEMLOCK limit, fragmenting physical memory and > breaking swap? Right, it does not look like the mlock limits are honored. > > Regarding the udmabuf_vm_fault(), I assume we're mapping pages we > obtained from the memfd ourselves into a special VMA (mmap() of the mmap operation is really needed only if any component on the Host needs CPU access to the buffer. But in most scenarios, we try to ensure direct GPU access (h/w acceleration via gl) to these pages. > udmabuf). I'm not sure how well shmem pages are prepared for getting > mapped by someone else into an arbitrary VMA (page->index?). Most drm/gpu drivers use shmem pages as the backing store for FBs and other buffers and also provide mmap capability. What concerns do you see with this approach? > > ... also, just imagine someone doing FALLOC_FL_PUNCH_HOLE / ftruncate() > on the memfd. What's mapped into the memfd no longer corresponds to > what's pinned / mapped into the VMA. IIUC, making use of the DMA_BUF_IOCTL_SYNC ioctl would help with any coherency issues: https://www.kernel.org/doc/html/v6.2/driver-api/dma-buf.html#c.dma_buf_sync > > > Was linux-mm (and especially shmem maintainers, ccing Hugh) involved in > the upstreaming of udmabuf? It does not appear so from the link below although other key lists were cc'd: https://patchwork.freedesktop.org/patch/246100/?series=39879=7 Thanks, Vivek > > -- > Cheers, > > David / dhildenb
RE: [PATCH] udmabuf: revert 'Add support for mapping hugepages (v4)'
Hi Mike,
Sorry for the late reply; I just got back from vacation.
If it is unsafe to directly use the subpages of a hugetlb page, then reverting
this patch seems like the only option for addressing this issue immediately.
So, this patch is
Acked-by: Vivek Kasireddy
As far as the use-case is concerned, there are two main users of the udmabuf
driver: Qemu and CrosVM VMMs. However, it appears Qemu is the only one
that uses hugetlb pages (when hugetlb=on is set) as the backing store for
Guest (Linux, Android and Windows) system memory. The main goal is to
share the pages associated with the Guest allocated framebuffer (FB) with
the Host GPU driver and other components in a zero-copy way. To that end,
the guest GPU driver (virtio-gpu) allocates 4k size pages (associated with
the FB) and pins them before sharing the (guest) physical (or dma) addresses
(and lengths) with Qemu. Qemu then translates the addresses into file
offsets and shares these offsets with udmabuf.
The udmabuf driver obtains the pages associated with the file offsets and
uses these pages to eventually populate a scatterlist. It also creates a
dmabuf fd and acts as the exporter. AFAIK, it should be possible to populate
the scatterlist with physical/dma addresses (of huge pages) instead of using
subpages but this might limit the capabilities of some (dmabuf) importers.
I'll try to figure out a solution using physical/dma addresses and see if it
works as expected, and will share the patch on linux-mm to request
feedback once it is ready.
Thanks,
Vivek
>
> This effectively reverts commit 16c243e99d33 ("udmabuf: Add support
> for mapping hugepages (v4)"). Recently, Junxiao Chang found a BUG
> with page map counting as described here [1]. This issue pointed out
> that the udmabuf driver was making direct use of subpages of hugetlb
> pages. This is not a good idea, and no other mm code attempts such use.
> In addition to the mapcount issue, this also causes issues with hugetlb
> vmemmap optimization and page poisoning.
>
> For now, remove hugetlb support.
>
> If udmabuf wants to be used on hugetlb mappings, it should be changed to
> only use complete hugetlb pages. This will require different alignment
> and size requirements on the UDMABUF_CREATE API.
>
> [1] https://lore.kernel.org/linux-mm/20230512072036.1027784-1-
> junxiao.ch...@intel.com/
>
> Fixes: 16c243e99d33 ("udmabuf: Add support for mapping hugepages (v4)")
> Cc:
> Signed-off-by: Mike Kravetz
> ---
> drivers/dma-buf/udmabuf.c | 47 +--
> 1 file changed, 6 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 01f2e86f3f7c..12cf6bb2e3ce 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -12,7 +12,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
>
> @@ -207,9 +206,7 @@ static long udmabuf_create(struct miscdevice
> *device,
> struct udmabuf *ubuf;
> struct dma_buf *buf;
> pgoff_t pgoff, pgcnt, pgidx, pgbuf = 0, pglimit;
> - struct page *page, *hpage = NULL;
> - pgoff_t subpgoff, maxsubpgs;
> - struct hstate *hpstate;
> + struct page *page;
> int seals, ret = -EINVAL;
> u32 i, flags;
>
> @@ -245,7 +242,7 @@ static long udmabuf_create(struct miscdevice
> *device,
> if (!memfd)
> goto err;
> mapping = memfd->f_mapping;
> - if (!shmem_mapping(mapping) &&
> !is_file_hugepages(memfd))
> + if (!shmem_mapping(mapping))
> goto err;
> seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
> if (seals == -EINVAL)
> @@ -256,48 +253,16 @@ static long udmabuf_create(struct miscdevice
> *device,
> goto err;
> pgoff = list[i].offset >> PAGE_SHIFT;
> pgcnt = list[i].size >> PAGE_SHIFT;
> - if (is_file_hugepages(memfd)) {
> - hpstate = hstate_file(memfd);
> - pgoff = list[i].offset >> huge_page_shift(hpstate);
> - subpgoff = (list[i].offset &
> - ~huge_page_mask(hpstate)) >>
> PAGE_SHIFT;
> - maxsubpgs = huge_page_size(hpstate) >>
> PAGE_SHIFT;
> - }
> for (pgidx = 0; pgidx < pgcnt; pgidx++) {
> - if (is_file_hugepages(memfd)) {
> - if (!hpage) {
> - hpage =
> find_get_page_flags(mapping, pgoff,
> -
> FGP_ACCESSED);
> - if (!hpage) {
> - ret = -EINVAL;
> - goto err;
> - }
> - }
> - page = hpage + subpgoff;
> - get_page(page);
> -
RE: [PATCH 1/2] ui/gtk: use widget size for cursor motion event
Hi Erico,
> >
> >>
> >> The gd_motion_event size has some calculations for the cursor position,
> >> which also take into account things like different size of the
> >> framebuffer compared to the window size.
> >> The use of window size makes things more difficult though, as at least
> >> in the case of Wayland includes the size of ui elements like a menu bar
> >> at the top of the window. This leads to a wrong position calculation by
> >> a few pixels.
> >> Fix it by using the size of the widget, which already returns the size
> >> of the actual space to render the framebuffer.
> >>
> >> Signed-off-by: Erico Nunes
> >> ---
> >> ui/gtk.c | 8 +++-
> >> 1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/ui/gtk.c b/ui/gtk.c
> >> index fd82e9b1ca..d1b2a80c2b 100644
> >> --- a/ui/gtk.c
> >> +++ b/ui/gtk.c
> >> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
> >> GdkEventMotion *motion,
> >> {
> >> VirtualConsole *vc = opaque;
> >> GtkDisplayState *s = vc->s;
> >> -GdkWindow *window;
> >> int x, y;
> >> int mx, my;
> >> int fbh, fbw;
> >> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget
> *widget,
> >> GdkEventMotion *motion,
> >> fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
> >> fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
> >>
> >> -window = gtk_widget_get_window(vc->gfx.drawing_area);
> >> -ww = gdk_window_get_width(window);
> >> -wh = gdk_window_get_height(window);
> >> -ws = gdk_window_get_scale_factor(window);
> >> +ww = gtk_widget_get_allocated_width(widget);
> >> +wh = gtk_widget_get_allocated_height(widget);
> > [Kasireddy, Vivek] Could you please confirm if this works in X-based
> > compositor
> > environments as well? Last time I checked (with Fedora 36 and Gnome + X),
> > the
> > get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
> > I restricted the above change to Wayland-based environments only:
> > https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html
>
> Yes, I tested again and it seems to work fine for me even with the gtk
> ui running on X. I'm using Fedora 37.
[Kasireddy, Vivek] Ok, in that case, this patch is
Acked-by: Vivek Kasireddy
>
> I was not aware of that patch series though and just spent some time
> debugging these ui issues. It looks like your series was missed?
[Kasireddy, Vivek] Yeah, not sure why my series was missed but in
retrospect, I probably should have separated out bug fix patches
from new feature enablement patches.
>
> I'm still debugging additional issues with cursor position calculation,
> especially in wayland environments (and in particular with
> vhost-user-gpu now). Do those patches address more cursor issues?
[Kasireddy, Vivek] They do address more cursor issues but not sure how
helpful they would be for you as most of them deal with relative mode +
Wayland environment. However, there is another one that deals with
cursor/pointer in absolute mode + multiple monitors:
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03097.html
Thanks,
Vivek
>
> Thank you
>
> Erico
>
RE: [PATCH 1/2] ui/gtk: use widget size for cursor motion event
Hi Erico,
>
> The gd_motion_event size has some calculations for the cursor position,
> which also take into account things like different size of the
> framebuffer compared to the window size.
> The use of window size makes things more difficult though, as at least
> in the case of Wayland includes the size of ui elements like a menu bar
> at the top of the window. This leads to a wrong position calculation by
> a few pixels.
> Fix it by using the size of the widget, which already returns the size
> of the actual space to render the framebuffer.
>
> Signed-off-by: Erico Nunes
> ---
> ui/gtk.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index fd82e9b1ca..d1b2a80c2b 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
> GdkEventMotion *motion,
> {
> VirtualConsole *vc = opaque;
> GtkDisplayState *s = vc->s;
> -GdkWindow *window;
> int x, y;
> int mx, my;
> int fbh, fbw;
> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget *widget,
> GdkEventMotion *motion,
> fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
> fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
>
> -window = gtk_widget_get_window(vc->gfx.drawing_area);
> -ww = gdk_window_get_width(window);
> -wh = gdk_window_get_height(window);
> -ws = gdk_window_get_scale_factor(window);
> +ww = gtk_widget_get_allocated_width(widget);
> +wh = gtk_widget_get_allocated_height(widget);
[Kasireddy, Vivek] Could you please confirm if this works in X-based compositor
environments as well? Last time I checked (with Fedora 36 and Gnome + X), the
get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
I restricted the above change to Wayland-based environments only:
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html
Thanks,
Vivek
> +ws = gtk_widget_get_scale_factor(widget);
>
> mx = my = 0;
> if (ww > fbw) {
> --
> 2.39.2
>
RE: [RFC v2 2/2] spice: Add an option to forward the dmabuf directly to the encoder (v2)
Hi Frediano, Gerd,
>
> Il giorno mar 24 gen 2023 alle ore 06:41 Kasireddy, Vivek
> ha scritto:
> >
> > + Frediano
> >
> > Hi Gerd,
> >
> > >
> > > Hi,
> > >
> > > > Here is the flow of things from the Qemu side:
> > > > - Call gl_scanout (to update the fd) and gl_draw_async just like
> > > > in the local display case.
> > >
> > > Ok.
> > >
> > > > - Additionally, create an update with the cmd set to QXL_CMD_DRAW
> > > > to trigger the creation of a new drawable (associated with the fd)
> > > > by the Spice server.
> > > > - Wait (or block) until the Encoder is done encoding the content.
> > > > - Unblock the pipeline once the async completion cookie is received.
> > >
> > > Care to explain? For qemu it should make a difference what spice-server
> > > does with the dma-bufs passed (local display / encode video + send to
> > > remote).
> > [Kasireddy, Vivek] I agree that Qemu shouldn't care what the spice-server
> > does
> > with the dmabuf fds but somehow a drawable has to be created in the remote
> client
> > case. This is needed as most of the core functions in the server
> > (associated with
> > display-channel, video-stream, encoder, etc) operate on drawables.
> > Therefore, I
> > figured since Qemu already tells the server to create a drawable in the
> > non-gl
> case
> > (by creating an update that includes a QXL_CMD_DRAW cmd), the same thing
> > can be done in the gl + remote client case as well.
> >
>
> This is a hack. It's combining an invalid command in order to cause
> some side effects on spice server but it also needs a change in spice
> server to detect and handle this hack. QXL_CMD_DRAW is mainly bound to
> 2D commands and should come with valid bitmap data.
>
> > Alternatively, we could make the server create a drawable as a response to
> gl_scanout,
> > when it detects a remote client. IIUC, I think this can be done but seems
> > rather
> messy
> > given that currently, the server only creates a drawable (inside
> red_process_display)
> > in the case of QXL_CMD_DRAW sent by Qemu/applications:
> > switch (ext_cmd.cmd.type) {
> > case QXL_CMD_DRAW: {
> > auto red_drawable = red_drawable_new(worker->qxl,
> >mem_slots,
> > ext_cmd.group_id,
> > ext_cmd.cmd.data,
> > ext_cmd.flags); // returns
> > with 1 ref
> >
> > if (red_drawable) {
> > display_channel_process_draw(worker->display_channel,
> std::move(red_drawable),
> >
> > worker->process_display_generation);
> > }
> >
>
> Yes, it sounds a bit long but surely better than hacking Qemu, spice
> server and defining a new hacky ABI that will be hard to maintain and
> understand.
>
> > The other option I can think of is to just not deal with drawables at all
> > and
> somehow
> > directly share the dmabuf fd with the Encoder. This solution also seems very
> messy
> > and invasive to me as we'd not be able to leverage the existing APIs (in
> > display-
> channel,
> > video-stream, etc) that create and manage streams efficiently.
> >
>
> Yes, that currently seems pretty long as a change but possibly the
> most clean in future, it's up to some refactory. The Encoder does not
> either need technically a RedDrawable, Drawable but frame data encoded
> in a format it can manage (either raw memory data or dmabuf at the
> moment).
[Kasireddy, Vivek] Ok, I'll work on refactoring Spice server code to pass
the dmabuf fd directly to the Encoder without having to creating drawables.
Thanks,
Vivek
>
> > >
> > > > #ifdef HAVE_SPICE_GL
> > > > +} else if (spice_dmabuf_encode) {
> > > > +if (g_strcmp0(preferred_codec, "gstreamer:h264")) {
> > > > +error_report("dmabuf-encode=on currently only works
> > > > and tested"
> > > > + "with gstreamer:h264");
> > > > +exit(1);
> > > > +}
> > >
> > > IMHO we should not hard-code todays spice-server capabilities like this.
> > > For starters this isn't true for spice-server versions which don't (yet)
> > > have your patches. Also the capability might depend on hardware
> > > support. IMHO we need some feature negotiation between qemu and spice
> > > here.
> > [Kasireddy, Vivek] Ok, I can get rid of this chunk in v3. However, given the
> numerous
> > features supported by the Spice server, I suspect implementing feature
> negotiation
> > might get really challenging. Is there any other way around this that you
> > can
> think of?
> >
> > Thanks,
> > Vivek
> >
> > >
> > > take care,
> > > Gerd
> >
>
> Frediano
RE: [RFC v2 2/2] spice: Add an option to forward the dmabuf directly to the encoder (v2)
+ Frediano
Hi Gerd,
>
> Hi,
>
> > Here is the flow of things from the Qemu side:
> > - Call gl_scanout (to update the fd) and gl_draw_async just like
> > in the local display case.
>
> Ok.
>
> > - Additionally, create an update with the cmd set to QXL_CMD_DRAW
> > to trigger the creation of a new drawable (associated with the fd)
> > by the Spice server.
> > - Wait (or block) until the Encoder is done encoding the content.
> > - Unblock the pipeline once the async completion cookie is received.
>
> Care to explain? For qemu it should make a difference what spice-server
> does with the dma-bufs passed (local display / encode video + send to
> remote).
[Kasireddy, Vivek] I agree that Qemu shouldn't care what the spice-server does
with the dmabuf fds but somehow a drawable has to be created in the remote
client
case. This is needed as most of the core functions in the server (associated
with
display-channel, video-stream, encoder, etc) operate on drawables. Therefore, I
figured since Qemu already tells the server to create a drawable in the non-gl
case
(by creating an update that includes a QXL_CMD_DRAW cmd), the same thing
can be done in the gl + remote client case as well.
Alternatively, we could make the server create a drawable as a response to
gl_scanout,
when it detects a remote client. IIUC, I think this can be done but seems
rather messy
given that currently, the server only creates a drawable (inside
red_process_display)
in the case of QXL_CMD_DRAW sent by Qemu/applications:
switch (ext_cmd.cmd.type) {
case QXL_CMD_DRAW: {
auto red_drawable = red_drawable_new(worker->qxl,
>mem_slots,
ext_cmd.group_id,
ext_cmd.cmd.data,
ext_cmd.flags); // returns
with 1 ref
if (red_drawable) {
display_channel_process_draw(worker->display_channel,
std::move(red_drawable),
worker->process_display_generation);
}
The other option I can think of is to just not deal with drawables at all and
somehow
directly share the dmabuf fd with the Encoder. This solution also seems very
messy
and invasive to me as we'd not be able to leverage the existing APIs (in
display-channel,
video-stream, etc) that create and manage streams efficiently.
>
> > #ifdef HAVE_SPICE_GL
> > +} else if (spice_dmabuf_encode) {
> > +if (g_strcmp0(preferred_codec, "gstreamer:h264")) {
> > +error_report("dmabuf-encode=on currently only works and
> > tested"
> > + "with gstreamer:h264");
> > +exit(1);
> > +}
>
> IMHO we should not hard-code todays spice-server capabilities like this.
> For starters this isn't true for spice-server versions which don't (yet)
> have your patches. Also the capability might depend on hardware
> support. IMHO we need some feature negotiation between qemu and spice
> here.
[Kasireddy, Vivek] Ok, I can get rid of this chunk in v3. However, given the
numerous
features supported by the Spice server, I suspect implementing feature
negotiation
might get really challenging. Is there any other way around this that you can
think of?
Thanks,
Vivek
>
> take care,
> Gerd
RE: [PATCH v2 4/4] virtio-gpu: Don't require udmabuf when blob support is enabled
Hi Dmitry, > > On 9/27/22 11:32, Gerd Hoffmann wrote: > > On Mon, Sep 26, 2022 at 09:32:40PM +0300, Dmitry Osipenko wrote: > >> On 9/23/22 15:32, Gerd Hoffmann wrote: > >>> On Tue, Sep 13, 2022 at 12:50:22PM +0200, Antonio Caggiano wrote: > >>>> From: Dmitry Osipenko > >>>> > >>>> Host blobs don't need udmabuf, it's only needed by guest blobs. The host > >>>> blobs are utilized by the Mesa virgl driver when persistent memory > >>>> mapping > >>>> is needed by a GL buffer, otherwise virgl driver doesn't use blobs. > >>>> Persistent mapping support bumps GL version from 4.3 to 4.5 in guest. > >>>> Relax the udmabuf requirement. > >>> > >>> What about blob=on,virgl=off? > >>> > >>> In that case qemu manages the resources and continued to require > >>> udmabuf. > >> > >> The udmabuf is used only by the blob resource-creation command in Qemu. > >> I couldn't find when we could hit that udmabuf code path in Qemu because > >> BLOB_MEM_GUEST resource type is used only by crosvm+Venus when crosvm > >> uses a dedicated render-server for virglrenderer. > > > > Recent enough linux guest driver will use BLOB_MEM_GUEST resources > > with blob=on + virgl=off > > I reproduced this case today using "-device > virtio-gpu-device,blob=true". You're right, it doesn't work without udmabuf. > > [8.369306] virtio_gpu_dequeue_ctrl_func: 90 callbacks suppressed > [8.369311] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response > 0x1200 (command 0x105) > [8.371848] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response > 0x1205 (command 0x104) > [8.373085] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response > 0x1200 (command 0x105) > [8.376273] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response > 0x1205 (command 0x104) > [8.416972] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response > 0x1200 (command 0x105) > [8.418841] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response > 0x1205 (command 0x104) > > I see now why you're wanting to keep the udmabuf requirement for > blob=on,virgl=off. > > > >> - /dev/udmabuf isn't accessible by normal user > >> - udmabuf driver isn't shipped by all of the popular Linux distros, > >> for example Debian doesn't ship it > > > > That's why blob resources are off by default. > > > >> Because of all of the above, I don't think it makes sense to > >> hard-require udmabuf at the start of Qemu. It's much better to fail > >> resource creation dynamically. > > > > Disagree. When virgl/venus is enabled, then yes, qemu would let > > virglrenderer manage resources and I'm ok with whatever requirements > > virglrenderer has. When qemu manages resources by itself udmabuf is > > a hard requirement for blob support though. > > Let's try to relax the udmabuf requirement only for blob=on,virgl=on. > I'll update this patch. [Kasireddy, Vivek] In addition to what Gerd mentioned and what you have discovered, I wanted to briefly add that we use Udmabuf/Guest Blobs for Headless GPU passthrough use-cases (blob=on and virgl=off). Here are some more details about our use-case: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592#note_851314 Thanks, Vivek > > Thank you very much for the review! > > -- > Best regards, > Dmitry >
RE: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs
Hi Markus,
Thank you for the review.
> Vivek Kasireddy writes:
>
> > The new parameter named "connector" can be used to assign physical
> > monitors/connectors to individual GFX VCs such that when the monitor
> > is connected or hotplugged, the associated GTK window would be
> > fullscreened on it. If the monitor is disconnected or unplugged,
> > the associated GTK window would be destroyed and a relevant
> > disconnect event would be sent to the Guest.
> >
> > Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
> >-display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.
> >
> > Cc: Dongwon Kim
> > Cc: Gerd Hoffmann
> > Cc: Daniel P. Berrangé
> > Cc: Markus Armbruster
> > Cc: Philippe Mathieu-Daudé
> > Cc: Marc-André Lureau
> > Cc: Thomas Huth
> > Signed-off-by: Vivek Kasireddy
> > ---
> > qapi/ui.json| 9 ++-
> > qemu-options.hx | 1 +
> > ui/gtk.c| 168
> > 3 files changed, 177 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 286c5731d1..86787a4c95 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1199,13 +1199,20 @@
> > # interfaces (e.g. VGA and virtual console character devices)
> > # by default.
> > # Since 7.1
> > +# @connector: List of physical monitor/connector names where the GTK
> > windows
> > +# containing the respective graphics virtual consoles (VCs)
> > are
> > +# to be placed. If a mapping exists for a VC, it will be
> > +# fullscreened on that specific monitor or else it would not
> > be
> > +# displayed anywhere and would appear disconnected to the
> > guest.
>
> Let's see whether I understand this... We have VCs numbered 0, 1, ...
> VC #i is mapped to the i-th element of @connector, counting from zero.
> Correct?
[Kasireddy, Vivek] Yes, correct.
>
> Ignorant question: what's a "physical monitor/connector name"?
[Kasireddy, Vivek] IIUC, while the HDMI/DP protocols refer to a receiver (RX)
as a "sink", the DRM Graphics subsystem in the kernel uses the term "connector"
and the GTK library prefers the term "monitor". All of these terms can be
used interchangeably but I chose the term connector for the new parameter
after debating between connector, monitor, output, etc.
The connector name (e.g. DP-1, HDMI-A-2, etc) uniquely identifies a connector
or a monitor on any given system:
https://elixir.bootlin.com/linux/v6.0-rc6/source/include/drm/drm_connector.h#L1328
If you are on Intel hardware, you can find more info related to connectors by
doing:
cat /sys/kernel/debug/dri/0/i915_display_info
>
> Would you mind breaking the lines a bit earlier, between column 70 and
> 75?
[Kasireddy, Vivek] Np, will do that.
>
> > +# Since 7.2
> > #
> > # Since: 2.12
> > ##
> > { 'struct' : 'DisplayGTK',
> >'data': { '*grab-on-hover' : 'bool',
> > '*zoom-to-fit' : 'bool',
> > -'*show-tabs' : 'bool' } }
> > +'*show-tabs' : 'bool',
> > +'*connector' : ['str'] } }
> >
> > ##
> > # @DisplayEGLHeadless:
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 31c04f7eea..576b65ef9f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
> > #if defined(CONFIG_GTK)
> > "-display
> > gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> > "
> > [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
> > +"[,connector.=]\n"
>
> Is "" a VC number?
[Kasireddy, Vivek] Yes.
>
> > #endif
> > #if defined(CONFIG_VNC)
> > "-display vnc=[,]\n"
>
> Remainder of my review is on style only. Style suggestions are not
> demands :)
[Kasireddy, Vivek] No problem; most of your suggestions are sensible
and I'll include them all in v2.
>
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 945c550909..651aaaf174 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -37,6 +37,7 @@
> > #include "qapi/qapi-commands-misc.h"
> > #include "qemu/cutils.h"
> > #include "qemu/main-loop.h"
> > +#include "qemu/option.h"
> >
> > #include "
RE: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows
Hi Markus, > Any overlap with Dongwon Kim's "[PATCH v5 0/2] handling guest multiple > displays"? [Kasireddy, Vivek] Yes, there is some overlap but as I mentioned in the cover letter, this series is intended to replace Dongwon's series dealing with multiple displays. > > Message-Id: <20220718233009.18780-1-dongwon@intel.com> > https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03212.html [Kasireddy, Vivek] We felt that using monitor numbers for display/VC assignment would be cumbersome for users. And, given that his series does not take into account monitor unplug/hotplug events, it's effectiveness would be limited compared to this one. Thanks, Vivek
RE: [PATCH v1] ui/gtk-egl: Check for a valid context before making EGL calls
Hi Marc-Andre,
>
> Hi
>
> On Mon, Mar 7, 2022 at 10:00 PM Kasireddy, Vivek
> wrote:
> >
> > Hi Marc-Andre,
> >
> > >
> > > Hi Vivek
> > >
> > > On Mon, Mar 7, 2022 at 8:39 AM Vivek Kasireddy
> > > wrote:
> > > >
> > > > Since not all listeners (i.e VirtualConsoles) of GL events have
> > > > a valid EGL context, make sure that there is a valid context
> > > > before making EGL calls.
> > > >
> > > > This fixes the following crash seen while launching the VM with
> > > > "-device virtio-gpu-pci,max_outputs=1,blob=true -display gtk,gl=on"
> > > >
> > > > No provider of eglCreateImageKHR found. Requires one of:
> > > > EGL_KHR_image
> > > > EGL_KHR_image_base
> > > >
> > > > Fixes: 7cc712e9862ff ("ui: dispatch GL events to all listeners")
> > >
> > > I am not able to reproduce on current master.
> > [Kasireddy, Vivek] I can still see it with current master. I think this
> > issue
> > is only seen when running Qemu in an Xorg based Host environment and
> > cannot be reproduced in a Wayland based environment -- as Qemu UI
> > uses the GLArea widget in the Wayland case where the EGL context
> > is managed by GTK.
> >
> > >
> > > Isn't it fixed with commit a9fbce5e9 ("ui/console: fix crash when
> > > using gl context with non-gl listeners") ?
> > [Kasireddy, Vivek] No, it unfortunately does not fix the issue I am seeing.
> > In
> > my case, there are three VirtualConsoles created ("parallel0",
> > "compatmonitor0",
> > "virtio-gpu-pci") and all three of them seem to have a valid
> > dpy_gl_scanout_dmabuf()
> > but only virtio-gpu-pci has a valid EGL context.
> >
> > >
> > > Could you also check after "[PATCH v3 00/12] GL & D-Bus display related
> > > fixes" ?
> > [Kasireddy, Vivek] I can check but I don't think this issue can be fixed in
> > ui/console.c
> > as all three VirtualConsoles pass the console_has_gl() check and one of the
> > only things
> > that distinguishes them is whether they have a valid EGL context.
> >
>
> Under X11, I get the same error on v6.2.0 and master:
> qemu-system-x86_64 -m 4G -object
> memory-backend-memfd,id=mem,size=4G,share=on -machine
> q35,accel=kvm,memory-backend=mem -device
> virtio-gpu-pci,max_outputs=1,blob=true -display gtk,gl=on -cdrom
> rawhide.iso
> No provider of eglCreateImageKHR found. Requires one of:
> EGL_KHR_image
> EGL_KHR_image_base
>
> Note that with virtio-gpu-gl-pci I get:
> qemu-system-x86_64: ../src/dispatch_common.c:868:
> epoxy_get_proc_address: Assertion `0 && "Couldn't find current GLX or
> EGL context.\n"' failed.
[Kasireddy, Vivek] It looks like this particular error and the one I saw are
both resolved by this commit:
Author: Akihiko Odaki
Date: Sat Mar 26 01:12:16 2022 +0900
ui/console: Check console before emitting GL event
On a completely different note, I am wondering if you have any plan to
eventually integrate the Rust based Gtk4 client into Qemu source repo?
Or, is it going to stay out-of-tree even after it is no longer WIP?
Thanks,
Vivek
RE: [PATCH v1] ui/gtk-egl: Check for a valid context before making EGL calls
Hi Marc-Andre,
>
> Hi Vivek
>
> On Mon, Mar 7, 2022 at 8:39 AM Vivek Kasireddy
> wrote:
> >
> > Since not all listeners (i.e VirtualConsoles) of GL events have
> > a valid EGL context, make sure that there is a valid context
> > before making EGL calls.
> >
> > This fixes the following crash seen while launching the VM with
> > "-device virtio-gpu-pci,max_outputs=1,blob=true -display gtk,gl=on"
> >
> > No provider of eglCreateImageKHR found. Requires one of:
> > EGL_KHR_image
> > EGL_KHR_image_base
> >
> > Fixes: 7cc712e9862ff ("ui: dispatch GL events to all listeners")
>
> I am not able to reproduce on current master.
[Kasireddy, Vivek] I can still see it with current master. I think this issue
is only seen when running Qemu in an Xorg based Host environment and
cannot be reproduced in a Wayland based environment -- as Qemu UI
uses the GLArea widget in the Wayland case where the EGL context
is managed by GTK.
>
> Isn't it fixed with commit a9fbce5e9 ("ui/console: fix crash when
> using gl context with non-gl listeners") ?
[Kasireddy, Vivek] No, it unfortunately does not fix the issue I am seeing. In
my case, there are three VirtualConsoles created ("parallel0", "compatmonitor0",
"virtio-gpu-pci") and all three of them seem to have a valid
dpy_gl_scanout_dmabuf()
but only virtio-gpu-pci has a valid EGL context.
>
> Could you also check after "[PATCH v3 00/12] GL & D-Bus display related
> fixes" ?
[Kasireddy, Vivek] I can check but I don't think this issue can be fixed in
ui/console.c
as all three VirtualConsoles pass the console_has_gl() check and one of the
only things
that distinguishes them is whether they have a valid EGL context.
Thanks,
Vivek
>
> thanks
>
> >
> > Cc: Marc-André Lureau
> > Cc: Gerd Hoffmann
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > ---
> > ui/gtk-egl.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> > index e3bd4bc274..31175827d0 100644
> > --- a/ui/gtk-egl.c
> > +++ b/ui/gtk-egl.c
> > @@ -244,6 +244,10 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
> > #ifdef CONFIG_GBM
> > VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.ectx || !vc->gfx.esurface) {
> > +return;
> > +}
> > +
> > eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> > vc->gfx.esurface, vc->gfx.ectx);
> >
> > @@ -269,6 +273,10 @@ void gd_egl_cursor_dmabuf(DisplayChangeListener *dcl,
> > #ifdef CONFIG_GBM
> > VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.ectx || !vc->gfx.esurface) {
> > +return;
> > +}
> > +
> > if (dmabuf) {
> > egl_dmabuf_import_texture(dmabuf);
> > if (!dmabuf->texture) {
> > --
> > 2.35.1
> >
RE: virtio-gpu: Get FD for texture
Hi Antonio, > > I am starting to believe that the error is due to the fact that no EGLContext > is active on the > current thread (the one running the Vulkan application). [Kasireddy, Vivek] Which UI module (and Host environment) are you testing with? gtk? egl-headless? Could you please provide more details about the environment and the use-case? > > Trying to call eglMakeCurrent within this thread gives me an EGL_BAD_ACCESS > error > as the EGLContext associated to the GL texture belongs to a different thread. [Kasireddy, Vivek] IIUC, contexts can only be bound to one thread at a time. So you either need to release the context in the other thread (eglMakeCurrent(NULL, NULL) before making it current in your current thread or create a shared context between both the threads to be able to share textures. Thanks, Vivek > > Does that make sense? > > Kind regards, > Antonio Caggiano > > On 27/09/21 12:21, Antonio Caggiano wrote: > > Hi, > > > > I am trying to support a Vulkan application in the guest > > (GTKGlArea+VirGL+venus) which needs to import a GL texture from a GL > > context. > > > > Before doing that, I need to get a FD for that texture, therefore I > > tried with calling egl-helpers.h:egl_get_fd_for_texture() but I get an > > epoxy error: > > > > > No provider of eglCreateImageKHR found. Requires one of: > > > > > EGL_KHR_image > > > > > EGL_KHR_image_base > > > > This is a bit weird to me as I am sure I am running QEMU with iris and > > according to eglinfo both of these extensions are available. > > > > Do you think my approach makes sense or I am doing something wrong > > somewhere? > > > > > > Kind regards, > > Antonio Caggiano
RE: [RFC v2 0/2] ui: Add a Wayland backend for Qemu UI (v2)
Hi Daniel, > On Mon, Sep 13, 2021 at 03:20:34PM -0700, Vivek Kasireddy wrote: > > Why does Qemu need a new Wayland UI backend? > > The main reason why there needs to be a plain and simple Wayland backend > > for Qemu UI is to eliminate the Blit (aka GPU copy) that happens if using > > a toolkit like GTK or SDL (because they use EGL). The Blit can be eliminated > > by sharing the dmabuf fd -- associated with the Guest scanout buffer -- > > directly with the Host compositor via the linux-dmabuf (unstable) protocol. > > Once properly integrated, it would be potentially possible to have the > > scanout buffer created by the Guest compositor be placed directly on a > > hardware plane on the Host thereby improving performance. Only Guest > > compositors that use multiple back buffers (at-least 1 front and 1 back) > > and virtio-gpu would benefit from this work. > > IME, QEMU already suffers from having too many barely maintained UI > implementations and iti s confusing to users. Using a toolkit like GTK > is generally a good thing, even if they don't enable the maximum > theoretical performance, because they reduce the long term maint burden. [Kasireddy, Vivek] The Wayland UI is ~600 lines of code and more than half of that is just boilerplate; it is also fairly standalone. We don't have any major complaints against GTK UI (which is ~3000 lines of code, just the Qemu piece excluding libgtk) except that there is no way to dissociate EGL from it. And, to keep the UI codebase up-to-date and leverage latest features from GTK (for example GTK4 and beyond) it would require non-trivial amount of work. Therefore, I think it'd not be too onerous to maintain something lightweight like the Wayland UI module. > > I'm far from convinced that we should take on the maint of yet another > UI in QEMU, even if it does have some performance benefit, especially [Kasireddy, Vivek] There are novel use-cases coming up particularly with the arrival of technologies like SRIOV where the Guest is expected to do all the rendering and share the fully composited buffer with the Host whose job is to just present it on the screen. And, in this scenario, if we were to use GTK UI, the (fullscreen sized) Blits incurred would quickly add up if there are 4-6 Guests running and presenting content at the same time locally on multiple monitors. Wayland UI helps in this situation as it does not do any additional rendering on the Host (no Blit) as it just merely forwards the Guest composited buffer to the Host compositor. > if implemented using a very low level API like Wayland, that won't let > us easily add rich UI features. [Kasireddy, Vivek] Yes, it is a drawback of Wayland UI that it'd not be possible to do window decorations/rich UI features but there are users/customers that do not care for them. I think it should be possible to have both Wayland and GTK UI co-exist where users can choose GTK UI for fancy features and Wayland UI for performance. Thanks, Vivek > > Regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
RE: [RFC v2 2/2] ui: Add a plain Wayland backend for Qemu UI
Hi Daniel,
> On Mon, Sep 13, 2021 at 03:20:36PM -0700, Vivek Kasireddy wrote:
> > Cc: Gerd Hoffmann
> > Signed-off-by: Vivek Kasireddy
> > ---
> > configure | 8 +-
> > meson.build | 33 +++
> > meson_options.txt | 2 +
> > qapi/ui.json | 3 +
> > ui/meson.build| 52
> > ui/wayland.c | 628 ++
> > 6 files changed, 725 insertions(+), 1 deletion(-) create mode 100644
> > ui/wayland.c
>
>
> > diff --git a/ui/meson.build b/ui/meson.build index
> > a73beb0e54..86fc324c82 100644
> > --- a/ui/meson.build
> > +++ b/ui/meson.build
> > @@ -64,6 +64,58 @@ if config_host.has_key('CONFIG_OPENGL') and gbm.found()
> >ui_modules += {'egl-headless' : egl_headless_ss} endif
> >
> > +wayland_scanner = find_program('wayland-scanner') proto_sources = [
> > + ['xdg-shell', 'stable', ],
> > + ['fullscreen-shell', 'unstable', 'v1', ],
> > + ['linux-dmabuf', 'unstable', 'v1', ], ] wayland_headers = []
> > +wayland_proto_sources = []
> > +
> > +if wayland.found()
> > + foreach p: proto_sources
> > +proto_name = p.get(0)
> > +proto_stability = p.get(1)
> > +
> > +if proto_stability == 'stable'
> > + output_base = proto_name
> > + input = files(join_paths(wlproto_dir,
> '@0@/@1@/@2@.xml'.format(proto_stability, proto_name, output_base)))
> > +else
> > + proto_version = p.get(2)
> > + output_base = '@0@-@1@-@2@'.format(proto_name, proto_stability,
> proto_version)
> > + input = files(join_paths(wlproto_dir,
> '@0@/@1@/@2@.xml'.format(proto_stability, proto_name, output_base)))
> > +endif
> > +
> > +wayland_headers += custom_target('@0@ client
> > header'.format(output_base),
> > + input: input,
> > + output: '@0@-client-protocol.h'.format(output_base),
> > + command: [
> > +wayland_scanner,
> > +'client-header',
> > +'@INPUT@', '@OUTPUT@',
> > + ], build_by_default: true
> > +)
> > +
> > +wayland_proto_sources += custom_target('@0@
> > source'.format(output_base),
> > + input: input,
> > + output: '@0@-protocol.c'.format(output_base),
> > + command: [
> > +wayland_scanner,
> > +'private-code',
> > +'@INPUT@', '@OUTPUT@',
> > + ], build_by_default: true
> > +)
> > + endforeach
> > +endif
> > +
> > +if wayland.found()
> > + wayland_ss = ss.source_set()
> > + wayland_ss.add(when: wayland, if_true: files('wayland.c',
> > +'xdg-shell-protocol.c',
> > +'fullscreen-shell-unstable-v1-protocol.c','linux-dmabuf-unstable-v1-p
> > +rotocol.c'))
> > + #wayland_ss.add(when: wayland, if_true: files('wayland.c'),
> > +[wayland_proto_sources])
> > + ui_modules += {'wayland' : wayland_ss} endif
>
> Configure fails on this
>
> Program wayland-scanner found: YES (/usr/bin/wayland-scanner)
>
> ../ui/meson.build:114:13: ERROR: File xdg-shell-protocol.c does not exist.
>
>
> the code a few lines above generates xdg-shell-protocol.c, but that isn't run
> until you type
> "make", so when meson is resolving the source files they don't exist.
>
> The alternative line you have commented out looks more like what we would
> need, but it
> doesn't work either as its syntax is invalid.
[Kasireddy, Vivek] Right, the commented line is the one we'd need but despite
exhaustively
trying various different combinations, I couldn't get Meson to include the
auto-generated
protocol sources. If it is not too much trouble, could you please point me to
an example
where this is done elsewhere in Qemu source that I can look at?
>
> How did you actually compile this series ?
[Kasireddy, Vivek] Oh, as a workaround, I just manually added the protocol
sources. I am
sorry I did not realize that this code would be compiled/tested; I mainly
posted these
RFC/WIP patches to provide additional context to the discussion associated with
the DRM/
Virtio-gpu kernel patches.
Thanks,
Vivek
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
RE: [PULL 0/6] Vga 20210910 patches
Hi Gerd, Peter, > > On Fri, Sep 10, 2021 at 05:52:55PM +0100, Peter Maydell wrote: > > On Fri, 10 Sept 2021 at 14:19, Gerd Hoffmann wrote: > > > > > > The following changes since commit > > > bd662023e683850c085e98c8ff8297142c2dd9f2: > > > > > > Merge remote-tracking branch > > > 'remotes/mcayland/tags/qemu-openbios-20210908' into staging > > > (2021-09-08 11:06:17 +0100) > > > > > > are available in the Git repository at: > > > > > > git://git.kraxel.org/qemu tags/vga-20210910-pull-request > > > > > > for you to fetch changes up to 6335c0b56819a5d1219ea84a11a732d0861542db: > > > > > > qxl: fix pre-save logic (2021-09-10 12:23:12 +0200) > > > > > > > > > virtio-gpu + ui: fence syncronization. > > > qxl: unbreak live migration. > > > > > > > > > > Hi; this fails to build on the ppc64 system: > > > > ../../ui/egl-helpers.c:79:6: error: no previous prototype for > > 'egl_dmabuf_create_sync' [-Werror=missing-prototypes] > >79 | void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf) > > | ^~ > > ../../ui/egl-helpers.c:95:6: error: no previous prototype for > > 'egl_dmabuf_create_fence' [-Werror=missing-prototypes] > >95 | void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf) > > | ^~~ > > > > > > The prototype is hidden behind CONFIG_GBM, but the definition is not. > > > > Then the callsites fail: > > > > ../../ui/gtk-gl-area.c: In function 'gd_gl_area_draw': > > ../../ui/gtk-gl-area.c:77:9: error: implicit declaration of function > > 'egl_dmabuf_create_sync' [-Werror=implicit-function-declaration] > >77 | egl_dmabuf_create_sync(dmabuf); > > | ^~ > > ../../ui/gtk-gl-area.c:77:9: error: nested extern declaration of > > 'egl_dmabuf_create_sync' [-Werror=nested-externs] > > ../../ui/gtk-gl-area.c:81:9: error: implicit declaration of function > > 'egl_dmabuf_create_fence' [-Werror=implicit-function-declaration] > >81 | egl_dmabuf_create_fence(dmabuf); > > | ^~~ > > ../../ui/gtk-gl-area.c:81:9: error: nested extern declaration of > > 'egl_dmabuf_create_fence' [-Werror=nested-externs] > > > > > > ../../ui/gtk-egl.c: In function 'gd_egl_draw': > > ../../ui/gtk-egl.c:100:9: error: implicit declaration of function > > 'egl_dmabuf_create_fence' [-Werror=implicit-function-declaration] > > 100 | egl_dmabuf_create_fence(dmabuf); > > | ^~~ > > ../../ui/gtk-egl.c:100:9: error: nested extern declaration of > > 'egl_dmabuf_create_fence' [-Werror=nested-externs] > > ../../ui/gtk-egl.c: In function 'gd_egl_scanout_flush': > > ../../ui/gtk-egl.c:301:9: error: implicit declaration of function > > 'egl_dmabuf_create_sync' [-Werror=implicit-function-declaration] > > 301 | egl_dmabuf_create_sync(vc->gfx.guest_fb.dmabuf); > > | ^~ > > ../../ui/gtk-egl.c:301:9: error: nested extern declaration of > > 'egl_dmabuf_create_sync' [-Werror=nested-externs] > > > > > > You can probably repro this on any system which has the opengl > > libraries installed but not libgbm. > > Vivek? Can you have a look please? [Kasireddy, Vivek] I sent a v6 that fixes these compilation errors: https://lists.nongnu.org/archive/html/qemu-devel/2021-09/msg03859.html Compile tested the patches with and without GBM. Thanks, Vivek
RE: vulkan support in qemu with virgil
Hi Tomeu, > On Wed, Jul 21, 2021 at 03:09:21PM +0200, Tomeu Vizoso wrote: > > Hi all, > > > > At Collabora we have started looking at Vulkan support in QEMU with Virgil. > > > > We have seen the work that Vivek has submitted to support the new virtio-gpu > > BLOB API (thanks!) and have identified a few holes that are still needed for > > Vulkan support. > > > > We would like to know if anybody else is working on Vulkan support or on > > common tasks such as host-side blobs, CONTEXT_INIT, CROSS_DEVICE, > > HOST_VISIBLE, venus capsets, [Kasireddy, Vivek] Nope; I am not aware of anyone working on these features for Qemu. Our focus is mainly on Passthrough (and Headless) GPUs for which Virgl is not needed. However, I am assuming you are already collaborating with CrossVM folks on the implementation of these features in Virtio-gpu Linux Guest driver? >> a new DisplayChangeListenerOps implementation, [Kasireddy, Vivek] Could you please elaborate on this? Which backend are you planning to add? I am working on adding a plain and simple Wayland UI backend for Qemu to reduce the GPU Blits: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg06482.html The above version does not have keyboard and mouse support added but I do have a version which does and it is more or less working. However, the only problem I have run into with this backend is: https://gitlab.freedesktop.org/wayland/weston/-/issues/514 Not sure if you'd run into a similar issue with your use-case or interested in the Wayland UI backend but I am currently working on trying to come up with a plan to decouple OUT_FENCE signalling from atomic pageflip completion event as suggested by Weston upstream (Pekka) to fix the above issue. Thanks, Vivek > > I'm not aware of anyone working on this in qemu specifically. > The crosvm guys are working on it, not sure what the status is. > > take care, > Gerd
RE: [PATCH 2/2] ui/gtk-egl: blitting partial guest fb to the proper scanout surface
Hi DW,
> eb_fb_blit needs more parameters which describe x and y offsets and width
> and height of the actual scanout to specify the size and cordination of
> partial image to blit in the guest fb in case the guest fb contains multiple
> display outputs.
>
> Signed-off-by: Dongwon Kim
> ---
> hw/display/virtio-gpu-udmabuf.c | 4 ++--
> include/ui/egl-helpers.h| 2 +-
> ui/egl-headless.c | 2 +-
> ui/egl-helpers.c| 10 ++
> ui/gtk-egl.c| 7 ---
> ui/sdl2-gl.c| 2 +-
> 6 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
[Kasireddy, Vivek] You might not want to mix virtio-gpu and UI changes in the
same patch.
> index a64194c6de..3ea6e76371 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -186,8 +186,8 @@ static VGPUDMABuf
> dmabuf->buf.stride = fb->stride;
> dmabuf->buf.x = r->x;
> dmabuf->buf.y = r->y;
> -dmabuf->buf.scanout_width;
> -dmabuf->buf.scanout_height;
> +dmabuf->buf.scanout_width = r->width;
> +dmabuf->buf.scanout_height = r->height;
> dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
> dmabuf->buf.fd = res->dmabuf_fd;
>
> diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
> index f1bf8f97fc..e21118501e 100644
> --- a/include/ui/egl-helpers.h
> +++ b/include/ui/egl-helpers.h
> @@ -26,7 +26,7 @@ void egl_fb_setup_default(egl_fb *fb, int width, int
> height);
> void egl_fb_setup_for_tex(egl_fb *fb, int width, int height,
>GLuint texture, bool delete);
> void egl_fb_setup_new_tex(egl_fb *fb, int width, int height);
> -void egl_fb_blit(egl_fb *dst, egl_fb *src, bool flip);
> +void egl_fb_blit(egl_fb *dst, egl_fb *src, int x, int y, int w, int h, bool
> flip);
> void egl_fb_read(DisplaySurface *dst, egl_fb *src);
>
> void egl_texture_blit(QemuGLShader *gls, egl_fb *dst, egl_fb *src, bool
> flip);
> diff --git a/ui/egl-headless.c b/ui/egl-headless.c
> index da377a74af..bdf10fec84 100644
> --- a/ui/egl-headless.c
> +++ b/ui/egl-headless.c
> @@ -144,7 +144,7 @@ static void egl_scanout_flush(DisplayChangeListener *dcl,
>1.0, 1.0);
> } else {
> /* no cursor -> use simple framebuffer blit */
> -egl_fb_blit(>blit_fb, >guest_fb, edpy->y_0_top);
> +egl_fb_blit(>blit_fb, >guest_fb, x, y, w, h,
> edpy->y_0_top);
> }
>
> egl_fb_read(edpy->ds, >blit_fb);
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> index 6d0cb2b5cb..2af3dcc0a6 100644
> --- a/ui/egl-helpers.c
> +++ b/ui/egl-helpers.c
> @@ -88,16 +88,18 @@ void egl_fb_setup_new_tex(egl_fb *fb, int width, int
> height)
> egl_fb_setup_for_tex(fb, width, height, texture, true);
> }
>
> -void egl_fb_blit(egl_fb *dst, egl_fb *src, bool flip)
> +void egl_fb_blit(egl_fb *dst, egl_fb *src, int x, int y, int w, int h, bool
> flip)
[Kasireddy, Vivek] Instead of explicitly passing x, y, w, h to egl_fb_blit,
would you be not
be able to use the dmabuf member that would be added to egl_fb that would
contain x, y, w and h:
https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg06746.html
> {
> GLuint y1, y2;
>
> glBindFramebuffer(GL_READ_FRAMEBUFFER, src->framebuffer);
> glBindFramebuffer(GL_DRAW_FRAMEBUFFER, dst->framebuffer);
> glViewport(0, 0, dst->width, dst->height);
> -y1 = flip ? src->height : 0;
> -y2 = flip ? 0 : src->height;
> -glBlitFramebuffer(0, y1, src->width, y2,
> +w = (x + w) > src->width ? src->width - x : w;
> +h = (y + h) > src->height ? src->height - y : h;
> +y1 = flip ? h + y : y;
> +y2 = flip ? y : h + y;
> +glBlitFramebuffer(x, y1, x + w, y2,
[Kasireddy, Vivek] While you are at it, could you please create new local
variables x1, y1, x2, y2
to store the above values and pass them to glBlitFramebuffer to improve the
readability of this code?
Thanks,
Vivek
>0, 0, dst->width, dst->height,
>GL_COLOR_BUFFER_BIT, GL_LINEAR);
> }
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> index 2a2e6d3a17..ceb52b1045 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -73,7 +73,7 @@ void gd_egl_draw(VirtualConsole *vc)
> wh = gdk_window_get_height(window);
>
> if (vc->gfx.scanout_mode) {
> -gd_egl_scanout_flush(>gfx.dcl, 0, 0, vc->gfx.w, vc->gfx.h);
> + gd_egl_scanout_flush(>
RE: [PATCH 1/2] virtio-gpu: splitting one extended mode guest fb into n-scanouts
Hi DW,
> When guest is running Linux/X11 with extended multiple displays mode enabled,
> the guest shares one scanout resource each time containing whole surface
> rather than sharing individual display output separately. This extended frame
> is properly splited and rendered on the corresponding scanout surfaces but
> not in case of blob-resource (zero copy).
>
> This code change lets the qemu split this one large surface data into multiple
> in case of blob-resource as well so that each sub frame then can be blitted
> properly to each scanout.
>
> Signed-off-by: Dongwon Kim
> ---
> hw/display/virtio-gpu-udmabuf.c | 19 +++
> hw/display/virtio-gpu.c | 5 +++--
> include/hw/virtio/virtio-gpu.h | 5 +++--
> include/ui/console.h| 4
> stubs/virtio-gpu-udmabuf.c | 3 ++-
> 5 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> index 3c01a415e7..a64194c6de 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -171,7 +171,8 @@ static VGPUDMABuf
> *virtio_gpu_create_dmabuf(VirtIOGPU *g,
>uint32_t scanout_id,
>struct virtio_gpu_simple_resource *res,
> - struct virtio_gpu_framebuffer *fb)
> + struct virtio_gpu_framebuffer *fb,
> + struct virtio_gpu_rect *r)
> {
> VGPUDMABuf *dmabuf;
>
> @@ -183,6 +184,10 @@ static VGPUDMABuf
> dmabuf->buf.width = fb->width;
> dmabuf->buf.height = fb->height;
> dmabuf->buf.stride = fb->stride;
> + dmabuf->buf.x = r->x;
> +dmabuf->buf.y = r->y;
> +dmabuf->buf.scanout_width;
> +dmabuf->buf.scanout_height;
[Kasireddy, Vivek] Would you not be able to use buf.width and buf.height?
What is the difference between these and scanout_width/height?
> dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
> dmabuf->buf.fd = res->dmabuf_fd;
>
> @@ -195,24 +200,22 @@ static VGPUDMABuf
> int virtio_gpu_update_dmabuf(VirtIOGPU *g,
> uint32_t scanout_id,
> struct virtio_gpu_simple_resource *res,
> - struct virtio_gpu_framebuffer *fb)
> + struct virtio_gpu_framebuffer *fb,
> + struct virtio_gpu_rect *r)
> {
> struct virtio_gpu_scanout *scanout = >parent_obj.scanout[scanout_id];
> VGPUDMABuf *new_primary, *old_primary = NULL;
>
> -new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb);
> +new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
> if (!new_primary) {
> return -EINVAL;
> }
>
> if (g->dmabuf.primary) {
> -old_primary = g->dmabuf.primary;
> +old_primary = g->dmabuf.primary[scanout_id];
> }
>
> -g->dmabuf.primary = new_primary;
> -qemu_console_resize(scanout->con,
> -new_primary->buf.width,
> -new_primary->buf.height);
> +g->dmabuf.primary[scanout_id] = new_primary;
> dpy_gl_scanout_dmabuf(scanout->con, _primary->buf);
>
> if (old_primary) {
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index e183f4ecda..11a87dad79 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -523,9 +523,9 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> console_has_gl(scanout->con)) {
> dpy_gl_update(scanout->con, 0, 0, scanout->width,
[Kasireddy, Vivek] Unrelated to your change but shouldn't 0,0 be replaced with
scanout->x and scanout->y?
>scanout->height);
> -return;
> }
> }
> +return;
> }
>
> if (!res->blob &&
> @@ -598,6 +598,7 @@ static void virtio_gpu_update_scanout(VirtIOGPU *g,
> scanout->y = r->y;
> scanout->width = r->width;
> scanout->height = r->height;
> +qemu_console_resize(scanout->con, scanout->width, scanout->height);
[Kasireddy, Vivek] IIRC, there was no qemu_console_resize for the non-blob
resources case.
Moving this call to update_scanout means that it will also be called for
non-blob resources
Path; not sure if this is OK but you might want restrict this call to only blob.
> }
>
> static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
> @@ -633,7 +634,7 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
>
>
RE: [PATCH 4/4] ui/gtk-egl: guest fb texture needs to be regenerated when reinitializing egl
>
> If guest fb is backed by dmabuf (blob-resource), the texture bound to the old
> context needs
> to be recreated in case the egl is re-initialized (e.g.
> new window for vc is created in case of detaching/reattaching of the tab)
>
> Signed-off-by: Dongwon Kim
> ---
> ui/gtk-egl.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 32516b806c..5dd7c7e1f5 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -129,6 +129,10 @@ void gd_egl_refresh(DisplayChangeListener *dcl)
> surface_gl_destroy_texture(vc->gfx.gls, vc->gfx.ds);
> surface_gl_create_texture(vc->gfx.gls, vc->gfx.ds);
> }
> +if (vc->gfx.guest_fb.dmabuf) {
> +vc->gfx.guest_fb.dmabuf->texture = 0;
[Kasireddy, Vivek] Shouldn't you call egl_dmabuf_release_texture() instead?
Thanks,
Vivek
> +gd_egl_scanout_dmabuf(dcl, vc->gfx.guest_fb.dmabuf);
> +}
> }
>
> graphic_hw_update(dcl->con);
> --
> 2.17.1
>
RE: [PATCH 2/3] ui/gtk-egl: make sure the right context is set as the current
Acknowledged-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Dongwon Kim
> Sent: Friday, July 02, 2021 5:28 PM
> To: qemu-devel@nongnu.org
> Cc: Kim, Dongwon
> Subject: [PATCH 2/3] ui/gtk-egl: make sure the right context is set as the
> current
>
> Making the vc->gfx.ectx current before handling textures
> associated with it
>
> Signed-off-by: Dongwon Kim
> ---
> ui/gtk-egl.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> index 2a2e6d3a17..32516b806c 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -126,6 +126,7 @@ void gd_egl_refresh(DisplayChangeListener *dcl)
> }
> vc->gfx.gls = qemu_gl_init_shader();
> if (vc->gfx.ds) {
> +surface_gl_destroy_texture(vc->gfx.gls, vc->gfx.ds);
> surface_gl_create_texture(vc->gfx.gls, vc->gfx.ds);
> }
> }
> @@ -152,6 +153,8 @@ void gd_egl_switch(DisplayChangeListener *dcl,
> surface_height(vc->gfx.ds) == surface_height(surface)) {
> resized = false;
> }
> +eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> + vc->gfx.esurface, vc->gfx.ectx);
>
> surface_gl_destroy_texture(vc->gfx.gls, vc->gfx.ds);
> vc->gfx.ds = surface;
> @@ -209,6 +212,11 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
> QemuDmaBuf *dmabuf)
> {
> #ifdef CONFIG_GBM
> +VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> +
> +eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> + vc->gfx.esurface, vc->gfx.ectx);
> +
> egl_dmabuf_import_texture(dmabuf);
> if (!dmabuf->texture) {
> return;
> --
> 2.17.1
>
RE: [PATCH 1/3] ui/gtk-egl: un-tab and re-tab should destroy egl surface and context
Reviewed-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Dongwon Kim
> Sent: Friday, July 02, 2021 5:28 PM
> To: qemu-devel@nongnu.org
> Cc: Romli, Khairul Anuar ; Kim, Dongwon
>
> Subject: [PATCH 1/3] ui/gtk-egl: un-tab and re-tab should destroy egl surface
> and context
>
> An old esurface should be destroyed and set to be NULL when doing
> un-tab and re-tab so that a new esurface an context can be created
> for the window widget that those will be bound to.
>
> Signed-off-by: Dongwon Kim
> Signed-off-by: Khairul Anuar Romli
> ---
> ui/gtk.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 98046f577b..bfb95f3b4b 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1232,6 +1232,14 @@ static gboolean gd_tab_window_close(GtkWidget *widget,
> GdkEvent *event,
> vc->tab_item, vc->label);
> gtk_widget_destroy(vc->window);
> vc->window = NULL;
> +if (vc->gfx.esurface) {
> +eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
> +vc->gfx.esurface = NULL;
> +}
> +if (vc->gfx.ectx) {
> +eglDestroyContext(qemu_egl_display, vc->gfx.ectx);
> +vc->gfx.ectx = NULL;
> +}
> return TRUE;
> }
>
> @@ -1261,6 +1269,14 @@ static void gd_menu_untabify(GtkMenuItem *item, void
> *opaque)
> if (!vc->window) {
> gtk_widget_set_sensitive(vc->menu_item, false);
> vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> +if (vc->gfx.esurface) {
> +eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
> +vc->gfx.esurface = NULL;
> +}
> +if (vc->gfx.esurface) {
> +eglDestroyContext(qemu_egl_display, vc->gfx.ectx);
> +vc->gfx.ectx = NULL;
> +}
> gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
>
> g_signal_connect(vc->window, "delete-event",
> --
> 2.17.1
>
RE: [PATCH 3/3] ui/gtk: gd_draw_event returns FALSE when no cairo surface is bound
Reviewed-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Dongwon Kim
> Sent: Friday, July 02, 2021 5:28 PM
> To: qemu-devel@nongnu.org
> Cc: Kim, Dongwon
> Subject: [PATCH 3/3] ui/gtk: gd_draw_event returns FALSE when no cairo
> surface is
> bound
>
> gd_draw_event shouldn't try to repaint if surface does not exist
> for the VC.
>
> Signed-off-by: Dongwon Kim
> ---
> ui/gtk.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index bfb95f3b4b..0a38deedc7 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -756,6 +756,9 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t
> *cr,
> void *opaque)
> if (!vc->gfx.ds) {
> return FALSE;
> }
> +if (!vc->gfx.surface) {
> +return FALSE;
> +}
>
> vc->gfx.dcl.update_interval =
> gd_monitor_update_interval(vc->window ? vc->window : s->window);
> --
> 2.17.1
>
RE: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Hi Philippe,
I am really sorry for the late review; I totally missed it until now.
Please find my comments inline.
From: Marc-André Lureau
Sent: Wednesday, July 07, 2021 4:05 AM
To: Philippe Mathieu-Daudé ; Kasireddy, Vivek
Cc: QEMU ; Gerd Hoffmann ; Michael S.
Tsirkin
Subject: Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Hi
On Mon, May 31, 2021 at 2:20 PM Philippe Mathieu-Daudé
mailto:phi...@redhat.com>> wrote:
To avoid leaking memory on the error path, reorder the
code as:
- check the parameters first
- check resource already existing
- finally allocate memory
Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Signed-off-by: Philippe Mathieu-Daudé
mailto:phi...@redhat.com>>
---
RFC because the s->iov check is dubious.
Yes, that looks wrong. Before the patch, the condition is always false / dead
code.
Furthermore, the init_udmabuf seems to really make sense when iov != NULL and
remapping takes place.
Vivek, please review
thanks
---
hw/display/virtio-gpu.c | 28 +++-
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc..8d047007bbb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
return;
}
-res = virtio_gpu_find_resource(g, cblob.resource_id);
-if (res) {
+if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
+cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
+ __func__);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+return;
+}
+
+if (virtio_gpu_find_resource(g, cblob.resource_id)) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
__func__, cblob.resource_id);
cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
@@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
res->resource_id = cblob.resource_id;
res->blob_size = cblob.size;
-if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
-cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
- __func__);
-cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-g_free(res);
-return;
-}
-
-if (res->iov) {
- cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-return;
-}
[Kasireddy, Vivek] Yeah, removing this makes sense. Basically,
resource_create_blob = resource_create + attach_backing and I was trying to do
what
attach_backing was doing but this conditional does not make sense for
create_blob as
we just created the resource few lines above.
-
ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
cmd, >addrs, >iov,
>iov_cnt);
-if (ret != 0) {
+if (ret != 0 || res->iov) {
[Kasireddy, Vivek] Would be redundant to check for res->iov as iov == NULL case
will be covered by ret != 0.
With this change,
Reviewed-by: Vivek Kasireddy
mailto:vivek.kasire...@intel.com>>
Thanks,
Vivek
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+g_free(res);
return;
}
--
2.26.3
--
Marc-André Lureau
RE: [PATCH v3 3/5] ui: Create sync objects and fences only for blobs
Hi Gerd, > > > > > > > dmabuf->buf.fd = res->dmabuf_fd; > > > > +dmabuf->buf.blob = true; > > > > > > Do you actually need the 'blob' field? > > > I think checking 'fd' instead should work too. > > [Kasireddy, Vivek] I want these changes to be limited to blob resources > > only as I do not > > know how they might affect other use-cases or whether they are needed there > > or not. I > > don't think I can rely on fd as vfio/display.c also populates the fd field: > > dmabuf = g_new0(VFIODMABuf, 1); > > dmabuf->dmabuf_id = plane.dmabuf_id; > > dmabuf->buf.width = plane.width; > > dmabuf->buf.height = plane.height; > > dmabuf->buf.stride = plane.stride; > > dmabuf->buf.fourcc = plane.drm_format; > > dmabuf->buf.modifier = plane.drm_format_mod; > > dmabuf->buf.fd = fd; > > > > Therefore, I need a way to identify a dmabuf that is associated with blobs > > vs others. > > And it actually is a dma-buf too (the guest display provided by i915 gvt > mdev driver). So fencing that should work, right? [Kasireddy, Vivek] Well, for virtio-gpu, as you know we are adding a dma fence to resource_flush to make it wait until it gets signalled by Qemu. We might have to do to something similar on i915 GVT side but I do not have the hardware to write a patch and test it out -- as i915 GVT is not supported for > Gen 9 platforms. > > Even if we have to restrict it to some kinds of dma-bufs the field > should have a more descriptive name like "allow_fences". [Kasireddy, Vivek] I think limiting this to blobs makes sense at the moment. If need be, we can extend it to include dma-bufs generated by i915 GVT later. Let me send a v4 with your suggestion to change the name. Thanks, Vivek > > take care, > Gerd
RE: [RFC v1 0/1] ui: Add a Wayland backend for Qemu UI
Hi Gerd, > > Why does Qemu need a new Wayland UI backend? > > The main reason why there needs to be a plain and simple Wayland backend > > for Qemu UI is to eliminate the Blit (aka GPU copy) that happens if using > > a toolkit like GTK or SDL (because they use EGL). The Blit can be eliminated > > by sharing the dmabuf fd -- associated with the Guest scanout buffer -- > > directly with the Host compositor via the linux-dmabuf (unstable) protocol. > > Hmm, that probably means no window decorations (and other UI elements), [Kasireddy, Vivek] Right, unfortunately, no decorations or other UI elements. For that we can use GTK. > right? Also the code seems to not (yet?) handle mouse and kbd input. [Kasireddy, Vivek] Yes, kbd and mouse support not added yet and that is why I tagged it as WIP. But it should not be too hard to add that. > > > The patch(es) are still WIP and the only reason why I am sending them now > > is to get feedback and see if anyone thinks this work is interesting. And, > > even after this work is complete, it is not meant to be merged and can be > > used for performance testing purposes. Given Qemu UI's new direction, the > > proper way to add new backends is to create a separate UI/display module > > that is part of the dbus/pipewire infrastructure that Marc-Andre is > > working on: > > https://lists.nongnu.org/archive/html/qemu-devel/2021-03/msg04331.html > > Separating emulation and UI has the big advantage that the guest > lifecycle is decoupled from the desktop session lifecycle, i.e. > the guest can continue to run when the desktop session ends. > > Works today with spice (when using unix socket to connect it can pass > dma-buf handles from qemu to spice client). > > Using dbus instead certainly makes sense. Whenever we'll just go send > dma-buf handles over dbus or integrate with pipewire for display/sound > not clear yet. Marc-André thinks using pipewire doesn't bring benefits > and I havn't found the time yet to learn more about pipewire ... [Kasireddy, Vivek] On our side, we'll also try to learn how dbus and pipewire fit in and work. Having said that, can Marc-Andre's work be merged in stages -- first only dbus and no pipewire? Thanks, Vivek > > take care, > Gerd
RE: [PATCH v3 3/5] ui: Create sync objects and fences only for blobs
Hi Gerd, > Hi, > > > dmabuf->buf.fd = res->dmabuf_fd; > > +dmabuf->buf.blob = true; > > Do you actually need the 'blob' field? > I think checking 'fd' instead should work too. [Kasireddy, Vivek] I want these changes to be limited to blob resources only as I do not know how they might affect other use-cases or whether they are needed there or not. I don't think I can rely on fd as vfio/display.c also populates the fd field: dmabuf = g_new0(VFIODMABuf, 1); dmabuf->dmabuf_id = plane.dmabuf_id; dmabuf->buf.width = plane.width; dmabuf->buf.height = plane.height; dmabuf->buf.stride = plane.stride; dmabuf->buf.fourcc = plane.drm_format; dmabuf->buf.modifier = plane.drm_format_mod; dmabuf->buf.fd = fd; Therefore, I need a way to identify a dmabuf that is associated with blobs vs others. Thanks, Vivek
RE: [PATCH v2 8/8] virtio-gpu: Add gl_flushed callback
Hi Gerd,
>
> > -if (!cmd->finished) {
> > +if (!cmd->finished && !(cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_FENCE)) {
> > virtio_gpu_ctrl_response_nodata(g, cmd, cmd->error ? cmd->error :
> > VIRTIO_GPU_RESP_OK_NODATA);
> > }
>
> My idea would be more along the lines of ...
>
> if (!cmd->finished) {
> if (renderer_blocked) {
>g->pending_completion = cmd;
> } else {
>virtio_gpu_ctrl_response_nodata(...)
> }
> }
>
> Then, when resuming processing after unblock check pending_completion
> and call virtio_gpu_ctrl_response_nodata if needed.
>
> Workflow:
>
> virtio_gpu_simple_process_cmd()
> -> virtio_gpu_resource_flush()
> -> dpy_gfx_update()
> -> gd_gl_area_update()
> call graphic_hw_gl_block(true), create fence.
[Kasireddy, Vivek] So, with blobs, as you know we call dpy_gl_update() and this
call
just "queues" the render/redraw. And, GTK then later calls the render signal
callback
which in this case would be gd_gl_area_draw() which is where the actual Blit
happens
and also glFlush; only after which we can create a fence.
> virtio_gpu_simple_process_cmd()
> -> will see renderer_blocked and delays RESOURCE_FLUSH completion.
>
> Then, when the fence is ready, gtk will:
> - call graphic_hw_gl_block(false)
> - call graphic_hw_gl_flush()
>-> virtio-gpu resumes processing the cmd queue.
[Kasireddy, Vivek] Yeah, I think this can be done.
>
> When you use the existing block/unblock functionality the fence can be a
> gtk internal detail, virtio-gpu doesn't need to know that gtk uses a
> fence to wait for the moment when it can unblock virtio queue processing
> (the egl fence helpers still make sense).
[Kasireddy, Vivek] Ok, I'll try to include your suggestions in v3.
Thanks,
Vivek
>
> take care,
> Gerd
RE: [PATCH v2 2/8] ui/egl: Add egl helpers to help with synchronization
Hi Gerd,
> > +void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
> > +{
> > +if (dmabuf->sync) {
> > +dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
> > + dmabuf->sync);
> > +eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> > +dmabuf->sync = NULL;
> > +}
> > +}
>
> > +void egl_dmabuf_wait_sync(QemuDmaBuf *dmabuf)
> > +{
>
> Hmm, still the blocking wait. Can't you do something like
[Kasireddy, Vivek] Right, it is a blocking wait; but this gets called from a
new GTK thread
that does the actual drawing.
> "qemu_set_fd_handler(dmabuf->fence_fd, ...)" to avoid the
> eglClientWaitSyncKHR() completely?
[Kasireddy, Vivek] Yeah, I think this is also doable; let me look into it.
Thanks,
Vivek
>
> take care,
> Gerd
RE: [PATCH v1 3/5] ui/egl: Add egl helpers to help with synchronization
Hi Gerd,
> > +epoxy_has_egl_extension(qemu_egl_display,
> > +"EGL_ANDROID_native_fence_sync")) {
>
> What about non-android? Is the name there just for historical reasons?
> Or do we actually need something else for desktop systems?
[Kasireddy, Vivek] It is not specific to Android:
https://www.khronos.org/registry/EGL/extensions/ANDROID/EGL_ANDROID_native_fence_sync.txt
I have been using Linux (Fedora 33 for both Guest and Host) as my
test platform.
>
> > +void egl_dmabuf_wait_sync(QemuDmaBuf *dmabuf)
>
> See other mail on blocking wait. Otherwise looks sane.
>
> > +static void gd_gl_wait_dmabuf(DisplayChangeListener *dcl,
> > + QemuDmaBuf *dmabuf)
>
> separate patch for the gtk bits please.
[Kasireddy, Vivek] Ok, will do.
Thanks,
Vivek
>
> thanks,
> Gerd
RE: [PATCH] virtio-gpu: move scanout_id sanity check
Reviewed-by: Vivek Kasireddy
> -Original Message-
> From: Qemu-devel On
> Behalf Of Gerd Hoffmann
> Sent: Friday, June 04, 2021 12:50 AM
> To: qemu-devel@nongnu.org
> Cc: Alexander Bulekov ; Gerd Hoffmann ;
> Michael S. Tsirkin
> Subject: [PATCH] virtio-gpu: move scanout_id sanity check
>
> Checking scanout_id in virtio_gpu_do_set_scanout() is too late, for the
> "resource_id == 0" case (aka disable scanout) the scanout_id is used
> unchecked. Move the check into the callers to fix that.
>
> Fixes: e64d4b6a9bc3 ("virtio-gpu: Refactor virtio_gpu_set_scanout")
> Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/383
> Reported-by: Alexander Bulekov
> Signed-off-by: Gerd Hoffmann
> ---
> hw/display/virtio-gpu.c | 20 ++--
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4d549377cbc1..e183f4ecdaa5 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -610,12 +610,6 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
> struct virtio_gpu_scanout *scanout;
> uint8_t *data;
>
> -if (scanout_id >= g->parent_obj.conf.max_outputs) {
> -qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> - __func__, scanout_id);
> -*error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> -return;
> -}
> scanout = >parent_obj.scanout[scanout_id];
>
> if (r->x > fb->width ||
> @@ -694,6 +688,13 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
> trace_virtio_gpu_cmd_set_scanout(ss.scanout_id, ss.resource_id,
> ss.r.width, ss.r.height, ss.r.x,
> ss.r.y);
>
> +if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> + __func__, ss.scanout_id);
> +cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +return;
> +}
> +
> if (ss.resource_id == 0) {
> virtio_gpu_disable_scanout(g, ss.scanout_id);
> return;
> @@ -730,6 +731,13 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
>ss.r.width, ss.r.height, ss.r.x,
>ss.r.y);
>
> +if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> + __func__, ss.scanout_id);
> +cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +return;
> +}
> +
> if (ss.resource_id == 0) {
> virtio_gpu_disable_scanout(g, ss.scanout_id);
> return;
> --
> 2.31.1
>
RE: [PATCH v6 03/13] virtio-gpu: Add udmabuf helpers
Hi Gerd, > Hi, > > > +#ifdef CONFIG_LINUX > > Seems wee need "#if defined(CONFIG_LINUX) && defined(F_GET_SEALS)" here > to cover old linux versions. I see some build failures in gitlab CI due to > F_GET_SEALS not being defined (crypto-old-nettle for example). [Kasireddy, Vivek] Instead, I am planning to remove the ifdef and add a file in stubs with the pixman dependency like you suggested. For seals, I think I could just include qemu/memfd.h that has a definition for F_GET_SEALS. > > > +#include > > gitlab ci (build-system-alpine): > > /usr/include/sys/fcntl.h:1:2: error: #warning redirecting incorrect #include > to > [-Werror=cpp] > 1 | #warning redirecting incorrect #include to [Kasireddy, Vivek] Ok, I'll replace sys/fcntl.h with just fcntl.h > > Otherwise series looks good and survived basic testing. [Kasireddy, Vivek] Will send a v7 that would address the above issues. Thanks, Vivek > > take care, > Gerd
RE: [PATCH v5 03/13] virtio-gpu: Add udmabuf helpers
Hi Gerd,
>
> > +#ifdef CONFIG_LINUX
>
> > +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
>
> > +#else
>
> > +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
> > +{
> > +/* nothing (stub) */
> > +return NULL
> > +}
>
> Fails to build for !linux ...
>
> You can place the stubs in a file in the stubs/ directory instead.
> They'll be used via weak symbol references instead of #ifdefs then.
>
> Advantage: the stubs are compiled unconditionally so errors like this
> don't go unnoticed that easily.
[Kasireddy, Vivek] Gave it a try but because of res->image, we'd need to
consider the
Pixman dependency. I think we have the following options to address this:
1) Add pixman dependency to stubs. This may not be acceptable given that the
other
dependencies are glib, socket, etc which are pretty basic.
2) Cast the objects (such as virtio_gpu_simple_resource) as void * to the
functions that
we have stubs for. This also may not be acceptable as I don't see other stubs
doing this.
3) Move some core objects (such as VirtIOGPUBase and its dependencies that do
not
deal with pixman) into a new header and include that in virtio-gpu.h and the
new stubs
file. This seems like the way to go but needs some work as virtio-gpu.h has a
lot of stuff
and is included in a lot of places. If it is not a problem, I can do this in a
small separate
series but keep the ifdef for this series?
Will send out a v6 for this series soon.
Thanks,
Vivek
>
> take care,
> Gerd
RE: [PATCH v5 03/13] virtio-gpu: Add udmabuf helpers
Hi Gerd,
> > +#ifdef CONFIG_LINUX
>
> > +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
>
> > +#else
>
> > +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
> > +{
> > +/* nothing (stub) */
> > +return NULL
> > +}
>
> Fails to build for !linux ...
>
> You can place the stubs in a file in the stubs/ directory instead.
> They'll be used via weak symbol references instead of #ifdefs then.
[Kasireddy, Vivek] Will do; should I send the whole series (v6) again with this
and the
other error in patch #10 fixed or just the fixed versions of these specific
patches?
Sorry for the tangential discussion...
On a completely different topic, I wanted to ask a question on behalf of a
colleague who
is trying to enable passthrough with virtio-gpu but for a Windows guest. It
appears the
guest boots only if we specify the option -vga virtio (not sure what happens
with virtio=std)
as Windows launches a "Microsoft Basic Display Adapter" driver for this VGA
device
and Qemu displays the Desktop for this device (via the calls in virtio-vga.c).
However,
since we only care about virtio-gpu-pci device for which we created a guest
driver, we
want to have Qemu display the content/fb from virtio-gpu instead of the vga
device.
I see that in set_scanout:
g->parent_obj.enable = 1;
and, then this in virtio-vga.c:
static void virtio_vga_base_update_display(void *opaque)
VirtIOVGABase *vvga = opaque;
VirtIOGPUBase *g = vvga->vgpu;
if (g->enable) {
g->hw_ops->gfx_update(g);
} else {
vvga->vga.hw_ops->gfx_update(>vga);
}
Since the parent_obj is different the above code is always going into the else
part.
Is the goal here to show the content from virtio-gpu if there is a set_scanout?
The only way we are able to get everything to work as expected is to enable our
virtio-gpu
guest driver for the VGA device instead of the virtio-gpu PCI device. But we
are not sure
if this would be OK or not. We don't run into these issues for Linux guests as
we only
enable virtio-gpu-pci as we have -vga none. We'd like to the do the same for
Windows
guests but it looks like it needs the VGA device to be available to boot. So,
our other
option (as we cannot disable the vga device) is to have Qemu accept content
only from
virtio-gpu-pci instead of virtio-vga. Would it make sense to do this? Do you
think there
is a better way to do what we are trying to do?
Thanks,
Vivek
RE: [PATCH v4 00/13] virtio-gpu: Add support for Blob resources feature (v4)
Hi Gerd, > > On Tue, May 11, 2021 at 03:47:06PM -0700, Vivek Kasireddy wrote: > > 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 > > Doesn't apply cleanly to master, can you rebase and resend? [Kasireddy, Vivek] Sure, just sent a v5. Thanks, Vivek > > thanks, > Gerd
RE: [PATCH v3 12/20] virtio-gpu: Add virtio_gpu_set_scanout_blob
Hi Gerd,
> > -pixman_image_unref(res->image);
> > +if (res->image) {
> > +pixman_image_unref(res->image);
> > +}
>
> There is qemu_pixman_image_unref().
>
> Like pixman_image_unref except that it also accepts (and ignores) NULL
> pointers.
[Kasireddy, Vivek] Made the change in v4.
>
> > virtio_gpu_cleanup_mapping(g, res);
> > QTAILQ_REMOVE(>reslist, res, next);
> > g->hostmem -= res->hostmem;
> > @@ -494,6 +496,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> > {
> > struct virtio_gpu_simple_resource *res;
> > struct virtio_gpu_resource_flush rf;
> > +struct virtio_gpu_scanout *scanout;
> > pixman_region16_t flush_region;
> > int i;
> >
> > @@ -504,16 +507,28 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> >
> > res = virtio_gpu_find_check_resource(g, rf.resource_id, false,
> > __func__, >error);
> > -if (!res || res->blob) {
> > +if (!res) {
> > return;
> > }
> >
> > -if (rf.r.x > res->width ||
> > +if (res->blob && display_opengl) {
>
> console_has_gl(scanout->con)
[Kasireddy, Vivek] Made this change here and in other places in v4.
>
> > +if (!res->blob &&
> > +(rf.r.x > res->width ||
> > rf.r.y > res->height ||
> > rf.r.width > res->width ||
> > rf.r.height > res->height ||
> > rf.r.x + rf.r.width > res->width ||
> > -rf.r.y + rf.r.height > res->height) {
> > +rf.r.y + rf.r.height > res->height)) {
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: flush bounds outside resource"
> >" bounds for resource %d: %d %d %d %d vs %d %d\n",
> >__func__, rf.resource_id, rf.r.x, rf.r.y,
>
> Indent needs fixing.
> Do we need sanity checks for the res->blob == true case? I think so ...
[Kasireddy, Vivek] If a resource is a blob, it would not have valid width and
height and
instead only have valid blob_size; hence, the sanity checks would not be
applicable.
Thanks,
Vivek
>
> > g->parent_obj.enable = 1;
> > -data = (uint8_t *)pixman_image_get_data(res->image);
> > +
> > +if (res->blob) {
> > +if (display_opengl) {
>
> Again console_has_gl(scanout->con)
>
> > +if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb)) {
> > +virtio_gpu_update_scanout(g, scanout_id, res, r);
> > +return;
> > +}
> > +}
> > +
> > +data = res->blob;
> > +} else {
> > +data = (uint8_t *)pixman_image_get_data(res->image);
> > +}
> >
> > /* create a surface for this scanout */
> > -if (!scanout->ds ||
> > +if ((res->blob && !display_opengl) ||
>
> And again.
>
> take care,
> Gerd
RE: [PATCH v2 07/12] virtio-gpu: Add virtio_gpu_resource_create_blob
Hi Gerd,
> > res->remapsz = QEMU_ALIGN_UP(res->remapsz,
> > qemu_real_host_page_size);
> >
> > res->remapped = mmap(NULL, res->remapsz, PROT_READ, @@ -152,7
> > +155,9 @@ void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource
> > *res)
> > pdata = res->remapped;
> > }
> >
> > -(void) pdata;
> > +if (pdata) {
> > +res->blob = pdata;
> > +}
> > }
>
> That is confusing. Patch #2 introduces code which is removed here (probably
> leftovers
> from udmabuf prototype which didn't use blob resources). I think it would be
> useful to
> merge patch #2 with this one.
[Kasireddy, Vivek] Ok, will do.
>
> Also we might cleanup things a bit. Do we need separate remapsz + blob_size
> fields in the
> first place? Probably not ...
[Kasireddy, Vivek] Right, we don't. I was also going to slightly change the
names of some
of the new functions in v3 to keep them consistent with the rest of the code.
Do you have
any additional feedback for the other patches that I can include in v3?
>
> take care,
> Gerd
>
> PS: Can you explicitly cc me on the next version of the patch series?
> Some patches (this one for example) are on the list only and not in
> my inbox. Thanks.
[Kasireddy, Vivek] Sure. I was hoping that including the Based-on-patch-by
tag would do just that but looks like it doesn't.
Thanks,
Vivek
RE: [PATCH 00/11] Add support for Blob resources feature
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
RE: [PATCH 00/11] Add support for Blob resources feature
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
RE: [PATCH 00/11] Add support for Blob resources feature
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 > Sent: Tuesday, March 30, 2021 8:10 PM > To: qemu-devel@nongnu.org > Cc: Kasireddy, Vivek ; Gerd Hoffmann > ; Marc-André Lureau ; Kim, > Dongwon ; Zhang, Tina > 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 > Cc: Marc-André Lureau > Cc: Dongwon Kim > Cc: Tina Zhang > > 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
RE: [PATCH 00/15] virtio-gpu: split into two devices.
Hi Gerd, > On Fri, Mar 19, 2021 at 03:48:42PM +0400, Marc-André Lureau wrote: > > Hi Gerd > > > > On Fri, Mar 19, 2021 at 3:22 PM Gerd Hoffmann 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. [Kasireddy, Vivek] Just a random thought: if a user enables both these devices either intentionally or accidentally, can they play nice with each other? If this case is not allowed, where and how is that being enforced? Thanks, Vivek > > > > > > > Nice! > > We are post 6.0 soft freeze. I suppose you target those changes for 6.1? > > Yes, it's clearly 6.1 material at this point. > > take care, > Gerd
RE: [RFC 0/1] Use dmabufs for display updates instead of pixman
Hi Gerd, Thank you for taking the time to explain how support for blob resources needs to be added. We are going to get started soon and here are the tasks we are planning to do in order of priority: 1) Add support for VIRTIO_GPU_BLOB_MEM_GUEST + VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE 2) Upgrade Qemu GTK UI from 3.22 to 4.x 3) Add explicit sync support to GTK4 and Qemu UI 4) Add support for VIRTGPU_BLOB_MEM_HOST3D We'll start sending patches as we go along. Thanks, Vivek > > [Kasireddy, Vivek] Sure, we'll take a look at your work and use that > > as a starting point. Roughly, how much of your work can be reused? > > There are some small udmabuf support patches which can probably be reused > pretty much > as-is. Everything else needs larger changes I suspect, but it's been a while > I looked at this > ... > > > Also, given my limited understanding of how discrete GPUs work, I was > > wondering how many copies would there need to be with blob > > resources/dmabufs and whether a zero-copy goal would be feasible or not? > > Good question. > > Right now there are two copies (gtk ui): > > (1) guest ram -> DisplaySurface -> gtk widget (gl=off), or > (2) guest ram -> DisplaySurface -> texture (gl=on). > > You should be able to reduce this to one copy for gl=on ... > > (3) guest ram -> texture > > ... by taking DisplaySurface out of the picture, without any changes to the > guest/host > interface. Drawback is that it requires adding an opengl dependency to > virtio-gpu even > with virgl=off, because the virtio-gpu device will have to handle the copy to > the texture > then, in response to guest TRANSFER commands. > > When adding blob resource support: > > Easiest is probably supporting VIRTIO_GPU_BLOB_MEM_GUEST (largely identical to > non-blob resources) with VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE > (allows the host to create a shared mapping). Then you can go create a > udmabuf for the > resource on the host side. For the non-gl code path you can mmap() the > udmabuf (which > gives you a linear mapping for the scattered guest pages) and create a > DisplaySurface > backed by guest ram pages (removing the guest ram -> DisplaySurface copy). > For the gl > code path you can create a texture backed by the udmabuf and go render on the > host > without copying at all. > > Using VIRTIO_GPU_BLOB_MEM_GUEST + > VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE for resources needs guest changes too, > either in mesa (when using virgl) or the kernel driver's dumb buffer handling > (when not > using virgl). > > Alternatively (listed more for completeness): > > You can create a blob resource with VIRTGPU_BLOB_MEM_HOST3D (requires virgl, > see also virgl_drm_winsys_resource_create_blob in mesa). It will be > allocated by the > host, then mapped into the guest using a virtual pci memory bar. Guest > userspace (aka > mesa driver) can mmap() these resources and has direct, zero-copy access to > the host > resource. > > Going to dma-buf export that, import into i915, then let the gpu render > implies we are > doing p2p dma from a physical (pci-assigned) device to the memory bar of a > virtual pci > device. > > Doing that should be possible, but frankly I would be surprised if that > actually works out- > of-the-box. Dunno how many dragons are lurking here. > Could become an interesting challenge to make that fly. > > > > Beside the code duplication this is also a maintainance issue. This > > > adds one more configuration to virtio-gpu. Right now you can build > > > virtio-gpu with virgl (depends on opengl), or you can build without > > > virgl (doesn't use opengl then). I don't think it is a good idea to add > > > a third mode, > without virgl support but using opengl for blob dma-bufs. > > [Kasireddy, Vivek] We'll have to re-visit this part but for our > > use-case with virtio-gpu, we are disabling virglrenderer in Qemu and > > virgl DRI driver in the Guest. However, we still need to use > > Opengl/EGL to convert the dmabuf (guest fb) to texture and render as part > > of the > UI/GTK updates. > > Well, VIRTGPU_BLOB_MEM_HOST3D blob resources are created using virgl renderer > commands (VIRGL_CCMD_PIPE_RESOURCE_CREATE). So supporting that without > virglrenderer is not an option. > > VIRTIO_GPU_BLOB_MEM_GUEST might be possible without too much effort. > > > > > On a different note, any particular reason why Qemu UI EGL > > > > implementation is limited to Xorg and not extended to > > > > Wayland/Weston for which there is GTK glarea? > > > > > > Well, ideally I'd love to ju
RE: [RFC 0/1] Use dmabufs for display updates instead of pixman
Hi Gerd,
Sorry for the delayed response. I wanted to wait until I finished my
proof-of-concept --
that included adding synchronization -- to ask follow up questions.
> >
> > Does your work above not count for anything?
>
> It is quite old, and I think not up-to-date with the final revision of the
> blob resource
> specification. I wouldn't be able to update this in near future due to being
> busy with other
> projects. Feel free to grab & update & submit these patches though.
[Kasireddy, Vivek] Sure, we'll take a look at your work and use that as a
starting
point. Roughly, how much of your work can be reused?
Also, given my limited understanding of how discrete GPUs work, I was wondering
how
many copies would there need to be with blob resources/dmabufs and whether a
zero-copy
goal would be feasible or not?
>
> Beside the code duplication this is also a maintainance issue. This adds one
> more
> configuration to virtio-gpu. Right now you can build virtio-gpu with virgl
> (depends on
> opengl), or you can build without virgl (doesn't use opengl then). I don't
> think it is a good
> idea to add a third mode, without virgl support but using opengl for blob
> dma-bufs.
[Kasireddy, Vivek] We'll have to re-visit this part but for our use-case with
virtio-gpu, we
are disabling virglrenderer in Qemu and virgl DRI driver in the Guest. However,
we still
need to use Opengl/EGL to convert the dmabuf (guest fb) to texture and render
as part of
the UI/GTK updates.
>
>
> > On a different note, any particular reason why Qemu UI EGL
> > implementation is limited to Xorg and not extended to Wayland/Weston
> > for which there is GTK glarea?
>
> Well, ideally I'd love to just use glarea. Which happens on wayland.
>
> The problem with Xorg is that the gtk x11 backend uses glx not egl to create
> an opengl
> context for glarea. At least that used to be the case in the past, maybe
> that has changed
> with newer versions. qemu needs egl contexts though, otherwise dma-bufs
> don't work. So
> we are stuck with our own egl widget implementation for now. Probably we
> will be able
> to drop it at some point in the future.
[Kasireddy, Vivek] GTK X11 backend still uses GLX and it seems like that is not
going to
change anytime soon. Having said that, I was wondering if it makes sense to add
a new
purely Wayland backend besides GtkGlArea so that Qemu UI can more quickly adopt
new
features such as explicit sync. I was thinking about the new backend being
similar to this:
https://cgit.freedesktop.org/wayland/weston/tree/clients/simple-dmabuf-egl.c
The reason why I am proposing this idea is because even if we manage to add
explicit
sync support to GTK and it gets merged, upgrading Qemu GTK support from 3.22
to > 4.x may prove to be daunting. Currently, the way I am doing explicit sync
is
by adding these new APIs to GTK and calling them from Qemu:
static int
create_egl_fence_fd(EGLDisplay dpy)
{
EGLSyncKHR sync = eglCreateSyncKHR(dpy,
EGL_SYNC_NATIVE_FENCE_ANDROID,
NULL);
int fd;
g_assert(sync != EGL_NO_SYNC_KHR);
fd = eglDupNativeFenceFDANDROID(dpy, sync);
g_assert(fd >= 0);
eglDestroySyncKHR(dpy, sync);
return fd;
}
static void
wait_for_buffer_release_fence(EGLDisplay dpy)
{
int ret;
EGLint attrib_list[] = {
EGL_SYNC_NATIVE_FENCE_FD_ANDROID, release_fence_fd,
EGL_NONE,
};
if (release_fence_fd < 0)
return;
EGLSyncKHR sync = eglCreateSyncKHR(dpy,
EGL_SYNC_NATIVE_FENCE_ANDROID,
attrib_list);
g_assert(sync);
release_fence_fd = -1;
eglClientWaitSyncKHR(dpy, sync, 0,
EGL_FOREVER_KHR);
eglDestroySyncKHR(dpy, sync);
}
And, of-course, I am tying the wait above to a dma_fence associated with the
previous guest FB that is signalled to ensure that the Host is done using the
FB
thereby providing explicit synchronization between Guest and Host. It seems to
work OK but I was wondering if you had any alternative ideas or suggestions
for doing explicit or implicit sync that are more easier.
Lastly, on a different note, I noticed that there is a virtio-gpu Windows
driver here:
https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/viogpu
We are going to try it out but do you know how up to date it is kept?
Thanks,
Vivek
RE: [RFC 1/1] virtio-gpu: Use dmabuf for display updates if possible instead of pixman
Hi Marc-Andre,
+create = g_malloc0(sizeof(*create) +
+ res->iov_cnt * sizeof (struct udmabuf_create_item));
+if (!create)
+return NULL;
Pointless allocation check (g_malloc will abort if it failed to allocate)
[Kasireddy, Vivek] Ok, will remove it in v2.
+
+create->count = res->iov_cnt;
+create->flags = UDMABUF_FLAGS_CLOEXEC;
+for (i = 0; i < res->iov_cnt; i++) {
+rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, );
+if (!rb || rb->fd < 0) {
+g_free(create);
+return NULL;
+}
+
+create->list[i].memfd = rb->fd;
+create->list[i].__pad = 0;
+create->list[i].offset = offset;
+create->list[i].size = res->iov[i].iov_len;
+}
+
+dmabuf_fd = ioctl(udmabuf_fd, UDMABUF_CREATE_LIST, create);
+if (dmabuf_fd < 0) {
It would be useful to print the error with errno.
[Kasireddy, Vivek] Sure.
+g_free(create);
(we now like new code to use g_auto)
+return NULL;
+}
+
+/* FIXME: We should get the modifier and format info with blob resources */
+modifier_hi = fourcc_mod_code(INTEL, I915_FORMAT_MOD_X_TILED) >> 32;
+modifier_lo = fourcc_mod_code(INTEL,I915_FORMAT_MOD_X_TILED) & 0x;
I have no idea if this format is going to work with various drivers and matches
the underlying memory representation.
[Kasireddy, Vivek] No, it won’t work with other drivers. The modifier
information should
be obtained from the Guest that allocates the buffers. I just added it as a
temporary hack
for testing with a Intel GPU.
+modifier = ((uint64_t)modifier_hi << 32) | modifier_lo;
+
+dmabuf = g_new0(VGPUDMABuf, 1);
+dmabuf->dmabuf_id = ids++;
+dmabuf->buf.width = res->width;
+dmabuf->buf.height = res->height;
+dmabuf->buf.stride = pixman_image_get_stride(res->image);
+dmabuf->buf.fourcc = DRM_FORMAT_XRGB;
+dmabuf->buf.modifier = modifier;
+dmabuf->buf.fd = dmabuf_fd;
+
+QTAILQ_INSERT_HEAD(>dmabuf.bufs, dmabuf, next);
+g_free(create);
+
+return dmabuf;
+}
+udmabuf_fd = open("/dev/udmabuf", O_RDWR);
If udmabuf_fd is already opened, this will leak fds.
[Kasireddy, Vivek] Yup, will take care of it in v2.
+if (udmabuf_fd < 0) {
+error_setg_errno(errp, errno,
+ "udmabuf: failed to open vhost device");
+return;
The fallback path should keep working
[Kasireddy, Vivek] Makes sense; will fix it in v2.
It would be worth doing a similar change in vhost-user-gpu.
[Kasireddy, Vivek] Sure, will look into it after understanding the deltas
between
vhost-user and the in-qemu implementations.
Thanks,
Vivek
--
Marc-André Lureau
RE: [RFC 0/1] Use dmabufs for display updates instead of pixman
Hi Gerd, > Yes, it surely makes sense to go into that direction. > The patch as-is doesn't, it breaks the guest/host interface. > That's ok-ish for a quick proof-of-concept, but clearly not merge-able. > > > TODO: > > - Use Blob resources for getting meta-data such as modifier, format, etc. > > That is pretty much mandatory. Without blob resources there is no concept of > resources > shared between host and guest in virtio-gpu, all data is explicitly copied > with transfer > commands. [Kasireddy, Vivek] My understanding of virtio-gpu and the concept of resources is still fairly limited but are blob resources really needed for non-Virgl use-cases -- other than something like a dmabuf/scanout blob that shares the meta-data such as modifer? I thought the main motivation for blob resources would be to avoid the explicit copy you mentioned for Virgl workloads. > > Which implies quite a bit of work because we don't have blob resource support > in qemu > yet. [Kasireddy, Vivek] I was scrubbing through old mailing list messages to understand the motivation behind blob resources as to why they are needed and came across this: https://gitlab.freedesktop.org/virgl/qemu/-/commits/virtio-gpu-next Does your work above not count for anything? > > > - Test with Virgil rendered BOs to see if this can be used in that case.. > > That also opens up the question how to go forward with virtio-gpu in general. > The object > hierarchy we have right now (skipping pci + vga variants for simplicity): > > TYPE_VIRTIO_GPU_BASE (abstract base) >-> TYPE_VIRTIO_GPU (in-qemu implementation) >-> TYPE_VHOST_USER_GPU (vhost-user implementation) > > When compiled with opengl + virgl TYPE_VIRTIO_GPU has a virgl=on/off property. > Having a single device is not ideal for modular builds. > because the hw-display-virtio-gpu.so module has a dependency on ui-opengl.so > so that is > needed (due to symbol references) even for the virgl=off case. Also the code > is a bit of a > #ifdef mess. > > I think we should split TYPE_VIRTIO_GPU into two devices. Remove > virgl+opengl support from TYPE_VIRTIO_GPU. Add a new > TYPE_VIRTIO_GPU_VIRGL, with either TYPE_VIRTIO_GPU or > TYPE_VIRTIO_GPU_BASE as parent (not sure which is easier), have all > opengl/virgl > support code there. > > I think when using opengl it makes sense to also require virgl, so we can use > the > virglrenderer library to manage blob resources (even when the actual > rendering isn't done > with virgl). Also reduces the complexity and test matrix. [Kasireddy, Vivek] When you say "using opengl" are you referring to the presentation of the rendered buffer via dmabuf or pixman? If yes, I am not sure why this would need to depend on Virgl. For our use-case(s) where we are using virtio-gpu in buffer sharing mode, we'd still need opengl for submitting the dmabuf to UI, IIUC. > > Maybe it even makes sense to deprecate in-qemu virgl support and focus > exclusively on > the vhost-user implementation, so we don't have to duplicate all work for both > implementations. [Kasireddy, Vivek] Is the vhost-user implementation better in terms of performance, generally? > > case, how do we make sure that Weston and Qemu UI are not using the same > > buffer at > any given time? > > There is graphic_hw_gl_block + graphic_hw_gl_flushed for syncronization. > Right now this is only wired up in spice, and it is rather simple (just > stalls virgl rendering > instead of providing per-buffer syncronization). [Kasireddy, Vivek] I guess that might work for Virgl rendering but not for our use-case. What we need is a way to tell if the previously submitted dmabuf has been consumed by the Host compositor or not before we release/close it. Weston (wl_buffer.release event and fences) and EGL (sync and fences) do provide few options but I am not sure if GTK lets us use any of those or not. Any recommendations? EGLSync objects? On a different note, any particular reason why Qemu UI EGL implementation is limited to Xorg and not extended to Wayland/Weston for which there is GTK glarea? Thanks, Vivek > > take care, > Gerd
