Hi Marc-André,
> -Original Message-
> From: Marc-André Lureau
> Sent: Tuesday, March 12, 2024 4:27 AM
> To: Kim, Dongwon
> 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 wrote:
> >
> > From: Dongwon Kim
> >
> > 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
> > Cc: Vivek Kasireddy
> > Signed-off-by: Dongwon Kim
> > ---
> > 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 = >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,
> > + 1);
> > +
> > +/* 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
