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 to 60 FPS
using the GUI timer. This matches the behavior of GTK UI where the
display updates are submitted at 60 FPS (assuming the underlying
mode is WxY@60).

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 | 53 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index bf4caf0d1b..2c4daa0707 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -842,12 +842,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 stream at 60 FPS, the (GUI) timer delay needs to be ~17 ms */
+            dcl->update_interval = 1000 / (2 * GUI_REFRESH_INTERVAL_DEFAULT) + 1;
+        }
         return;
     }
 
@@ -855,11 +875,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;
     }
 }
@@ -926,6 +943,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_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
     qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
     ssd->have_surface = false;
@@ -1029,7 +1060,6 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint stride = 0, fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
-    uint64_t cookie;
     int fd;
     uint32_t width, height, texture;
 
@@ -1107,8 +1137,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

Hi Vivek

On Tue, Apr 29, 2025 at 10:19 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 to 60 FPS
> using the GUI timer. This matches the behavior of GTK UI where the
> display updates are submitted at 60 FPS (assuming the underlying
> mode is WxY@60).

I guess it would make sense to make it configurable, for any UI/remote protocol.

For some UI, refresh rate is set via dpy_set_ui_info(). Unfortunately,
none of the vnc, spice or dbus protocols provide the refresh rate.

I wonder if it would make sense to set it on the GPU.. Perhaps a
"max-refresh-rate" device property?

>
> 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 | 53 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index bf4caf0d1b..2c4daa0707 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -842,12 +842,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 stream at 60 FPS, the (GUI) timer delay needs to be ~17 ms */
> +            dcl->update_interval = 1000 / (2 * GUI_REFRESH_INTERVAL_DEFAULT) + 1;

That expression doesn't make much sense to me.

"update_interval" is in ms. GUI_REFRESH_INTERVAL_DEFAULT is 30ms.
(iow, it's not 30fps)

If you need 60fps, just add a new constant/macro value instead?

> +        }
>          return;
>      }
>
> @@ -855,11 +875,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;
>      }
>  }
> @@ -926,6 +943,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_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
>      qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
>      ssd->have_surface = false;
> @@ -1029,7 +1060,6 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
>      EGLint stride = 0, fourcc = 0;
>      bool render_cursor = false;
>      bool y_0_top = false; /* FIXME */
> -    uint64_t cookie;
>      int fd;
>      uint32_t width, height, texture;
>
> @@ -1107,8 +1137,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
>
>


-- 
Marc-André Lureau

Hi Marc-Andre,

> Subject: Re: [PATCH v3 3/6] ui/spice: Submit the gl_draw requests at 60 FPS
> for remote clients
> 
> Hi Vivek
> 
> On Tue, Apr 29, 2025 at 10:19 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 to 60 FPS
> > using the GUI timer. This matches the behavior of GTK UI where the
> > display updates are submitted at 60 FPS (assuming the underlying
> > mode is WxY@60).
> 
> I guess it would make sense to make it configurable, for any UI/remote
> protocol.
Sure, I'll add a new parameter for this.

> 
> For some UI, refresh rate is set via dpy_set_ui_info(). Unfortunately,
> none of the vnc, spice or dbus protocols provide the refresh rate.
AFAIU, dpy_set_ui_info() sets the refresh rate for the Guest updates but
what I am trying to do in this patch is increase the submission/update rate
(to Host) from 30 FPS to 60 FPS. This is only needed for specific use-cases
where the update submission to Host (encode request) is done from the
refresh timer callback.

> 
> I wonder if it would make sense to set it on the GPU.. Perhaps a
> "max-refresh-rate" device property?
Not sure if a generic one is needed as not all UIs do Host submission
(encode or draw) from the refresh timer callback.

> 
> >
> > 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 | 53 +++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 43 insertions(+), 10 deletions(-)
> >
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index bf4caf0d1b..2c4daa0707 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -842,12 +842,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 stream at 60 FPS, the (GUI) timer delay needs to be ~17 ms
> */
> > +            dcl->update_interval = 1000 / (2 *
> GUI_REFRESH_INTERVAL_DEFAULT) + 1;
> 
> That expression doesn't make much sense to me.
> 
> "update_interval" is in ms. GUI_REFRESH_INTERVAL_DEFAULT is 30ms.
> (iow, it's not 30fps)
IIUC, update_interval needs to be ~17ms (16.66 to be more accurate) for the
refresh timer callback to get invoked 60 times per sec. 

> 
> If you need 60fps, just add a new constant/macro value instead?
Yeah, I'll do that as it would make it more clear.

Thanks,
Vivek

> 
> > +        }
> >          return;
> >      }
> >
> > @@ -855,11 +875,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;
> >      }
> >  }
> > @@ -926,6 +943,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_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> >      qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> >      ssd->have_surface = false;
> > @@ -1029,7 +1060,6 @@ static void
> qemu_spice_gl_update(DisplayChangeListener *dcl,
> >      EGLint stride = 0, fourcc = 0;
> >      bool render_cursor = false;
> >      bool y_0_top = false; /* FIXME */
> > -    uint64_t cookie;
> >      int fd;
> >      uint32_t width, height, texture;
> >
> > @@ -1107,8 +1137,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
> >
> >
> 
> 
> --
> Marc-André Lureau