[PULL v2 09/13] ui/spice: Blit the scanout texture if its memory layout is not linear

marcandre.lureau@redhat.com posted 13 patches 4 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Stefan Berger <stefanb@linux.vnet.ibm.com>
There is a newer version of this series
[PULL v2 09/13] ui/spice: Blit the scanout texture if its memory layout is not linear
Posted by marcandre.lureau@redhat.com 4 months ago
From: Vivek Kasireddy <vivek.kasireddy@intel.com>

In cases where the scanout buffer is provided as a texture (e.g. Virgl)
we need to check to see if it has a linear memory layout or not. If
it doesn't have a linear layout, then blitting it onto the texture
associated with the display surface (which already has a linear layout)
seems to ensure that there is no corruption seen regardless of which
encoder or decoder is used.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
Message-Id: <20250617043546.1022779-8-vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h |  3 ++
 ui/spice-display.c         | 81 +++++++++++++++++++++++++++++++++++---
 2 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index 9bdde78266..690ece7380 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -132,6 +132,9 @@ struct SimpleSpiceDisplay {
     egl_fb guest_fb;
     egl_fb blit_fb;
     egl_fb cursor_fb;
+    bool backing_y_0_top;
+    bool blit_scanout_texture;
+    bool new_scanout_texture;
     bool have_hot;
 #endif
 };
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 854a97c198..9ce622cefc 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1086,7 +1086,7 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
     EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
-    int fd[DMABUF_MAX_PLANES], num_planes;
+    int fd[DMABUF_MAX_PLANES], num_planes, i;
     uint64_t modifier;
 
     assert(tex_id);
@@ -1098,11 +1098,26 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
 
     trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
 
-    /* note: spice server will close the fd */
-    spice_server_gl_scanout(&ssd->qxl, fd, backing_width, backing_height,
-                            (uint32_t *)offset, (uint32_t *)stride, num_planes,
-                            fourcc, modifier, y_0_top);
-    qemu_spice_gl_monitor_config(ssd, x, y, w, h);
+    if (spice_remote_client && modifier != DRM_FORMAT_MOD_LINEAR) {
+        egl_fb_destroy(&ssd->guest_fb);
+        egl_fb_setup_for_tex(&ssd->guest_fb,
+                             backing_width, backing_height,
+                             tex_id, false);
+        ssd->backing_y_0_top = y_0_top;
+        ssd->blit_scanout_texture = true;
+        ssd->new_scanout_texture = true;
+
+        for (i = 0; i < num_planes; i++) {
+            close(fd[i]);
+        }
+    } else {
+        /* note: spice server will close the fd */
+        spice_server_gl_scanout(&ssd->qxl, fd, backing_width, backing_height,
+                                (uint32_t *)offset, (uint32_t *)stride,
+                                num_planes, fourcc, modifier, y_0_top);
+        qemu_spice_gl_monitor_config(ssd, x, y, w, h);
+    }
+
     ssd->have_surface = false;
     ssd->have_scanout = true;
 }
@@ -1168,6 +1183,50 @@ static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
     egl_dmabuf_release_texture(dmabuf);
 }
 
