[PATCH v2 00/12] GL & D-Bus display related fixes

[PATCH v2 00/12] GL & D-Bus display related fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console", Akihiko
Odaki reported a number of issues with the GL and D-Bus display. His series
propose a different design, and reverting some of my previous generic console
changes to fix those issues.

However, as I work through the issue so far, they can be solved by reasonable
simple fixes while keeping the console changes generic (not specific to the
D-Bus display backend). I belive a shared infrastructure is more beneficial long
term than having GL-specific code in the DBus code (in particular, the
egl-headless & VNC combination could potentially use it)

Thanks a lot Akihiko for reporting the issues proposing a different approach!
Please test this alternative series and let me know of any further issues. My
understanding is that you are mainly concerned with the Cocoa backend, and I
don't have a way to test it, so please check it. If necessary, we may well have
to revert my earlier changes and go your way, eventually.

Marc-André Lureau (12):
  ui/console: fix crash when using gl context with non-gl listeners
  ui/console: fix texture leak when calling surface_gl_create_texture()
  ui: do not create a surface when resizing a GL scanout
  ui/console: move check for compatible GL context
  ui/console: move dcl compatiblity check to a callback
  ui/console: egl-headless is compatible with non-gl listeners
  ui/dbus: associate the DBusDisplayConsole listener with the given
    console
  ui/console: move console compatibility check to dcl_display_console()
  ui/shader: fix potential leak of shader on error
  ui/shader: free associated programs
  ui/console: add a dpy_gfx_switch callback helper
  ui/dbus: fix texture sharing

 include/ui/console.h |  19 ++++---
 ui/dbus.h            |   3 ++
 ui/console-gl.c      |   4 ++
 ui/console.c         | 119 ++++++++++++++++++++++++++-----------------
 ui/dbus-console.c    |  27 +++++-----
 ui/dbus-listener.c   |  11 ----
 ui/dbus.c            |  33 +++++++++++-
 ui/egl-headless.c    |  17 ++++++-
 ui/gtk.c             |  18 ++++++-
 ui/sdl2.c            |   9 +++-
 ui/shader.c          |   9 +++-
 ui/spice-display.c   |   9 +++-
 12 files changed, 192 insertions(+), 86 deletions(-)

-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 01/12] ui/console: fix crash when using gl context with non-gl listeners

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The commit 7cc712e98 ("ui: dispatch GL events to all listener")
mechanically replaced the dpy_gl calls with a dispatch loop, using the
same pre-conditions. However, it didn't take into account that all
listeners do not have to implement the GL callbacks.

Add the missing pre-conditions before calling the callbacks.

Fix crash when running a GL-enabled VM with "-device virtio-gpu-gl-pci
-display egl-headless -vnc :0".

Fixes: 7cc712e98 ("ui: dispatch GL events to all listener")
Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 40eebb6d2cc2..79a01afd1ea7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1860,7 +1860,9 @@ void dpy_gl_scanout_disable(QemuConsole *con)
         con->scanout.kind = SCANOUT_NONE;
     }
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_scanout_disable(dcl);
+        if (dcl->ops->dpy_gl_scanout_disable) {
+            dcl->ops->dpy_gl_scanout_disable(dcl);
+        }
     }
 }
 
@@ -1881,10 +1883,12 @@ void dpy_gl_scanout_texture(QemuConsole *con,
         x, y, width, height
     };
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_scanout_texture(dcl, backing_id,
-                                         backing_y_0_top,
-                                         backing_width, backing_height,
-                                         x, y, width, height);
+        if (dcl->ops->dpy_gl_scanout_texture) {
+            dcl->ops->dpy_gl_scanout_texture(dcl, backing_id,
+                                             backing_y_0_top,
+                                             backing_width, backing_height,
+                                             x, y, width, height);
+        }
     }
 }
 
@@ -1897,7 +1901,9 @@ void dpy_gl_scanout_dmabuf(QemuConsole *con,
     con->scanout.kind = SCANOUT_DMABUF;
     con->scanout.dmabuf = dmabuf;
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_scanout_dmabuf(dcl, dmabuf);
+        if (dcl->ops->dpy_gl_scanout_dmabuf) {
+            dcl->ops->dpy_gl_scanout_dmabuf(dcl, dmabuf);
+        }
     }
 }
 
@@ -1951,7 +1957,9 @@ void dpy_gl_update(QemuConsole *con,
 
     graphic_hw_gl_block(con, true);
     QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_update(dcl, x, y, w, h);
+        if (dcl->ops->dpy_gl_update) {
+            dcl->ops->dpy_gl_update(dcl, x, y, w, h);
+        }
     }
     graphic_hw_gl_block(con, false);
 }
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 02/12] ui/console: fix texture leak when calling surface_gl_create_texture()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Make surface_gl_create_texture() idempotent: if the surface is already
bound to a texture, do not create a new one.

