[PATCH v3 0/2] Consolidate create-sync and create-fence

[PATCH v3 0/2] Consolidate create-sync and create-fence

[dongwon.kim]

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

Sync object itself is never used as is so can be removed
from QemuDmaBuf struct. So now sync is only temporarily needed
when creating fence for the object which means what was done in
egl_dmabuf_create_sync can now be a part of egl_dmabuf_create_fence
function. And egl_dmabuf_create_fence returns fence_fd so the
better function name will be egl_dmabuf_create_fence_fd.

v3: create fence only if current QemuDmaBuf->fence_fd = -1
    to make sure there is no fence currently bound to the
    QemuDmaBuf

Dongwon Kim (2):
  ui/egl-helpers: Consolidates create-sync and create-fence
  ui/dmabuf: Remove 'sync' from QemuDmaBuf struct

 include/ui/dmabuf.h      |  2 --
 include/ui/egl-helpers.h |  3 +--
 ui/dmabuf.c              | 14 --------------
 ui/egl-helpers.c         | 24 +++++++++---------------
 ui/gtk-egl.c             | 17 ++++-------------
 ui/gtk-gl-area.c         | 12 +++---------
 6 files changed, 17 insertions(+), 55 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] ui/egl-helpers: Consolidates create-sync and create-fence

[dongwon.kim]

From: Dongwon Kim <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>)

v3: create fence only if current QemuDmaBuf->fence_fd = -1
    to make sure there is no fence currently bound to the
    QemuDmaBuf

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 include/ui/egl-helpers.h |  3 +--
 ui/egl-helpers.c         | 24 +++++++++---------------
 ui/gtk-egl.c             | 17 ++++-------------
 ui/gtk-gl-area.c         | 12 +++---------
 4 files changed, 17 insertions(+), 39 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..67c90abe18 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -371,9 +371,10 @@ 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") &&
@@ -382,23 +383,16 @@ void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
         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..9983b0423a 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;
 
@@ -98,14 +97,13 @@ 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);
+        if (dmabuf && qemu_dmabuf_get_fence_fd(dmabuf) == -1) {
+            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);
             }
-            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..929bb49290 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -78,16 +78,9 @@ void gd_gl_area_draw(VirtualConsole *vc)
                           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) {
+        if (dmabuf && qemu_dmabuf_get_fence_fd(dmabuf) == -1) {
             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.43.0

[PATCH v3 2/2] ui/dmabuf: Remove 'sync' from QemuDmaBuf struct

[dongwon.kim]

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

Sync object is not used so removing it from QemuDmaBuf struct

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 include/ui/dmabuf.h |  2 --
 ui/dmabuf.c         | 14 --------------
 2 files changed, 16 deletions(-)

diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h
index dc74ba895a..7f0f7f3f6e 100644
--- a/include/ui/dmabuf.h
+++ b/include/ui/dmabuf.h
@@ -36,13 +36,11 @@ uint32_t qemu_dmabuf_get_y(QemuDmaBuf *dmabuf);
 uint32_t qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf);
 uint32_t qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf);
 bool qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf);
-void *qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf);
 int32_t qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf);
 bool qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf);
 bool qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf);
 void qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf, uint32_t texture);
 void qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd);
-void qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync);
 void qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool draw_submitted);
 void qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd);
 
diff --git a/ui/dmabuf.c b/ui/dmabuf.c
index df7a09703f..2332292a08 100644
--- a/ui/dmabuf.c
+++ b/ui/dmabuf.c
@@ -23,7 +23,6 @@ struct QemuDmaBuf {
     uint32_t  backing_width;
     uint32_t  backing_height;
     bool      y0_top;
-    void      *sync;
     int       fence_fd;
     bool      allow_fences;
     bool      draw_submitted;
@@ -170,13 +169,6 @@ bool qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf)
     return dmabuf->y0_top;
 }
 
-void *qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf)
-{
-    assert(dmabuf != NULL);
-
-    return dmabuf->sync;
-}
-
 int32_t qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf)
 {
     assert(dmabuf != NULL);
@@ -210,12 +202,6 @@ void qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd)
     dmabuf->fence_fd = fence_fd;
 }
 
