Re: [PATCH 1/2] ui/egl-helpers: Consolidate create-sync and create-fence

["Kim, Dongwon" <dongwon.kim@intel.com>]

Hi Marc-André,

On 7/3/2024 4:23 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 3, 2024 at 12:57 AM <dongwon.kim@intel.com 
> <mailto:dongwon.kim@intel.com>> wrote:
> 
>     From: Dongwon Kim <dongwon.kim@intel.com <mailto:dongwon.kim@intel.com>>
> 
>     There is no reason to split those two operations so combining
>     two functions - egl_dmabuf_create_sync and egl_dmabuf_create_fence.
> 
>     v2: egl_dmabuf_create_fence -> egl_dmabuf_create_fence_fd
>          (Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>)
> 
>     Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>
>     Cc: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
>     Cc: Vivek Kasireddy <vivek.kasireddy@intel.com
>     <mailto:vivek.kasireddy@intel.com>>
>     Signed-off-by: Dongwon Kim <dongwon.kim@intel.com
>     <mailto:dongwon.kim@intel.com>>
>     ---
>       include/ui/egl-helpers.h |  3 +--
>       ui/egl-helpers.c         | 27 +++++++++++----------------
>       ui/gtk-egl.c             | 15 +++------------
>       ui/gtk-gl-area.c         | 10 ++--------
>       4 files changed, 17 insertions(+), 38 deletions(-)
> 
>     diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
>     index 4b8c0d2281..221548e3c9 100644
>     --- a/include/ui/egl-helpers.h
>     +++ b/include/ui/egl-helpers.h
>     @@ -51,8 +51,7 @@ int egl_get_fd_for_texture(uint32_t tex_id, EGLint
>     *stride, EGLint *fourcc,
> 
>       void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf);
>       void egl_dmabuf_release_texture(QemuDmaBuf *dmabuf);
>     -void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf);
>     -void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf);
>     +int egl_dmabuf_create_fence_fd(QemuDmaBuf *dmabuf);
> 
>       #endif
> 
>     diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
>     index 99b2ebbe23..ddbf52bf5f 100644
>     --- a/ui/egl-helpers.c
>     +++ b/ui/egl-helpers.c
>     @@ -371,34 +371,29 @@ void egl_dmabuf_release_texture(QemuDmaBuf
>     *dmabuf)
>           qemu_dmabuf_set_texture(dmabuf, 0);
>       }
> 
>     -void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
>     +int egl_dmabuf_create_fence_fd(QemuDmaBuf *dmabuf)
>       {
>           EGLSyncKHR sync;
>     +    int fence_fd = -1;
> 
>           if (epoxy_has_egl_extension(qemu_egl_display,
>                                       "EGL_KHR_fence_sync") &&
>               epoxy_has_egl_extension(qemu_egl_display,
>     -                                "EGL_ANDROID_native_fence_sync")) {
>     +                                "EGL_ANDROID_native_fence_sync") &&
>     +        qemu_dmabuf_get_fence_fd(dmabuf) == -1) {
>               sync = eglCreateSyncKHR(qemu_egl_display,
>                                       EGL_SYNC_NATIVE_FENCE_ANDROID, NULL);
>               if (sync != EGL_NO_SYNC_KHR) {
>     -            qemu_dmabuf_set_sync(dmabuf, sync);
>     +            fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
>     +                                                  sync);
>     +            if (fence_fd >= 0) {
>     +                qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
>     +            }
>     +            eglDestroySyncKHR(qemu_egl_display, sync);
>               }
>           }
>     -}
>     -
>     -void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
>     -{
>     -    void *sync = qemu_dmabuf_get_sync(dmabuf);
>     -    int fence_fd;
> 
>     -    if (sync) {
>     -        fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
>     -                                              sync);
>     -        qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
>     -        eglDestroySyncKHR(qemu_egl_display, sync);
>     -        qemu_dmabuf_set_sync(dmabuf, NULL);
>     -    }
>     +    return fence_fd;
>       }
> 
>       #endif /* CONFIG_GBM */
>     diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
>     index 9831c10e1b..8869cdee4f 100644
>     --- a/ui/gtk-egl.c
>     +++ b/ui/gtk-egl.c
>     @@ -68,7 +68,6 @@ void gd_egl_draw(VirtualConsole *vc)
>           GdkWindow *window;
>       #ifdef CONFIG_GBM
>           QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
>     -    int fence_fd;
>       #endif
>           int ww, wh, ws;
> 
>     @@ -99,13 +98,12 @@ void gd_egl_draw(VirtualConsole *vc)
>               glFlush();
>       #ifdef CONFIG_GBM
>               if (dmabuf) {
>     -            egl_dmabuf_create_fence(dmabuf);
>     -            fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
>     +            int fence_fd = egl_dmabuf_create_fence_fd(dmabuf);
>                   if (fence_fd >= 0) {
>                       qemu_set_fd_handler(fence_fd, gd_hw_gl_flushed,
>     NULL, vc);
>     -                return;
>     +            } else {
>     +                graphic_hw_gl_block(vc->gfx.dcl.con, false);
>                   }
> 
> 
> If gd_egl_draw()/gd_egl_refresh() is called multiple times before the 
> flushed callback, create_fence_fd() is going to return -1 and unblock 
> the hw. This is probably not desired. I think you need to comment on the 
> code to explain the interaction between dmabuf_get_draw_submitted, 
> gd_egl_flush(), fences and hw_block, I can't make sense of it.
> 
> thanks

Yes, indeed it is not desired flow. Let me have some time to take a look 
at this case and get back with a better patch.

> 
>     -            graphic_hw_gl_block(vc->gfx.dcl.con, false);
>               }
>       #endif
>           } else {
>     @@ -364,12 +362,6 @@ void gd_egl_scanout_flush(DisplayChangeListener
>     *dcl,
>               egl_fb_blit(&vc->gfx.win_fb, &vc->gfx.guest_fb,
>     !vc->gfx.y0_top);
>           }
> 
>     -#ifdef CONFIG_GBM
>     -    if (vc->gfx.guest_fb.dmabuf) {
>     -        egl_dmabuf_create_sync(vc->gfx.guest_fb.dmabuf);
>     -    }
>     -#endif
>     -
>           eglSwapBuffers(qemu_egl_display, vc->gfx.esurface);
>       }
> 
>     @@ -387,7 +379,6 @@ void gd_egl_flush(DisplayChangeListener *dcl,
>               gtk_widget_queue_draw_area(area, x, y, w, h);
>               return;
>           }
>     -
>           gd_egl_scanout_flush(&vc->gfx.dcl, x, y, w, h);
>       }
> 
>     diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
>     index b628b35451..a3c21c3c59 100644
>     --- a/ui/gtk-gl-area.c
>     +++ b/ui/gtk-gl-area.c
>     @@ -77,17 +77,10 @@ void gd_gl_area_draw(VirtualConsole *vc)
>               glBlitFramebuffer(0, y1, vc->gfx.w, y2,
>                                 0, 0, ww, wh,
>                                 GL_COLOR_BUFFER_BIT, GL_NEAREST);
>     -#ifdef CONFIG_GBM
>     -        if (dmabuf) {
>     -            egl_dmabuf_create_sync(dmabuf);
>     -        }
>     -#endif
>     -        glFlush();
>       #ifdef CONFIG_GBM
>               if (dmabuf) {
>                   int fence_fd;
>     -            egl_dmabuf_create_fence(dmabuf);
>     -            fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
>     +            fence_fd = egl_dmabuf_create_fence_fd(dmabuf);
>                   if (fence_fd >= 0) {
>                       qemu_set_fd_handler(fence_fd, gd_hw_gl_flushed,
>     NULL, vc);
>                       return;
>     @@ -95,6 +88,7 @@ void gd_gl_area_draw(VirtualConsole *vc)
>                   graphic_hw_gl_block(vc->gfx.dcl.con, false);
>               }
>       #endif
>     +        glFlush();
>           } else {
>               if (!vc->gfx.ds) {
>                   return;
>     -- 
>     2.34.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau