RE: [PATCH v3] ui/gtk: flush display pipeline before saving vmstate when blob=true

[Kim, Dongwon]

Hi Marc-André,

> -----Original Message-----
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> Sent: Tuesday, March 12, 2024 4:27 AM
> To: Kim, Dongwon <dongwon.kim@intel.com>
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH v3] ui/gtk: flush display pipeline before saving vmstate 
> when
> blob=true
> 
> Hi
> 
> On Thu, Mar 7, 2024 at 2:27 AM <dongwon.kim@intel.com> wrote:
> >
> > From: Dongwon Kim <dongwon.kim@intel.com>
> >
> > If the guest state is paused before it gets a response for the current
> > scanout frame submission (resource-flush), it won't flush new frames
> > after being restored as it still waits for the old response, which is
> > accepted as a scanout render done signal. So it's needed to unblock
> > the current scanout render pipeline before the run state is changed to
> > make sure the guest receives the response for the current frame
> > submission.
> >
> > v2: Giving some time for the fence to be signaled before flushing
> >     the pipeline
> >
> > v3: Prevent redundant call of gd_hw_gl_flushed by checking dmabuf
> >     and fence_fd >= 0 in it (e.g. during and after eglClientWaitSync
> >     in gd_change_runstate).
> >
> >     Destroy sync object later in gd_hw_fl_flushed
> >
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > ---
> >  ui/egl-helpers.c |  2 --
> >  ui/gtk.c         | 31 +++++++++++++++++++++++++++----
> >  2 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c index
> > 3d19dbe382..a77f9e57d9 100644
> > --- a/ui/egl-helpers.c
> > +++ b/ui/egl-helpers.c
> > @@ -385,8 +385,6 @@ void egl_dmabuf_create_fence(QemuDmaBuf
> *dmabuf)
> >      if (dmabuf->sync) {
> 
> We may want to check that no fence_fd exists before, to avoid leaks.

[Kim, Dongwon]  OK
> 
> I also notice that fence_fd is initialized with 0 in 
> vfio_display_get_dmabuf(). It
> would probably make sense to introduce functions to allocate, set and get 
> fields
> from QemuDmaBuf and make the struct private, as it is too easy to do a wrong
> initialization...
[Kim, Dongwon] Yeah I will look into this.
> 
> 
> >          dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
> >                                                        dmabuf->sync);
> > -        eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> > -        dmabuf->sync = NULL;
> >      }
> >  }
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..eaca890cba 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -597,10 +597,14 @@ void gd_hw_gl_flushed(void *vcon)
> >      VirtualConsole *vc = vcon;
> >      QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> >
> > -    qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> > -    close(dmabuf->fence_fd);
> > -    dmabuf->fence_fd = -1;
> > -    graphic_hw_gl_block(vc->gfx.dcl.con, false);
> > +    if (dmabuf && dmabuf->fence_fd >= 0) {
> 
> It may have failed to create the fence_fd, but succeeded in creating the 
> sync, in
> which case it will leak the sync.
> 
> Btw, can't the fence_fd be created at the same time as the sync instead of
> having two functions?
[Kim, Dongwon] I will take a look. 
> 
> I also noticed that fenced_fd is incorrectly checked for > 0 instead of >= 0 
> in gtk-
> egl.c and gtk-gl-area.c. Can you fix that as well?
[Kim, Dongwon] Sure I will work on v2 with suggested changes.

Thanks,
DW

> 
> > +        qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> > +        close(dmabuf->fence_fd);
> > +        dmabuf->fence_fd = -1;
> > +        eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> > +        dmabuf->sync = NULL;
> > +        graphic_hw_gl_block(vc->gfx.dcl.con, false);
> > +    }
> >  }
> >
> >  /** DisplayState Callbacks (opengl version) **/ @@ -678,6 +682,25 @@
> > static const DisplayGLCtxOps egl_ctx_ops = {  static void
> > gd_change_runstate(void *opaque, bool running, RunState state)  {
> >      GtkDisplayState *s = opaque;
> > +    int i;
> > +
> > +    if (state == RUN_STATE_SAVE_VM) {
> > +        for (i = 0; i < s->nb_vcs; i++) {
> > +            VirtualConsole *vc = &s->vc[i];
> > +
> > +            if (vc->gfx.guest_fb.dmabuf &&
> > +                vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
> > +                eglClientWaitSync(qemu_egl_display,
> > +                                  vc->gfx.guest_fb.dmabuf->sync,
> > +                                  EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
> > +                                  100000000);
> > +
> > +                /* force flushing current scanout blob rendering process
> > +                 * just in case the fence is still not signaled */
> > +                gd_hw_gl_flushed(vc);
> > +            }
> > +        }
> > +    }
> >
> >      gd_update_caption(s);
> >  }
> > --
> > 2.34.1
> >
> >
> 
> thanks
> 
> 
> --
> Marc-André Lureau