* [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res
@ 2021-09-16 23:41 Dongwon Kim
2021-09-17 10:02 ` Gerd Hoffmann
2021-09-24 22:51 ` [PATCH v2] " Dongwon Kim
0 siblings, 2 replies; 5+ messages in thread
From: Dongwon Kim @ 2021-09-16 23:41 UTC (permalink / raw)
To: qemu-devel; +Cc: Vivek Kasireddy, Dongwon Kim, Gerd Hoffmann
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(-)
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));
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res
2021-09-16 23:41 [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res Dongwon Kim
@ 2021-09-17 10:02 ` Gerd Hoffmann
2021-09-17 16:34 ` Dongwon Kim
2021-09-23 23:41 ` Dongwon Kim
2021-09-24 22:51 ` [PATCH v2] " Dongwon Kim
1 sibling, 2 replies; 5+ messages in thread
From: Gerd Hoffmann @ 2021-09-17 10:02 UTC (permalink / raw)
To: Dongwon Kim; +Cc: qemu-devel, Vivek Kasireddy
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res
2021-09-17 10:02 ` Gerd Hoffmann
@ 2021-09-17 16:34 ` Dongwon Kim
2021-09-23 23:41 ` Dongwon Kim
1 sibling, 0 replies; 5+ messages in thread
From: Dongwon Kim @ 2021-09-17 16:34 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel, Vivek Kasireddy
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res
2021-09-17 10:02 ` Gerd Hoffmann
2021-09-17 16:34 ` Dongwon Kim
@ 2021-09-23 23:41 ` Dongwon Kim
1 sibling, 0 replies; 5+ messages in thread
From: Dongwon Kim @ 2021-09-23 23:41 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel, Vivek Kasireddy
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] ui/gtk: skip any extra draw of same guest scanout blob res
2021-09-16 23:41 [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res Dongwon Kim
2021-09-17 10:02 ` Gerd Hoffmann
@ 2021-09-24 22:51 ` Dongwon Kim
1 sibling, 0 replies; 5+ messages in thread
From: Dongwon Kim @ 2021-09-24 22:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Vivek Kasireddy, Dongwon Kim, Gerd Hoffmann
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).
v2:
- removed mutex
- updated commit msg
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 | 2 +-
include/ui/console.h | 1 +
ui/gtk-egl.c | 40 ++++++++++++++++++---------
ui/gtk-gl-area.c | 49 ++++++++++++++++++++-------------
4 files changed, 59 insertions(+), 33 deletions(-)
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index c6f7f58784..60ea7f8f49 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -186,7 +186,7 @@ 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;
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..b6bedc5f41 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -171,6 +171,7 @@ typedef struct QemuDmaBuf {
void *sync;
int fence_fd;
bool allow_fences;
+ bool draw_submitted;
} QemuDmaBuf;
typedef struct DisplayState DisplayState;
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 72ce5e1f8f..e912b20075 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,31 @@ void gd_egl_draw(VirtualConsole *vc)
wh = gdk_window_get_height(window);
if (vc->gfx.scanout_mode) {
+#ifdef CONFIG_GBM
+ if (dmabuf) {
+ if (!dmabuf->draw_submitted) {
+ 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);
+ return;
+ }
+ graphic_hw_gl_block(vc->gfx.dcl.con, false);
+ }
+#endif
} else {
if (!vc->gfx.ds) {
return;
@@ -92,21 +116,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 +330,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..20035aae38 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,16 @@ void gd_gl_area_draw(VirtualConsole *vc)
return;
}
+#ifdef CONFIG_GBM
+ if (dmabuf) {
+ if (!dmabuf->draw_submitted) {
+ 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 +75,22 @@ 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);
+ return;
+ }
+ graphic_hw_gl_block(vc->gfx.dcl.con, false);
+ }
+#endif
} else {
if (!vc->gfx.ds) {
return;
@@ -72,25 +101,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 +244,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));
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-24 22:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 23:41 [PATCH] ui/gtk: skip any extra draw of same guest scanout blob res Dongwon Kim
2021-09-17 10:02 ` Gerd Hoffmann
2021-09-17 16:34 ` Dongwon Kim
2021-09-23 23:41 ` Dongwon Kim
2021-09-24 22:51 ` [PATCH v2] " Dongwon Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).