[PATCH v4 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[PATCH v4 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Vivek Kasireddy]

To address the limitation that this option is incompatible with
remote clients, this patch series adds an option to select a
preferred codec and also enable gl=on option for clients that
are connected via the network. In other words, with this option
enabled (and the below linked Spice series merged), it would be
possible to have Qemu share a dmabuf fd with Spice, which would
then forward it to a hardware or software based encoder and
eventually send the data associated with the fd to a client that
could be located on a different machine.

Essentially, this patch series provides a hardware accelerated,
opensource VDI option for users using Qemu and Spice by leveraging
the iGPU/dGPU on the host machine to encode the Guest FB via the
Gstreamer framework.

v3 -> v4 (suggestions from Marc-André):
- Add a new parameter to make max_refresh_rate configurable
- Have surface_gl_create_texture_from_fd() return bool after checking
  for errors
- Remove the check for PIXMAN_r5g6b5() in spice_gl_replace_fd_texture()
- Report errors in spice_gl_replace_fd_texture() when someting fails
- Use glGetError() correctly by adding an additional (dummy) call
  before checking for actual errors (Dmitry)
- Add a new patch to check fd values in egl_dmabuf_export_texture()
- Rebase on Qemu master

v2 -> v3:
- Check for errors after invoking glImportMemoryFdEXT() using
  glGetError() and report the error to user (Dmitry)

v1 -> v2:
- Replace the option name preferred-codec with video-codecs (Marc-André)
- Add a warning when an fd cannot be created from texture (Marc-André)
- Add a new patch to blit the scanout texture into a linear one to
  make it work with virgl
- Rebased and tested against the latest Spice master

Tested with the following Qemu parameters:
-device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
-spice port=3001,gl=on,disable-ticketing=on,video-codecs=gstreamer:h264

and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.

Associated Spice server MR (merged):
https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229

---
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: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Alex Bennée <alex.bennee@linaro.org>

Vivek Kasireddy (7):
  ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
  ui/spice: Add an option for users to provide a preferred codec
  ui/spice: Enable gl=on option for non-local or remote clients
  ui/spice: Add an option to submit gl_draw requests at fixed rate
  ui/console-gl: Add a helper to create a texture with linear memory
    layout
  ui/spice: Create a new texture with linear layout when gl=on is
    enabled
  ui/spice: Blit the scanout texture if its memory layout is not linear

 include/ui/console.h       |   2 +
 include/ui/spice-display.h |   5 +
 qemu-options.hx            |  10 ++
 ui/console-gl.c            |  32 ++++++
 ui/egl-helpers.c           |   6 ++
 ui/spice-core.c            |  27 +++++
 ui/spice-display.c         | 212 ++++++++++++++++++++++++++++++++++---
 7 files changed, 278 insertions(+), 16 deletions(-)

-- 
2.49.0

[PATCH v4 1/7] ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()

[Vivek Kasireddy]

While trying to export and obtain fds associated with a texture, it
is possible that the fds returned after eglExportDMABUFImageMESA()
call are -1. Therefore, we need to evaluate the value of all fds
and return false if any of them are -1.

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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/egl-helpers.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 9cda2bbbee..07d8edd3dc 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -289,6 +289,7 @@ bool egl_dmabuf_export_texture(uint32_t tex_id, int *fd, EGLint *offset,
 {
     EGLImageKHR image;
     EGLuint64KHR modifiers[DMABUF_MAX_PLANES];
+    int i;
 
     image = eglCreateImageKHR(qemu_egl_display, eglGetCurrentContext(),
                               EGL_GL_TEXTURE_2D_KHR,
@@ -308,6 +309,11 @@ bool egl_dmabuf_export_texture(uint32_t tex_id, int *fd, EGLint *offset,
         *modifier = modifiers[0];
     }
 
+    for (i = 0; i < *num_planes; i++) {
+        if (fd[i] < 0) {
+            return false;
+        }
+    }
     return true;
 }
 
-- 
2.49.0

[PATCH v4 2/7] ui/spice: Add an option for users to provide a preferred codec

[Vivek Kasireddy]

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 <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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 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 aab53bcfe8..97c63d9b31 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2281,6 +2281,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"
+    "       [,video-codecs=<encoder>:<codec>\n"
     "       [,gl=[on|off]][,rendernode=<file>]\n"
     "                enable spice\n"
     "                at least one of {port, tls-port} is mandatory\n",
@@ -2369,6 +2370,10 @@ SRST
     ``seamless-migration=[on|off]``
         Enable/disable spice seamless migration. Default is off.
 
+    ``video-codecs=<encoder>:<codec>``
+        Provide the preferred codec the Spice server should use.
+        Default would be spice:mjpeg.
+
     ``gl=[on|off]``
         Enable/disable OpenGL context. Default is off.
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 0326c63bec..907b0e9a81 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 = "video-codecs",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "agent-mouse",
             .type = QEMU_OPT_BOOL,
@@ -662,6 +665,7 @@ static void qemu_spice_init(void)
     char *x509_key_file = NULL,
         *x509_cert_file = NULL,
         *x509_cacert_file = NULL;
+    const char *video_codecs = NULL;
     int port, tls_port, addr_flags;
     spice_image_compression_t compression;
     spice_wan_compression_t wan_compr;
@@ -801,6 +805,14 @@ static void qemu_spice_init(void)
         spice_server_set_streaming_video(spice_server, SPICE_STREAM_VIDEO_OFF);
     }
 
+    video_codecs = qemu_opt_get(opts, "video-codecs");
+    if (video_codecs) {
+        if (spice_server_set_video_codecs(spice_server, video_codecs)) {
+            error_report("invalid video codecs");
+            exit(1);
+        }
+    }
+
     spice_server_set_agent_mouse
         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
     spice_server_set_playback_compression
-- 
2.49.0

[PATCH v4 3/7] ui/spice: Enable gl=on option for non-local or remote clients

[Vivek Kasireddy]

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 <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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 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 907b0e9a81..6c3bfe1d0f 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -848,9 +848,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 */
+            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"), DISPLAY_GL_MODE_ON, &error_fatal);
         spice_opengl = 1;
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 9c39d2c5c8..9140169015 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -31,6 +31,7 @@
 #include "standard-headers/drm/drm_fourcc.h"
 
 bool spice_opengl;
+bool remote_client;
 
 int qemu_spice_rect_is_empty(const QXLRect* r)
 {
-- 
2.49.0

[PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate

[Vivek Kasireddy]

In the specific case where the display layer (virtio-gpu) is using
dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
it makes sense to limit the maximum (streaming) rate (refresh rate)
to a fixed value using the GUI refresh timer. Otherwise, the updates
or gl_draw requests would be sent as soon as the Guest submits a new
frame which is not optimal as it would lead to increased network
traffic and wastage of GPU cycles if the frames get dropped.

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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h |  1 +
 qemu-options.hx            |  5 ++++
 ui/spice-core.c            | 11 ++++++++
 ui/spice-display.c         | 54 +++++++++++++++++++++++++++++++-------
 4 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index f4922dd74b..2fe524b59c 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -152,6 +152,7 @@ struct SimpleSpiceCursor {
 
 extern bool spice_opengl;
 extern bool remote_client;
+extern int max_refresh_rate;
 
 int qemu_spice_rect_is_empty(const QXLRect* r);
 void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
diff --git a/qemu-options.hx b/qemu-options.hx
index 97c63d9b31..4e9f4edfdc 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2282,6 +2282,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
     "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
     "       [,video-codecs=<encoder>:<codec>\n"
+    "       [,max-refresh-rate=rate\n"
     "       [,gl=[on|off]][,rendernode=<file>]\n"
     "                enable spice\n"
     "                at least one of {port, tls-port} is mandatory\n",
@@ -2374,6 +2375,10 @@ SRST
         Provide the preferred codec the Spice server should use.
         Default would be spice:mjpeg.
 
+    ``max-refresh-rate=rate``
+        Provide the maximum refresh rate (or FPS) at which the encoding
+        requests should be sent to the Spice server. Default would be 30.
+
     ``gl=[on|off]``
         Enable/disable OpenGL context. Default is off.
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 6c3bfe1d0f..d8925207b1 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -56,6 +56,8 @@ struct SpiceTimer {
     QEMUTimer *timer;
 };
 
+#define MAX_REFRESH_RATE 30
+
 static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
 {
     SpiceTimer *timer;
@@ -491,6 +493,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "video-codecs",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "max-refresh-rate",
+            .type = QEMU_OPT_NUMBER,
         },{
             .name = "agent-mouse",
             .type = QEMU_OPT_BOOL,
@@ -813,6 +818,12 @@ static void qemu_spice_init(void)
         }
     }
 
+    max_refresh_rate = qemu_opt_get_number(opts, "max-refresh-rate", MAX_REFRESH_RATE);
+    if (max_refresh_rate < 0) {
+        error_report("max refresh rate/fps is invalid");
+        exit(1);
+    }
+
     spice_server_set_agent_mouse
         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
     spice_server_set_playback_compression
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 9140169015..ed91521ac2 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -32,6 +32,7 @@
 
 bool spice_opengl;
 bool remote_client;
+int max_refresh_rate;
 
 int qemu_spice_rect_is_empty(const QXLRect* r)
 {
@@ -844,12 +845,32 @@ 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(&ssd->qxl, x, y, w, h, cookie);
+}
+
 static void spice_gl_refresh(DisplayChangeListener *dcl)
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
-    uint64_t cookie;
 
-    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 && ssd->have_scanout) {
+            glFlush();
+            spice_gl_draw(ssd, 0, 0,
+                          surface_width(ssd->ds), surface_height(ssd->ds));
+            ssd->gl_updates = 0;
+            /* To update at 60 FPS, update_interval needs to be ~16.66 ms */
+            dcl->update_interval = 1000 / max_refresh_rate;
+        }
         return;
     }
 
@@ -857,11 +878,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(&ssd->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;
     }
 }
@@ -954,6 +972,20 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
 
     trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
+
+    /*
+     * We need to check for the case of "lost" updates, where a gl_draw
+     * was not submitted because the timer did not get a chance to run.
+     * One case where this happens is when the Guest VM is getting
+     * rebooted. If the console is blocked in this situation, we need
+     * to unblock it. Otherwise, newer updates would not take effect.
+     */
+    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
+        if (remote_client && ssd->gl_updates && ssd->have_scanout) {
+            ssd->gl_updates = 0;
+            qemu_spice_gl_block(ssd, false);
+        }
+    }
     spice_server_gl_scanout(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
                             DRM_FORMAT_MOD_INVALID, false);
     qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
