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.kasire...@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 <kra...@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Cc: Dmitry Osipenko <dmitry.osipe...@collabora.com>
> > Cc: Frediano Ziglio <fredd...@gmail.com>
> > Cc: Dongwon Kim <dongwon....@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasire...@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
