diff mbox series

ui/gtk: skip any extra draw of same guest scanout blob res

Message ID 20210916234156.5505-1-dongwon.kim@intel.com (mailing list archive)
State New, archived
Headers show
Series ui/gtk: skip any extra draw of same guest scanout blob res | expand

Commit Message

Kim, Dongwon Sept. 16, 2021, 11:41 p.m. UTC 
Any extra draw call for the same blob resource representing guest scanout
before the previous drawing is not finished can break synchronous draw
sequence. To prevent this, drawing is now done only once for each draw
submission (when draw_submitted == true). Mutex is added to protect this
draw iteration and the flag.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 hw/display/virtio-gpu-udmabuf.c |  3 ++
 include/ui/console.h            |  3 ++
 ui/gtk-egl.c                    | 44 +++++++++++++++++++--------
 ui/gtk-gl-area.c                | 53 +++++++++++++++++++++------------
 4 files changed, 71 insertions(+), 32 deletions(-)

Comments

Gerd Hoffmann Sept. 17, 2021, 10:02 a.m. UTC | #1 
Hi,

> +    bool      draw_submitted;
> +    QemuMutex mutex;

Why the mutex?  I think all the code runs while holding the BQL so it
should be serialized.

> +#ifdef CONFIG_GBM
> +        if (dmabuf) {
> +            qemu_mutex_lock(&dmabuf->mutex);
> +            if (!dmabuf->draw_submitted) {
> +                qemu_mutex_unlock(&dmabuf->mutex);
> +                return;
> +            } else {
> +                dmabuf->draw_submitted = false;
> +            }
> +        }
> +#endif

Factoring out that into helper functions is probably a good idea.  Then
have stub functions for the CONFIG_GBM=no case and *alot* less #ifdefs
in the code ...

thanks,
  Gerd
Kim, Dongwon Sept. 17, 2021, 4:34 p.m. UTC | #2 
On Fri, Sep 17, 2021 at 12:02:02PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > +    bool      draw_submitted;
> > +    QemuMutex mutex;
> 
> Why the mutex?  I think all the code runs while holding the BQL so it
> should be serialized.

Guest drawing process using blob is serialized (gd_egl_flush->scheduling
draw call->gd_egl_draw) but an asynchronous draw event from another thread
is causing a problem.

I initially thought using a flag (draw_submitted) would be enough to get this
worked around, but it wasn't as the asynchronous draw could take it over before,

dambuf->draw_submitted = false;

happens during normal draw sequence. I thought mutex would be a reasonable
solution for this case.

> 
> > +#ifdef CONFIG_GBM
> > +        if (dmabuf) {
> > +            qemu_mutex_lock(&dmabuf->mutex);
> > +            if (!dmabuf->draw_submitted) {
> > +                qemu_mutex_unlock(&dmabuf->mutex);
> > +                return;
> > +            } else {
> > +                dmabuf->draw_submitted = false;
> > +            }
> > +        }
> > +#endif
> 
> Factoring out that into helper functions is probably a good idea.  Then
> have stub functions for the CONFIG_GBM=no case and *alot* less #ifdefs
> in the code ...

I will look into this part.
Thanks,
DW

> 
> thanks,
>   Gerd
>
Kim, Dongwon Sept. 23, 2021, 11:41 p.m. UTC | #3 
On Fri, Sep 17, 2021 at 12:02:02PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > +    bool      draw_submitted;
> > +    QemuMutex mutex;
> 
> Why the mutex?  I think all the code runs while holding the BQL so it
> should be serialized.

Gerd, I did more experiment and verified mutex is actually not required.
I think I had some wrong belief after seeing some of sync problem
resolved after adding mutex but the code sequence was different at that
time.. I will remove mutex.

> 
> > +#ifdef CONFIG_GBM
> > +        if (dmabuf) {
> > +            qemu_mutex_lock(&dmabuf->mutex);
> > +            if (!dmabuf->draw_submitted) {
> > +                qemu_mutex_unlock(&dmabuf->mutex);
> > +                return;
> > +            } else {
> > +                dmabuf->draw_submitted = false;
> > +            }
> > +        }
> > +#endif
> 
> Factoring out that into helper functions is probably a good idea.  Then
> have stub functions for the CONFIG_GBM=no case and *alot* less #ifdefs
> in the code ...
> 

There are oter places controlled by #ifdef CONFIG_GBM.
What about taking care of CONFIG_GBM altogher after v2 (same but no
mutex) of this patch?

> thanks,
>   Gerd
>
diff mbox series

Patch

diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index c6f7f58784..aabc7b81b5 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -186,6 +186,9 @@  static VGPUDMABuf
     dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
     dmabuf->buf.fd = res->dmabuf_fd;
     dmabuf->buf.allow_fences = true;
+    dmabuf->buf.draw_submitted = false;
+
+    qemu_mutex_init(&dmabuf->buf.mutex);
 
     dmabuf->scanout_id = scanout_id;
     QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
diff --git a/include/ui/console.h b/include/ui/console.h
index 244664d727..0f931c3696 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -5,6 +5,7 @@ 
 #include "qom/object.h"
 #include "qemu/notify.h"
 #include "qemu/error-report.h"
+#include "qemu/lockable.h"
 #include "qapi/qapi-types-ui.h"
 
 #ifdef CONFIG_OPENGL
@@ -171,6 +172,8 @@  typedef struct QemuDmaBuf {
     void      *sync;
     int       fence_fd;
     bool      allow_fences;
+    bool      draw_submitted;
+    QemuMutex mutex;
 } QemuDmaBuf;
 
 typedef struct DisplayState DisplayState;
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 72ce5e1f8f..17727d040b 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -63,6 +63,9 @@  void gd_egl_init(VirtualConsole *vc)
 void gd_egl_draw(VirtualConsole *vc)
 {
     GdkWindow *window;
+#ifdef CONFIG_GBM
+    QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
+#endif
     int ww, wh;
 
     if (!vc->gfx.gls) {
@@ -74,10 +77,35 @@  void gd_egl_draw(VirtualConsole *vc)
     wh = gdk_window_get_height(window);
 
     if (vc->gfx.scanout_mode) {
+#ifdef CONFIG_GBM
+        if (dmabuf) {
+            qemu_mutex_lock(&dmabuf->mutex);
+            if (!dmabuf->draw_submitted) {
+                qemu_mutex_unlock(&dmabuf->mutex);
+                return;
+            } else {
+                dmabuf->draw_submitted = false;
+            }
+        }
+#endif
         gd_egl_scanout_flush(&vc->gfx.dcl, 0, 0, vc->gfx.w, vc->gfx.h);
 
         vc->gfx.scale_x = (double)ww / vc->gfx.w;
         vc->gfx.scale_y = (double)wh / vc->gfx.h;
+
+        glFlush();
+#ifdef CONFIG_GBM
+        if (dmabuf) {
+            egl_dmabuf_create_fence(dmabuf);
+            if (dmabuf->fence_fd > 0) {
+                qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, vc);
+                qemu_mutex_unlock(&dmabuf->mutex);
+                return;
+            }
+            graphic_hw_gl_block(vc->gfx.dcl.con, false);
+            qemu_mutex_unlock(&dmabuf->mutex);
+        }
+#endif
     } else {
         if (!vc->gfx.ds) {
             return;
@@ -92,21 +120,10 @@  void gd_egl_draw(VirtualConsole *vc)
 
         vc->gfx.scale_x = (double)ww / surface_width(vc->gfx.ds);
         vc->gfx.scale_y = (double)wh / surface_height(vc->gfx.ds);
-    }
-
-    glFlush();
-#ifdef CONFIG_GBM
-    if (vc->gfx.guest_fb.dmabuf) {
-        QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
 
-        egl_dmabuf_create_fence(dmabuf);
-        if (dmabuf->fence_fd > 0) {
-            qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, vc);
-            return;
-        }
-        graphic_hw_gl_block(vc->gfx.dcl.con, false);
+        glFlush();
     }
-#endif
+
     graphic_hw_gl_flushed(vc->gfx.dcl.con);
 }
 
@@ -317,6 +334,7 @@  void gd_egl_flush(DisplayChangeListener *dcl,
 
     if (vc->gfx.guest_fb.dmabuf) {
         graphic_hw_gl_block(vc->gfx.dcl.con, true);
+        vc->gfx.guest_fb.dmabuf->draw_submitted = true;
         gtk_widget_queue_draw_area(area, x, y, w, h);
         return;
     }
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index b23523748e..7b432ad7f4 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -38,6 +38,9 @@  static void gtk_gl_area_set_scanout_mode(VirtualConsole *vc, bool scanout)
 
 void gd_gl_area_draw(VirtualConsole *vc)
 {
+#ifdef CONFIG_GBM
+    QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
+#endif
     int ww, wh, y1, y2;
 
     if (!vc->gfx.gls) {
@@ -53,6 +56,18 @@  void gd_gl_area_draw(VirtualConsole *vc)
             return;
         }
 
+#ifdef CONFIG_GBM
+        if (dmabuf) {
+            qemu_mutex_lock(&dmabuf->mutex);
+            if (!dmabuf->draw_submitted) {
+                qemu_mutex_unlock(&dmabuf->mutex);
+                return;
+            } else {
+                dmabuf->draw_submitted = false;
+            }
+        }
+#endif
+
         glBindFramebuffer(GL_READ_FRAMEBUFFER, vc->gfx.guest_fb.framebuffer);
         /* GtkGLArea sets GL_DRAW_FRAMEBUFFER for us */
 
@@ -62,6 +77,24 @@  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) {
+            egl_dmabuf_create_fence(dmabuf);
+            if (dmabuf->fence_fd > 0) {
+                qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, vc);
+                qemu_mutex_unlock(&dmabuf->mutex);
+                return;
+            }
+            qemu_mutex_unlock(&dmabuf->mutex);
+            graphic_hw_gl_block(vc->gfx.dcl.con, false);
+        }
+#endif
     } else {
         if (!vc->gfx.ds) {
             return;
@@ -72,25 +105,6 @@  void gd_gl_area_draw(VirtualConsole *vc)
         surface_gl_render_texture(vc->gfx.gls, vc->gfx.ds);
     }
 
-#ifdef CONFIG_GBM
-    if (vc->gfx.guest_fb.dmabuf) {
-        egl_dmabuf_create_sync(vc->gfx.guest_fb.dmabuf);
-    }
-#endif
-
-    glFlush();
-#ifdef CONFIG_GBM
-    if (vc->gfx.guest_fb.dmabuf) {
-        QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
-
-        egl_dmabuf_create_fence(dmabuf);
-        if (dmabuf->fence_fd > 0) {
-            qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, vc);
-            return;
-        }
-        graphic_hw_gl_block(vc->gfx.dcl.con, false);
-    }
-#endif
     graphic_hw_gl_flushed(vc->gfx.dcl.con);
 }
 
@@ -234,6 +248,7 @@  void gd_gl_area_scanout_flush(DisplayChangeListener *dcl,
 
     if (vc->gfx.guest_fb.dmabuf) {
         graphic_hw_gl_block(vc->gfx.dcl.con, true);
+        vc->gfx.guest_fb.dmabuf->draw_submitted = true;
     }
     gtk_gl_area_queue_render(GTK_GL_AREA(vc->gfx.drawing_area));
 }