@@ -1061,7 +1093,6 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
-    uint64_t cookie;
     uint32_t width, height, texture;
 
     if (!ssd->have_scanout) {
@@ -1159,8 +1190,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 = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
-    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
+    if (remote_client) {
+        ssd->gl_updates++;
+    } else {
+        spice_gl_draw(ssd, x, y, w, h);
+    }
 }
 
 static const DisplayChangeListenerOps display_listener_gl_ops = {
-- 
2.49.0

[PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Vivek Kasireddy]

There are cases where we do not want the memory layout of a texture to
be tiled as the component processing the texture would not know how to
de-tile either via software or hardware. Therefore, ensuring that the
memory backing the texture has a linear layout is absolutely necessary
in these situations.

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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/console.h |  2 ++
 ui/console-gl.c      | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 46b3128185..5cfa6ae215 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -422,6 +422,8 @@ bool console_gl_check_format(DisplayChangeListener *dcl,
                              pixman_format_code_t format);
 void surface_gl_create_texture(QemuGLShader *gls,
                                DisplaySurface *surface);
+bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
+                                       int fd, GLuint *texture);
 void surface_gl_update_texture(QemuGLShader *gls,
                                DisplaySurface *surface,
                                int x, int y, int w, int h);
diff --git a/ui/console-gl.c b/ui/console-gl.c
index 103b954017..97f7989651 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -25,6 +25,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "ui/console.h"
 #include "ui/shader.h"
 
@@ -96,6 +97,37 @@ void surface_gl_create_texture(QemuGLShader *gls,
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
 }
 
+bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
+                                       int fd, GLuint *texture)
+{
+    unsigned long size = surface_stride(surface) * surface_height(surface);
+    GLenum err = glGetError();
+    GLuint mem_obj;
+
+    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
+        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
+        return false;
+    }
+
+#ifdef GL_EXT_memory_object_fd
+    glCreateMemoryObjectsEXT(1, &mem_obj);
+    glImportMemoryFdEXT(mem_obj, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, fd);
+
+    err = glGetError();
+    if (err != GL_NO_ERROR) {
+        error_report("spice: cannot import memory object from fd");
+        return false;
+    }
+
+    glGenTextures(1, texture);
+    glBindTexture(GL_TEXTURE_2D, *texture);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT, GL_LINEAR_TILING_EXT);
+    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8, surface_width(surface),
+                         surface_height(surface), mem_obj, 0);
+#endif
+    return *texture > 0 && glGetError() == GL_NO_ERROR;
+}
+
 void surface_gl_update_texture(QemuGLShader *gls,
                                DisplaySurface *surface,
                                int x, int y, int w, int h)
-- 
2.49.0

[PATCH v4 6/7] ui/spice: Create a new texture with linear layout when gl=on is enabled

[Vivek Kasireddy]

Since most encoders/decoders (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 need to first
create a tiled texture and import that as a memory object to create
a new texture that has a linear memory layout.

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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/spice-display.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index ed91521ac2..fb56da4ab0 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -893,6 +893,75 @@ static void spice_gl_update(DisplayChangeListener *dcl,
     ssd->gl_updates++;
 }
 
+static bool spice_gl_replace_fd_texture(SimpleSpiceDisplay *ssd,
+                                        int *fds, uint64_t *modifier,
+                                        int *num_planes)
+{
+    uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
+    GLuint texture = 0;
+    int fourcc;
+    bool ret;
+
+    if (!remote_client) {
+        return true;
+    }
+
+    if (*modifier == DRM_FORMAT_MOD_LINEAR) {
+        return true;
+    }
+
+    if (*num_planes > 1) {
+        error_report("spice: cannot replace texture with multiple planes");
+        return false;
+    }
+
+    /*
+     * We really want to ensure that the memory layout of the texture
+     * is linear; otherwise, the encoder's output may show corruption.
+     */
+    if (!surface_gl_create_texture_from_fd(ssd->ds, fds[0], &texture)) {
+        error_report("spice: cannot create new texture from fd");
+        return false;
+    }
+
+    /*
+     * 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.
+     */
+    ret = egl_dmabuf_export_texture(texture,
+                                    fds,
+                                    (EGLint *)offsets,
+                                    (EGLint *)strides,
+                                    &fourcc,
+                                    num_planes,
+                                    modifier);
+    if (!ret) {
+        glDeleteTextures(1, &texture);
+
+        /*
+         * Since we couldn't export our newly create texture (or create,
+         * an fd associated with it) we need to backtrack and try to
+         * recreate the fd associated with the original texture.
+         */
+        ret = egl_dmabuf_export_texture(ssd->ds->texture,
+                                        fds,
+                                        (EGLint *)offsets,
+                                        (EGLint *)strides,
+                                        &fourcc,
+                                        num_planes,
+                                        modifier);
+        if (!ret) {
+            surface_gl_destroy_texture(ssd->gls, ssd->ds);
+            warn_report("spice: no texture available to display");
+        }
+    } else {
+        surface_gl_destroy_texture(ssd->gls, ssd->ds);
+        ssd->ds->texture = texture;
+    }
+    return ret;
+}
+
 static void spice_server_gl_scanout(QXLInstance *qxl,
                                     const int *fd,
                                     uint32_t width, uint32_t height,
@@ -917,6 +986,7 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
                             struct DisplaySurface *new_surface)
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
+    bool ret;
 
     if (ssd->ds) {
         surface_gl_destroy_texture(ssd->gls, ssd->ds);
@@ -939,6 +1009,12 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
             return;
         }
 
+        ret = spice_gl_replace_fd_texture(ssd, fd, &modifier, &num_planes);
+        if (!ret) {
+            surface_gl_destroy_texture(ssd->gls, ssd->ds);
+            return;
+        }
+
         trace_qemu_spice_gl_surface(ssd->qxl.id,
                                     surface_width(ssd->ds),
                                     surface_height(ssd->ds),
-- 
2.49.0

[PATCH v4 7/7] ui/spice: Blit the scanout texture if its memory layout is not linear

[Vivek Kasireddy]

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>
Signed-off-by: Vivek Kasireddy <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 2fe524b59c..0f5e4e563b 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 fb56da4ab0..66b5cdd79e 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1080,7 +1080,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);
@@ -1092,11 +1092,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 (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;
 }
@@ -1162,6 +1177,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)
 {
@@ -1169,6 +1228,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) {
@@ -1263,6 +1323,15 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
         glFlush();
     }
 
