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

[Marc-André Lureau]

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