If the guest state is paused before it gets a response for the current
scanout frame submission (resource-flush), it won't start submitting
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.

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/ui/gtk.c b/ui/gtk.c
index 810d7fc796..0f6237dd2f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -678,6 +678,18 @@ 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) {
+                /* force flushing current scanout blob rendering process */
+                gd_hw_gl_flushed(vc);
+            }
+        }
+    }
 
     gd_update_caption(s);
 }
-- 
2.34.1

Hi

On Tue, Dec 5, 2023 at 6:40 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 start submitting
> 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.
>
> 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 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 810d7fc796..0f6237dd2f 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -678,6 +678,18 @@ 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) {

&& ..dmabuf->fence_fd >= 0

> +                /* force flushing current scanout blob rendering process */
> +                gd_hw_gl_flushed(vc);

This defeats the purpose of the fence, maybe we should wait for it to
be signaled first. At worse, I suppose the client can have some
glitches. Although since the guest is stopped, this is unlikely.

Hi Marc-André,

> -----Original Message-----
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> Sent: Monday, December 4, 2023 11:15 PM
> To: Kim, Dongwon <dongwon.kim@intel.com>
> Cc: qemu-devel@nongnu.org; Kasireddy, Vivek <vivek.kasireddy@intel.com>
> Subject: Re: [PATCH] ui/gtk: flush display pipeline before saving vmstate when
> blob=true
> 
> Hi
> 
> On Tue, Dec 5, 2023 at 6:40 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 start submitting
> > 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.
> >
> > 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 | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..0f6237dd2f 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -678,6 +678,18 @@ 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) {
> 
> && ..dmabuf->fence_fd >= 0
> 
> > +                /* force flushing current scanout blob rendering process */
> > +                gd_hw_gl_flushed(vc);
> 
> This defeats the purpose of the fence, maybe we should wait for it to be
> signaled first. At worse, I suppose the client can have some glitches. Although
> since the guest is stopped, this is unlikely.
[Kim, Dongwon] 
So this is the flow you are referring to?

            if (vc->gfx.guest_fb.dmabuf &&
                vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
                EGLint ret = eglClientWaitSync(qemu_egl_display,
                                               vc->gfx.guest_fb.dmabuf->sync,
                                               EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
                                               100000000); /* timeout of 100ms */

                if (ret != EGL_CONDITION_SATISFIED_KHR) {
                    /* force flushing current scanout blob rendering process */
                    gd_hw_gl_flushed(vc);
                }
            }

If yes, I will test this then create v2 based on this flow.
Thanks 

Hi

On Wed, Dec 6, 2023 at 2:05 AM Kim, Dongwon <dongwon.kim@intel.com> wrote:
>
> Hi Marc-André,
>
> > -----Original Message-----
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Sent: Monday, December 4, 2023 11:15 PM
> > To: Kim, Dongwon <dongwon.kim@intel.com>
> > Cc: qemu-devel@nongnu.org; Kasireddy, Vivek <vivek.kasireddy@intel.com>
> > Subject: Re: [PATCH] ui/gtk: flush display pipeline before saving vmstate when
> > blob=true
> >
> > Hi
> >
> > On Tue, Dec 5, 2023 at 6:40 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 start submitting
> > > 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.
> > >
> > > 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 | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > index 810d7fc796..0f6237dd2f 100644
> > > --- a/ui/gtk.c
> > > +++ b/ui/gtk.c
> > > @@ -678,6 +678,18 @@ 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) {
> >
> > && ..dmabuf->fence_fd >= 0
> >
> > > +                /* force flushing current scanout blob rendering process */
> > > +                gd_hw_gl_flushed(vc);
> >
> > This defeats the purpose of the fence, maybe we should wait for it to be
> > signaled first. At worse, I suppose the client can have some glitches. Although
> > since the guest is stopped, this is unlikely.
> [Kim, Dongwon]
> So this is the flow you are referring to?
>
>             if (vc->gfx.guest_fb.dmabuf &&
>                 vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
>                 EGLint ret = eglClientWaitSync(qemu_egl_display,
>                                                vc->gfx.guest_fb.dmabuf->sync,
>                                                EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
>                                                100000000); /* timeout of 100ms */
>
>                 if (ret != EGL_CONDITION_SATISFIED_KHR) {
>                     /* force flushing current scanout blob rendering process */
>                     gd_hw_gl_flushed(vc);
>                 }
>             }
>
> If yes, I will test this then create v2 based on this flow.

Yes, you may want to call gd_hw_gl_flushed() even on success, to avoid
waiting for the main loop dispatch.

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);
+
+                /* 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, 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



-- 
Marc-André Lureau

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

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

It is needed to unblock the pipeline only if there is an active dmabuf
to be rendered and the fence for it is not yet signaled.

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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index ea8d07833e..073c9eadb8 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -597,10 +597,16 @@ 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) {
+        return;
+    }
+
+    if (dmabuf->fence_fd > 0) {
+        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);
+    }
 }
 
 /** DisplayState Callbacks (opengl version) **/
-- 
2.34.1

Hi

On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
>
> It is needed to unblock the pipeline only if there is an active dmabuf
> to be rendered and the fence for it is not yet signaled.
>
> 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 | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index ea8d07833e..073c9eadb8 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -597,10 +597,16 @@ 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) {
> +        return;
> +    }

When is this function called with dmabuf == NULL or fence_fd < 0?

> +
> +    if (dmabuf->fence_fd > 0) {

this should be >= 0

> +        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);
> +    }
>  }
>
>  /** DisplayState Callbacks (opengl version) **/
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

Hi Marc-André,

I reviewed and realized these conditions won't be met in normal situations in given upstream code. But we've initially added those conditions in our internal code base for dev because we often had to call gd_hw_gl_flushed to forcefully unblock from HPD code (i.e. 'connectors' param. Not Upstreamed yet) when VM display is disconnected. In such cases, it is needed to make sure there is a frame in the pipeline already. Anyway, I think we can check dmabuf==NULL and fence_fd < 0 before calling gd_hw_flushed in HPD code just as in gd_change_runstate ([PATCH 1/3] ui/gtk: flush display pipeline before saving vmstate when blob=true). 

> Subject: Re: [PATCH 2/3] ui/gtk: unblock pipeline only if fence hasn't been
> signaled yet
> 
> Hi
> 
> On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com>
> wrote:
> >
> > It is needed to unblock the pipeline only if there is an active dmabuf
> > to be rendered and the fence for it is not yet signaled.
> >
> > 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 | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index ea8d07833e..073c9eadb8 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -597,10 +597,16 @@ 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) {
> > +        return;
> > +    }
> 
> When is this function called with dmabuf == NULL or fence_fd < 0?
> 
> > +
> > +    if (dmabuf->fence_fd > 0) {
> 
> this should be >= 0
> 
> > +        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);
> > +    }
> >  }
> >
> >  /** DisplayState Callbacks (opengl version) **/
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau

On Fri, 15 Dec 2023, Marc-André Lureau wrote:
> Hi
>
> On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
>>
>> It is needed to unblock the pipeline only if there is an active dmabuf
>> to be rendered and the fence for it is not yet signaled.
>>
>> 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 | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/ui/gtk.c b/ui/gtk.c
>> index ea8d07833e..073c9eadb8 100644
>> --- a/ui/gtk.c
>> +++ b/ui/gtk.c
>> @@ -597,10 +597,16 @@ 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) {
>> +        return;
>> +    }
>
> When is this function called with dmabuf == NULL or fence_fd < 0?
>
>> +
>> +    if (dmabuf->fence_fd > 0) {
>
> this should be >= 0

There's another in line 102 already and one in gtk-gl-area.c too.

Regards,
BALATON Zoltan

>> +        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);
>> +    }
>>  }
>>
>>  /** DisplayState Callbacks (opengl version) **/
>> --
>> 2.34.1
>>
>>
>
>
>

The guest surface needs to be updated with a blob scanout after resumed
from saved vm state if blob is enabled.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 hw/display/virtio-gpu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index b016d3bac8..34dc0ab9fc 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1417,19 +1417,27 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
         if (!res) {
             return -EINVAL;
         }
-        scanout->ds = qemu_create_displaysurface_pixman(res->image);
-        if (!scanout->ds) {
-            return -EINVAL;
-        }
+
+        if (res->blob_size) {
+            assert(g->dmabuf.primary[i] != NULL);
+            g->dmabuf.primary[i]->buf.fd = res->dmabuf_fd;
+            dpy_gl_scanout_dmabuf(scanout->con, &g->dmabuf.primary[i]->buf);
+        } else {
+            scanout->ds = qemu_create_displaysurface_pixman(res->image);
+            if (!scanout->ds) {
+                return -EINVAL;
+            }
 #ifdef WIN32
-        qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
+            qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
 #endif
+            dpy_gfx_replace_surface(scanout->con, scanout->ds);
+            dpy_gfx_update_full(scanout->con);
+        }
 
-        dpy_gfx_replace_surface(scanout->con, scanout->ds);
-        dpy_gfx_update_full(scanout->con);
         if (scanout->cursor.resource_id) {
             update_cursor(g, &scanout->cursor);
         }
+
         res->scanout_bitmask |= (1 << i);
     }
 
-- 
2.34.1

Hi Dongwon Kim

On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim <dongwon.kim@intel.com> wrote:
>
> The guest surface needs to be updated with a blob scanout after resumed
> from saved vm state if blob is enabled.
>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>

Can you resend the last version of the series?

thanks