+    if (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.49.0

Re: [PATCH v4 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3676 bytes --]

Hi

On Thu, May 15, 2025 at 4:49 AM Vivek Kasireddy <vivek.kasireddy@intel.com>
wrote:

> To address the limitation that this option is incompatible with
> remote clients, this patch series adds an option to select a
> preferred codec and also enable gl=on option for clients that
> are connected via the network. In other words, with this option
> enabled (and the below linked Spice series merged), it would be
> possible to have Qemu share a dmabuf fd with Spice, which would
> then forward it to a hardware or software based encoder and
> eventually send the data associated with the fd to a client that
> could be located on a different machine.
>
> Essentially, this patch series provides a hardware accelerated,
> opensource VDI option for users using Qemu and Spice by leveraging
> the iGPU/dGPU on the host machine to encode the Guest FB via the
> Gstreamer framework.
>
>
for v5, please fix the patches to pass scripts/checkpatch.pl.


> v3 -> v4 (suggestions from Marc-André):
> - Add a new parameter to make max_refresh_rate configurable
> - Have surface_gl_create_texture_from_fd() return bool after checking
>   for errors
> - Remove the check for PIXMAN_r5g6b5() in spice_gl_replace_fd_texture()
> - Report errors in spice_gl_replace_fd_texture() when someting fails
> - Use glGetError() correctly by adding an additional (dummy) call
>   before checking for actual errors (Dmitry)
> - Add a new patch to check fd values in egl_dmabuf_export_texture()
> - Rebase on Qemu master
>
> v2 -> v3:
> - Check for errors after invoking glImportMemoryFdEXT() using
>   glGetError() and report the error to user (Dmitry)
>
> v1 -> v2:
> - Replace the option name preferred-codec with video-codecs (Marc-André)
> - Add a warning when an fd cannot be created from texture (Marc-André)
> - Add a new patch to blit the scanout texture into a linear one to
>   make it work with virgl
> - Rebased and tested against the latest Spice master
>
> Tested with the following Qemu parameters:
> -device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
> -spice port=3001,gl=on,disable-ticketing=on,video-codecs=gstreamer:h264
>
> and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.
>
> Associated Spice server MR (merged):
> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229
>
> ---
> 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: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
>
> Vivek Kasireddy (7):
>   ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
>   ui/spice: Add an option for users to provide a preferred codec
>   ui/spice: Enable gl=on option for non-local or remote clients
>   ui/spice: Add an option to submit gl_draw requests at fixed rate
>   ui/console-gl: Add a helper to create a texture with linear memory
>     layout
>   ui/spice: Create a new texture with linear layout when gl=on is
>     enabled
>   ui/spice: Blit the scanout texture if its memory layout is not linear
>
>  include/ui/console.h       |   2 +
>  include/ui/spice-display.h |   5 +
>  qemu-options.hx            |  10 ++
>  ui/console-gl.c            |  32 ++++++
>  ui/egl-helpers.c           |   6 ++
>  ui/spice-core.c            |  27 +++++
>  ui/spice-display.c         | 212 ++++++++++++++++++++++++++++++++++---
>  7 files changed, 278 insertions(+), 16 deletions(-)
>
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 5020 bytes --]

Re: [PATCH v4 3/7] ui/spice: Enable gl=on option for non-local or remote clients

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

Hi

On Thu, May 15, 2025 at 4:49 AM Vivek Kasireddy <vivek.kasireddy@intel.com>
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 <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>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  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;
>

All the globals you introduce here should have a spice_ prefix, for
readability and consistency.


>
>  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 907b0e9a81..6c3bfe1d0f 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -848,9 +848,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 */
> +            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"), DISPLAY_GL_MODE_ON,
> &error_fatal);
>          spice_opengl = 1;
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 9c39d2c5c8..9140169015 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -31,6 +31,7 @@
>  #include "standard-headers/drm/drm_fourcc.h"
>
>  bool spice_opengl;
> +bool remote_client;
>
>  int qemu_spice_rect_is_empty(const QXLRect* r)
>  {
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 3694 bytes --]

Re: [PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 8129 bytes --]

On Thu, May 15, 2025 at 4:49 AM Vivek Kasireddy <vivek.kasireddy@intel.com>
wrote:

> In the specific case where the display layer (virtio-gpu) is using
> dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
> it makes sense to limit the maximum (streaming) rate (refresh rate)
> to a fixed value using the GUI refresh timer. Otherwise, the updates
> or gl_draw requests would be sent as soon as the Guest submits a new
> frame which is not optimal as it would lead to increased network
> traffic and wastage of GPU cycles if the frames get dropped.
>
> 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>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  include/ui/spice-display.h |  1 +
>  qemu-options.hx            |  5 ++++
>  ui/spice-core.c            | 11 ++++++++
>  ui/spice-display.c         | 54 +++++++++++++++++++++++++++++++-------
>  4 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> index f4922dd74b..2fe524b59c 100644
> --- a/include/ui/spice-display.h
> +++ b/include/ui/spice-display.h
> @@ -152,6 +152,7 @@ struct SimpleSpiceCursor {
>
>  extern bool spice_opengl;
>  extern bool remote_client;
> +extern int max_refresh_rate;
>

(use spice_ prefix)


>
>  int qemu_spice_rect_is_empty(const QXLRect* r);
>  void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 97c63d9b31..4e9f4edfdc 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2282,6 +2282,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
>      "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
>      "
>  [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
>      "       [,video-codecs=<encoder>:<codec>\n"
> +    "       [,max-refresh-rate=rate\n"
>      "       [,gl=[on|off]][,rendernode=<file>]\n"
>      "                enable spice\n"
>      "                at least one of {port, tls-port} is mandatory\n",
> @@ -2374,6 +2375,10 @@ SRST
>          Provide the preferred codec the Spice server should use.
>          Default would be spice:mjpeg.
>
> +    ``max-refresh-rate=rate``
> +        Provide the maximum refresh rate (or FPS) at which the encoding
> +        requests should be sent to the Spice server. Default would be 30.
> +
>      ``gl=[on|off]``
>          Enable/disable OpenGL context. Default is off.
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 6c3bfe1d0f..d8925207b1 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -56,6 +56,8 @@ struct SpiceTimer {
>      QEMUTimer *timer;
>  };
>
> +#define MAX_REFRESH_RATE 30
>

Better call it DEFAULT_MAX_REFRESH_RATE.


> +
>  static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
>  {
>      SpiceTimer *timer;
> @@ -491,6 +493,9 @@ static QemuOptsList qemu_spice_opts = {
>          },{
>              .name = "video-codecs",
>              .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "max-refresh-rate",
> +            .type = QEMU_OPT_NUMBER,
>          },{
>              .name = "agent-mouse",
>              .type = QEMU_OPT_BOOL,
> @@ -813,6 +818,12 @@ static void qemu_spice_init(void)
>          }
>      }
>
> +    max_refresh_rate = qemu_opt_get_number(opts, "max-refresh-rate",
> MAX_REFRESH_RATE);
> +    if (max_refresh_rate < 0) {
> +        error_report("max refresh rate/fps is invalid");
> +        exit(1);
> +    }
> +
>      spice_server_set_agent_mouse
>          (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
>      spice_server_set_playback_compression
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 9140169015..ed91521ac2 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -32,6 +32,7 @@
>
>  bool spice_opengl;
>  bool remote_client;
> +int max_refresh_rate;
>
>  int qemu_spice_rect_is_empty(const QXLRect* r)
>  {
> @@ -844,12 +845,32 @@ 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(&ssd->qxl, x, y, w, h, cookie);
> +}
> +
>  static void spice_gl_refresh(DisplayChangeListener *dcl)
>  {
>      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> -    uint64_t cookie;
>
> -    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 && ssd->have_scanout) {
> +            glFlush();
> +            spice_gl_draw(ssd, 0, 0,
> +                          surface_width(ssd->ds),
> surface_height(ssd->ds));
> +            ssd->gl_updates = 0;
> +            /* To update at 60 FPS, update_interval needs to be ~16.66 ms
> */
> +            dcl->update_interval = 1000 / max_refresh_rate;
> +        }
>          return;
>      }
>
> @@ -857,11 +878,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(&ssd->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;
>      }
>  }
> @@ -954,6 +972,20 @@ static void
> qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
>      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
>
>      trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> +
> +    /*
> +     * We need to check for the case of "lost" updates, where a gl_draw
> +     * was not submitted because the timer did not get a chance to run.
> +     * One case where this happens is when the Guest VM is getting
> +     * rebooted. If the console is blocked in this situation, we need
> +     * to unblock it. Otherwise, newer updates would not take effect.
> +     */
> +    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
> +        if (remote_client && ssd->gl_updates && ssd->have_scanout) {
> +            ssd->gl_updates = 0;
> +            qemu_spice_gl_block(ssd, false);
> +        }
> +    }
>      spice_server_gl_scanout(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0,
> DRM_FORMAT_INVALID,
>                              DRM_FORMAT_MOD_INVALID, false);
>      qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> @@ -1061,7 +1093,6 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
>      EGLint fourcc = 0;
>      bool render_cursor = false;
>      bool y_0_top = false; /* FIXME */
> -    uint64_t cookie;
>      uint32_t width, height, texture;
>
>      if (!ssd->have_scanout) {
> @@ -1159,8 +1190,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 = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> -    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> +    if (remote_client) {
> +        ssd->gl_updates++;
> +    } else {
> +        spice_gl_draw(ssd, x, y, w, h);
> +    }
>  }
>
>  static const DisplayChangeListenerOps display_listener_gl_ops = {
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 10551 bytes --]

