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

Vivek Kasireddy posted 7 patches 7 months, 1 week ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>
There is a newer version of this series
[PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate
Posted by Vivek Kasireddy 7 months, 1 week ago
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


Re: [PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate
Posted by Marc-André Lureau 6 months, 4 weeks ago
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
>
>
RE: [PATCH v4 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate
Posted by Kasireddy, Vivek 6 months, 3 weeks ago
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 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate
Posted by Marc-André Lureau 6 months, 4 weeks ago
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
>
>