> ---
>  hw/display/virtio-gpu.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index b016d3bac8..34dc0ab9fc 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1417,19 +1417,27 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
>          if (!res) {
>              return -EINVAL;
>          }
> -        scanout->ds = qemu_create_displaysurface_pixman(res->image);
> -        if (!scanout->ds) {
> -            return -EINVAL;
> -        }
> +
> +        if (res->blob_size) {
> +            assert(g->dmabuf.primary[i] != NULL);
> +            g->dmabuf.primary[i]->buf.fd = res->dmabuf_fd;
> +            dpy_gl_scanout_dmabuf(scanout->con, &g->dmabuf.primary[i]->buf);
> +        } else {
> +            scanout->ds = qemu_create_displaysurface_pixman(res->image);
> +            if (!scanout->ds) {
> +                return -EINVAL;
> +            }
>  #ifdef WIN32
> -        qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
> +            qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
>  #endif
> +            dpy_gfx_replace_surface(scanout->con, scanout->ds);
> +            dpy_gfx_update_full(scanout->con);
> +        }
>
> -        dpy_gfx_replace_surface(scanout->con, scanout->ds);
> -        dpy_gfx_update_full(scanout->con);
>          if (scanout->cursor.resource_id) {
>              update_cursor(g, &scanout->cursor);
>          }
> +
>          res->scanout_bitmask |= (1 << i);
>      }
>
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

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

From: Dongwon Kim <dongwon.kim@intel.com>

The guest surface needs to be updated with a blob scanout after resumed
from saved vm state if blob is enabled.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 hw/display/virtio-gpu.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 1c1ee230b3..01bc4f9565 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1422,16 +1422,23 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
         if (!res) {
             return -EINVAL;
         }
-        scanout->ds = qemu_create_displaysurface_pixman(res->image);
-        if (!scanout->ds) {
-            return -EINVAL;
-        }
+
+        if (res->blob_size) {
+            assert(g->dmabuf.primary[i] != NULL);
+            g->dmabuf.primary[i]->buf.fd = res->dmabuf_fd;
+            dpy_gl_scanout_dmabuf(scanout->con, &g->dmabuf.primary[i]->buf);
+        } else {
+            scanout->ds = qemu_create_displaysurface_pixman(res->image);
+            if (!scanout->ds) {
+                return -EINVAL;
+            }
 #ifdef WIN32
-        qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
+            qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
 #endif
+            dpy_gfx_replace_surface(scanout->con, scanout->ds);
+            dpy_gfx_update_full(scanout->con);
+        }
 
-        dpy_gfx_replace_surface(scanout->con, scanout->ds);
-        dpy_gfx_update_full(scanout->con);
         if (scanout->cursor.resource_id) {
             update_cursor(g, &scanout->cursor);
         }
-- 
2.34.1

Hi Dongwon Kim

On Wed, Mar 6, 2024 at 2:24 AM <dongwon.kim@intel.com> wrote:
>
> From: Dongwon Kim <dongwon.kim@intel.com>
>
> The guest surface needs to be updated with a blob scanout after resumed
> from saved vm state if blob is enabled.
>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  hw/display/virtio-gpu.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>


The way you send patches to the ML confuses patchew, and they are not
properly triaged or tested.
(https://patchew.org/search?q=project%3AQEMU+from%3Adongwon.kim%40intel.com)

Please send a series of your pending patches, with a new version. It's
confusing with the various mails otherwise. And it's then easier to
deal with regular patch series that are tracked by patchew.

thanks

> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 1c1ee230b3..01bc4f9565 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1422,16 +1422,23 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
>          if (!res) {
>              return -EINVAL;
>          }
> -        scanout->ds = qemu_create_displaysurface_pixman(res->image);
> -        if (!scanout->ds) {
> -            return -EINVAL;
> -        }
> +
> +        if (res->blob_size) {
> +            assert(g->dmabuf.primary[i] != NULL);
> +            g->dmabuf.primary[i]->buf.fd = res->dmabuf_fd;
> +            dpy_gl_scanout_dmabuf(scanout->con, &g->dmabuf.primary[i]->buf);
> +        } else {
> +            scanout->ds = qemu_create_displaysurface_pixman(res->image);
> +            if (!scanout->ds) {
> +                return -EINVAL;
> +            }
>  #ifdef WIN32
> -        qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
> +            qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
>  #endif
> +            dpy_gfx_replace_surface(scanout->con, scanout->ds);
> +            dpy_gfx_update_full(scanout->con);
> +        }
>
> -        dpy_gfx_replace_surface(scanout->con, scanout->ds);
> -        dpy_gfx_update_full(scanout->con);
>          if (scanout->cursor.resource_id) {
>              update_cursor(g, &scanout->cursor);
>          }
> --
> 2.34.1
>


-- 
Marc-André Lureau