Re: [PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 8308 bytes --]

Hi

On Thu, May 15, 2025 at 4:49 AM Vivek Kasireddy <vivek.kasireddy@intel.com>
wrote:

> In the specific case where the display layer (virtio-gpu) is using
> dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
> it makes sense to limit the maximum (streaming) rate (refresh rate)
> to a fixed value using the GUI refresh timer. Otherwise, the updates
> or gl_draw requests would be sent as soon as the Guest submits a new
> frame which is not optimal as it would lead to increased network
> traffic and wastage of GPU cycles if the frames get dropped.
>

> 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>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  include/ui/spice-display.h |  1 +
>  qemu-options.hx            |  5 ++++
>  ui/spice-core.c            | 11 ++++++++
>  ui/spice-display.c         | 54 +++++++++++++++++++++++++++++++-------
>  4 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> index f4922dd74b..2fe524b59c 100644
> --- a/include/ui/spice-display.h
> +++ b/include/ui/spice-display.h
> @@ -152,6 +152,7 @@ struct SimpleSpiceCursor {
>
>  extern bool spice_opengl;
>  extern bool remote_client;
> +extern int max_refresh_rate;
>
>  int qemu_spice_rect_is_empty(const QXLRect* r);
>  void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 97c63d9b31..4e9f4edfdc 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2282,6 +2282,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
>      "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
>      "
>  [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
>      "       [,video-codecs=<encoder>:<codec>\n"
> +    "       [,max-refresh-rate=rate\n"
>      "       [,gl=[on|off]][,rendernode=<file>]\n"
>      "                enable spice\n"
>      "                at least one of {port, tls-port} is mandatory\n",
> @@ -2374,6 +2375,10 @@ SRST
>          Provide the preferred codec the Spice server should use.
>          Default would be spice:mjpeg.
>
> +    ``max-refresh-rate=rate``
> +        Provide the maximum refresh rate (or FPS) at which the encoding
> +        requests should be sent to the Spice server. Default would be 30.
> +
>      ``gl=[on|off]``
>          Enable/disable OpenGL context. Default is off.
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 6c3bfe1d0f..d8925207b1 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -56,6 +56,8 @@ struct SpiceTimer {
>      QEMUTimer *timer;
>  };
>
> +#define MAX_REFRESH_RATE 30
> +
>  static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
>  {
>      SpiceTimer *timer;
> @@ -491,6 +493,9 @@ static QemuOptsList qemu_spice_opts = {
>          },{
>              .name = "video-codecs",
>              .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "max-refresh-rate",
> +            .type = QEMU_OPT_NUMBER,
>          },{
>              .name = "agent-mouse",
>              .type = QEMU_OPT_BOOL,
> @@ -813,6 +818,12 @@ static void qemu_spice_init(void)
>          }
>      }
>
> +    max_refresh_rate = qemu_opt_get_number(opts, "max-refresh-rate",
> MAX_REFRESH_RATE);
> +    if (max_refresh_rate < 0) {
> +        error_report("max refresh rate/fps is invalid");
> +        exit(1);
> +    }
> +
>      spice_server_set_agent_mouse
>          (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
>      spice_server_set_playback_compression
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 9140169015..ed91521ac2 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -32,6 +32,7 @@
>
>  bool spice_opengl;
>  bool remote_client;
> +int max_refresh_rate;
>
>  int qemu_spice_rect_is_empty(const QXLRect* r)
>  {
> @@ -844,12 +845,32 @@ 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(&ssd->qxl, x, y, w, h, cookie);
> +}
> +
>  static void spice_gl_refresh(DisplayChangeListener *dcl)
>  {
>      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> -    uint64_t cookie;
>
> -    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 && ssd->have_scanout) {
> +            glFlush();
> +            spice_gl_draw(ssd, 0, 0,
> +                          surface_width(ssd->ds),
> surface_height(ssd->ds));
> +            ssd->gl_updates = 0;
> +            /* To update at 60 FPS, update_interval needs to be ~16.66 ms
> */
> +            dcl->update_interval = 1000 / max_refresh_rate;
> +        }
>          return;
>      }
>
> @@ -857,11 +878,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(&ssd->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;
>      }
>  }
> @@ -954,6 +972,20 @@ static void
> qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
>      SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
>
>      trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> +
> +    /*
> +     * We need to check for the case of "lost" updates, where a gl_draw
> +     * was not submitted because the timer did not get a chance to run.
> +     * One case where this happens is when the Guest VM is getting
> +     * rebooted. If the console is blocked in this situation, we need
> +     * to unblock it. Otherwise, newer updates would not take effect.
> +     */
> +    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
> +        if (remote_client && ssd->gl_updates && ssd->have_scanout) {
> +            ssd->gl_updates = 0;
> +            qemu_spice_gl_block(ssd, false);
> +        }
> +    }
>      spice_server_gl_scanout(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0,
> DRM_FORMAT_INVALID,
>                              DRM_FORMAT_MOD_INVALID, false);
>      qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> @@ -1061,7 +1093,6 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
>      EGLint fourcc = 0;
>      bool render_cursor = false;
>      bool y_0_top = false; /* FIXME */
> -    uint64_t cookie;
>      uint32_t width, height, texture;
>
>      if (!ssd->have_scanout) {
> @@ -1159,8 +1190,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 = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
> -    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
> +    if (remote_client) {
> +        ssd->gl_updates++;
> +    } else {
> +        spice_gl_draw(ssd, x, y, w, h);
> +    }
>

I think this is not the right place to handle the remote vs local case. It
should be done at the spice server level, since the server can serve
various sockets / connections (not just the one it listens to, but the one
it has been given).


>  }
>
>  static const DisplayChangeListenerOps display_listener_gl_ops = {
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 10681 bytes --]

Re: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Dmitry Osipenko]

On 5/15/25 05:45, Vivek Kasireddy wrote:
> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> +                                       int fd, GLuint *texture)
> +{
> +    unsigned long size = surface_stride(surface) * surface_height(surface);
> +    GLenum err = glGetError();
> +    GLuint mem_obj;
> +
> +    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
> +        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
> +        return false;
> +    }
> +
> +#ifdef GL_EXT_memory_object_fd
> +    glCreateMemoryObjectsEXT(1, &mem_obj);
> +    glImportMemoryFdEXT(mem_obj, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, fd);
> +
> +    err = glGetError();
> +    if (err != GL_NO_ERROR) {
> +        error_report("spice: cannot import memory object from fd");
> +        return false;
> +    }

mem_obj should be destroyed on error and when created texture is released

-- 
Best regards,
Dmitry

[PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Michael Scherle]

Great to see this patch making progress.

I've tested it extensively, and unfortunately, I’ve noticed a memory 
leak in surface_gl_create_texture_from_fd(). The memory leak is hard to 
see since the memory is owned by the gpu driver.
On Intel hardware, it's possible to observe the leak using:

cat /sys/module/i915/refcnt
or
xpu-smi ps

In on of my use case—which involves frequent scanout disable/enable 
cycles—the leak is quite apparent. However, in more typical scenarios, 
it might be difficult to catch.

The issue stems from the mem_obj not being deleted after use. I’ve put 
together a minimal modification to address it:



On 15.05.25 04:45, Vivek Kasireddy wrote:
> There are cases where we do not want the memory layout of a texture to
> be tiled as the component processing the texture would not know how to
> de-tile either via software or hardware. Therefore, ensuring that the
> memory backing the texture has a linear layout is absolutely necessary
> in these situations.
> 
> 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>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   include/ui/console.h |  2 ++
>   ui/console-gl.c      | 32 ++++++++++++++++++++++++++++++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 46b3128185..5cfa6ae215 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -422,6 +422,8 @@ bool console_gl_check_format(DisplayChangeListener *dcl,
>                                pixman_format_code_t format);
>   void surface_gl_create_texture(QemuGLShader *gls,
>                                  DisplaySurface *surface);
> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> +                                       int fd, GLuint *texture);
>   void surface_gl_update_texture(QemuGLShader *gls,
>                                  DisplaySurface *surface,
>                                  int x, int y, int w, int h);
> diff --git a/ui/console-gl.c b/ui/console-gl.c
> index 103b954017..97f7989651 100644
> --- a/ui/console-gl.c
> +++ b/ui/console-gl.c
> @@ -25,6 +25,7 @@
>    * THE SOFTWARE.
>    */
>   #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>   #include "ui/console.h"
>   #include "ui/shader.h"
>   
> @@ -96,6 +97,37 @@ void surface_gl_create_texture(QemuGLShader *gls,
>       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
>   }
>   
> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> +                                       int fd, GLuint *texture)
> +{
> +    unsigned long size = surface_stride(surface) * surface_height(surface);
> +    GLenum err = glGetError();
> +    GLuint mem_obj;

+    GLuint mem_obj = 0;

> +
> +    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
> +        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
> +        return false;
> +    }
> +
> +#ifdef GL_EXT_memory_object_fd
> +    glCreateMemoryObjectsEXT(1, &mem_obj);
> +    glImportMemoryFdEXT(mem_obj, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, fd);
> +
> +    err = glGetError();
> +    if (err != GL_NO_ERROR) {

+          if (mem_obj) {
+              glDeleteMemoryObjectsEXT(1, &mem_obj);
+          }

> +        error_report("spice: cannot import memory object from fd");
> +        return false;
> +    }
> +
> +    glGenTextures(1, texture);
> +    glBindTexture(GL_TEXTURE_2D, *texture);
> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT, GL_LINEAR_TILING_EXT);
> +    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8, surface_width(surface),
> +                         surface_height(surface), mem_obj, 0);
> +#endif

+    glDeleteMemoryObjectsEXT(1, &mem_obj);

> +    return *texture > 0 && glGetError() == GL_NO_ERROR;
> +}
> +
>   void surface_gl_update_texture(QemuGLShader *gls,
>                                  DisplaySurface *surface,
>                                  int x, int y, int w, int h)



That said, my OpenGL knowledge is somewhat limited, and the 
documentation wasn’t entirely clear to me on whether deleting the memory 
object while the texture is still being used, is always safe. Based on a 
quick look at the iris and llvmpipe implementations, it appears to be 
acceptable.

If that's not the case, an alternative fix could follow this approach 
instead: 
https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/commit/4ca806871c141089be16af25c1820d3e04f3e27d

Greetings Michael

Re: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Michael Scherle]

Hi all,

I just noticed that Dmitry Osipenko had already pointed out a similar 
issue earlier—so my message was somewhat redundant. Apologies for the 
duplication.

Also, I made a small mistake in the patch I proposed:
The call to glDeleteMemoryObjectsEXT(1, &mem_obj); should be placed 
above the #endif, not after it. Sorry about that oversight!

Best regards,
Michael


On 26.05.25 15:06, Michael Scherle wrote:
> Great to see this patch making progress.
> 
> I've tested it extensively, and unfortunately, I’ve noticed a memory 
> leak in surface_gl_create_texture_from_fd(). The memory leak is hard to 
> see since the memory is owned by the gpu driver.
> On Intel hardware, it's possible to observe the leak using:
> 
> cat /sys/module/i915/refcnt
> or
> xpu-smi ps
> 
> In on of my use case—which involves frequent scanout disable/enable 
> cycles—the leak is quite apparent. However, in more typical scenarios, 
> it might be difficult to catch.
> 
> The issue stems from the mem_obj not being deleted after use. I’ve put 
> together a minimal modification to address it:
> 
> 
> 
> On 15.05.25 04:45, Vivek Kasireddy wrote:
>> There are cases where we do not want the memory layout of a texture to
>> be tiled as the component processing the texture would not know how to
>> de-tile either via software or hardware. Therefore, ensuring that the
>> memory backing the texture has a linear layout is absolutely necessary
>> in these situations.
>>
>> 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>
>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> ---
>>   include/ui/console.h |  2 ++
>>   ui/console-gl.c      | 32 ++++++++++++++++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/include/ui/console.h b/include/ui/console.h
>> index 46b3128185..5cfa6ae215 100644
>> --- a/include/ui/console.h
>> +++ b/include/ui/console.h
>> @@ -422,6 +422,8 @@ bool console_gl_check_format(DisplayChangeListener 
>> *dcl,
>>                                pixman_format_code_t format);
>>   void surface_gl_create_texture(QemuGLShader *gls,
>>                                  DisplaySurface *surface);
>> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
>> +                                       int fd, GLuint *texture);
>>   void surface_gl_update_texture(QemuGLShader *gls,
>>                                  DisplaySurface *surface,
>>                                  int x, int y, int w, int h);
>> diff --git a/ui/console-gl.c b/ui/console-gl.c
>> index 103b954017..97f7989651 100644
>> --- a/ui/console-gl.c
>> +++ b/ui/console-gl.c
>> @@ -25,6 +25,7 @@
>>    * THE SOFTWARE.
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "ui/console.h"
>>   #include "ui/shader.h"
>> @@ -96,6 +97,37 @@ void surface_gl_create_texture(QemuGLShader *gls,
>>       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
>>   }
>> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
>> +                                       int fd, GLuint *texture)
>> +{
>> +    unsigned long size = surface_stride(surface) * 
>> surface_height(surface);
>> +    GLenum err = glGetError();
>> +    GLuint mem_obj;
> 
> +    GLuint mem_obj = 0;
> 
>> +
>> +    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
>> +        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
>> +        return false;
>> +    }
>> +
>> +#ifdef GL_EXT_memory_object_fd
>> +    glCreateMemoryObjectsEXT(1, &mem_obj);
>> +    glImportMemoryFdEXT(mem_obj, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, 
>> fd);
>> +
>> +    err = glGetError();
>> +    if (err != GL_NO_ERROR) {
> 
> +          if (mem_obj) {
> +              glDeleteMemoryObjectsEXT(1, &mem_obj);
> +          }
> 
>> +        error_report("spice: cannot import memory object from fd");
>> +        return false;
>> +    }
>> +
>> +    glGenTextures(1, texture);
>> +    glBindTexture(GL_TEXTURE_2D, *texture);
>> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT, 
>> GL_LINEAR_TILING_EXT);
>> +    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8, 
>> surface_width(surface),
>> +                         surface_height(surface), mem_obj, 0);
>> +#endif
> 
> +    glDeleteMemoryObjectsEXT(1, &mem_obj);
> 
>> +    return *texture > 0 && glGetError() == GL_NO_ERROR;
>> +}
>> +
>>   void surface_gl_update_texture(QemuGLShader *gls,
>>                                  DisplaySurface *surface,
>>                                  int x, int y, int w, int h)
> 
> 
> 
> That said, my OpenGL knowledge is somewhat limited, and the 
> documentation wasn’t entirely clear to me on whether deleting the memory 
> object while the texture is still being used, is always safe. Based on a 
> quick look at the iris and llvmpipe implementations, it appears to be 
> acceptable.
> 
> If that's not the case, an alternative fix could follow this approach 
> instead: https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/ 
> commit/4ca806871c141089be16af25c1820d3e04f3e27d
> 
> Greetings Michael

RE: [PATCH v4 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Kasireddy, Vivek]

Hi Marc-André,