-void qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync)
-{
-    assert(dmabuf != NULL);
-    dmabuf->sync = sync;
-}
-
 void qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool draw_submitted)
 {
     assert(dmabuf != NULL);
-- 
2.43.0

Re: [PATCH v3 0/2] Consolidate create-sync and create-fence

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

Hi

On Wed, Jul 24, 2024 at 2:05 AM <dongwon.kim@intel.com> wrote:

> From: Dongwon Kim <dongwon.kim@intel.com>
>
> Sync object itself is never used as is so can be removed
> from QemuDmaBuf struct. So now sync is only temporarily needed
> when creating fence for the object which means what was done in
> egl_dmabuf_create_sync can now be a part of egl_dmabuf_create_fence
> function. And egl_dmabuf_create_fence returns fence_fd so the
> better function name will be egl_dmabuf_create_fence_fd.
>
> v3: create fence only if current QemuDmaBuf->fence_fd = -1
>     to make sure there is no fence currently bound to the
>     QemuDmaBuf
>

Why not check it from egl_dmabuf_create_fence_fd() ? calling the function
twice can still potentially leak.

Also, please gather the v1/v2/v3/... summary on the cover letter.

thanks


> Dongwon Kim (2):
>   ui/egl-helpers: Consolidates create-sync and create-fence
>   ui/dmabuf: Remove 'sync' from QemuDmaBuf struct
>
>  include/ui/dmabuf.h      |  2 --
>  include/ui/egl-helpers.h |  3 +--
>  ui/dmabuf.c              | 14 --------------
>  ui/egl-helpers.c         | 24 +++++++++---------------
>  ui/gtk-egl.c             | 17 ++++-------------
>  ui/gtk-gl-area.c         | 12 +++---------
>  6 files changed, 17 insertions(+), 55 deletions(-)
>
> --
> 2.43.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2184 bytes --]

Re: [PATCH v3 0/2] Consolidate create-sync and create-fence

[Kim, Dongwon]

Hey Marc-André,

On 7/24/2024 3:37 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 24, 2024 at 2:05 AM <dongwon.kim@intel.com 
> <mailto:dongwon.kim@intel.com>> wrote:
> 
>     From: Dongwon Kim <dongwon.kim@intel.com <mailto:dongwon.kim@intel.com>>
> 
>     Sync object itself is never used as is so can be removed
>     from QemuDmaBuf struct. So now sync is only temporarily needed
>     when creating fence for the object which means what was done in
>     egl_dmabuf_create_sync can now be a part of egl_dmabuf_create_fence
>     function. And egl_dmabuf_create_fence returns fence_fd so the
>     better function name will be egl_dmabuf_create_fence_fd.
> 
>     v3: create fence only if current QemuDmaBuf->fence_fd = -1
>          to make sure there is no fence currently bound to the
>          QemuDmaBuf
> 
> 
> Why not check it from egl_dmabuf_create_fence_fd() ? calling the 
> function twice can still potentially leak.

It is called from only two locations in gtk-egl.c and gtk-gl-draw.c
and dmabuf->fence_fd == -1 is checked beforehand so I thought it's
not needed but I think your point is the completeness of the function
itself. Do you think we can do assert 'dmabuf->fence_fd >= 0'?

> 
> Also, please gather the v1/v2/v3/... summary on the cover letter.

Sure

> 
> thanks
> 
> 
>     Dongwon Kim (2):
>        ui/egl-helpers: Consolidates create-sync and create-fence
>        ui/dmabuf: Remove 'sync' from QemuDmaBuf struct
> 
>       include/ui/dmabuf.h      |  2 --
>       include/ui/egl-helpers.h |  3 +--
>       ui/dmabuf.c              | 14 --------------
>       ui/egl-helpers.c         | 24 +++++++++---------------
>       ui/gtk-egl.c             | 17 ++++-------------
>       ui/gtk-gl-area.c         | 12 +++---------
>       6 files changed, 17 insertions(+), 55 deletions(-)
> 
>     -- 
>     2.43.0
> 
> 
> 
> 
> -- 
> Marc-André Lureau