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

[Kim, Dongwon]

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