+static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
+                                          egl_fb *scanout_tex_fb)
+{
+    uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
+    int fds[DMABUF_MAX_PLANES], num_planes, fourcc;
+    uint64_t modifier;
+    bool ret;
+
+    egl_fb_destroy(scanout_tex_fb);
+    egl_fb_setup_for_tex(scanout_tex_fb,
+                         surface_width(ssd->ds), surface_height(ssd->ds),
+                         ssd->ds->texture, false);
+    egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
+    glFlush();
+
+    if (!ssd->new_scanout_texture) {
+        return true;
+    }
+
+    ret = egl_dmabuf_export_texture(ssd->ds->texture,
+                                    fds,
+                                    (EGLint *)offsets,
+                                    (EGLint *)strides,
+                                    &fourcc,
+                                    &num_planes,
+                                    &modifier);
+    if (!ret) {
+        error_report("spice: failed to get fd for texture");
+        return false;
+    }
+
+    spice_server_gl_scanout(&ssd->qxl, fds,
+                            surface_width(ssd->ds),
+                            surface_height(ssd->ds),
+                            (uint32_t *)offsets, (uint32_t *)strides,
+                            num_planes, fourcc, modifier,
+                            ssd->backing_y_0_top);
+    qemu_spice_gl_monitor_config(ssd, 0, 0,
+                                 surface_width(ssd->ds),
+                                 surface_height(ssd->ds));
+    ssd->new_scanout_texture = false;
+    return true;
+}
+
 static void qemu_spice_gl_update(DisplayChangeListener *dcl,
                                  uint32_t x, uint32_t y, uint32_t w, uint32_t h)
 {
@@ -1175,6 +1234,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
+    bool ret;
     uint32_t width, height, texture;
 
     if (!ssd->have_scanout) {
@@ -1269,6 +1329,15 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
         glFlush();
     }
 
+    if (spice_remote_client && ssd->blit_scanout_texture) {
+        egl_fb scanout_tex_fb;
+
+        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
+        if (!ret) {
+            return;
+        }
+    }
+
     trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
     qemu_spice_gl_block(ssd, true);
     glFlush();
-- 
2.50.0


Re: [PULL v2 09/13] ui/spice: Blit the scanout texture if its memory layout is not linear
Posted by Peter Maydell 4 months ago
On Tue, 15 Jul 2025 at 07:43, <marcandre.lureau@redhat.com> wrote:
>
> From: Vivek Kasireddy <vivek.kasireddy@intel.com>
>
> In cases where the scanout buffer is provided as a texture (e.g. Virgl)
> we need to check to see if it has a linear memory layout or not. If
> it doesn't have a linear layout, then blitting it onto the texture
> associated with the display surface (which already has a linear layout)
> seems to ensure that there is no corruption seen regardless of which
> encoder or decoder is used.

Hi; Coverity points out (CID 1612367) that this change
introduces a use of an uninitialized variable:

> @@ -1269,6 +1329,15 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>          glFlush();
>      }
>
> +    if (spice_remote_client && ssd->blit_scanout_texture) {
> +        egl_fb scanout_tex_fb;

We don't initialize scanout_tex_fb...

> +
> +        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);

...and we immediately pass it to spice_gl_blit_scanout_texture(),
which will unconditionally call egl_fb_destroy() on it.

> +        if (!ret) {
> +            return;
> +        }
> +    }

thanks
-- PMM
RE: [PULL v2 09/13] ui/spice: Blit the scanout texture if its memory layout is not linear
Posted by Kasireddy, Vivek 4 months ago
Hi Peter, Marc-Andre,

> Subject: Re: [PULL v2 09/13] ui/spice: Blit the scanout texture if its memory
> layout is not linear
> 
> On Tue, 15 Jul 2025 at 07:43, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >
> > In cases where the scanout buffer is provided as a texture (e.g. Virgl)
> > we need to check to see if it has a linear memory layout or not. If
> > it doesn't have a linear layout, then blitting it onto the texture
> > associated with the display surface (which already has a linear layout)
> > seems to ensure that there is no corruption seen regardless of which
> > encoder or decoder is used.
> 
> Hi; Coverity points out (CID 1612367) that this change
> introduces a use of an uninitialized variable:
> 
> > @@ -1269,6 +1329,15 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> >          glFlush();
> >      }
> >
> > +    if (spice_remote_client && ssd->blit_scanout_texture) {
> > +        egl_fb scanout_tex_fb;
> 
> We don't initialize scanout_tex_fb...
> 
> > +
> > +        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
> 
> ...and we immediately pass it to spice_gl_blit_scanout_texture(),
> which will unconditionally call egl_fb_destroy() on it.
I'll send out a patch in a day or two to fix this issue.

Thanks,
Vivek

> 
> > +        if (!ret) {
> > +            return;
> > +        }
> > +    }
> 
> thanks
> -- PMM