diff mbox series

[1/3] ui/gtk: flush display pipeline before saving vmstate when blob=true

Message ID 20231213202419.15459-1-dongwon.kim@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/3] ui/gtk: flush display pipeline before saving vmstate when blob=true | expand

Commit Message

Kim, Dongwon Dec. 13, 2023, 8:24 p.m. UTC
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

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/gtk.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Marc-André Lureau Dec. 15, 2023, 8:33 a.m. UTC | #1
Hi

On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
>
> 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
>
> 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/gtk.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 810d7fc796..ea8d07833e 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -678,6 +678,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);

This won't work. dmabuf->sync is NULL after egl_dmabuf_create_sync.

I will let Vivek, who wrote the sync code, comment.

thanks
Kasireddy, Vivek Dec. 29, 2023, 7:28 a.m. UTC | #2
Hi,

> 
> On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com>
> wrote:
> >
> > 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
> >
> > 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/gtk.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..ea8d07833e 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -678,6 +678,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);
> 
> This won't work. dmabuf->sync is NULL after egl_dmabuf_create_sync.
Right, we destroy the sync object after we create the fence from it. If you
want to use eglClientWaitSync() here, you either need to recreate the sync
object using fence_fd or don't destroy it in egl_dmabuf_create_fence(). 
Either way should be ok but make sure you destroy it when the fence_fd
is closed.

Thanks,
Vivek

> 
> I will let Vivek, who wrote the sync code, comment.
> 
> thanks
> 
> 
> 
> --
> Marc-André Lureau
Kim, Dongwon Dec. 29, 2023, 7:22 p.m. UTC | #3
Hi,

> Subject: RE: [PATCH 1/3] ui/gtk: flush display pipeline before saving vmstate
> when blob=true
> 
> Hi,
> 
> >
> > On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com>
> > wrote:
> > >
> > > 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
> > >
> > > 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/gtk.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > index 810d7fc796..ea8d07833e 100644
> > > --- a/ui/gtk.c
> > > +++ b/ui/gtk.c
> > > @@ -678,6 +678,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);
> >
> > This won't work. dmabuf->sync is NULL after egl_dmabuf_create_sync.
> Right, we destroy the sync object after we create the fence from it. If you want
> to use eglClientWaitSync() here, you either need to recreate the sync object
> using fence_fd or don't destroy it in egl_dmabuf_create_fence().
> Either way should be ok but make sure you destroy it when the fence_fd is
> closed.

I guess it makes sense to destroy sync object inside gd_hw_gl_flushed if that is the case.
Another thing is I mentioned that "dmabuf == NULL or fence_fd < 0" doesn't seem to be checked
in gd_hw_flushed in my previous reply but now I am thinking it is needed because gd_hw_gl_flushed
can be called twice with given code change - once when rendering is done and the fence is signaled during
eglClientWaitSync and second after eglClientWaitSync. I will come up with a new version of patches.
 
> 
> Thanks,
> Vivek
> 
> >
> > I will let Vivek, who wrote the sync code, comment.
> >
> > thanks
> >
> >
> >
> > --
> > Marc-André Lureau
diff mbox series

Patch

diff --git a/ui/gtk.c b/ui/gtk.c
index 810d7fc796..ea8d07833e 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -678,6 +678,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);
 }