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) {
         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) {
+        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

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.

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...


>          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?

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?

> +        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

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