RE: [PATCH v1 3/7] ui/spice: Submit the gl_draw requests at 60 FPS for remote clients

[Kasireddy, Vivek]

Hi Marc-Andre,

> 
> Hi
> 
> On Sat, Jan 20, 2024 at 4:54 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).
> 
> One of the issues with this approach is that the DMABUF isn't owned by
> the consumer. By delaying the usage of it, we risk having
> damaged/invalid updates.
This patch is only relevant with blob=true option. And, in this case, the
Guest (virtio-gpu kernel driver) is blocked (on a fence) until the Host
has consumed the buffer, which in this case happens after the encoder
signals the async to indicate that encoding is completed. Therefore,
AFAIU, there is no risk of missing or invalid updates. Ideally, the rate
should be driven by the consumer which in this case is the Spice client,
and given that it doesn't make sense to submit new frames faster than
the refresh rate, I figured it is ok to limit the producer to 60 FPS as well.
Note that Spice also does rate limiting based on network latencies and
dropped frames.


> 
> It would be great if we could have a mechanism for double/triple
> buffering with virtio-gpu, as far as I know this is not possible yet.
Given that virtio-gpu is a drm/kms driver, there can only be one buffer
in flight at any given time. And, it doesn't make sense to submit frames
faster than the refresh rate as they would simply get dropped. However,
I tried to address this issue few years ago but it did not go anywhere:
https://lore.kernel.org/all/20211014124402.46f95ebc@eldfell/T/

> 
> I wonder if setting dpy_set_ui_info() with the fixed refresh_rate is
> enough for the guest to have a fixed FPS.
I am not sure. Let me try to experiment with it and see how things work.

Thanks,
Vivek
> It will only work with gfx hw that support ui_info() though.
> 
> 
> 
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.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 | 38 ++++++++++++++++++++++++++++----------
> >  1 file changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 384b8508d4..90c04623ec 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -841,12 +841,31 @@ 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;
> > +    QemuDmaBuf *dmabuf = ssd->guest_dmabuf;
> >
> > -    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 && dmabuf) {
> > +            spice_gl_draw(ssd, 0, 0, dmabuf->width, dmabuf->height);
> > +            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;
> >      }
> >
> > @@ -854,11 +873,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;
> >      }
> >  }
> > @@ -1025,7 +1041,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;
> >
> >      if (!ssd->have_scanout) {
> > @@ -1098,8 +1113,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.39.2
> >
> >
> 
> 
> --
> Marc-André Lureau