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