> Subject: Re: [PATCH v4 0/7] ui/spice: Enable gl=on option for non-local or
> remote clients
> 
> Hi
> 
> On Thu, May 15, 2025 at 4:49 AM Vivek Kasireddy
> <vivek.kasireddy@intel.com <mailto:vivek.kasireddy@intel.com> > wrote:
> 
> 
> 	To address the limitation that this option is incompatible with
> 	remote clients, this patch series adds an option to select a
> 	preferred codec and also enable gl=on option for clients that
> 	are connected via the network. In other words, with this option
> 	enabled (and the below linked Spice series merged), it would be
> 	possible to have Qemu share a dmabuf fd with Spice, which would
> 	then forward it to a hardware or software based encoder and
> 	eventually send the data associated with the fd to a client that
> 	could be located on a different machine.
> 
> 	Essentially, this patch series provides a hardware accelerated,
> 	opensource VDI option for users using Qemu and Spice by leveraging
> 	the iGPU/dGPU on the host machine to encode the Guest FB via the
> 	Gstreamer framework.
> 
> 
> 
> 
> for v5, please fix the patches to pass scripts/checkpatch.pl
> <http://checkpatch.pl> .
Sure, will do this and include all your suggestions in v5.

Thanks,
Vivek

> 
> 
> 
> 	v3 -> v4 (suggestions from Marc-André):
> 	- Add a new parameter to make max_refresh_rate configurable
> 	- Have surface_gl_create_texture_from_fd() return bool after checking
> 	  for errors
> 	- Remove the check for PIXMAN_r5g6b5() in
> spice_gl_replace_fd_texture()
> 	- Report errors in spice_gl_replace_fd_texture() when someting fails
> 	- Use glGetError() correctly by adding an additional (dummy) call
> 	  before checking for actual errors (Dmitry)
> 	- Add a new patch to check fd values in egl_dmabuf_export_texture()
> 	- Rebase on Qemu master
> 
> 	v2 -> v3:
> 	- Check for errors after invoking glImportMemoryFdEXT() using
> 	  glGetError() and report the error to user (Dmitry)
> 
> 	v1 -> v2:
> 	- Replace the option name preferred-codec with video-codecs (Marc-
> André)
> 	- Add a warning when an fd cannot be created from texture (Marc-
> André)
> 	- Add a new patch to blit the scanout texture into a linear one to
> 	  make it work with virgl
> 	- Rebased and tested against the latest Spice master
> 
> 	Tested with the following Qemu parameters:
> 	-device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
> 	-spice port=3001,gl=on,disable-ticketing=on,video-
> codecs=gstreamer:h264
> 
> 	and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.
> 
> 	Associated Spice server MR (merged):
> 	https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229
> 
> 	---
> 	Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>
> >
> 	Cc: Marc-André Lureau <marcandre.lureau@redhat.com
> <mailto:marcandre.lureau@redhat.com> >
> 	Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com
> <mailto:dmitry.osipenko@collabora.com> >
> 	Cc: Frediano Ziglio <freddy77@gmail.com
> <mailto:freddy77@gmail.com> >
> 	Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de
> <mailto:michael.scherle@rz.uni-freiburg.de> >
> 	Cc: Dongwon Kim <dongwon.kim@intel.com
> <mailto:dongwon.kim@intel.com> >
> 	Cc: Alex Bennée <alex.bennee@linaro.org
> <mailto:alex.bennee@linaro.org> >
> 
> 	Vivek Kasireddy (7):
> 	  ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
> 	  ui/spice: Add an option for users to provide a preferred codec
> 	  ui/spice: Enable gl=on option for non-local or remote clients
> 	  ui/spice: Add an option to submit gl_draw requests at fixed rate
> 	  ui/console-gl: Add a helper to create a texture with linear memory
> 	    layout
> 	  ui/spice: Create a new texture with linear layout when gl=on is
> 	    enabled
> 	  ui/spice: Blit the scanout texture if its memory layout is not linear
> 
> 	 include/ui/console.h       |   2 +
> 	 include/ui/spice-display.h |   5 +
> 	 qemu-options.hx            |  10 ++
> 	 ui/console-gl.c            |  32 ++++++
> 	 ui/egl-helpers.c           |   6 ++
> 	 ui/spice-core.c            |  27 +++++
> 	 ui/spice-display.c         | 212 ++++++++++++++++++++++++++++++++++-
> --
> 	 7 files changed, 278 insertions(+), 16 deletions(-)
> 
> 	--
> 	2.49.0
> 
> 

RE: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Kasireddy, Vivek]

Hi Michael,

> Subject: Re: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture
> with linear memory layout
> 
> Hi all,
> 
> I just noticed that Dmitry Osipenko had already pointed out a similar issue
> earlier-so my message was somewhat redundant. Apologies for the
> duplication.
Yeah, looks like you and Dmitry identified the leak independently, almost at the
same time.

> 
> Also, I made a small mistake in the patch I proposed:
> The call to glDeleteMemoryObjectsEXT(1, &mem_obj); should be placed
> above the #endif, not after it. Sorry about that oversight!
Your patch from Open Source VDI repo seems like a better solution, so, I'll add it
to this series and send it out for review in v5. Please add description/commit message
and your signed-off-by line to it. 

Thanks,
Vivek

> 
> Best regards,
> Michael
> 
> 
> On 26.05.25 15:06, Michael Scherle wrote:
> > Great to see this patch making progress.
> >
> > I've tested it extensively, and unfortunately, I've noticed a memory
> > leak in surface_gl_create_texture_from_fd(). The memory leak is hard
> > to see since the memory is owned by the gpu driver.
> > On Intel hardware, it's possible to observe the leak using:
> >
> > cat /sys/module/i915/refcnt
> > or
> > xpu-smi ps
> >
> > In on of my use case-which involves frequent scanout disable/enable
> > cycles-the leak is quite apparent. However, in more typical scenarios,
> > it might be difficult to catch.
> >
> > The issue stems from the mem_obj not being deleted after use. I've put
> > together a minimal modification to address it:
> >
> >
> >
> > On 15.05.25 04:45, Vivek Kasireddy wrote:
> >> There are cases where we do not want the memory layout of a texture
> >> to be tiled as the component processing the texture would not know
> >> how to de-tile either via software or hardware. Therefore, ensuring
> >> that the memory backing the texture has a linear layout is absolutely
> >> necessary in these situations.
> >>
> >> 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>
> >> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >> ---
> >>   include/ui/console.h |  2 ++
> >>   ui/console-gl.c      | 32 ++++++++++++++++++++++++++++++++
> >>   2 files changed, 34 insertions(+)
> >>
> >> diff --git a/include/ui/console.h b/include/ui/console.h index
> >> 46b3128185..5cfa6ae215 100644
> >> --- a/include/ui/console.h
> >> +++ b/include/ui/console.h
> >> @@ -422,6 +422,8 @@ bool
> >> console_gl_check_format(DisplayChangeListener
> >> *dcl,
> >>                                pixman_format_code_t format);
> >>   void surface_gl_create_texture(QemuGLShader *gls,
> >>                                  DisplaySurface *surface);
> >> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> >> +                                       int fd, GLuint *texture);
> >>   void surface_gl_update_texture(QemuGLShader *gls,
> >>                                  DisplaySurface *surface,
> >>                                  int x, int y, int w, int h); diff
> >> --git a/ui/console-gl.c b/ui/console-gl.c index
> >> 103b954017..97f7989651 100644
> >> --- a/ui/console-gl.c
> >> +++ b/ui/console-gl.c
> >> @@ -25,6 +25,7 @@
> >>    * THE SOFTWARE.
> >>    */
> >>   #include "qemu/osdep.h"
> >> +#include "qemu/error-report.h"
> >>   #include "ui/console.h"
> >>   #include "ui/shader.h"
> >> @@ -96,6 +97,37 @@ void surface_gl_create_texture(QemuGLShader *gls,
> >>       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
> >> GL_LINEAR);
> >>   }
> >> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
> >> +                                       int fd, GLuint *texture) {
> >> +    unsigned long size = surface_stride(surface) *
> >> surface_height(surface);
> >> +    GLenum err = glGetError();
> >> +    GLuint mem_obj;
> >
> > +    GLuint mem_obj = 0;
> >
> >> +
> >> +    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
> >> +        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
> >> +        return false;
> >> +    }
> >> +
> >> +#ifdef GL_EXT_memory_object_fd
> >> +    glCreateMemoryObjectsEXT(1, &mem_obj);
> >> +    glImportMemoryFdEXT(mem_obj, size,
> GL_HANDLE_TYPE_OPAQUE_FD_EXT,
> >> fd);
> >> +
> >> +    err = glGetError();
> >> +    if (err != GL_NO_ERROR) {
> >
> > +          if (mem_obj) {
> > +              glDeleteMemoryObjectsEXT(1, &mem_obj);
> > +          }
> >
> >> +        error_report("spice: cannot import memory object from fd");
> >> +        return false;
> >> +    }
> >> +
> >> +    glGenTextures(1, texture);
> >> +    glBindTexture(GL_TEXTURE_2D, *texture);
> >> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT,
> >> GL_LINEAR_TILING_EXT);
> >> +    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8,
> >> surface_width(surface),
> >> +                         surface_height(surface), mem_obj, 0);
> >> +#endif
> >
> > +    glDeleteMemoryObjectsEXT(1, &mem_obj);
> >
> >> +    return *texture > 0 && glGetError() == GL_NO_ERROR; }
> >> +
> >>   void surface_gl_update_texture(QemuGLShader *gls,
> >>                                  DisplaySurface *surface,
> >>                                  int x, int y, int w, int h)
> >
> >
> >
> > That said, my OpenGL knowledge is somewhat limited, and the
> > documentation wasn't entirely clear to me on whether deleting the
> > memory object while the texture is still being used, is always safe.
> > Based on a quick look at the iris and llvmpipe implementations, it
> > appears to be acceptable.
> >
> > If that's not the case, an alternative fix could follow this approach
> > instead: https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/
> > commit/4ca806871c141089be16af25c1820d3e04f3e27d
> >
> > Greetings Michael