This fixes texture leaks when there are multiple DBus listeners, for
example.

Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console-gl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ui/console-gl.c b/ui/console-gl.c
index 7c9894a51d99..8e3c9a3c8c01 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -49,6 +49,10 @@ void surface_gl_create_texture(QemuGLShader *gls,
     assert(gls);
     assert(QEMU_IS_ALIGNED(surface_stride(surface), surface_bytes_per_pixel(surface)));
 
+    if (surface->texture) {
+        return;
+    }
+
     switch (surface->format) {
     case PIXMAN_BE_b8g8r8x8:
     case PIXMAN_BE_b8g8r8a8:
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 03/12] ui: do not create a surface when resizing a GL scanout

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

qemu_console_resize() will create a blank surface and replace the
current scanout with it if called while the current scanout is
GL (texture or dmabuf).

This is not only very costly, but also can produce glitches on the
display/listener side.

Instead, compare the current console size with the fitting console
functions, which also works when the scanout is GL.

Note: there might be still an unnecessary surface creation on calling
qemu_console_resize() when the size is actually changing, but display
backends currently rely on DisplaySurface details during
dpy_gfx_switch() to handle various resize aspects. We would need more
refactoring to handle resize without DisplaySurface, this is left for a
future improvement.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 79a01afd1ea7..365a2c14b809 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2400,13 +2400,12 @@ static void vc_chr_open(Chardev *chr,
 
 void qemu_console_resize(QemuConsole *s, int width, int height)
 {
-    DisplaySurface *surface = qemu_console_surface(s);
+    DisplaySurface *surface;
 
     assert(s->console_type == GRAPHIC_CONSOLE);
 
-    if (surface && (surface->flags & QEMU_ALLOCATED_FLAG) &&
-        pixman_image_get_width(surface->image) == width &&
-        pixman_image_get_height(surface->image) == height) {
+    if (qemu_console_get_width(s, -1) == width &&
+        qemu_console_get_height(s, -1) == height) {
         return;
     }
 
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 04/12] ui/console: move check for compatible GL context

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Move GL context compatibility check in dpy_compatible_with(), and use
recommended error reporting.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 365a2c14b809..57e431d9e609 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1482,6 +1482,12 @@ static bool dpy_compatible_with(QemuConsole *con,
 
     flags = con->hw_ops->get_flags ? con->hw_ops->get_flags(con->hw) : 0;
 
+    if (console_has_gl(con) && con->gl->ops->compatible_dcl != dcl->ops) {
+        error_setg(errp, "Display %s is incompatible with the GL context",
+                   dcl->ops->dpy_name);
+        return false;
+    }
+
     if (flags & GRAPHIC_FLAGS_GL &&
         !console_has_gl(con)) {
         error_setg(errp, "The console requires a GL context.");
@@ -1509,27 +1515,12 @@ void qemu_console_set_display_gl_ctx(QemuConsole *con, DisplayGLCtx *gl)
     con->gl = gl;
 }
 
-static bool dpy_gl_compatible_with(QemuConsole *con, DisplayChangeListener *dcl)
-{
-    if (!con->gl) {
-        return true;
-    }
-
-    return con->gl->ops->compatible_dcl == dcl->ops;
-}
-
 void register_displaychangelistener(DisplayChangeListener *dcl)
 {
     QemuConsole *con;
 
     assert(!dcl->ds);
 
-    if (dcl->con && !dpy_gl_compatible_with(dcl->con, dcl)) {
-        error_report("Display %s is incompatible with the GL context",
-                     dcl->ops->dpy_name);
-        exit(1);
-    }
-
     if (dcl->con) {
         dpy_compatible_with(dcl->con, dcl, &error_fatal);
     }
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 05/12] ui/console: move dcl compatiblity check to a callback

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

As expected from the "compatible_dcl" comment, a simple comparison of
ops isn't enough. The following patch will fix a regression introduced
by this limited check by extending the compatibility callback for
egl-headless.

For now, this patch simply replaces the the "compatible_dcl" ops pointer
with a "dpy_gl_ctx_is_compatible_ctx" callback.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/console.h |  9 ++-------
 ui/console.c         |  3 ++-
 ui/dbus.c            |  9 ++++++++-
 ui/egl-headless.c    |  9 ++++++++-
 ui/gtk.c             | 18 ++++++++++++++++--
 ui/sdl2.c            |  9 ++++++++-
 ui/spice-display.c   |  9 ++++++++-
 7 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index f590819880b5..18a10c0b7db0 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -282,13 +282,8 @@ struct DisplayChangeListener {
 };
 
 typedef struct DisplayGLCtxOps {
-    /*
-     * We only check if the GLCtx is compatible with a DCL via ops. A natural
-     * evolution of this would be a callback to check some runtime requirements
-     * and allow various DCL kinds.
-     */
-    const DisplayChangeListenerOps *compatible_dcl;
-
+    bool (*dpy_gl_ctx_is_compatible_dcl)(DisplayGLCtx *dgc,
+                                         DisplayChangeListener *dcl);
     QEMUGLContext (*dpy_gl_ctx_create)(DisplayGLCtx *dgc,
                                        QEMUGLParams *params);
     void (*dpy_gl_ctx_destroy)(DisplayGLCtx *dgc,
diff --git a/ui/console.c b/ui/console.c
index 57e431d9e609..c9318552871b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1482,7 +1482,8 @@ static bool dpy_compatible_with(QemuConsole *con,
 
     flags = con->hw_ops->get_flags ? con->hw_ops->get_flags(con->hw) : 0;
 
-    if (console_has_gl(con) && con->gl->ops->compatible_dcl != dcl->ops) {
+    if (console_has_gl(con) &&
+        !con->gl->ops->dpy_gl_ctx_is_compatible_dcl(con->gl, dcl)) {
         error_setg(errp, "Display %s is incompatible with the GL context",
                    dcl->ops->dpy_name);
         return false;
diff --git a/ui/dbus.c b/ui/dbus.c
index 0074424c1fed..f00a44421cf7 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -48,8 +48,15 @@ static QEMUGLContext dbus_create_context(DisplayGLCtx *dgc,
     return qemu_egl_create_context(dgc, params);
 }
 
+static bool
+dbus_is_compatible_dcl(DisplayGLCtx *dgc,
+                       DisplayChangeListener *dcl)
+{
+    return dcl->ops == &dbus_gl_dcl_ops;
+}
+
 static const DisplayGLCtxOps dbus_gl_ops = {
-    .compatible_dcl          = &dbus_gl_dcl_ops,
+    .dpy_gl_ctx_is_compatible_dcl = dbus_is_compatible_dcl,
     .dpy_gl_ctx_create       = dbus_create_context,
     .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
     .dpy_gl_ctx_make_current = qemu_egl_make_context_current,
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index 94082a9da951..9aff115280bc 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -166,8 +166,15 @@ static const DisplayChangeListenerOps egl_ops = {
     .dpy_gl_update           = egl_scanout_flush,
 };
 
+static bool
+egl_is_compatible_dcl(DisplayGLCtx *dgc,
+                      DisplayChangeListener *dcl)
+{
+    return dcl->ops == &egl_ops;
+}
+
 static const DisplayGLCtxOps eglctx_ops = {
-    .compatible_dcl          = &egl_ops,
+    .dpy_gl_ctx_is_compatible_dcl = egl_is_compatible_dcl,
     .dpy_gl_ctx_create       = egl_create_context,
     .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
     .dpy_gl_ctx_make_current = qemu_egl_make_context_current,
diff --git a/ui/gtk.c b/ui/gtk.c
index a8567b9ddc8f..1b24a67d7964 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -614,8 +614,15 @@ static const DisplayChangeListenerOps dcl_gl_area_ops = {
     .dpy_has_dmabuf          = gd_has_dmabuf,
 };
 
+static bool
+gd_gl_area_is_compatible_dcl(DisplayGLCtx *dgc,
+                             DisplayChangeListener *dcl)
+{
+    return dcl->ops == &dcl_gl_area_ops;
+}
+
 static const DisplayGLCtxOps gl_area_ctx_ops = {
-    .compatible_dcl          = &dcl_gl_area_ops,
+    .dpy_gl_ctx_is_compatible_dcl = gd_gl_area_is_compatible_dcl,
     .dpy_gl_ctx_create       = gd_gl_area_create_context,
     .dpy_gl_ctx_destroy      = gd_gl_area_destroy_context,
     .dpy_gl_ctx_make_current = gd_gl_area_make_current,
@@ -641,8 +648,15 @@ static const DisplayChangeListenerOps dcl_egl_ops = {
     .dpy_has_dmabuf          = gd_has_dmabuf,
 };
 
+static bool
+gd_egl_is_compatible_dcl(DisplayGLCtx *dgc,
+                         DisplayChangeListener *dcl)
+{
+    return dcl->ops == &dcl_egl_ops;
+}
+
 static const DisplayGLCtxOps egl_ctx_ops = {
-    .compatible_dcl          = &dcl_egl_ops,
+    .dpy_gl_ctx_is_compatible_dcl = gd_egl_is_compatible_dcl,
     .dpy_gl_ctx_create       = gd_egl_create_context,
     .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
     .dpy_gl_ctx_make_current = gd_egl_make_current,
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 46a252d7d9d7..d3741f9b754d 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -788,8 +788,15 @@ static const DisplayChangeListenerOps dcl_gl_ops = {
     .dpy_gl_update           = sdl2_gl_scanout_flush,
 };
 
+static bool
+sdl2_gl_is_compatible_dcl(DisplayGLCtx *dgc,
+                          DisplayChangeListener *dcl)
+{
+    return dcl->ops == &dcl_gl_ops;
+}
+
 static const DisplayGLCtxOps gl_ctx_ops = {
-    .compatible_dcl          = &dcl_gl_ops,
+    .dpy_gl_ctx_is_compatible_dcl = sdl2_gl_is_compatible_dcl,
     .dpy_gl_ctx_create       = sdl2_gl_create_context,
     .dpy_gl_ctx_destroy      = sdl2_gl_destroy_context,
     .dpy_gl_ctx_make_current = sdl2_gl_make_context_current,
diff --git a/ui/spice-display.c b/ui/spice-display.c
index a3078adf91ec..494168e7fe75 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1125,8 +1125,15 @@ static const DisplayChangeListenerOps display_listener_gl_ops = {
     .dpy_gl_update           = qemu_spice_gl_update,
 };
 
+static bool
+qemu_spice_is_compatible_dcl(DisplayGLCtx *dgc,
+                             DisplayChangeListener *dcl)
+{
+    return dcl->ops == &display_listener_gl_ops;
+}
+
 static const DisplayGLCtxOps gl_ctx_ops = {
-    .compatible_dcl          = &display_listener_gl_ops,
+    .dpy_gl_ctx_is_compatible_dcl = qemu_spice_is_compatible_dcl,
     .dpy_gl_ctx_create       = qemu_spice_gl_create_context,
     .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
     .dpy_gl_ctx_make_current = qemu_egl_make_context_current,
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 06/12] ui/console: egl-headless is compatible with non-gl listeners

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Fix a regression introduced by commit 5e79d516e ("ui: split the GL
context in a different object").

Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/egl-headless.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index 9aff115280bc..7a30fd977765 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -170,6 +170,14 @@ static bool
 egl_is_compatible_dcl(DisplayGLCtx *dgc,
                       DisplayChangeListener *dcl)
 {
+    if (!dcl->ops->dpy_gl_update) {
+        /*
+         * egl-headless is compatible with all 2d listeners, as it blits the GL
+         * updates on the 2d console surface.
+         */
+        return true;
+    }
+
     return dcl->ops == &egl_ops;
 }
 
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 07/12] ui/dbus: associate the DBusDisplayConsole listener with the given console

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

DBusDisplayConsole is specific to a given QemuConsole.

Fixes: commit 142ca628 ("ui: add a D-Bus display backend")
Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus.h         |  3 +++
 ui/dbus-console.c | 27 +++++++++++++--------------
 ui/dbus.c         |  2 +-
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/ui/dbus.h b/ui/dbus.h
index 64c77cab4441..5f5c1f759c9b 100644
--- a/ui/dbus.h
+++ b/ui/dbus.h
@@ -79,6 +79,9 @@ dbus_display_console_new(DBusDisplay *display, QemuConsole *con);
 int
 dbus_display_console_get_index(DBusDisplayConsole *ddc);
 
+
+extern const DisplayChangeListenerOps dbus_console_dcl_ops;
+
 #define DBUS_DISPLAY_TYPE_LISTENER dbus_display_listener_get_type()
 G_DECLARE_FINAL_TYPE(DBusDisplayListener,
                      dbus_display_listener,
diff --git a/ui/dbus-console.c b/ui/dbus-console.c
index e062f721d761..898a4ac8a5ba 100644
--- a/ui/dbus-console.c
+++ b/ui/dbus-console.c
@@ -36,7 +36,6 @@ struct _DBusDisplayConsole {
     DisplayChangeListener dcl;
 
     DBusDisplay *display;
-    QemuConsole *con;
     GHashTable *listeners;
     QemuDBusDisplay1Console *iface;
 
@@ -118,7 +117,7 @@ dbus_gl_scanout_update(DisplayChangeListener *dcl,
 {
 }
 
-static const DisplayChangeListenerOps dbus_console_dcl_ops = {
+const DisplayChangeListenerOps dbus_console_dcl_ops = {
     .dpy_name                = "dbus-console",
     .dpy_gfx_switch          = dbus_gfx_switch,
     .dpy_gfx_update          = dbus_gfx_update,
@@ -191,7 +190,7 @@ dbus_console_set_ui_info(DBusDisplayConsole *ddc,
         .height = arg_height,
     };
 
-    if (!dpy_ui_info_supported(ddc->con)) {
+    if (!dpy_ui_info_supported(ddc->dcl.con)) {
         g_dbus_method_invocation_return_error(invocation,
                                               DBUS_DISPLAY_ERROR,
                                               DBUS_DISPLAY_ERROR_UNSUPPORTED,
@@ -199,7 +198,7 @@ dbus_console_set_ui_info(DBusDisplayConsole *ddc,
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
 
-    dpy_set_ui_info(ddc->con, &info, false);
+    dpy_set_ui_info(ddc->dcl.con, &info, false);
     qemu_dbus_display1_console_complete_set_uiinfo(ddc->iface, invocation);
     return DBUS_METHOD_INVOCATION_HANDLED;
 }
@@ -335,8 +334,8 @@ dbus_mouse_rel_motion(DBusDisplayConsole *ddc,
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
 
-    qemu_input_queue_rel(ddc->con, INPUT_AXIS_X, dx);
-    qemu_input_queue_rel(ddc->con, INPUT_AXIS_Y, dy);
+    qemu_input_queue_rel(ddc->dcl.con, INPUT_AXIS_X, dx);
+    qemu_input_queue_rel(ddc->dcl.con, INPUT_AXIS_Y, dy);
     qemu_input_event_sync();
 
     qemu_dbus_display1_mouse_complete_rel_motion(ddc->iface_mouse,
@@ -362,8 +361,8 @@ dbus_mouse_set_pos(DBusDisplayConsole *ddc,
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
 
-    width = qemu_console_get_width(ddc->con, 0);
-    height = qemu_console_get_height(ddc->con, 0);
+    width = qemu_console_get_width(ddc->dcl.con, 0);
+    height = qemu_console_get_height(ddc->dcl.con, 0);
     if (x >= width || y >= height) {
         g_dbus_method_invocation_return_error(
             invocation, DBUS_DISPLAY_ERROR,
@@ -371,8 +370,8 @@ dbus_mouse_set_pos(DBusDisplayConsole *ddc,
             "Invalid mouse position");
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
-    qemu_input_queue_abs(ddc->con, INPUT_AXIS_X, x, 0, width);
-    qemu_input_queue_abs(ddc->con, INPUT_AXIS_Y, y, 0, height);
+    qemu_input_queue_abs(ddc->dcl.con, INPUT_AXIS_X, x, 0, width);
+    qemu_input_queue_abs(ddc->dcl.con, INPUT_AXIS_Y, y, 0, height);
     qemu_input_event_sync();
 
     qemu_dbus_display1_mouse_complete_set_abs_position(ddc->iface_mouse,
@@ -388,7 +387,7 @@ dbus_mouse_press(DBusDisplayConsole *ddc,
 {
     trace_dbus_mouse_press(button);
 
-    qemu_input_queue_btn(ddc->con, button, true);
+    qemu_input_queue_btn(ddc->dcl.con, button, true);
     qemu_input_event_sync();
 
     qemu_dbus_display1_mouse_complete_press(ddc->iface_mouse, invocation);
@@ -403,7 +402,7 @@ dbus_mouse_release(DBusDisplayConsole *ddc,
 {
     trace_dbus_mouse_release(button);
 
-    qemu_input_queue_btn(ddc->con, button, false);
+    qemu_input_queue_btn(ddc->dcl.con, button, false);
     qemu_input_event_sync();
 
     qemu_dbus_display1_mouse_complete_release(ddc->iface_mouse, invocation);
@@ -424,7 +423,7 @@ dbus_mouse_mode_change(Notifier *notify, void *data)
 
 int dbus_display_console_get_index(DBusDisplayConsole *ddc)
 {
-    return qemu_console_get_index(ddc->con);
+    return qemu_console_get_index(ddc->dcl.con);
 }
 
 DBusDisplayConsole *
@@ -446,7 +445,7 @@ dbus_display_console_new(DBusDisplay *display, QemuConsole *con)
                         "g-object-path", path,
                         NULL);
     ddc->display = display;
-    ddc->con = con;
+    ddc->dcl.con = con;
     /* handle errors, and skip non graphics? */
     qemu_console_fill_device_address(
         con, device_addr, sizeof(device_addr), NULL);
diff --git a/ui/dbus.c b/ui/dbus.c
index f00a44421cf7..22c82d2f323a 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -52,7 +52,7 @@ static bool
 dbus_is_compatible_dcl(DisplayGLCtx *dgc,
                        DisplayChangeListener *dcl)
 {
-    return dcl->ops == &dbus_gl_dcl_ops;
+    return dcl->ops == &dbus_gl_dcl_ops || dcl->ops == &dbus_console_dcl_ops;
 }
 
 static const DisplayGLCtxOps dbus_gl_ops = {
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 08/12] ui/console: move console compatibility check to dcl_display_console()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The current checks are done at registration time only. However, if a DCL
has no specific console specified, it may be switched dynamically with
console_select() later on.

Let's move the checks when displaychangelistener_display_console() is
called, which includes registration time and remains fatal if the
specified console is incompatible.

Note: we may want to display the compatibility error to the DCL, this is
left for a future improvement.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index c9318552871b..d3ecbb215736 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -148,6 +148,8 @@ static DisplayState *get_alloc_displaystate(void);
 static void text_console_update_cursor_timer(void);
 static void text_console_update_cursor(void *opaque);
 static bool displaychangelistener_has_dmabuf(DisplayChangeListener *dcl);
+static bool console_compatible_with(QemuConsole *con,
+                                    DisplayChangeListener *dcl, Error **errp);
 
 static void gui_update(void *opaque)
 {
@@ -1057,13 +1059,14 @@ static void console_putchar(QemuConsole *s, int ch)
 }
 
 static void displaychangelistener_display_console(DisplayChangeListener *dcl,
-                                                  QemuConsole *con)
+                                                  QemuConsole *con,
+                                                  Error **errp)
 {
     static const char nodev[] =
         "This VM has no graphic display device.";
     static DisplaySurface *dummy;
 
-    if (!con) {
+    if (!con || !console_compatible_with(con, dcl, errp)) {
         if (!dcl->ops->dpy_gfx_switch) {
             return;
         }
@@ -1114,7 +1117,7 @@ void console_select(unsigned int index)
                 if (dcl->con != NULL) {
                     continue;
                 }
-                displaychangelistener_display_console(dcl, s);
+                displaychangelistener_display_console(dcl, s, NULL);
             }
         }
         if (ds->have_text) {
@@ -1475,8 +1478,8 @@ static bool displaychangelistener_has_dmabuf(DisplayChangeListener *dcl)
     return false;
 }
 
-static bool dpy_compatible_with(QemuConsole *con,
-                                DisplayChangeListener *dcl, Error **errp)
+static bool console_compatible_with(QemuConsole *con,
+                                    DisplayChangeListener *dcl, Error **errp)
 {
     int flags;
 
@@ -1522,10 +1525,6 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
 
     assert(!dcl->ds);
 
-    if (dcl->con) {
-        dpy_compatible_with(dcl->con, dcl, &error_fatal);
-    }
-
     trace_displaychangelistener_register(dcl, dcl->ops->dpy_name);
     dcl->ds = get_alloc_displaystate();
     QLIST_INSERT_HEAD(&dcl->ds->listeners, dcl, next);
@@ -1536,7 +1535,7 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
     } else {
         con = active_console;
     }
-    displaychangelistener_display_console(dcl, con);
+    displaychangelistener_display_console(dcl, con, dcl->con ? &error_fatal : NULL);
     text_console_update_cursor(NULL);
 }
 
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 09/12] ui/shader: fix potential leak of shader on error

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Value of 0 for program and shaders are silently ignored and indicate error.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/shader.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ui/shader.c b/ui/shader.c
index e8b8d321b7c7..4c80fc831f68 100644
--- a/ui/shader.c
+++ b/ui/shader.c
@@ -130,15 +130,17 @@ static GLuint qemu_gl_create_link_program(GLuint vert, GLuint frag)
 static GLuint qemu_gl_create_compile_link_program(const GLchar *vert_src,
                                                   const GLchar *frag_src)
 {
-    GLuint vert_shader, frag_shader, program;
+    GLuint vert_shader, frag_shader, program = 0;
 
     vert_shader = qemu_gl_create_compile_shader(GL_VERTEX_SHADER, vert_src);
     frag_shader = qemu_gl_create_compile_shader(GL_FRAGMENT_SHADER, frag_src);
     if (!vert_shader || !frag_shader) {
-        return 0;
+        goto end;
     }
 
     program = qemu_gl_create_link_program(vert_shader, frag_shader);
+
+end:
     glDeleteShader(vert_shader);
     glDeleteShader(frag_shader);
 
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 10/12] ui/shader: free associated programs

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/shader.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/shader.c b/ui/shader.c
index 4c80fc831f68..ab448c41d4c6 100644
--- a/ui/shader.c
+++ b/ui/shader.c
@@ -172,5 +172,8 @@ void qemu_gl_fini_shader(QemuGLShader *gls)
     if (!gls) {
         return;
     }
+    glDeleteProgram(gls->texture_blit_prog);
+    glDeleteProgram(gls->texture_blit_flip_prog);
+    glDeleteProgram(gls->texture_blit_vao);
     g_free(gls);
 }
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 11/12] ui/console: add a dpy_gfx_switch callback helper

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Slight code improvement.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index d3ecbb215736..102fcf0a5068 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1058,6 +1058,15 @@ static void console_putchar(QemuConsole *s, int ch)
     }
 }
 
+static void displaychangelistener_gfx_switch(DisplayChangeListener *dcl,
+                                             struct DisplaySurface *new_surface)
+{
+    if (dcl->ops->dpy_gfx_switch) {
+        dcl->ops->dpy_gfx_switch(dcl, new_surface);
+    }
+}
+
+
 static void displaychangelistener_display_console(DisplayChangeListener *dcl,
                                                   QemuConsole *con,
                                                   Error **errp)
@@ -1067,13 +1076,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
     static DisplaySurface *dummy;
 
     if (!con || !console_compatible_with(con, dcl, errp)) {
-        if (!dcl->ops->dpy_gfx_switch) {
-            return;
-        }
         if (!dummy) {
             dummy = qemu_create_placeholder_surface(640, 480, nodev);
         }
-        dcl->ops->dpy_gfx_switch(dcl, dummy);
+        displaychangelistener_gfx_switch(dcl, dummy);
         return;
     }
 
@@ -1091,9 +1097,8 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
                                          con->scanout.texture.y,
                                          con->scanout.texture.width,
                                          con->scanout.texture.height);
-    } else if (con->scanout.kind == SCANOUT_SURFACE &&
-               dcl->ops->dpy_gfx_switch) {
-        dcl->ops->dpy_gfx_switch(dcl, con->surface);
+    } else if (con->scanout.kind == SCANOUT_SURFACE) {
+        displaychangelistener_gfx_switch(dcl, con->surface);
     }
 
     dcl->ops->dpy_gfx_update(dcl, 0, 0,
@@ -1677,9 +1682,7 @@ void dpy_gfx_replace_surface(QemuConsole *con,
         if (con != (dcl->con ? dcl->con : active_console)) {
             continue;
         }
-        if (dcl->ops->dpy_gfx_switch) {
-            dcl->ops->dpy_gfx_switch(dcl, surface);
-        }
+        displaychangelistener_gfx_switch(dcl, surface);
     }
     qemu_free_displaysurface(old_surface);
 }
-- 
2.34.1.428.gdcc0cd074f0c

[PATCH v2 12/12] ui/dbus: fix texture sharing

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The DBus listener naively create, update and destroy textures without
taking into account other listeners. The texture were shared, but
texture update was unnecessarily duplicated.

Teach DisplayGLCtx to do optionally shared texture handling. This is
only implemented for DBus display at this point, however the same
infrastructure could potentially be used for egl-headless & VNC
listeners for example, or other future combinations.

Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/console.h | 10 ++++++++++
 ui/console.c         | 26 ++++++++++++++++++++++++++
 ui/dbus-listener.c   | 11 -----------
 ui/dbus.c            | 24 ++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 18a10c0b7db0..0f84861933e1 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -290,10 +290,20 @@ typedef struct DisplayGLCtxOps {
                                QEMUGLContext ctx);
     int (*dpy_gl_ctx_make_current)(DisplayGLCtx *dgc,
                                    QEMUGLContext ctx);
+    void (*dpy_gl_ctx_create_texture)(DisplayGLCtx *dgc,
+                                      DisplaySurface *surface);
+    void (*dpy_gl_ctx_destroy_texture)(DisplayGLCtx *dgc,
+                                      DisplaySurface *surface);
+    void (*dpy_gl_ctx_update_texture)(DisplayGLCtx *dgc,
+                                      DisplaySurface *surface,
+                                      int x, int y, int w, int h);
 } DisplayGLCtxOps;
 
 struct DisplayGLCtx {
     const DisplayGLCtxOps *ops;
+#ifdef CONFIG_OPENGL
+    QemuGLShader *gls; /* optional shared shader */
+#endif
 };
 
 DisplayState *init_displaystate(void);
diff --git a/ui/console.c b/ui/console.c
index 102fcf0a5068..b9188559fb12 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1066,6 +1066,27 @@ static void displaychangelistener_gfx_switch(DisplayChangeListener *dcl,
     }
 }
 
+static void dpy_gfx_create_texture(QemuConsole *con, DisplaySurface *surface)
+{
+    if (con->gl && con->gl->ops->dpy_gl_ctx_create_texture) {
+        con->gl->ops->dpy_gl_ctx_create_texture(con->gl, surface);
+    }
+}
+
+static void dpy_gfx_destroy_texture(QemuConsole *con, DisplaySurface *surface)
+{
+    if (con->gl && con->gl->ops->dpy_gl_ctx_destroy_texture) {
+        con->gl->ops->dpy_gl_ctx_destroy_texture(con->gl, surface);
+    }
+}
+
+static void dpy_gfx_update_texture(QemuConsole *con, DisplaySurface *surface,
+                                   int x, int y, int w, int h)
+{
+    if (con->gl && con->gl->ops->dpy_gl_ctx_update_texture) {
+        con->gl->ops->dpy_gl_ctx_update_texture(con->gl, surface, x, y, w, h);
+    }
+}
 
 static void displaychangelistener_display_console(DisplayChangeListener *dcl,
                                                   QemuConsole *con,
@@ -1078,6 +1099,7 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
     if (!con || !console_compatible_with(con, dcl, errp)) {
         if (!dummy) {
             dummy = qemu_create_placeholder_surface(640, 480, nodev);
+            dpy_gfx_create_texture(con, dummy);
         }
         displaychangelistener_gfx_switch(dcl, dummy);
         return;
@@ -1098,6 +1120,7 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
                                          con->scanout.texture.width,
                                          con->scanout.texture.height);
     } else if (con->scanout.kind == SCANOUT_SURFACE) {
+        dpy_gfx_create_texture(con, con->surface);
         displaychangelistener_gfx_switch(dcl, con->surface);
     }
 
@@ -1634,6 +1657,7 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
     if (!qemu_console_is_visible(con)) {
         return;
     }
+    dpy_gfx_update_texture(con, con->surface, x, y, w, h);
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (con != (dcl->con ? dcl->con : active_console)) {
             continue;
@@ -1678,12 +1702,14 @@ void dpy_gfx_replace_surface(QemuConsole *con,
 
     con->scanout.kind = SCANOUT_SURFACE;
     con->surface = surface;
+    dpy_gfx_create_texture(con, surface);
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (con != (dcl->con ? dcl->con : active_console)) {
             continue;
         }
         displaychangelistener_gfx_switch(dcl, surface);
     }
+    dpy_gfx_destroy_texture(con, old_surface);
     qemu_free_displaysurface(old_surface);
 }
 
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 81c119b13a2c..a287edd2fc15 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -42,7 +42,6 @@ struct _DBusDisplayListener {
 
     DisplayChangeListener dcl;
     DisplaySurface *ds;
-    QemuGLShader *gls;
     int gl_updates;
 };
 
@@ -240,10 +239,6 @@ static void dbus_gl_gfx_update(DisplayChangeListener *dcl,
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
 
-    if (ddl->ds) {
-        surface_gl_update_texture(ddl->gls, ddl->ds, x, y, w, h);
-    }
-
     ddl->gl_updates++;
 }
 
@@ -285,15 +280,11 @@ static void dbus_gl_gfx_switch(DisplayChangeListener *dcl,
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
 
-    if (ddl->ds) {
-        surface_gl_destroy_texture(ddl->gls, ddl->ds);
-    }
     ddl->ds = new_surface;
     if (ddl->ds) {
         int width = surface_width(ddl->ds);
         int height = surface_height(ddl->ds);
 
-        surface_gl_create_texture(ddl->gls, ddl->ds);
         /* TODO: lazy send dmabuf (there are unnecessary sent otherwise) */
         dbus_scanout_texture(&ddl->dcl, ddl->ds->texture, false,
                              width, height, 0, 0, width, height);
@@ -403,7 +394,6 @@ dbus_display_listener_dispose(GObject *object)
     g_clear_object(&ddl->conn);
     g_clear_pointer(&ddl->bus_name, g_free);
     g_clear_object(&ddl->proxy);
-    g_clear_pointer(&ddl->gls, qemu_gl_fini_shader);
 
     G_OBJECT_CLASS(dbus_display_listener_parent_class)->dispose(object);
 }
@@ -414,7 +404,6 @@ dbus_display_listener_constructed(GObject *object)
     DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(object);
 
     if (display_opengl) {
-        ddl->gls = qemu_gl_init_shader();
         ddl->dcl.ops = &dbus_gl_dcl_ops;
     } else {
         ddl->dcl.ops = &dbus_dcl_ops;
diff --git a/ui/dbus.c b/ui/dbus.c
index 22c82d2f323a..8e2e4c9cad28 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -55,11 +55,33 @@ dbus_is_compatible_dcl(DisplayGLCtx *dgc,
     return dcl->ops == &dbus_gl_dcl_ops || dcl->ops == &dbus_console_dcl_ops;
 }
 
+static void
+dbus_create_texture(DisplayGLCtx *ctx, DisplaySurface *surface)
+{
+    surface_gl_create_texture(ctx->gls, surface);
+}
+
+static void
+dbus_destroy_texture(DisplayGLCtx *ctx, DisplaySurface *surface)
+{
+    surface_gl_destroy_texture(ctx->gls, surface);
+}
+
+static void
+dbus_update_texture(DisplayGLCtx *ctx, DisplaySurface *surface,
+                    int x, int y, int w, int h)
+{
+    surface_gl_update_texture(ctx->gls, surface, x, y, w, h);
+}
+
 static const DisplayGLCtxOps dbus_gl_ops = {
     .dpy_gl_ctx_is_compatible_dcl = dbus_is_compatible_dcl,
     .dpy_gl_ctx_create       = dbus_create_context,
     .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
     .dpy_gl_ctx_make_current = qemu_egl_make_context_current,
+    .dpy_gl_ctx_create_texture = dbus_create_texture,
+    .dpy_gl_ctx_destroy_texture = dbus_destroy_texture,
+    .dpy_gl_ctx_update_texture = dbus_update_texture,
 };
 
 static NotifierList dbus_display_notifiers =
@@ -90,6 +112,7 @@ dbus_display_init(Object *o)
     g_autoptr(GDBusObjectSkeleton) vm = NULL;
 
     dd->glctx.ops = &dbus_gl_ops;
+    dd->glctx.gls = qemu_gl_init_shader();
     dd->iface = qemu_dbus_display1_vm_skeleton_new();
     dd->consoles = g_ptr_array_new_with_free_func(g_object_unref);
 
@@ -126,6 +149,7 @@ dbus_display_finalize(Object *o)
     g_clear_object(&dd->iface);
     g_free(dd->dbus_addr);
     g_free(dd->audiodev);
+    g_clear_pointer(&dd->glctx.gls, qemu_gl_fini_shader);
     dbus_display = NULL;
 }
 
-- 
2.34.1.428.gdcc0cd074f0c

Fwd: [PATCH v2 12/12] ui/dbus: fix texture sharing

[Akihiko Odaki]

I mistakenly dropped CC (again). My apologies.

---------- Forwarded message ---------
From: Akihiko Odaki <akihiko.odaki@gmail.com>
Date: Thu, Feb 17, 2022 at 9:38 PM
Subject: Re: [PATCH v2 12/12] ui/dbus: fix texture sharing
To: Marc-André Lureau <marcandre.lureau@redhat.com>


On Thu, Feb 17, 2022 at 9:02 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The DBus listener naively create, update and destroy textures without
> taking into account other listeners. The texture were shared, but
> texture update was unnecessarily duplicated.
>
> Teach DisplayGLCtx to do optionally shared texture handling. This is
> only implemented for DBus display at this point, however the same
> infrastructure could potentially be used for egl-headless & VNC
> listeners for example, or other future combinations.

egl-headless only needs one DisplayChangeListener per console since
its output is propagated by ui/console. The vnc protocol is for remote
access and therefore cannot have OpenGL textures. The functionality is
unique to dbus.

>
> Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/ui/console.h | 10 ++++++++++
>  ui/console.c         | 26 ++++++++++++++++++++++++++
>  ui/dbus-listener.c   | 11 -----------
>  ui/dbus.c            | 24 ++++++++++++++++++++++++
>  4 files changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 18a10c0b7db0..0f84861933e1 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -290,10 +290,20 @@ typedef struct DisplayGLCtxOps {
>                                 QEMUGLContext ctx);
>      int (*dpy_gl_ctx_make_current)(DisplayGLCtx *dgc,
>                                     QEMUGLContext ctx);
> +    void (*dpy_gl_ctx_create_texture)(DisplayGLCtx *dgc,
> +                                      DisplaySurface *surface);
> +    void (*dpy_gl_ctx_destroy_texture)(DisplayGLCtx *dgc,
> +                                      DisplaySurface *surface);
> +    void (*dpy_gl_ctx_update_texture)(DisplayGLCtx *dgc,
> +                                      DisplaySurface *surface,
> +                                      int x, int y, int w, int h);
>  } DisplayGLCtxOps;
>
>  struct DisplayGLCtx {
>      const DisplayGLCtxOps *ops;
> +#ifdef CONFIG_OPENGL
> +    QemuGLShader *gls; /* optional shared shader */
> +#endif
>  };
>
>  DisplayState *init_displaystate(void);
> diff --git a/ui/console.c b/ui/console.c
> index 102fcf0a5068..b9188559fb12 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1066,6 +1066,27 @@ static void displaychangelistener_gfx_switch(DisplayChangeListener *dcl,
>      }
>  }
>
> +static void dpy_gfx_create_texture(QemuConsole *con, DisplaySurface *surface)
> +{
> +    if (con->gl && con->gl->ops->dpy_gl_ctx_create_texture) {
> +        con->gl->ops->dpy_gl_ctx_create_texture(con->gl, surface);
> +    }
> +}
> +
> +static void dpy_gfx_destroy_texture(QemuConsole *con, DisplaySurface *surface)
> +{
> +    if (con->gl && con->gl->ops->dpy_gl_ctx_destroy_texture) {
> +        con->gl->ops->dpy_gl_ctx_destroy_texture(con->gl, surface);
> +    }
> +}
> +
> +static void dpy_gfx_update_texture(QemuConsole *con, DisplaySurface *surface,
> +                                   int x, int y, int w, int h)
> +{
> +    if (con->gl && con->gl->ops->dpy_gl_ctx_update_texture) {
> +        con->gl->ops->dpy_gl_ctx_update_texture(con->gl, surface, x, y, w, h);
> +    }
> +}
>
>  static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>                                                    QemuConsole *con,
> @@ -1078,6 +1099,7 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>      if (!con || !console_compatible_with(con, dcl, errp)) {
>          if (!dummy) {
>              dummy = qemu_create_placeholder_surface(640, 480, nodev);
> +            dpy_gfx_create_texture(con, dummy);
>          }
>          displaychangelistener_gfx_switch(dcl, dummy);
>          return;
> @@ -1098,6 +1120,7 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>                                           con->scanout.texture.width,
>                                           con->scanout.texture.height);
>      } else if (con->scanout.kind == SCANOUT_SURFACE) {
> +        dpy_gfx_create_texture(con, con->surface);
>          displaychangelistener_gfx_switch(dcl, con->surface);
>      }
>
> @@ -1634,6 +1657,7 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
>      if (!qemu_console_is_visible(con)) {
>          return;
>      }
> +    dpy_gfx_update_texture(con, con->surface, x, y, w, h);
>      QLIST_FOREACH(dcl, &s->listeners, next) {
>          if (con != (dcl->con ? dcl->con : active_console)) {
>              continue;
> @@ -1678,12 +1702,14 @@ void dpy_gfx_replace_surface(QemuConsole *con,
>
>      con->scanout.kind = SCANOUT_SURFACE;
>      con->surface = surface;
> +    dpy_gfx_create_texture(con, surface);
>      QLIST_FOREACH(dcl, &s->listeners, next) {
>          if (con != (dcl->con ? dcl->con : active_console)) {
>              continue;
>          }
>          displaychangelistener_gfx_switch(dcl, surface);
>      }
> +    dpy_gfx_destroy_texture(con, old_surface);
>      qemu_free_displaysurface(old_surface);
>  }
>
> diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> index 81c119b13a2c..a287edd2fc15 100644
> --- a/ui/dbus-listener.c
> +++ b/ui/dbus-listener.c
> @@ -42,7 +42,6 @@ struct _DBusDisplayListener {
>
>      DisplayChangeListener dcl;
>      DisplaySurface *ds;
> -    QemuGLShader *gls;
>      int gl_updates;
>  };
>
> @@ -240,10 +239,6 @@ static void dbus_gl_gfx_update(DisplayChangeListener *dcl,
>  {
>      DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
>
> -    if (ddl->ds) {
> -        surface_gl_update_texture(ddl->gls, ddl->ds, x, y, w, h);
> -    }
> -
>      ddl->gl_updates++;
>  }
>
> @@ -285,15 +280,11 @@ static void dbus_gl_gfx_switch(DisplayChangeListener *dcl,
>  {
>      DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
>
> -    if (ddl->ds) {
> -        surface_gl_destroy_texture(ddl->gls, ddl->ds);
> -    }
>      ddl->ds = new_surface;
>      if (ddl->ds) {
>          int width = surface_width(ddl->ds);
>          int height = surface_height(ddl->ds);
>
> -        surface_gl_create_texture(ddl->gls, ddl->ds);
>          /* TODO: lazy send dmabuf (there are unnecessary sent otherwise) */
>          dbus_scanout_texture(&ddl->dcl, ddl->ds->texture, false,
>                               width, height, 0, 0, width, height);
> @@ -403,7 +394,6 @@ dbus_display_listener_dispose(GObject *object)
>      g_clear_object(&ddl->conn);
>      g_clear_pointer(&ddl->bus_name, g_free);
>      g_clear_object(&ddl->proxy);
> -    g_clear_pointer(&ddl->gls, qemu_gl_fini_shader);
>
>      G_OBJECT_CLASS(dbus_display_listener_parent_class)->dispose(object);
>  }
> @@ -414,7 +404,6 @@ dbus_display_listener_constructed(GObject *object)
>      DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(object);
>
>      if (display_opengl) {
> -        ddl->gls = qemu_gl_init_shader();
>          ddl->dcl.ops = &dbus_gl_dcl_ops;
>      } else {
>          ddl->dcl.ops = &dbus_dcl_ops;
> diff --git a/ui/dbus.c b/ui/dbus.c
> index 22c82d2f323a..8e2e4c9cad28 100644
> --- a/ui/dbus.c
> +++ b/ui/dbus.c
> @@ -55,11 +55,33 @@ dbus_is_compatible_dcl(DisplayGLCtx *dgc,
>      return dcl->ops == &dbus_gl_dcl_ops || dcl->ops == &dbus_console_dcl_ops;
>  }
>
> +static void
> +dbus_create_texture(DisplayGLCtx *ctx, DisplaySurface *surface)
> +{
> +    surface_gl_create_texture(ctx->gls, surface);
> +}
> +
> +static void
> +dbus_destroy_texture(DisplayGLCtx *ctx, DisplaySurface *surface)
> +{
> +    surface_gl_destroy_texture(ctx->gls, surface);
> +}
> +
> +static void
> +dbus_update_texture(DisplayGLCtx *ctx, DisplaySurface *surface,
> +                    int x, int y, int w, int h)
> +{
> +    surface_gl_update_texture(ctx->gls, surface, x, y, w, h);
> +}
> +
>  static const DisplayGLCtxOps dbus_gl_ops = {
>      .dpy_gl_ctx_is_compatible_dcl = dbus_is_compatible_dcl,
>      .dpy_gl_ctx_create       = dbus_create_context,
>      .dpy_gl_ctx_destroy      = qemu_egl_destroy_context,
>      .dpy_gl_ctx_make_current = qemu_egl_make_context_current,
> +    .dpy_gl_ctx_create_texture = dbus_create_texture,
> +    .dpy_gl_ctx_destroy_texture = dbus_destroy_texture,
> +    .dpy_gl_ctx_update_texture = dbus_update_texture,
>  };
>
>  static NotifierList dbus_display_notifiers =
> @@ -90,6 +112,7 @@ dbus_display_init(Object *o)
>      g_autoptr(GDBusObjectSkeleton) vm = NULL;
>
>      dd->glctx.ops = &dbus_gl_ops;
> +    dd->glctx.gls = qemu_gl_init_shader();
>      dd->iface = qemu_dbus_display1_vm_skeleton_new();
>      dd->consoles = g_ptr_array_new_with_free_func(g_object_unref);
>
> @@ -126,6 +149,7 @@ dbus_display_finalize(Object *o)
>      g_clear_object(&dd->iface);
>      g_free(dd->dbus_addr);
>      g_free(dd->audiodev);
> +    g_clear_pointer(&dd->glctx.gls, qemu_gl_fini_shader);
>      dbus_display = NULL;
>  }
>
> --
> 2.34.1.428.gdcc0cd074f0c
>

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Akihiko Odaki]

On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console", Akihiko
> Odaki reported a number of issues with the GL and D-Bus display. His series
> propose a different design, and reverting some of my previous generic console
> changes to fix those issues.
>
> However, as I work through the issue so far, they can be solved by reasonable
> simple fixes while keeping the console changes generic (not specific to the
> D-Bus display backend). I belive a shared infrastructure is more beneficial long
> term than having GL-specific code in the DBus code (in particular, the
> egl-headless & VNC combination could potentially use it)
>
> Thanks a lot Akihiko for reporting the issues proposing a different approach!
> Please test this alternative series and let me know of any further issues. My
> understanding is that you are mainly concerned with the Cocoa backend, and I
> don't have a way to test it, so please check it. If necessary, we may well have
> to revert my earlier changes and go your way, eventually.
>
> Marc-André Lureau (12):
>   ui/console: fix crash when using gl context with non-gl listeners
>   ui/console: fix texture leak when calling surface_gl_create_texture()
>   ui: do not create a surface when resizing a GL scanout
>   ui/console: move check for compatible GL context
>   ui/console: move dcl compatiblity check to a callback
>   ui/console: egl-headless is compatible with non-gl listeners
>   ui/dbus: associate the DBusDisplayConsole listener with the given
>     console
>   ui/console: move console compatibility check to dcl_display_console()
>   ui/shader: fix potential leak of shader on error
>   ui/shader: free associated programs
>   ui/console: add a dpy_gfx_switch callback helper
>   ui/dbus: fix texture sharing
>
>  include/ui/console.h |  19 ++++---
>  ui/dbus.h            |   3 ++
>  ui/console-gl.c      |   4 ++
>  ui/console.c         | 119 ++++++++++++++++++++++++++-----------------
>  ui/dbus-console.c    |  27 +++++-----
>  ui/dbus-listener.c   |  11 ----
>  ui/dbus.c            |  33 +++++++++++-
>  ui/egl-headless.c    |  17 ++++++-
>  ui/gtk.c             |  18 ++++++-
>  ui/sdl2.c            |   9 +++-
>  ui/shader.c          |   9 +++-
>  ui/spice-display.c   |   9 +++-
>  12 files changed, 192 insertions(+), 86 deletions(-)
>
> --
> 2.34.1.428.gdcc0cd074f0c
>
>

You missed only one thing:
>- that console_select and register_displaychangelistener may not call
> dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
> incompatible with non-OpenGL displays

displaychangelistener_display_console always has to call
dpy_gfx_switch for non-OpenGL displays, but it still doesn't.

Anything else should be addressed with this patch series. (And it also
has nice fixes for shader leaks.)

cocoa doesn't have OpenGL output and egl-headless, the cause of many
pains addressed here, does not work on macOS so you need little
attention. I have an out-of-tree OpenGL support for cocoa but it is
out-of-tree anyway and I can fix it anytime.

Regards,
Akihiko Odaki

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Akihiko Odaki]

On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
>> >
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Hi,
>> >
>> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console", Akihiko
>> > Odaki reported a number of issues with the GL and D-Bus display. His series
>> > propose a different design, and reverting some of my previous generic console
>> > changes to fix those issues.
>> >
>> > However, as I work through the issue so far, they can be solved by reasonable
>> > simple fixes while keeping the console changes generic (not specific to the
>> > D-Bus display backend). I belive a shared infrastructure is more beneficial long
>> > term than having GL-specific code in the DBus code (in particular, the
>> > egl-headless & VNC combination could potentially use it)
>> >
>> > Thanks a lot Akihiko for reporting the issues proposing a different approach!
>> > Please test this alternative series and let me know of any further issues. My
>> > understanding is that you are mainly concerned with the Cocoa backend, and I
>> > don't have a way to test it, so please check it. If necessary, we may well have
>> > to revert my earlier changes and go your way, eventually.
>> >
>> > Marc-André Lureau (12):
>> >   ui/console: fix crash when using gl context with non-gl listeners
>> >   ui/console: fix texture leak when calling surface_gl_create_texture()
>> >   ui: do not create a surface when resizing a GL scanout
>> >   ui/console: move check for compatible GL context
>> >   ui/console: move dcl compatiblity check to a callback
>> >   ui/console: egl-headless is compatible with non-gl listeners
>> >   ui/dbus: associate the DBusDisplayConsole listener with the given
>> >     console
>> >   ui/console: move console compatibility check to dcl_display_console()
>> >   ui/shader: fix potential leak of shader on error
>> >   ui/shader: free associated programs
>> >   ui/console: add a dpy_gfx_switch callback helper
>> >   ui/dbus: fix texture sharing
>> >
>> >  include/ui/console.h |  19 ++++---
>> >  ui/dbus.h            |   3 ++
>> >  ui/console-gl.c      |   4 ++
>> >  ui/console.c         | 119 ++++++++++++++++++++++++++-----------------
>> >  ui/dbus-console.c    |  27 +++++-----
>> >  ui/dbus-listener.c   |  11 ----
>> >  ui/dbus.c            |  33 +++++++++++-
>> >  ui/egl-headless.c    |  17 ++++++-
>> >  ui/gtk.c             |  18 ++++++-
>> >  ui/sdl2.c            |   9 +++-
>> >  ui/shader.c          |   9 +++-
>> >  ui/spice-display.c   |   9 +++-
>> >  12 files changed, 192 insertions(+), 86 deletions(-)
>> >
>> > --
>> > 2.34.1.428.gdcc0cd074f0c
>> >
>> >
>>
>> You missed only one thing:
>> >- that console_select and register_displaychangelistener may not call
>> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
>> > incompatible with non-OpenGL displays
>>
>> displaychangelistener_display_console always has to call
>> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
>
>
> Ok, would that be what you have in mind?
>
>  --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1122,6 +1122,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>      } else if (con->scanout.kind == SCANOUT_SURFACE) {
>          dpy_gfx_create_texture(con, con->surface);
>          displaychangelistener_gfx_switch(dcl, con->surface);
> +    } else {
> +        /* use the fallback surface, egl-headless keeps it updated */
> +        assert(con->surface);
> +        displaychangelistener_gfx_switch(dcl, con->surface);
>      }

It should call displaychangelistener_gfx_switch even when e.g.
con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
to the last DisplaySurface it received while con->scanout.kind ==
SCANOUT_TEXTURE.

>
> I wish such egl-headless specific code would be there, but we would need more refactoring.
>
> I think I would rather have a backend split for GL context, like "-object egl-context". egl-headless-specific copy code would be handled by common/util code for anything that wants a pixman surface (VNC, screen capture, non-GL display etc).
>
> This split would allow sharing the context code, and introduce other system specific GL initialization, such as WGL etc. Right now, I doubt the EGL code works on anything but Linux.

Sharing the context code is unlikely to happen. Usually the toolkit
(GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should
be used and how the context should be created for a particular window.
The context sharing can be achieved only for headless displays, namely
dbus, egl-headless and spice. Few people would want to use them in
combination.

>
>>
>> Anything else should be addressed with this patch series. (And it also
>> has nice fixes for shader leaks.)
>
>
> thanks
>
>>
>>
>> cocoa doesn't have OpenGL output and egl-headless, the cause of many
>> pains addressed here, does not work on macOS so you need little
>> attention. I have an out-of-tree OpenGL support for cocoa but it is
>> out-of-tree anyway and I can fix it anytime.
>
>
> Great!
>
> btw, I suppose you checked your DBus changes against the WIP "qemu-display" project. What was your experience? I don't think many people have tried it yet. Do you think this could be made to work on macOS? at least the non-dmabuf support should work, as long as Gtk4 has good macOS support. I don't know if dmabuf or similar exist there, any idea?

I tested it on Fedora. I think it would probably work on macOS but
maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
macOS but its situation is bad; it must be delivered via macOS's own
IPC mechanisms (Mach port and XPC), but they are for server-client
model and not for P2P. fileport mechanism allows to convert Mach port
to file descriptor, but it is not documented. (In reality, I think all
of the major browsers, Chromium, Firefox and Safari use fileport for
this purpose. Apple should really document it if they use it for their
app.) It is also possible to share IOSurface with a global number, but
it can be brute-forced and is insecure.

Regards,
Akihiko Odaki

>
>
> --
> Marc-André Lureau

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Marc-André Lureau]

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

Hi

On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Hi,
> >
> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console",
> Akihiko
> > Odaki reported a number of issues with the GL and D-Bus display. His
> series
> > propose a different design, and reverting some of my previous generic
> console
> > changes to fix those issues.
> >
> > However, as I work through the issue so far, they can be solved by
> reasonable
> > simple fixes while keeping the console changes generic (not specific to
> the
> > D-Bus display backend). I belive a shared infrastructure is more
> beneficial long
> > term than having GL-specific code in the DBus code (in particular, the
> > egl-headless & VNC combination could potentially use it)
> >
> > Thanks a lot Akihiko for reporting the issues proposing a different
> approach!
> > Please test this alternative series and let me know of any further
> issues. My
> > understanding is that you are mainly concerned with the Cocoa backend,
> and I
> > don't have a way to test it, so please check it. If necessary, we may
> well have
> > to revert my earlier changes and go your way, eventually.
> >
> > Marc-André Lureau (12):
> >   ui/console: fix crash when using gl context with non-gl listeners
> >   ui/console: fix texture leak when calling surface_gl_create_texture()
> >   ui: do not create a surface when resizing a GL scanout
> >   ui/console: move check for compatible GL context
> >   ui/console: move dcl compatiblity check to a callback
> >   ui/console: egl-headless is compatible with non-gl listeners
> >   ui/dbus: associate the DBusDisplayConsole listener with the given
> >     console
> >   ui/console: move console compatibility check to dcl_display_console()
> >   ui/shader: fix potential leak of shader on error
> >   ui/shader: free associated programs
> >   ui/console: add a dpy_gfx_switch callback helper
> >   ui/dbus: fix texture sharing
> >
> >  include/ui/console.h |  19 ++++---
> >  ui/dbus.h            |   3 ++
> >  ui/console-gl.c      |   4 ++
> >  ui/console.c         | 119 ++++++++++++++++++++++++++-----------------
> >  ui/dbus-console.c    |  27 +++++-----
> >  ui/dbus-listener.c   |  11 ----
> >  ui/dbus.c            |  33 +++++++++++-
> >  ui/egl-headless.c    |  17 ++++++-
> >  ui/gtk.c             |  18 ++++++-
> >  ui/sdl2.c            |   9 +++-
> >  ui/shader.c          |   9 +++-
> >  ui/spice-display.c   |   9 +++-
> >  12 files changed, 192 insertions(+), 86 deletions(-)
> >
> > --
> > 2.34.1.428.gdcc0cd074f0c
> >
> >
>
> You missed only one thing:
> >- that console_select and register_displaychangelistener may not call
> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
> > incompatible with non-OpenGL displays
>
> displaychangelistener_display_console always has to call
> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
>

Ok, would that be what you have in mind?

 --- a/ui/console.c
+++ b/ui/console.c
@@ -1122,6 +1122,10 @@ static void
displaychangelistener_display_console(DisplayChangeListener *dcl,
     } else if (con->scanout.kind == SCANOUT_SURFACE) {
         dpy_gfx_create_texture(con, con->surface);
         displaychangelistener_gfx_switch(dcl, con->surface);
+    } else {
+        /* use the fallback surface, egl-headless keeps it updated */
+        assert(con->surface);
+        displaychangelistener_gfx_switch(dcl, con->surface);
     }

I wish such egl-headless specific code would be there, but we would need
more refactoring.

I think I would rather have a backend split for GL context, like "-object
egl-context". egl-headless-specific copy code would be handled by
common/util code for anything that wants a pixman surface (VNC, screen
capture, non-GL display etc).

This split would allow sharing the context code, and introduce other system
specific GL initialization, such as WGL etc. Right now, I doubt the EGL
code works on anything but Linux.


> Anything else should be addressed with this patch series. (And it also
> has nice fixes for shader leaks.)
>

thanks


>
> cocoa doesn't have OpenGL output and egl-headless, the cause of many
> pains addressed here, does not work on macOS so you need little
> attention. I have an out-of-tree OpenGL support for cocoa but it is
> out-of-tree anyway and I can fix it anytime.
>

Great!

btw, I suppose you checked your DBus changes against the WIP "qemu-display"
project. What was your experience? I don't think many people have tried it
yet. Do you think this could be made to work on macOS? at least the
non-dmabuf support should work, as long as Gtk4 has good macOS support. I
don't know if dmabuf or similar exist there, any idea?


-- 
Marc-André Lureau

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

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Marc-André Lureau]

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

Hi

On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.odaki@gmail.com>
> wrote:
> >>
> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
> >> >
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > Hi,
> >> >
> >> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a
> console", Akihiko
> >> > Odaki reported a number of issues with the GL and D-Bus display. His
> series
> >> > propose a different design, and reverting some of my previous generic
> console
> >> > changes to fix those issues.
> >> >
> >> > However, as I work through the issue so far, they can be solved by
> reasonable
> >> > simple fixes while keeping the console changes generic (not specific
> to the
> >> > D-Bus display backend). I belive a shared infrastructure is more
> beneficial long
> >> > term than having GL-specific code in the DBus code (in particular, the
> >> > egl-headless & VNC combination could potentially use it)
> >> >
> >> > Thanks a lot Akihiko for reporting the issues proposing a different
> approach!
> >> > Please test this alternative series and let me know of any further
> issues. My
> >> > understanding is that you are mainly concerned with the Cocoa
> backend, and I
> >> > don't have a way to test it, so please check it. If necessary, we may
> well have
> >> > to revert my earlier changes and go your way, eventually.
> >> >
> >> > Marc-André Lureau (12):
> >> >   ui/console: fix crash when using gl context with non-gl listeners
> >> >   ui/console: fix texture leak when calling
> surface_gl_create_texture()
> >> >   ui: do not create a surface when resizing a GL scanout
> >> >   ui/console: move check for compatible GL context
> >> >   ui/console: move dcl compatiblity check to a callback
> >> >   ui/console: egl-headless is compatible with non-gl listeners
> >> >   ui/dbus: associate the DBusDisplayConsole listener with the given
> >> >     console
> >> >   ui/console: move console compatibility check to
> dcl_display_console()
> >> >   ui/shader: fix potential leak of shader on error
> >> >   ui/shader: free associated programs
> >> >   ui/console: add a dpy_gfx_switch callback helper
> >> >   ui/dbus: fix texture sharing
> >> >
> >> >  include/ui/console.h |  19 ++++---
> >> >  ui/dbus.h            |   3 ++
> >> >  ui/console-gl.c      |   4 ++
> >> >  ui/console.c         | 119
> ++++++++++++++++++++++++++-----------------
> >> >  ui/dbus-console.c    |  27 +++++-----
> >> >  ui/dbus-listener.c   |  11 ----
> >> >  ui/dbus.c            |  33 +++++++++++-
> >> >  ui/egl-headless.c    |  17 ++++++-
> >> >  ui/gtk.c             |  18 ++++++-
> >> >  ui/sdl2.c            |   9 +++-
> >> >  ui/shader.c          |   9 +++-
> >> >  ui/spice-display.c   |   9 +++-
> >> >  12 files changed, 192 insertions(+), 86 deletions(-)
> >> >
> >> > --
> >> > 2.34.1.428.gdcc0cd074f0c
> >> >
> >> >
> >>
> >> You missed only one thing:
> >> >- that console_select and register_displaychangelistener may not call
> >> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
> >> > incompatible with non-OpenGL displays
> >>
> >> displaychangelistener_display_console always has to call
> >> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
> >
> >
> > Ok, would that be what you have in mind?
> >
> >  --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -1122,6 +1122,10 @@ static void
> displaychangelistener_display_console(DisplayChangeListener *dcl,
> >      } else if (con->scanout.kind == SCANOUT_SURFACE) {
> >          dpy_gfx_create_texture(con, con->surface);
> >          displaychangelistener_gfx_switch(dcl, con->surface);
> > +    } else {
> > +        /* use the fallback surface, egl-headless keeps it updated */
> > +        assert(con->surface);
> > +        displaychangelistener_gfx_switch(dcl, con->surface);
> >      }
>
> It should call displaychangelistener_gfx_switch even when e.g.
> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
> to the last DisplaySurface it received while con->scanout.kind ==
> SCANOUT_TEXTURE.
>

I see, egl-headless is really not a "listener".


> >
> > I wish such egl-headless specific code would be there, but we would need
> more refactoring.
> >
> > I think I would rather have a backend split for GL context, like
> "-object egl-context". egl-headless-specific copy code would be handled by
> common/util code for anything that wants a pixman surface (VNC, screen
> capture, non-GL display etc).
> >
> > This split would allow sharing the context code, and introduce other
> system specific GL initialization, such as WGL etc. Right now, I doubt the
> EGL code works on anything but Linux.
>
> Sharing the context code is unlikely to happen. Usually the toolkit
> (GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should
> be used and how the context should be created for a particular window.
> The context sharing can be achieved only for headless displays, namely
> dbus, egl-headless and spice. Few people would want to use them in
> combination.
>

Ok for toolkits, they usually have their own context. But ideally, qemu
should be "headless". And the GL contexts should be working on other
systems than EGL Linux.

Any of the spice, vnc, dbus display etc may legitimately be fixed to work
with WGL etc. Doing this repeatedly on the various display backends would
be bad design.

Although my idea is that display servers (spice, vnc, rdp, etc) & various
UI (gtk, cocoa, sdl, etc) should be outside of qemu. The display would use
IPC, based on DBus if it fits the job, or something else if necessary.
Obviously, there is still a lot of work to do to improve surface & texture
sharing and portability, but that should be possible...


>
> >
> >>
> >> Anything else should be addressed with this patch series. (And it also
> >> has nice fixes for shader leaks.)
> >
> >
> > thanks
> >
> >>
> >>
> >> cocoa doesn't have OpenGL output and egl-headless, the cause of many
> >> pains addressed here, does not work on macOS so you need little
> >> attention. I have an out-of-tree OpenGL support for cocoa but it is
> >> out-of-tree anyway and I can fix it anytime.
> >
> >
> > Great!
> >
> > btw, I suppose you checked your DBus changes against the WIP
> "qemu-display" project. What was your experience? I don't think many people
> have tried it yet. Do you think this could be made to work on macOS? at
> least the non-dmabuf support should work, as long as Gtk4 has good macOS
> support. I don't know if dmabuf or similar exist there, any idea?
>
> I tested it on Fedora. I think it would probably work on macOS but
> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
> macOS but its situation is bad; it must be delivered via macOS's own
> IPC mechanisms (Mach port and XPC), but they are for server-client
> model and not for P2P. fileport mechanism allows to convert Mach port
> to file descriptor, but it is not documented. (In reality, I think all
> of the major browsers, Chromium, Firefox and Safari use fileport for
> this purpose. Apple should really document it if they use it for their
> app.) It is also possible to share IOSurface with a global number, but
> it can be brute-forced and is insecure.
>
>
Thanks, the Gtk developers might have some clue. They have been working on
improving macOS support, and it can use opengl now (
https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/
).


> Regards,
> Akihiko Odaki
>
> >
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

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

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Akihiko Odaki]

On Fri, Feb 18, 2022 at 2:07 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>> >>
>> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
>> >> >
>> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >
>> >> > Hi,
>> >> >
>> >> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console", Akihiko
>> >> > Odaki reported a number of issues with the GL and D-Bus display. His series
>> >> > propose a different design, and reverting some of my previous generic console
>> >> > changes to fix those issues.
>> >> >
>> >> > However, as I work through the issue so far, they can be solved by reasonable
>> >> > simple fixes while keeping the console changes generic (not specific to the
>> >> > D-Bus display backend). I belive a shared infrastructure is more beneficial long
>> >> > term than having GL-specific code in the DBus code (in particular, the
>> >> > egl-headless & VNC combination could potentially use it)
>> >> >
>> >> > Thanks a lot Akihiko for reporting the issues proposing a different approach!
>> >> > Please test this alternative series and let me know of any further issues. My
>> >> > understanding is that you are mainly concerned with the Cocoa backend, and I
>> >> > don't have a way to test it, so please check it. If necessary, we may well have
>> >> > to revert my earlier changes and go your way, eventually.
>> >> >
>> >> > Marc-André Lureau (12):
>> >> >   ui/console: fix crash when using gl context with non-gl listeners
>> >> >   ui/console: fix texture leak when calling surface_gl_create_texture()
>> >> >   ui: do not create a surface when resizing a GL scanout
>> >> >   ui/console: move check for compatible GL context
>> >> >   ui/console: move dcl compatiblity check to a callback
>> >> >   ui/console: egl-headless is compatible with non-gl listeners
>> >> >   ui/dbus: associate the DBusDisplayConsole listener with the given
>> >> >     console
>> >> >   ui/console: move console compatibility check to dcl_display_console()
>> >> >   ui/shader: fix potential leak of shader on error
>> >> >   ui/shader: free associated programs
>> >> >   ui/console: add a dpy_gfx_switch callback helper
>> >> >   ui/dbus: fix texture sharing
>> >> >
>> >> >  include/ui/console.h |  19 ++++---
>> >> >  ui/dbus.h            |   3 ++
>> >> >  ui/console-gl.c      |   4 ++
>> >> >  ui/console.c         | 119 ++++++++++++++++++++++++++-----------------
>> >> >  ui/dbus-console.c    |  27 +++++-----
>> >> >  ui/dbus-listener.c   |  11 ----
>> >> >  ui/dbus.c            |  33 +++++++++++-
>> >> >  ui/egl-headless.c    |  17 ++++++-
>> >> >  ui/gtk.c             |  18 ++++++-
>> >> >  ui/sdl2.c            |   9 +++-
>> >> >  ui/shader.c          |   9 +++-
>> >> >  ui/spice-display.c   |   9 +++-
>> >> >  12 files changed, 192 insertions(+), 86 deletions(-)
>> >> >
>> >> > --
>> >> > 2.34.1.428.gdcc0cd074f0c
>> >> >
>> >> >
>> >>
>> >> You missed only one thing:
>> >> >- that console_select and register_displaychangelistener may not call
>> >> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
>> >> > incompatible with non-OpenGL displays
>> >>
>> >> displaychangelistener_display_console always has to call
>> >> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
>> >
>> >
>> > Ok, would that be what you have in mind?
>> >
>> >  --- a/ui/console.c
>> > +++ b/ui/console.c
>> > @@ -1122,6 +1122,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>> >      } else if (con->scanout.kind == SCANOUT_SURFACE) {
>> >          dpy_gfx_create_texture(con, con->surface);
>> >          displaychangelistener_gfx_switch(dcl, con->surface);
>> > +    } else {
>> > +        /* use the fallback surface, egl-headless keeps it updated */
>> > +        assert(con->surface);
>> > +        displaychangelistener_gfx_switch(dcl, con->surface);
>> >      }
>>
>> It should call displaychangelistener_gfx_switch even when e.g.
>> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
>> to the last DisplaySurface it received while con->scanout.kind ==
>> SCANOUT_TEXTURE.
>
>
> I see, egl-headless is really not a "listener".
>
>>
>> >
>> > I wish such egl-headless specific code would be there, but we would need more refactoring.
>> >
>> > I think I would rather have a backend split for GL context, like "-object egl-context". egl-headless-specific copy code would be handled by common/util code for anything that wants a pixman surface (VNC, screen capture, non-GL display etc).
>> >
>> > This split would allow sharing the context code, and introduce other system specific GL initialization, such as WGL etc. Right now, I doubt the EGL code works on anything but Linux.
>>
>> Sharing the context code is unlikely to happen. Usually the toolkit
>> (GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should
>> be used and how the context should be created for a particular window.
>> The context sharing can be achieved only for headless displays, namely
>> dbus, egl-headless and spice. Few people would want to use them in
>> combination.
>
>
> Ok for toolkits, they usually have their own context. But ideally, qemu should be "headless". And the GL contexts should be working on other systems than EGL Linux.
>
> Any of the spice, vnc, dbus display etc may legitimately be fixed to work with WGL etc. Doing this repeatedly on the various display backends would be bad design.

We already have ui/egl-context.c to share the code for EGL. We can
have ui/headless-context.c or something which creates a context for
headless but the implementation can be anything proper there. It
doesn't require modifying ui/console.c or adding something like
"-object egl-context".

>
> Although my idea is that display servers (spice, vnc, rdp, etc) & various UI (gtk, cocoa, sdl, etc) should be outside of qemu. The display would use IPC, based on DBus if it fits the job, or something else if necessary. Obviously, there is still a lot of work to do to improve surface & texture sharing and portability, but that should be possible...

Maybe we can rework the present UIs of QEMU to make them compatible
with both in-process communication and D-Bus inter-process
communication. If the user has a requirement incompatible with IPC
(e.g. OpenGL on macOS), the user can opt for in-process communication.
D-Bus would be used otherwise. (Of course that would require
substantial effort.)

>
>>
>>
>> >
>> >>
>> >> Anything else should be addressed with this patch series. (And it also
>> >> has nice fixes for shader leaks.)
>> >
>> >
>> > thanks
>> >
>> >>
>> >>
>> >> cocoa doesn't have OpenGL output and egl-headless, the cause of many
>> >> pains addressed here, does not work on macOS so you need little
>> >> attention. I have an out-of-tree OpenGL support for cocoa but it is
>> >> out-of-tree anyway and I can fix it anytime.
>> >
>> >
>> > Great!
>> >
>> > btw, I suppose you checked your DBus changes against the WIP "qemu-display" project. What was your experience? I don't think many people have tried it yet. Do you think this could be made to work on macOS? at least the non-dmabuf support should work, as long as Gtk4 has good macOS support. I don't know if dmabuf or similar exist there, any idea?
>>
>> I tested it on Fedora. I think it would probably work on macOS but
>> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
>> macOS but its situation is bad; it must be delivered via macOS's own
>> IPC mechanisms (Mach port and XPC), but they are for server-client
>> model and not for P2P. fileport mechanism allows to convert Mach port
>> to file descriptor, but it is not documented. (In reality, I think all
>> of the major browsers, Chromium, Firefox and Safari use fileport for
>> this purpose. Apple should really document it if they use it for their
>> app.) It is also possible to share IOSurface with a global number, but
>> it can be brute-forced and is insecure.
>>
>
> Thanks, the Gtk developers might have some clue. They have been working on improving macOS support, and it can use opengl now (https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/).

They don't need IPC for passing textures so that is a different story.

>
>>
>> Regards,
>> Akihiko Odaki
>>
>> >
>> >
>> > --
>> > Marc-André Lureau
>
>
>
> --
> Marc-André Lureau

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Marc-André Lureau]

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

Hi

On Thu, Feb 17, 2022 at 9:25 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On Fri, Feb 18, 2022 at 2:07 AM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.odaki@gmail.com>
> wrote:
> >>
> >> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
> >> <marcandre.lureau@gmail.com> wrote:
> >> >
> >> > Hi
> >> >
> >> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <
> akihiko.odaki@gmail.com> wrote:
> >> >>
> >> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
> >> >> >
> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >
> >> >> > Hi,
> >> >> >
> >> >> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a
> console", Akihiko
> >> >> > Odaki reported a number of issues with the GL and D-Bus display.
> His series
> >> >> > propose a different design, and reverting some of my previous
> generic console
> >> >> > changes to fix those issues.
> >> >> >
> >> >> > However, as I work through the issue so far, they can be solved by
> reasonable
> >> >> > simple fixes while keeping the console changes generic (not
> specific to the
> >> >> > D-Bus display backend). I belive a shared infrastructure is more
> beneficial long
> >> >> > term than having GL-specific code in the DBus code (in particular,
> the
> >> >> > egl-headless & VNC combination could potentially use it)
> >> >> >
> >> >> > Thanks a lot Akihiko for reporting the issues proposing a
> different approach!
> >> >> > Please test this alternative series and let me know of any further
> issues. My
> >> >> > understanding is that you are mainly concerned with the Cocoa
> backend, and I
> >> >> > don't have a way to test it, so please check it. If necessary, we
> may well have
> >> >> > to revert my earlier changes and go your way, eventually.
> >> >> >
> >> >> > Marc-André Lureau (12):
> >> >> >   ui/console: fix crash when using gl context with non-gl listeners
> >> >> >   ui/console: fix texture leak when calling
> surface_gl_create_texture()
> >> >> >   ui: do not create a surface when resizing a GL scanout
> >> >> >   ui/console: move check for compatible GL context
> >> >> >   ui/console: move dcl compatiblity check to a callback
> >> >> >   ui/console: egl-headless is compatible with non-gl listeners
> >> >> >   ui/dbus: associate the DBusDisplayConsole listener with the given
> >> >> >     console
> >> >> >   ui/console: move console compatibility check to
> dcl_display_console()
> >> >> >   ui/shader: fix potential leak of shader on error
> >> >> >   ui/shader: free associated programs
> >> >> >   ui/console: add a dpy_gfx_switch callback helper
> >> >> >   ui/dbus: fix texture sharing
> >> >> >
> >> >> >  include/ui/console.h |  19 ++++---
> >> >> >  ui/dbus.h            |   3 ++
> >> >> >  ui/console-gl.c      |   4 ++
> >> >> >  ui/console.c         | 119
> ++++++++++++++++++++++++++-----------------
> >> >> >  ui/dbus-console.c    |  27 +++++-----
> >> >> >  ui/dbus-listener.c   |  11 ----
> >> >> >  ui/dbus.c            |  33 +++++++++++-
> >> >> >  ui/egl-headless.c    |  17 ++++++-
> >> >> >  ui/gtk.c             |  18 ++++++-
> >> >> >  ui/sdl2.c            |   9 +++-
> >> >> >  ui/shader.c          |   9 +++-
> >> >> >  ui/spice-display.c   |   9 +++-
> >> >> >  12 files changed, 192 insertions(+), 86 deletions(-)
> >> >> >
> >> >> > --
> >> >> > 2.34.1.428.gdcc0cd074f0c
> >> >> >
> >> >> >
> >> >>
> >> >> You missed only one thing:
> >> >> >- that console_select and register_displaychangelistener may not
> call
> >> >> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
> >> >> > incompatible with non-OpenGL displays
> >> >>
> >> >> displaychangelistener_display_console always has to call
> >> >> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
> >> >
> >> >
> >> > Ok, would that be what you have in mind?
> >> >
> >> >  --- a/ui/console.c
> >> > +++ b/ui/console.c
> >> > @@ -1122,6 +1122,10 @@ static void
> displaychangelistener_display_console(DisplayChangeListener *dcl,
> >> >      } else if (con->scanout.kind == SCANOUT_SURFACE) {
> >> >          dpy_gfx_create_texture(con, con->surface);
> >> >          displaychangelistener_gfx_switch(dcl, con->surface);
> >> > +    } else {
> >> > +        /* use the fallback surface, egl-headless keeps it updated */
> >> > +        assert(con->surface);
> >> > +        displaychangelistener_gfx_switch(dcl, con->surface);
> >> >      }
> >>
> >> It should call displaychangelistener_gfx_switch even when e.g.
> >> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
> >> to the last DisplaySurface it received while con->scanout.kind ==
> >> SCANOUT_TEXTURE.
> >
> >
> > I see, egl-headless is really not a "listener".
> >
> >>
> >> >
> >> > I wish such egl-headless specific code would be there, but we would
> need more refactoring.
> >> >
> >> > I think I would rather have a backend split for GL context, like
> "-object egl-context". egl-headless-specific copy code would be handled by
> common/util code for anything that wants a pixman surface (VNC, screen
> capture, non-GL display etc).
> >> >
> >> > This split would allow sharing the context code, and introduce other
> system specific GL initialization, such as WGL etc. Right now, I doubt the
> EGL code works on anything but Linux.
> >>
> >> Sharing the context code is unlikely to happen. Usually the toolkit
> >> (GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should
> >> be used and how the context should be created for a particular window.
> >> The context sharing can be achieved only for headless displays, namely
> >> dbus, egl-headless and spice. Few people would want to use them in
> >> combination.
> >
> >
> > Ok for toolkits, they usually have their own context. But ideally, qemu
> should be "headless". And the GL contexts should be working on other
> systems than EGL Linux.
> >
> > Any of the spice, vnc, dbus display etc may legitimately be fixed to
> work with WGL etc. Doing this repeatedly on the various display backends
> would be bad design.
>
> We already have ui/egl-context.c to share the code for EGL. We can
> have ui/headless-context.c or something which creates a context for
> headless but the implementation can be anything proper there. It
> doesn't require modifying ui/console.c or adding something like
> "-object egl-context".
>

Agree, as long as you have only a single context provider per system.  But
that's not my experience with GL in the past. Maybe this is true today.


> >
> > Although my idea is that display servers (spice, vnc, rdp, etc) &
> various UI (gtk, cocoa, sdl, etc) should be outside of qemu. The display
> would use IPC, based on DBus if it fits the job, or something else if
> necessary. Obviously, there is still a lot of work to do to improve surface
> & texture sharing and portability, but that should be possible...
>
> Maybe we can rework the present UIs of QEMU to make them compatible
> with both in-process communication and D-Bus inter-process
> communication. If the user has a requirement incompatible with IPC
> (e.g. OpenGL on macOS), the user can opt for in-process communication.
> D-Bus would be used otherwise. (Of course that would require
> substantial effort.)
>

That should be possible, as long the IPC is very close to the inner qemu
API, we could have an IPC-based display code turned into a shared library
instead and run in process. Although I think that would limit the kind of
UI you can expect (it would be a bare display, like qemu-display today, not
something that would bring you a full user-friendly UI, virt-manager/Boxes
kind)



> >
> >>
> >>
> >> >
> >> >>
> >> >> Anything else should be addressed with this patch series. (And it
> also
> >> >> has nice fixes for shader leaks.)
> >> >
> >> >
> >> > thanks
> >> >
> >> >>
> >> >>
> >> >> cocoa doesn't have OpenGL output and egl-headless, the cause of many
> >> >> pains addressed here, does not work on macOS so you need little
> >> >> attention. I have an out-of-tree OpenGL support for cocoa but it is
> >> >> out-of-tree anyway and I can fix it anytime.
> >> >
> >> >
> >> > Great!
> >> >
> >> > btw, I suppose you checked your DBus changes against the WIP
> "qemu-display" project. What was your experience? I don't think many people
> have tried it yet. Do you think this could be made to work on macOS? at
> least the non-dmabuf support should work, as long as Gtk4 has good macOS
> support. I don't know if dmabuf or similar exist there, any idea?
> >>
> >> I tested it on Fedora. I think it would probably work on macOS but
> >> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
> >> macOS but its situation is bad; it must be delivered via macOS's own
> >> IPC mechanisms (Mach port and XPC), but they are for server-client
> >> model and not for P2P. fileport mechanism allows to convert Mach port
> >> to file descriptor, but it is not documented. (In reality, I think all
> >> of the major browsers, Chromium, Firefox and Safari use fileport for
> >> this purpose. Apple should really document it if they use it for their
> >> app.) It is also possible to share IOSurface with a global number, but
> >> it can be brute-forced and is insecure.
> >>
> >
> > Thanks, the Gtk developers might have some clue. They have been working
> on improving macOS support, and it can use opengl now (
> https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/
> ).
>
> They don't need IPC for passing textures so that is a different story.
>

Yes but they have web-engine and video decoding concerns (beside
qemu/dmabuf gtk display they should be aware of).  I'll try to reach
Christian about it.

thanks


> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >> >
> >> >
> >> > --
> >> > Marc-André Lureau
> >
> >
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

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

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Akihiko Odaki]

On Fri, Feb 18, 2022 at 2:36 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 17, 2022 at 9:25 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On Fri, Feb 18, 2022 at 2:07 AM Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>> >>
>> >> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
>> >> <marcandre.lureau@gmail.com> wrote:
>> >> >
>> >> > Hi
>> >> >
>> >> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>> >> >>
>> >> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
>> >> >> >
>> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >> >
>> >> >> > Hi,
>> >> >> >
>> >> >> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console", Akihiko
>> >> >> > Odaki reported a number of issues with the GL and D-Bus display. His series
>> >> >> > propose a different design, and reverting some of my previous generic console
>> >> >> > changes to fix those issues.
>> >> >> >
>> >> >> > However, as I work through the issue so far, they can be solved by reasonable
>> >> >> > simple fixes while keeping the console changes generic (not specific to the
>> >> >> > D-Bus display backend). I belive a shared infrastructure is more beneficial long
>> >> >> > term than having GL-specific code in the DBus code (in particular, the
>> >> >> > egl-headless & VNC combination could potentially use it)
>> >> >> >
>> >> >> > Thanks a lot Akihiko for reporting the issues proposing a different approach!
>> >> >> > Please test this alternative series and let me know of any further issues. My
>> >> >> > understanding is that you are mainly concerned with the Cocoa backend, and I
>> >> >> > don't have a way to test it, so please check it. If necessary, we may well have
>> >> >> > to revert my earlier changes and go your way, eventually.
>> >> >> >
>> >> >> > Marc-André Lureau (12):
>> >> >> >   ui/console: fix crash when using gl context with non-gl listeners
>> >> >> >   ui/console: fix texture leak when calling surface_gl_create_texture()
>> >> >> >   ui: do not create a surface when resizing a GL scanout
>> >> >> >   ui/console: move check for compatible GL context
>> >> >> >   ui/console: move dcl compatiblity check to a callback
>> >> >> >   ui/console: egl-headless is compatible with non-gl listeners
>> >> >> >   ui/dbus: associate the DBusDisplayConsole listener with the given
>> >> >> >     console
>> >> >> >   ui/console: move console compatibility check to dcl_display_console()
>> >> >> >   ui/shader: fix potential leak of shader on error
>> >> >> >   ui/shader: free associated programs
>> >> >> >   ui/console: add a dpy_gfx_switch callback helper
>> >> >> >   ui/dbus: fix texture sharing
>> >> >> >
>> >> >> >  include/ui/console.h |  19 ++++---
>> >> >> >  ui/dbus.h            |   3 ++
>> >> >> >  ui/console-gl.c      |   4 ++
>> >> >> >  ui/console.c         | 119 ++++++++++++++++++++++++++-----------------
>> >> >> >  ui/dbus-console.c    |  27 +++++-----
>> >> >> >  ui/dbus-listener.c   |  11 ----
>> >> >> >  ui/dbus.c            |  33 +++++++++++-
>> >> >> >  ui/egl-headless.c    |  17 ++++++-
>> >> >> >  ui/gtk.c             |  18 ++++++-
>> >> >> >  ui/sdl2.c            |   9 +++-
>> >> >> >  ui/shader.c          |   9 +++-
>> >> >> >  ui/spice-display.c   |   9 +++-
>> >> >> >  12 files changed, 192 insertions(+), 86 deletions(-)
>> >> >> >
>> >> >> > --
>> >> >> > 2.34.1.428.gdcc0cd074f0c
>> >> >> >
>> >> >> >
>> >> >>
>> >> >> You missed only one thing:
>> >> >> >- that console_select and register_displaychangelistener may not call
>> >> >> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
>> >> >> > incompatible with non-OpenGL displays
>> >> >>
>> >> >> displaychangelistener_display_console always has to call
>> >> >> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
>> >> >
>> >> >
>> >> > Ok, would that be what you have in mind?
>> >> >
>> >> >  --- a/ui/console.c
>> >> > +++ b/ui/console.c
>> >> > @@ -1122,6 +1122,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>> >> >      } else if (con->scanout.kind == SCANOUT_SURFACE) {
>> >> >          dpy_gfx_create_texture(con, con->surface);
>> >> >          displaychangelistener_gfx_switch(dcl, con->surface);
>> >> > +    } else {
>> >> > +        /* use the fallback surface, egl-headless keeps it updated */
>> >> > +        assert(con->surface);
>> >> > +        displaychangelistener_gfx_switch(dcl, con->surface);
>> >> >      }
>> >>
>> >> It should call displaychangelistener_gfx_switch even when e.g.
>> >> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
>> >> to the last DisplaySurface it received while con->scanout.kind ==
>> >> SCANOUT_TEXTURE.
>> >
>> >
>> > I see, egl-headless is really not a "listener".
>> >
>> >>
>> >> >
>> >> > I wish such egl-headless specific code would be there, but we would need more refactoring.
>> >> >
>> >> > I think I would rather have a backend split for GL context, like "-object egl-context". egl-headless-specific copy code would be handled by common/util code for anything that wants a pixman surface (VNC, screen capture, non-GL display etc).
>> >> >
>> >> > This split would allow sharing the context code, and introduce other system specific GL initialization, such as WGL etc. Right now, I doubt the EGL code works on anything but Linux.
>> >>
>> >> Sharing the context code is unlikely to happen. Usually the toolkit
>> >> (GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should
>> >> be used and how the context should be created for a particular window.
>> >> The context sharing can be achieved only for headless displays, namely
>> >> dbus, egl-headless and spice. Few people would want to use them in
>> >> combination.
>> >
>> >
>> > Ok for toolkits, they usually have their own context. But ideally, qemu should be "headless". And the GL contexts should be working on other systems than EGL Linux.
>> >
>> > Any of the spice, vnc, dbus display etc may legitimately be fixed to work with WGL etc. Doing this repeatedly on the various display backends would be bad design.
>>
>> We already have ui/egl-context.c to share the code for EGL. We can
>> have ui/headless-context.c or something which creates a context for
>> headless but the implementation can be anything proper there. It
>> doesn't require modifying ui/console.c or adding something like
>> "-object egl-context".
>
>
> Agree, as long as you have only a single context provider per system.  But that's not my experience with GL in the past. Maybe this is true today.

It doesn't matter even if a system has multiple context providers.
ui/headless-context.c may just choose the context provider according
to the flag a user provided.

>
>>
>> >
>> > Although my idea is that display servers (spice, vnc, rdp, etc) & various UI (gtk, cocoa, sdl, etc) should be outside of qemu. The display would use IPC, based on DBus if it fits the job, or something else if necessary. Obviously, there is still a lot of work to do to improve surface & texture sharing and portability, but that should be possible...
>>
>> Maybe we can rework the present UIs of QEMU to make them compatible
>> with both in-process communication and D-Bus inter-process
>> communication. If the user has a requirement incompatible with IPC
>> (e.g. OpenGL on macOS), the user can opt for in-process communication.
>> D-Bus would be used otherwise. (Of course that would require
>> substantial effort.)
>
>
> That should be possible, as long the IPC is very close to the inner qemu API, we could have an IPC-based display code turned into a shared library instead and run in process.

That is exactly what I'm thinking of.

> Although I think that would limit the kind of UI you can expect (it would be a bare display, like qemu-display today, not something that would bring you a full user-friendly UI, virt-manager/Boxes kind)

I don't think QEMU has to provide anything fancier than the current
UIs. Users can combine QEMU and the external UI implementation like
virt-manager, which allows the UI to evolve in a separate way. The UIs
in QEMU would remain as references.

>
>
>>
>> >
>> >>
>> >>
>> >> >
>> >> >>
>> >> >> Anything else should be addressed with this patch series. (And it also
>> >> >> has nice fixes for shader leaks.)
>> >> >
>> >> >
>> >> > thanks
>> >> >
>> >> >>
>> >> >>
>> >> >> cocoa doesn't have OpenGL output and egl-headless, the cause of many
>> >> >> pains addressed here, does not work on macOS so you need little
>> >> >> attention. I have an out-of-tree OpenGL support for cocoa but it is
>> >> >> out-of-tree anyway and I can fix it anytime.
>> >> >
>> >> >
>> >> > Great!
>> >> >
>> >> > btw, I suppose you checked your DBus changes against the WIP "qemu-display" project. What was your experience? I don't think many people have tried it yet. Do you think this could be made to work on macOS? at least the non-dmabuf support should work, as long as Gtk4 has good macOS support. I don't know if dmabuf or similar exist there, any idea?
>> >>
>> >> I tested it on Fedora. I think it would probably work on macOS but
>> >> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
>> >> macOS but its situation is bad; it must be delivered via macOS's own
>> >> IPC mechanisms (Mach port and XPC), but they are for server-client
>> >> model and not for P2P. fileport mechanism allows to convert Mach port
>> >> to file descriptor, but it is not documented. (In reality, I think all
>> >> of the major browsers, Chromium, Firefox and Safari use fileport for
>> >> this purpose. Apple should really document it if they use it for their
>> >> app.) It is also possible to share IOSurface with a global number, but
>> >> it can be brute-forced and is insecure.
>> >>
>> >
>> > Thanks, the Gtk developers might have some clue. They have been working on improving macOS support, and it can use opengl now (https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/).
>>
>> They don't need IPC for passing textures so that is a different story.
>
>
> Yes but they have web-engine and video decoding concerns (beside qemu/dmabuf gtk display they should be aware of).  I'll try to reach Christian about it.
>
> thanks
>
>>
>> >
>> >>
>> >> Regards,
>> >> Akihiko Odaki
>> >>
>> >> >
>> >> >
>> >> > --
>> >> > Marc-André Lureau
>> >
>> >
>> >
>> > --
>> > Marc-André Lureau
>
>
>
> --
> Marc-André Lureau

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Akihiko Odaki]

On 2022/02/18 1:11, Akihiko Odaki wrote:
>>> You missed only one thing:
>>>> - that console_select and register_displaychangelistener may not call
>>>> dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
>>>> incompatible with non-OpenGL displays
>>>
>>> displaychangelistener_display_console always has to call
>>> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
>>
>>
>> Ok, would that be what you have in mind?
>>
>>   --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -1122,6 +1122,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>>       } else if (con->scanout.kind == SCANOUT_SURFACE) {
>>           dpy_gfx_create_texture(con, con->surface);
>>           displaychangelistener_gfx_switch(dcl, con->surface);
>> +    } else {
>> +        /* use the fallback surface, egl-headless keeps it updated */
>> +        assert(con->surface);
>> +        displaychangelistener_gfx_switch(dcl, con->surface);
>>       }
> 
> It should call displaychangelistener_gfx_switch even when e.g.
> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
> to the last DisplaySurface it received while con->scanout.kind ==
> SCANOUT_TEXTURE.

Hi,

Let me remind that the release date is approaching but the regression 
which breaks switching the console for vnc with egl-headless is still 
not fixed. (vnc has the feature to switch consoles with Ctrl+Alt+[1-9] 
if it is not bound to a particular console.)

Please fix this or, if not possible, revert the changes related to dbus.

My patch series is available for fixing the problem. The design it 
adopted is somewhat controversial and it cannot be applied to the 
current master branch. Please tell me if it is necessary to rebase this.
https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/

Regards,
Akihiko Odaki

Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Marc-André Lureau]

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

Hi

On Thu, Feb 17, 2022 at 9:36 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

>
> On Thu, Feb 17, 2022 at 9:25 PM Akihiko Odaki <akihiko.odaki@gmail.com>
> wrote:
>
>> >> > btw, I suppose you checked your DBus changes against the WIP
>> "qemu-display" project. What was your experience? I don't think many people
>> have tried it yet. Do you think this could be made to work on macOS? at
>> least the non-dmabuf support should work, as long as Gtk4 has good macOS
>> support. I don't know if dmabuf or similar exist there, any idea?
>> >>
>> >> I tested it on Fedora. I think it would probably work on macOS but
>> >> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
>> >> macOS but its situation is bad; it must be delivered via macOS's own
>> >> IPC mechanisms (Mach port and XPC), but they are for server-client
>> >> model and not for P2P. fileport mechanism allows to convert Mach port
>> >> to file descriptor, but it is not documented. (In reality, I think all
>> >> of the major browsers, Chromium, Firefox and Safari use fileport for
>> >> this purpose. Apple should really document it if they use it for their
>> >> app.) It is also possible to share IOSurface with a global number, but
>> >> it can be brute-forced and is insecure.
>> >>
>> >
>> > Thanks, the Gtk developers might have some clue. They have been working
>> on improving macOS support, and it can use opengl now (
>> https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/
>> ).
>>
>> They don't need IPC for passing textures so that is a different story.
>>
>
> Yes but they have web-engine and video decoding concerns (beside
> qemu/dmabuf gtk display they should be aware of).  I'll try to reach
> Christian about it.
>
>

fwiw, here is Christian Hergert comment about texture sharing & gtk on
macos:

"There is, and we are using it in GTK 4 as of 4.6 to render from OpenGL
to a surface we can attach to CALayers or OpenGL textures. It has
allowed us to do a bunch of tricks to ensure we have opaque surfaces
since NSWindow doesn't have anything like set_opaque_region() from Wayland.

It's called IOSurface and the browsers use this to pass rendererings
between processes."

-- 
Marc-André Lureau

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