Re: [RFC PATCH 2/4] ui/egl-helpers: Consolidates create-sync and create-fence

[Kim, Dongwon]

Hi Marc-André,

I will come up with a separate patch for this.

On 7/2/2024 1:32 AM, Marc-André Lureau wrote:

Hi

On Fri, Jun 21, 2024 at 3:19 AM > wrote:


From: Dongwon Kim mailto:dongwon@intel.com>>

There is no reason to split those two operations so combining
two functions - egl_dmabuf_create_sync and egl_dmabuf_create_fence.

Cc: Gerd Hoffmann mailto:kra...@redhat.com>>
Cc: Marc-André Lureau mailto:marcandre.lur...@redhat.com>>
Cc: Vivek Kasireddy mailto:vivek.kasire...@intel.com>>
Signed-off-by: Dongwon Kim mailto:dongwon@intel.com>>


Can you send this change as a different patch series, or earlier in the 
series?


You should also remove qemu_dmabuf_{set,get}_sync() and associated fields

---
  include/ui/egl-helpers.h |  3 +--
  ui/egl-helpers.c         | 27 +++
  ui/gtk-egl.c             | 19 +++
  ui/gtk-gl-area.c         | 10 ++
  4 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
index 4b8c0d2281..606d6c8288 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(QemuDmaBuf *dmabuf);

  #endif

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 99b2ebbe23..e16f2cb23d 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(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;


It should probably return qemu_dmabuf_get_fence_fd() instead. The 
function should be renamed with _fd postfix since you make it return the fd.


  }

  #endif /* CONFIG_GBM */
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 9831c10e1b..55199f8659 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);
+            fence_fd = egl_dmabuf_create_fence(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);
              }
-            graphic_hw_gl_block(vc->gfx.dcl.con, false);
          }
  #endif
      } else {
@@ -336,7 +334,11 @@ void gd_egl_scanout_flush(DisplayChangeListener
*dcl,
  {
      VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
      GdkWindow *window;
+#ifdef CONFIG_GBM
+    QemuDmaBuf

Re: [RFC PATCH 2/4] ui/egl-helpers: Consolidates create-sync and create-fence

[Marc-André Lureau]

Hi

On Fri, Jun 21, 2024 at 3:19 AM  wrote:

> From: Dongwon Kim 
>
> There is no reason to split those two operations so combining
> two functions - egl_dmabuf_create_sync and egl_dmabuf_create_fence.
>
> Cc: Gerd Hoffmann 
> Cc: Marc-André Lureau 
> Cc: Vivek Kasireddy 
> Signed-off-by: Dongwon Kim 
>

Can you send this change as a different patch series, or earlier in the
series?

You should also remove qemu_dmabuf_{set,get}_sync() and associated fields

---
>  include/ui/egl-helpers.h |  3 +--
>  ui/egl-helpers.c | 27 +++
>  ui/gtk-egl.c | 19 +++
>  ui/gtk-gl-area.c | 10 ++
>  4 files changed, 21 insertions(+), 38 deletions(-)
>
> diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
> index 4b8c0d2281..606d6c8288 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(QemuDmaBuf *dmabuf);
>
>  #endif
>
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> index 99b2ebbe23..e16f2cb23d 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(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;
>

It should probably return qemu_dmabuf_get_fence_fd() instead. The function
should be renamed with _fd postfix since you make it return the fd.

 }
>
>  #endif /* CONFIG_GBM */
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> index 9831c10e1b..55199f8659 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);
> +fence_fd = egl_dmabuf_create_fence(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);
>  }
> -graphic_hw_gl_block(vc->gfx.dcl.con, false);
>  }
>  #endif
>  } else {
> @@ -336,7 +334,11 @@ void gd_egl_scanout_flush(DisplayChangeListener *dcl,
>  {
>  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
>  GdkWindow *window;
> +#ifdef CONFIG_GBM
> +QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> +#endif
>  int ww, wh, ws;
> +int fence_fd;
>
>  if (!vc->gfx.scanout_mode) {
>  return;
> @@ -364,12 +366,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 +383,6 @@ void gd_egl_fl

[RFC PATCH 2/4] ui/egl-helpers: Consolidates create-sync and create-fence

[dongwon . kim]

From: Dongwon Kim 

There is no reason to split those two operations so combining
two functions - egl_dmabuf_create_sync and egl_dmabuf_create_fence.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 include/ui/egl-helpers.h |  3 +--
 ui/egl-helpers.c | 27 +++
 ui/gtk-egl.c | 19 +++
 ui/gtk-gl-area.c | 10 ++
 4 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
index 4b8c0d2281..606d6c8288 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(QemuDmaBuf *dmabuf);
 
 #endif
 
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 99b2ebbe23..e16f2cb23d 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(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..55199f8659 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);
+fence_fd = egl_dmabuf_create_fence(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);
 }
-graphic_hw_gl_block(vc->gfx.dcl.con, false);
 }
 #endif
 } else {
@@ -336,7 +334,11 @@ void gd_egl_scanout_flush(DisplayChangeListener *dcl,
 {
 VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
 GdkWindow *window;
+#ifdef CONFIG_GBM
+QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
+#endif
 int ww, wh, ws;
+int fence_fd;
 
 if (!vc->gfx.scanout_mode) {
 return;
@@ -364,12 +366,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 +383,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..0b11423824 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(d