Hi Marc-André, > -----Original Message----- > From: Marc-André Lureau <marcandre.lur...@gmail.com> > Sent: Tuesday, March 12, 2024 4:27 AM > To: Kim, Dongwon <dongwon....@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....@intel.com> wrote: > > > > From: Dongwon Kim <dongwon....@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.lur...@redhat.com> > > Cc: Vivek Kasireddy <vivek.kasire...@intel.com> > > Signed-off-by: Dongwon Kim <dongwon....@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