RE: [PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate

[Kasireddy, Vivek]

Hi Marc-André,

> Hi
> 
> On Thu, May 15, 2025 at 4:49 AM Vivek Kasireddy
> <vivek.kasireddy@intel.com <mailto:vivek.kasireddy@intel.com> > wrote:
> 
> 
> 	In the specific case where the display layer (virtio-gpu) is using
> 	dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
> 	it makes sense to limit the maximum (streaming) rate (refresh rate)
> 	to a fixed value using the GUI refresh timer. Otherwise, the updates
> 	or gl_draw requests would be sent as soon as the Guest submits a new
> 	frame which is not optimal as it would lead to increased network
> 	traffic and wastage of GPU cycles if the frames get dropped.
> 
> 
> 
> 	Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>
> >
> 	Cc: Marc-André Lureau <marcandre.lureau@redhat.com
> <mailto:marcandre.lureau@redhat.com> >
> 	Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com
> <mailto:dmitry.osipenko@collabora.com> >
> 	Cc: Frediano Ziglio <freddy77@gmail.com
> <mailto:freddy77@gmail.com> >
> 	Cc: Dongwon Kim <dongwon.kim@intel.com
> <mailto:dongwon.kim@intel.com> >
> 	Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com
> <mailto:vivek.kasireddy@intel.com> >
> 	---
> 	 include/ui/spice-display.h |  1 +
> 	 qemu-options.hx            |  5 ++++
> 	 ui/spice-core.c            | 11 ++++++++
> 	 ui/spice-display.c         | 54 +++++++++++++++++++++++++++++++-------
> 	 4 files changed, 61 insertions(+), 10 deletions(-)
> 
> 	diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> 	index f4922dd74b..2fe524b59c 100644
> 	--- a/include/ui/spice-display.h
> 	+++ b/include/ui/spice-display.h
> 	@@ -152,6 +152,7 @@ struct SimpleSpiceCursor {
> 
> 	 extern bool spice_opengl;
> 	 extern bool remote_client;
> 	+extern int max_refresh_rate;
> 
> 	 int qemu_spice_rect_is_empty(const QXLRect* r);
> 	 void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> 	diff --git a/qemu-options.hx b/qemu-options.hx
> 	index 97c63d9b31..4e9f4edfdc 100644
> 	--- a/qemu-options.hx
> 	+++ b/qemu-options.hx
> 	@@ -2282,6 +2282,7 @@ DEF("spice", HAS_ARG,
> QEMU_OPTION_spice,
> 	     "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
> 	     "       [,playback-compression=[on|off]][,seamless-
> migration=[on|off]]\n"
> 	     "       [,video-codecs=<encoder>:<codec>\n"
> 	+    "       [,max-refresh-rate=rate\n"
> 	     "       [,gl=[on|off]][,rendernode=<file>]\n"
> 	     "                enable spice\n"
> 	     "                at least one of {port, tls-port} is mandatory\n",
> 	@@ -2374,6 +2375,10 @@ SRST
> 	         Provide the preferred codec the Spice server should use.
> 	         Default would be spice:mjpeg.
> 
> 	+    ``max-refresh-rate=rate``
> 	+        Provide the maximum refresh rate (or FPS) at which the encoding
> 	+        requests should be sent to the Spice server. Default would be 30.
> 	+
> 	     ``gl=[on|off]``
> 	         Enable/disable OpenGL context. Default is off.
> 
> 	diff --git a/ui/spice-core.c b/ui/spice-core.c
> 	index 6c3bfe1d0f..d8925207b1 100644
> 	--- a/ui/spice-core.c
> 	+++ b/ui/spice-core.c
> 	@@ -56,6 +56,8 @@ struct SpiceTimer {
> 	     QEMUTimer *timer;
> 	 };
> 
> 	+#define MAX_REFRESH_RATE 30
> 	+
> 	 static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
> 	 {
> 	     SpiceTimer *timer;
> 	@@ -491,6 +493,9 @@ static QemuOptsList qemu_spice_opts = {
> 	         },{
> 	             .name = "video-codecs",
> 	             .type = QEMU_OPT_STRING,
> 	+        },{
> 	+            .name = "max-refresh-rate",
> 	+            .type = QEMU_OPT_NUMBER,
> 	         },{
> 	             .name = "agent-mouse",
> 	             .type = QEMU_OPT_BOOL,
> 	@@ -813,6 +818,12 @@ static void qemu_spice_init(void)
> 	         }
> 	     }
> 
> 	+    max_refresh_rate = qemu_opt_get_number(opts, "max-refresh-
> rate", MAX_REFRESH_RATE);
> 	+    if (max_refresh_rate < 0) {
> 	+        error_report("max refresh rate/fps is invalid");
> 	+        exit(1);
> 	+    }
> 	+
> 	     spice_server_set_agent_mouse
> 	         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
> 	     spice_server_set_playback_compression
> 	diff --git a/ui/spice-display.c b/ui/spice-display.c
> 	index 9140169015..ed91521ac2 100644
> 	--- a/ui/spice-display.c
> 	+++ b/ui/spice-display.c
> 	@@ -32,6 +32,7 @@
> 
> 	 bool spice_opengl;
> 	 bool remote_client;
> 	+int max_refresh_rate;
> 
> 	 int qemu_spice_rect_is_empty(const QXLRect* r)
> 	 {
> 	@@ -844,12 +845,32 @@ 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(&ssd->qxl, x, y, w, h, cookie);
> 	+}
> 	+
> 	 static void spice_gl_refresh(DisplayChangeListener *dcl)
> 	 {
> 	     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay,
> dcl);
> 	-    uint64_t cookie;
> 
> 	-    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 && ssd->have_scanout) {
> 	+            glFlush();
> 	+            spice_gl_draw(ssd, 0, 0,
> 	+                          surface_width(ssd->ds), surface_height(ssd->ds));
> 	+            ssd->gl_updates = 0;
> 	+            /* To update at 60 FPS, update_interval needs to be ~16.66 ms
> */
> 	+            dcl->update_interval = 1000 / max_refresh_rate;
> 	+        }
> 	         return;
> 	     }
> 
> 	@@ -857,11 +878,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(&ssd->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;
> 	     }
> 	 }
> 	@@ -954,6 +972,20 @@ static void
> qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> 	     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay,
> dcl);
> 
> 	     trace_qemu_spice_gl_scanout_disable(ssd->qxl.id <http://qxl.id> );
> 	+
> 	+    /*
> 	+     * We need to check for the case of "lost" updates, where a gl_draw
> 	+     * was not submitted because the timer did not get a chance to run.
> 	+     * One case where this happens is when the Guest VM is getting
> 	+     * rebooted. If the console is blocked in this situation, we need
> 	+     * to unblock it. Otherwise, newer updates would not take effect.
> 	+     */
> 	+    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
> 	+        if (remote_client && ssd->gl_updates && ssd->have_scanout) {
> 	+            ssd->gl_updates = 0;
> 	+            qemu_spice_gl_block(ssd, false);
> 	+        }
> 	+    }
> 	     spice_server_gl_scanout(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0,
> DRM_FORMAT_INVALID,
> 	                             DRM_FORMAT_MOD_INVALID, false);
> 	     qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> 	@@ -1061,7 +1093,6 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> 	     EGLint fourcc = 0;
> 	     bool render_cursor = false;
> 	     bool y_0_top = false; /* FIXME */
> 	-    uint64_t cookie;
> 	     uint32_t width, height, texture;
> 
> 	     if (!ssd->have_scanout) {
> 	@@ -1159,8 +1190,11 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> 	     trace_qemu_spice_gl_update(ssd->qxl.id <http://qxl.id> , w, h, x, y);
> 	     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(&ssd->qxl, x, y, w, h, cookie);
> 	+    if (remote_client) {
> 	+        ssd->gl_updates++;
> 	+    } else {
> 	+        spice_gl_draw(ssd, x, y, w, h);
> 	+    }
> 
> 
> 
> I think this is not the right place to handle the remote vs local case. It should
> be done at the spice server level, since the server can serve various sockets /
> connections (not just the one it listens to, but the one it has been given).
What I am doing here is just postponing the submission of gl_draw request (to
spice server) for the remote client case. In other words, instead of submitting
the gl_draw request right away from qemu_spice_gl_update(), it will now be
submitted from the spice_gl_refresh() timer callback. This is to ensure that
updates from the Guest are submitted at a fixed rate (as indicated by
max-refresh-rate) instead of arbitrarily submitting them. I'll add comments
describing this behavior in the next version.

Thanks,
Vivek

> 
> 
> 	 }
> 
> 	 static const DisplayChangeListenerOps display_listener_gl_ops = {
> 	--
> 	2.49.0
> 
> 

RE: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Michael Scherle]

Hi Vivek,

On 27.05.25 06:20, Kasireddy, Vivek wrote:
> Hi Michael,
> 
>> Subject: Re: [PATCH v4 5/7] ui/console-gl: Add a helper to create a texture
>> with linear memory layout
>>
>> Hi all,
>>
>> I just noticed that Dmitry Osipenko had already pointed out a similar issue
>> earlier-so my message was somewhat redundant. Apologies for the
>> duplication.
> Yeah, looks like you and Dmitry identified the leak independently, almost at the
> same time.
> 
>>
>> Also, I made a small mistake in the patch I proposed:
>> The call to glDeleteMemoryObjectsEXT(1, &mem_obj); should be placed
>> above the #endif, not after it. Sorry about that oversight!
> Your patch from Open Source VDI repo seems like a better solution, so, I'll add it
> to this series and send it out for review in v5. Please add description/commit message
> and your signed-off-by line to it.
> 

Thanks! I’ve added it and made a few additional adjustments here:
https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/commit/859d500761b35cc785fcadd9d554a78712309e88

Feel free to merge it into your patches if you want.

Best regards,
Michael

> Thanks,
> Vivek
> 
>>
>> Best regards,
>> Michael
>>
>>
>> On 26.05.25 15:06, Michael Scherle wrote:
>>> Great to see this patch making progress.
>>>
>>> I've tested it extensively, and unfortunately, I've noticed a memory
>>> leak in surface_gl_create_texture_from_fd(). The memory leak is hard
>>> to see since the memory is owned by the gpu driver.
>>> On Intel hardware, it's possible to observe the leak using:
>>>
>>> cat /sys/module/i915/refcnt
>>> or
>>> xpu-smi ps
>>>
>>> In on of my use case-which involves frequent scanout disable/enable
>>> cycles-the leak is quite apparent. However, in more typical scenarios,
>>> it might be difficult to catch.
>>>
>>> The issue stems from the mem_obj not being deleted after use. I've put
>>> together a minimal modification to address it:
>>>
>>>
>>>
>>> On 15.05.25 04:45, Vivek Kasireddy wrote:
>>>> There are cases where we do not want the memory layout of a texture
>>>> to be tiled as the component processing the texture would not know
>>>> how to de-tile either via software or hardware. Therefore, ensuring
>>>> that the memory backing the texture has a linear layout is absolutely
>>>> necessary in these situations.
>>>>
>>>> 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>
>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>> ---
>>>>    include/ui/console.h |  2 ++
>>>>    ui/console-gl.c      | 32 ++++++++++++++++++++++++++++++++
>>>>    2 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/include/ui/console.h b/include/ui/console.h index
>>>> 46b3128185..5cfa6ae215 100644
>>>> --- a/include/ui/console.h
>>>> +++ b/include/ui/console.h
>>>> @@ -422,6 +422,8 @@ bool
>>>> console_gl_check_format(DisplayChangeListener
>>>> *dcl,
>>>>                                 pixman_format_code_t format);
>>>>    void surface_gl_create_texture(QemuGLShader *gls,
>>>>                                   DisplaySurface *surface);
>>>> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
>>>> +                                       int fd, GLuint *texture);
>>>>    void surface_gl_update_texture(QemuGLShader *gls,
>>>>                                   DisplaySurface *surface,
>>>>                                   int x, int y, int w, int h); diff
>>>> --git a/ui/console-gl.c b/ui/console-gl.c index
>>>> 103b954017..97f7989651 100644
>>>> --- a/ui/console-gl.c
>>>> +++ b/ui/console-gl.c
>>>> @@ -25,6 +25,7 @@
>>>>     * THE SOFTWARE.
>>>>     */
>>>>    #include "qemu/osdep.h"
>>>> +#include "qemu/error-report.h"
>>>>    #include "ui/console.h"
>>>>    #include "ui/shader.h"
>>>> @@ -96,6 +97,37 @@ void surface_gl_create_texture(QemuGLShader *gls,
>>>>        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
>>>> GL_LINEAR);
>>>>    }
>>>> +bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
>>>> +                                       int fd, GLuint *texture) {
>>>> +    unsigned long size = surface_stride(surface) *
>>>> surface_height(surface);
>>>> +    GLenum err = glGetError();
>>>> +    GLuint mem_obj;
>>>
>>> +    GLuint mem_obj = 0;
>>>
>>>> +
>>>> +    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
>>>> +        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +#ifdef GL_EXT_memory_object_fd
>>>> +    glCreateMemoryObjectsEXT(1, &mem_obj);
>>>> +    glImportMemoryFdEXT(mem_obj, size,
>> GL_HANDLE_TYPE_OPAQUE_FD_EXT,
>>>> fd);
>>>> +
>>>> +    err = glGetError();
>>>> +    if (err != GL_NO_ERROR) {
>>>
>>> +          if (mem_obj) {
>>> +              glDeleteMemoryObjectsEXT(1, &mem_obj);
>>> +          }
>>>
>>>> +        error_report("spice: cannot import memory object from fd");
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    glGenTextures(1, texture);
>>>> +    glBindTexture(GL_TEXTURE_2D, *texture);
>>>> +    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT,
>>>> GL_LINEAR_TILING_EXT);
>>>> +    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8,
>>>> surface_width(surface),
>>>> +                         surface_height(surface), mem_obj, 0);
>>>> +#endif
>>>
>>> +    glDeleteMemoryObjectsEXT(1, &mem_obj);
>>>
>>>> +    return *texture > 0 && glGetError() == GL_NO_ERROR; }
>>>> +
>>>>    void surface_gl_update_texture(QemuGLShader *gls,
>>>>                                   DisplaySurface *surface,
>>>>                                   int x, int y, int w, int h)
>>>
>>>
>>>
>>> That said, my OpenGL knowledge is somewhat limited, and the
>>> documentation wasn't entirely clear to me on whether deleting the
>>> memory object while the texture is still being used, is always safe.
>>> Based on a quick look at the iris and llvmpipe implementations, it
>>> appears to be acceptable.
>>>
>>> If that's not the case, an alternative fix could follow this approach
>>> instead: https://gitlab.uni-freiburg.de/opensourcevdi/qemu/-/
>>> commit/4ca806871c141089be16af25c1820d3e04f3e27d
>>>
>>> Greetings Michael
>