All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] ui/dbus: Share one listener for a console
@ 2022-02-13  2:42 Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 1/6] " Akihiko Odaki
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

ui/dbus required to have multiple DisplayChangeListeners (possibly with OpenGL)
for a console but that caused several problems:
- It broke egl-headless, an unusual display which implements OpenGL rendering
  for non-OpenGL displays. The code to support multiple DisplayChangeListeners
  does not consider the case where non-OpenGL displays listens OpenGL consoles.
- Multiple OpenGL DisplayChangeListeners of dbus shares one DisplaySurface and
  modifies its texture field, causing OpenGL texture leak and use-after-free.
- Introduced extra code to check the compatibility of OpenGL context providers
  and OpenGL texture renderers where those are often inherently tightly coupled
  since they must share the same hardware.
- Needed extra work to broadcast the same change to multiple dbus listeners.

This series solve them by implementing the change broadcast in ui/dbus, which
knows exactly what is needed. Changes for the common code to support multiple
OpenGL DisplayChangeListeners were reverted to fix egl-headless and reduce
the code size.

Akihiko Odaki (6):
  ui/dbus: Share one listener for a console
  Revert "console: save current scanout details"
  Revert "ui: split the GL context in a different object"
  Revert "ui: dispatch GL events to all listeners"
  Revert "ui: associate GL context outside of display listener
    registration"
  Revert "ui: factor out qemu_console_set_display_gl_ctx()"

 include/ui/console.h       |  60 +-----
 include/ui/egl-context.h   |   6 +-
 include/ui/gtk.h           |  11 +-
 include/ui/sdl2.h          |   7 +-
 include/ui/spice-display.h |   1 -
 ui/console.c               | 258 +++++++----------------
 ui/dbus-console.c          | 109 ++--------
 ui/dbus-listener.c         | 417 +++++++++++++++++++++++++++----------
 ui/dbus.c                  |  22 --
 ui/dbus.h                  |  36 +++-
 ui/egl-context.c           |   6 +-
 ui/egl-headless.c          |  20 +-
 ui/gtk-egl.c               |  10 +-
 ui/gtk-gl-area.c           |   8 +-
 ui/gtk.c                   |  25 +--
 ui/sdl2-gl.c               |  10 +-
 ui/sdl2.c                  |  14 +-
 ui/spice-display.c         |  19 +-
 18 files changed, 498 insertions(+), 541 deletions(-)

-- 
2.32.0 (Apple Git-132)



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/6] ui/dbus: Share one listener for a console
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
@ 2022-02-13  2:42 ` Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 2/6] Revert "console: save current scanout details" Akihiko Odaki
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

This fixes surface texture double free with multiple connections and
out-of-sync display size with multiple displays.

This also reduces resource usage a little and allows to remove code to
support multiple listeners for OpenGL displays.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/dbus-console.c  | 109 +++---------
 ui/dbus-listener.c | 401 +++++++++++++++++++++++++++++++++------------
 ui/dbus.h          |  32 +++-
 3 files changed, 344 insertions(+), 198 deletions(-)

diff --git a/ui/dbus-console.c b/ui/dbus-console.c
index e062f721d76..ec035c427db 100644
--- a/ui/dbus-console.c
+++ b/ui/dbus-console.c
@@ -33,11 +33,10 @@
 
 struct _DBusDisplayConsole {
     GDBusObjectSkeleton parent_instance;
-    DisplayChangeListener dcl;
 
     DBusDisplay *display;
     QemuConsole *con;
-    GHashTable *listeners;
+    DBusDisplayListener *listener;
     QemuDBusDisplay1Console *iface;
 
     QemuDBusDisplay1Keyboard *iface_kbd;
@@ -54,7 +53,7 @@ G_DEFINE_TYPE(DBusDisplayConsole,
               dbus_display_console,
               G_TYPE_DBUS_OBJECT_SKELETON)
 
-static void
+void
 dbus_display_console_set_size(DBusDisplayConsole *ddc,
                               uint32_t width, uint32_t height)
 {
@@ -64,78 +63,9 @@ dbus_display_console_set_size(DBusDisplayConsole *ddc,
                  NULL);
 }
 
-static void
-dbus_gfx_switch(DisplayChangeListener *dcl,
-                struct DisplaySurface *new_surface)
-{
-    DBusDisplayConsole *ddc = container_of(dcl, DBusDisplayConsole, dcl);
-
-    dbus_display_console_set_size(ddc,
-                                  surface_width(new_surface),
-                                  surface_height(new_surface));
-}
-
-static void
-dbus_gfx_update(DisplayChangeListener *dcl,
-                int x, int y, int w, int h)
-{
-}
-
-static void
-dbus_gl_scanout_disable(DisplayChangeListener *dcl)
-{
-}
-
-static void
-dbus_gl_scanout_texture(DisplayChangeListener *dcl,
-                        uint32_t tex_id,
-                        bool backing_y_0_top,
-                        uint32_t backing_width,
-                        uint32_t backing_height,
-                        uint32_t x, uint32_t y,
-                        uint32_t w, uint32_t h)
-{
-    DBusDisplayConsole *ddc = container_of(dcl, DBusDisplayConsole, dcl);
-
-    dbus_display_console_set_size(ddc, w, h);
-}
-
-static void
-dbus_gl_scanout_dmabuf(DisplayChangeListener *dcl,
-                       QemuDmaBuf *dmabuf)
-{
-    DBusDisplayConsole *ddc = container_of(dcl, DBusDisplayConsole, dcl);
-
-    dbus_display_console_set_size(ddc,
-                                  dmabuf->width,
-                                  dmabuf->height);
-}
-
-static void
-dbus_gl_scanout_update(DisplayChangeListener *dcl,
-                       uint32_t x, uint32_t y,
-                       uint32_t w, uint32_t h)
-{
-}
-
-static const DisplayChangeListenerOps dbus_console_dcl_ops = {
-    .dpy_name                = "dbus-console",
-    .dpy_gfx_switch          = dbus_gfx_switch,
-    .dpy_gfx_update          = dbus_gfx_update,
-    .dpy_gl_scanout_disable  = dbus_gl_scanout_disable,
-    .dpy_gl_scanout_texture  = dbus_gl_scanout_texture,
-    .dpy_gl_scanout_dmabuf   = dbus_gl_scanout_dmabuf,
-    .dpy_gl_update           = dbus_gl_scanout_update,
-};
-
 static void
 dbus_display_console_init(DBusDisplayConsole *object)
 {
-    DBusDisplayConsole *ddc = DBUS_DISPLAY_CONSOLE(object);
-
-    ddc->listeners = g_hash_table_new_full(g_str_hash, g_str_equal,
-                                            NULL, g_object_unref);
-    ddc->dcl.ops = &dbus_console_dcl_ops;
 }
 
 static void
@@ -143,10 +73,10 @@ dbus_display_console_dispose(GObject *object)
 {
     DBusDisplayConsole *ddc = DBUS_DISPLAY_CONSOLE(object);
 
-    unregister_displaychangelistener(&ddc->dcl);
     g_clear_object(&ddc->iface_kbd);
     g_clear_object(&ddc->iface);
-    g_clear_pointer(&ddc->listeners, g_hash_table_unref);
+    dbus_display_listener_unref_all_connections(ddc->listener);
+    g_clear_object(&ddc->listener);
     g_clear_pointer(&ddc->kbd, qkbd_state_free);
 
     G_OBJECT_CLASS(dbus_display_console_parent_class)->dispose(object);
@@ -161,14 +91,14 @@ dbus_display_console_class_init(DBusDisplayConsoleClass *klass)
 }
 
 static void
-listener_vanished_cb(DBusDisplayListener *listener)
+listener_vanished_cb(DBusDisplayListenerConnection *ddlc)
 {
-    DBusDisplayConsole *ddc = dbus_display_listener_get_console(listener);
-    const char *name = dbus_display_listener_get_bus_name(listener);
+    DBusDisplayConsole *ddc = dbus_display_listener_connection_get_console(ddlc);
+    const char *name = dbus_display_listener_connection_get_bus_name(ddlc);
 
     trace_dbus_listener_vanished(name);
 
-    g_hash_table_remove(ddc->listeners, name);
+    g_object_unref(ddlc);
     qkbd_state_lift_all_keys(ddc->kbd);
 }
 
@@ -211,15 +141,15 @@ dbus_console_register_listener(DBusDisplayConsole *ddc,
                                GVariant *arg_listener)
 {
     const char *sender = g_dbus_method_invocation_get_sender(invocation);
-    GDBusConnection *listener_conn;
+    GDBusConnection *conn;
     g_autoptr(GError) err = NULL;
     g_autoptr(GSocket) socket = NULL;
     g_autoptr(GSocketConnection) socket_conn = NULL;
     g_autofree char *guid = g_dbus_generate_guid();
-    DBusDisplayListener *listener;
+    DBusDisplayListenerConnection *listener_conn;
     int fd;
 
-    if (sender && g_hash_table_contains(ddc->listeners, sender)) {
+    if (sender && dbus_display_listener_has_connection(ddc->listener, sender)) {
         g_dbus_method_invocation_return_error(
             invocation,
             DBUS_DISPLAY_ERROR,
@@ -254,7 +184,7 @@ dbus_console_register_listener(DBusDisplayConsole *ddc,
     qemu_dbus_display1_console_complete_register_listener(
         ddc->iface, invocation, NULL);
 
-    listener_conn = g_dbus_connection_new_sync(
+    conn = g_dbus_connection_new_sync(
         G_IO_STREAM(socket_conn),
         guid,
         G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_SERVER,
@@ -264,16 +194,15 @@ dbus_console_register_listener(DBusDisplayConsole *ddc,
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
 
-    listener = dbus_display_listener_new(sender, listener_conn, ddc);
-    if (!listener) {
+    listener_conn = dbus_display_listener_add_connection(ddc->listener,
+                                                         sender, conn);
+    if (!listener_conn) {
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
 
-    g_hash_table_insert(ddc->listeners,
-                        (gpointer)dbus_display_listener_get_bus_name(listener),
-                        listener);
-    g_object_connect(listener_conn,
-                     "swapped-signal::closed", listener_vanished_cb, listener,
+    g_object_connect(conn,
+                     "swapped-signal::closed", listener_vanished_cb,
+                     listener_conn,
                      NULL);
 
     trace_dbus_registered_listener(sender);
@@ -489,7 +418,7 @@ dbus_display_console_new(DBusDisplay *display, QemuConsole *con)
     g_dbus_object_skeleton_add_interface(G_DBUS_OBJECT_SKELETON(ddc),
         G_DBUS_INTERFACE_SKELETON(ddc->iface_mouse));
 
-    register_displaychangelistener(&ddc->dcl);
+    ddc->listener = dbus_display_listener_new(ddc);
     ddc->mouse_mode_notifier.notify = dbus_mouse_mode_change;
     qemu_add_mouse_mode_change_notifier(&ddc->mouse_mode_notifier);
 
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 81c119b13a2..e4242d69de2 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -31,18 +31,36 @@
 #include "ui/egl-context.h"
 #include "trace.h"
 
-struct _DBusDisplayListener {
+struct _DBusDisplayListenerConnection {
     GObject parent;
 
     char *bus_name;
-    DBusDisplayConsole *console;
+    DBusDisplayListener *listener;
     GDBusConnection *conn;
 
     QemuDBusDisplay1Listener *proxy;
+};
+
+G_DEFINE_TYPE(DBusDisplayListenerConnection,
+              dbus_display_listener_connection,
+              G_TYPE_OBJECT)
+
+struct _DBusDisplayListener {
+    GObject parent;
+
+    GHashTable *conns;
+    DBusDisplayConsole *console;
 
     DisplayChangeListener dcl;
     DisplaySurface *ds;
     QemuGLShader *gls;
+    GUnixFDList *dmabuf_fd_list;
+    uint32_t dmabuf_width;
+    uint32_t dmabuf_height;
+    uint32_t dmabuf_stride;
+    uint32_t dmabuf_fourcc;
+    uint64_t dmabuf_modifier;
+    bool dmabuf_y0_top;
     int gl_updates;
 };
 
@@ -53,65 +71,98 @@ static void dbus_update_gl_cb(GObject *source_object,
                            gpointer user_data)
 {
     g_autoptr(GError) err = NULL;
-    DBusDisplayListener *ddl = user_data;
+    DBusDisplayListenerConnection *ddlc = user_data;
 
-    if (!qemu_dbus_display1_listener_call_update_dmabuf_finish(ddl->proxy,
+    if (!qemu_dbus_display1_listener_call_update_dmabuf_finish(ddlc->proxy,
                                                                res, &err)) {
         error_report("Failed to call update: %s", err->message);
     }
 
-    graphic_hw_gl_block(ddl->dcl.con, false);
-    g_object_unref(ddl);
+    graphic_hw_gl_block(ddlc->listener->dcl.con, false);
+    g_object_unref(ddlc);
 }
 
 static void dbus_call_update_gl(DBusDisplayListener *ddl,
                                 int x, int y, int w, int h)
 {
-    graphic_hw_gl_block(ddl->dcl.con, true);
-    glFlush();
-    qemu_dbus_display1_listener_call_update_dmabuf(ddl->proxy,
-        x, y, w, h,
-        G_DBUS_CALL_FLAGS_NONE,
-        DBUS_DEFAULT_TIMEOUT, NULL,
-        dbus_update_gl_cb,
-        g_object_ref(ddl));
+    GHashTableIter iter;
+    gpointer ddlc;
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        graphic_hw_gl_block(ddl->dcl.con, true);
+        glFlush();
+        qemu_dbus_display1_listener_call_update_dmabuf(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            x, y, w, h,
+            G_DBUS_CALL_FLAGS_NONE,
+            DBUS_DEFAULT_TIMEOUT, NULL,
+            dbus_update_gl_cb,
+            g_object_ref(ddlc));
+    }
 }
 
 static void dbus_scanout_disable(DisplayChangeListener *dcl)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
+    GHashTableIter iter;
+    gpointer ddlc;
 
     ddl->ds = NULL;
-    qemu_dbus_display1_listener_call_disable(
-        ddl->proxy, G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_disable(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+    }
 }
 
 static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
                                 QemuDmaBuf *dmabuf)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
+    GHashTableIter iter;
+    gpointer ddlc;
     g_autoptr(GError) err = NULL;
     g_autoptr(GUnixFDList) fd_list = NULL;
 
-    fd_list = g_unix_fd_list_new();
-    if (g_unix_fd_list_append(fd_list, dmabuf->fd, &err) != 0) {
+    if (ddl->dmabuf_fd_list) {
+        g_clear_object(&ddl->dmabuf_fd_list);
+    }
+
+    ddl->dmabuf_fd_list = g_unix_fd_list_new();
+    if (g_unix_fd_list_append(ddl->dmabuf_fd_list, dmabuf->fd, &err) != 0) {
+        g_clear_object(&ddl->dmabuf_fd_list);
         error_report("Failed to setup dmabuf fdlist: %s", err->message);
         return;
     }
 
-    qemu_dbus_display1_listener_call_scanout_dmabuf(
-        ddl->proxy,
-        g_variant_new_handle(0),
-        dmabuf->width,
-        dmabuf->height,
-        dmabuf->stride,
-        dmabuf->fourcc,
-        dmabuf->modifier,
-        dmabuf->y0_top,
-        G_DBUS_CALL_FLAGS_NONE,
-        -1,
-        fd_list,
-        NULL, NULL, NULL);
+    ddl->dmabuf_width = dmabuf->width;
+    ddl->dmabuf_height = dmabuf->height;
+    ddl->dmabuf_stride = dmabuf->stride;
+    ddl->dmabuf_fourcc = dmabuf->fourcc;
+    ddl->dmabuf_modifier = dmabuf->modifier;
+    ddl->dmabuf_y0_top = dmabuf->y0_top;
+
+    dbus_display_console_set_size(ddl->console, dmabuf->width, dmabuf->height);
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_scanout_dmabuf(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            g_variant_new_handle(0),
+            ddl->dmabuf_width,
+            ddl->dmabuf_height,
+            ddl->dmabuf_stride,
+            ddl->dmabuf_fourcc,
+            ddl->dmabuf_modifier,
+            ddl->dmabuf_y0_top,
+            G_DBUS_CALL_FLAGS_NONE,
+            -1,
+            ddl->dmabuf_fd_list,
+            NULL, NULL, NULL);
+    }
 }
 
 static void dbus_scanout_texture(DisplayChangeListener *dcl,
@@ -150,11 +201,16 @@ static void dbus_cursor_dmabuf(DisplayChangeListener *dcl,
     DisplaySurface *ds;
     GVariant *v_data = NULL;
     egl_fb cursor_fb;
+    GHashTableIter iter;
+    gpointer ddlc;
 
     if (!dmabuf) {
-        qemu_dbus_display1_listener_call_mouse_set(
-            ddl->proxy, 0, 0, false,
-            G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+        g_hash_table_iter_init(&iter, ddl->conns);
+        while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+            qemu_dbus_display1_listener_call_mouse_set(
+                ((DBusDisplayListenerConnection *)ddlc)->proxy, 0, 0, false,
+                G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+        }
         return;
     }
 
@@ -174,28 +230,37 @@ static void dbus_cursor_dmabuf(DisplayChangeListener *dcl,
         TRUE,
         (GDestroyNotify)qemu_free_displaysurface,
         ds);
-    qemu_dbus_display1_listener_call_cursor_define(
-        ddl->proxy,
-        surface_width(ds),
-        surface_height(ds),
-        hot_x,
-        hot_y,
-        v_data,
-        G_DBUS_CALL_FLAGS_NONE,
-        -1,
-        NULL,
-        NULL,
-        NULL);
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_cursor_define(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            surface_width(ds),
+            surface_height(ds),
+            hot_x,
+            hot_y,
+            v_data,
+            G_DBUS_CALL_FLAGS_NONE,
+            -1,
+            NULL,
+            NULL,
+            NULL);
+    }
 }
 
 static void dbus_cursor_position(DisplayChangeListener *dcl,
                                  uint32_t pos_x, uint32_t pos_y)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
+    GHashTableIter iter;
+    gpointer ddlc;
 
-    qemu_dbus_display1_listener_call_mouse_set(
-        ddl->proxy, pos_x, pos_y, true,
-        G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_mouse_set(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            pos_x, pos_y, true, G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+    }
 }
 
 static void dbus_release_dmabuf(DisplayChangeListener *dcl,
@@ -254,6 +319,8 @@ static void dbus_gfx_update(DisplayChangeListener *dcl,
     pixman_image_t *img;
     GVariant *v_data;
     size_t stride;
+    GHashTableIter iter;
+    gpointer ddlc;
 
     assert(ddl->ds);
     stride = w * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(surface_format(ddl->ds)), 8);
@@ -273,11 +340,16 @@ static void dbus_gfx_update(DisplayChangeListener *dcl,
         TRUE,
         (GDestroyNotify)pixman_image_unref,
         img);
-    qemu_dbus_display1_listener_call_update(ddl->proxy,
-        x, y, w, h, pixman_image_get_stride(img), pixman_image_get_format(img),
-        v_data,
-        G_DBUS_CALL_FLAGS_NONE,
-        DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_update(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            x, y, w, h, pixman_image_get_stride(img), pixman_image_get_format(img),
+            v_data,
+            G_DBUS_CALL_FLAGS_NONE,
+            DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+    }
 }
 
 static void dbus_gl_gfx_switch(DisplayChangeListener *dcl,
@@ -293,6 +365,8 @@ static void dbus_gl_gfx_switch(DisplayChangeListener *dcl,
         int width = surface_width(ddl->ds);
         int height = surface_height(ddl->ds);
 
+        dbus_display_console_set_size(ddl->console, width, height);
+
         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,
@@ -305,6 +379,8 @@ static void dbus_gfx_switch(DisplayChangeListener *dcl,
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
     GVariant *v_data = NULL;
+    GHashTableIter iter;
+    gpointer ddlc;
 
     ddl->ds = new_surface;
     if (!ddl->ds) {
@@ -312,6 +388,10 @@ static void dbus_gfx_switch(DisplayChangeListener *dcl,
         return;
     }
 
+    dbus_display_console_set_size(ddl->console,
+                                  surface_width(ddl->ds),
+                                  surface_height(ddl->ds));
+
     v_data = g_variant_new_from_data(
         G_VARIANT_TYPE("ay"),
         surface_data(ddl->ds),
@@ -319,23 +399,34 @@ static void dbus_gfx_switch(DisplayChangeListener *dcl,
         TRUE,
         (GDestroyNotify)pixman_image_unref,
         pixman_image_ref(ddl->ds->image));
-    qemu_dbus_display1_listener_call_scanout(ddl->proxy,
-        surface_width(ddl->ds),
-        surface_height(ddl->ds),
-        surface_stride(ddl->ds),
-        surface_format(ddl->ds),
-        v_data,
-        G_DBUS_CALL_FLAGS_NONE,
-        DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_scanout(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            surface_width(ddl->ds),
+            surface_height(ddl->ds),
+            surface_stride(ddl->ds),
+            surface_format(ddl->ds),
+            v_data,
+            G_DBUS_CALL_FLAGS_NONE,
+            DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+    }
 }
 
 static void dbus_mouse_set(DisplayChangeListener *dcl,
                            int x, int y, int on)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
+    GHashTableIter iter;
+    gpointer ddlc;
 
-    qemu_dbus_display1_listener_call_mouse_set(
-        ddl->proxy, x, y, on, G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_mouse_set(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            x, y, on, G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+    }
 }
 
 static void dbus_cursor_define(DisplayChangeListener *dcl,
@@ -343,6 +434,8 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
     GVariant *v_data = NULL;
+    GHashTableIter iter;
+    gpointer ddlc;
 
     cursor_get(c);
     v_data = g_variant_new_from_data(
@@ -353,18 +446,21 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
         (GDestroyNotify)cursor_put,
         c);
 
-    qemu_dbus_display1_listener_call_cursor_define(
-        ddl->proxy,
-        c->width,
-        c->height,
-        c->hot_x,
-        c->hot_y,
-        v_data,
-        G_DBUS_CALL_FLAGS_NONE,
-        -1,
-        NULL,
-        NULL,
-        NULL);
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        qemu_dbus_display1_listener_call_cursor_define(
+            ((DBusDisplayListenerConnection *)ddlc)->proxy,
+            c->width,
+            c->height,
+            c->hot_x,
+            c->hot_y,
+            v_data,
+            G_DBUS_CALL_FLAGS_NONE,
+            -1,
+            NULL,
+            NULL,
+            NULL);
+    }
 }
 
 const DisplayChangeListenerOps dbus_gl_dcl_ops = {
@@ -400,9 +496,8 @@ dbus_display_listener_dispose(GObject *object)
     DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(object);
 
     unregister_displaychangelistener(&ddl->dcl);
-    g_clear_object(&ddl->conn);
-    g_clear_pointer(&ddl->bus_name, g_free);
-    g_clear_object(&ddl->proxy);
+    g_clear_object(&ddl->dmabuf_fd_list);
+    g_clear_pointer(&ddl->conns, g_hash_table_unref);
     g_clear_pointer(&ddl->gls, qemu_gl_fini_shader);
 
     G_OBJECT_CLASS(dbus_display_listener_parent_class)->dispose(object);
@@ -435,46 +530,146 @@ dbus_display_listener_class_init(DBusDisplayListenerClass *klass)
 static void
 dbus_display_listener_init(DBusDisplayListener *ddl)
 {
+    ddl->conns = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, NULL);
 }
 
-const char *
-dbus_display_listener_get_bus_name(DBusDisplayListener *ddl)
-{
-    return ddl->bus_name ?: "p2p";
-}
-
-DBusDisplayConsole *
-dbus_display_listener_get_console(DBusDisplayListener *ddl)
+DBusDisplayListenerConnection *
+dbus_display_listener_add_connection(DBusDisplayListener *ddl,
+                                     const char *bus_name,
+                                     GDBusConnection *conn)
 {
-    return ddl->console;
-}
-
-DBusDisplayListener *
-dbus_display_listener_new(const char *bus_name,
-                          GDBusConnection *conn,
-                          DBusDisplayConsole *console)
-{
-    DBusDisplayListener *ddl;
-    QemuConsole *con;
+    DBusDisplayListenerConnection *ddlc;
+    QemuDBusDisplay1Listener *proxy;
     g_autoptr(GError) err = NULL;
 
-    ddl = g_object_new(DBUS_DISPLAY_TYPE_LISTENER, NULL);
-    ddl->proxy =
+    proxy =
         qemu_dbus_display1_listener_proxy_new_sync(conn,
             G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START,
             NULL,
             "/org/qemu/Display1/Listener",
             NULL,
             &err);
-    if (!ddl->proxy) {
+    if (!proxy) {
         error_report("Failed to setup proxy: %s", err->message);
         g_object_unref(conn);
-        g_object_unref(ddl);
         return NULL;
     }
 
-    ddl->bus_name = g_strdup(bus_name);
-    ddl->conn = conn;
+    ddlc = g_object_new(DBUS_DISPLAY_TYPE_LISTENER_CONNECTION, NULL);
+    ddlc->listener = g_object_ref(ddl);
+    ddlc->proxy = proxy;
+    ddlc->bus_name = g_strdup(bus_name);
+    ddlc->conn = conn;
+
+    g_hash_table_insert(ddl->conns, ddlc->bus_name, ddlc);
+
+    if (display_opengl) {
+        if (ddl->dmabuf_fd_list) {
+            qemu_dbus_display1_listener_call_scanout_dmabuf(
+                ddlc->proxy,
+                g_variant_new_handle(0),
+                ddl->dmabuf_width,
+                ddl->dmabuf_height,
+                ddl->dmabuf_stride,
+                ddl->dmabuf_fourcc,
+                ddl->dmabuf_modifier,
+                ddl->dmabuf_y0_top,
+                G_DBUS_CALL_FLAGS_NONE,
+                -1,
+                ddl->dmabuf_fd_list,
+                NULL, NULL, NULL);
+        }
+    } else {
+        if (ddl->ds) {
+            GVariant *v_data = NULL;
+
+            v_data = g_variant_new_from_data(
+                G_VARIANT_TYPE("ay"),
+                surface_data(ddl->ds),
+                surface_stride(ddl->ds) * surface_height(ddl->ds),
+                TRUE,
+                (GDestroyNotify)pixman_image_unref,
+                pixman_image_ref(ddl->ds->image));
+
+            qemu_dbus_display1_listener_call_scanout(
+                ((DBusDisplayListenerConnection *)ddlc)->proxy,
+                surface_width(ddl->ds),
+                surface_height(ddl->ds),
+                surface_stride(ddl->ds),
+                surface_format(ddl->ds),
+                v_data,
+                G_DBUS_CALL_FLAGS_NONE,
+                DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+        }
+    }
+
+    return ddlc;
+}
+
+bool
+dbus_display_listener_has_connection(DBusDisplayListener *ddl,
+                                     const char *bus_name)
+{
+    return g_hash_table_contains(ddl->conns, bus_name);
+}
+
+void
+dbus_display_listener_unref_all_connections(DBusDisplayListener *ddl)
+{
+    GHashTableIter iter;
+    gpointer ddlc;
+
+    g_hash_table_iter_init(&iter, ddl->conns);
+    while (g_hash_table_iter_next(&iter, NULL, &ddlc)) {
+        g_object_unref(ddlc);
+    }
+}
+
+static void
+dbus_display_listener_connection_dispose(GObject *object)
+{
+    DBusDisplayListenerConnection *ddlc =
+        DBUS_DISPLAY_LISTENER_CONNECTION(object);
+
+    g_hash_table_remove(ddlc->listener->conns, ddlc->bus_name);
+    g_clear_object(&ddlc->listener);
+    g_clear_object(&ddlc->conn);
+    g_clear_pointer(&ddlc->bus_name, g_free);
+    g_clear_object(&ddlc->proxy);
+
+    G_OBJECT_CLASS(dbus_display_listener_connection_parent_class)->dispose(object);
+}
+
+static void
+dbus_display_listener_connection_class_init(DBusDisplayListenerConnectionClass *klass)
+{
+    G_OBJECT_CLASS(klass)->dispose = dbus_display_listener_connection_dispose;
+}
+
+static void
+dbus_display_listener_connection_init(DBusDisplayListenerConnection *ddl)
+{
+}
+
+const char *
+dbus_display_listener_connection_get_bus_name(DBusDisplayListenerConnection *ddlc)
+{
+    return ddlc->bus_name ?: "p2p";
+}
+
+DBusDisplayConsole *
+dbus_display_listener_connection_get_console(DBusDisplayListenerConnection *ddlc)
+{
+    return ddlc->listener->console;
+}
+
+DBusDisplayListener *
+dbus_display_listener_new(DBusDisplayConsole *console)
+{
+    DBusDisplayListener *ddl;
+    QemuConsole *con;
+
+    ddl = g_object_new(DBUS_DISPLAY_TYPE_LISTENER, NULL);
     ddl->console = console;
 
     con = qemu_console_lookup_by_index(dbus_display_console_get_index(console));
diff --git a/ui/dbus.h b/ui/dbus.h
index 64c77cab444..fd24b299bbf 100644
--- a/ui/dbus.h
+++ b/ui/dbus.h
@@ -76,9 +76,19 @@ G_DECLARE_FINAL_TYPE(DBusDisplayConsole,
 DBusDisplayConsole *
 dbus_display_console_new(DBusDisplay *display, QemuConsole *con);
 
+void
+dbus_display_console_set_size(DBusDisplayConsole *ddc,
+                              uint32_t width, uint32_t height);
+
 int
 dbus_display_console_get_index(DBusDisplayConsole *ddc);
 
+G_DECLARE_FINAL_TYPE(DBusDisplayListenerConnection,
+                     dbus_display_listener_connection,
+                     DBUS_DISPLAY,
+                     LISTENER_CONNECTION,
+                     GObject)
+
 #define DBUS_DISPLAY_TYPE_LISTENER dbus_display_listener_get_type()
 G_DECLARE_FINAL_TYPE(DBusDisplayListener,
                      dbus_display_listener,
@@ -86,16 +96,28 @@ G_DECLARE_FINAL_TYPE(DBusDisplayListener,
                      LISTENER,
                      GObject)
 
+#define DBUS_DISPLAY_TYPE_LISTENER_CONNECTION \
+    dbus_display_listener_connection_get_type()
 DBusDisplayListener *
-dbus_display_listener_new(const char *bus_name,
-                          GDBusConnection *conn,
-                          DBusDisplayConsole *console);
+dbus_display_listener_new(DBusDisplayConsole *console);
+
+DBusDisplayListenerConnection *
+dbus_display_listener_add_connection(DBusDisplayListener *ddl,
+                                     const char *bus_name,
+                                     GDBusConnection *conn);
+
+bool
+dbus_display_listener_has_connection(DBusDisplayListener *ddl,
+                                     const char *bus_name);
+
+void
+dbus_display_listener_unref_all_connections(DBusDisplayListener *ddl);
 
 DBusDisplayConsole *
-dbus_display_listener_get_console(DBusDisplayListener *ddl);
+dbus_display_listener_connection_get_console(DBusDisplayListenerConnection *ddlc);
 
 const char *
-dbus_display_listener_get_bus_name(DBusDisplayListener *ddl);
+dbus_display_listener_connection_get_bus_name(DBusDisplayListenerConnection *ddlc);
 
 extern const DisplayChangeListenerOps dbus_gl_dcl_ops;
 extern const DisplayChangeListenerOps dbus_dcl_ops;
-- 
2.32.0 (Apple Git-132)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/6] Revert "console: save current scanout details"
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 1/6] " Akihiko Odaki
@ 2022-02-13  2:42 ` Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 3/6] Revert "ui: split the GL context in a different object" Akihiko Odaki
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

This reverts commit ebced091854517f063c46ce8e522ebc45e9bccdf.

This fixes the compatibility between egl-headless and displays
using console_select, most importantly vnc.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 include/ui/console.h |  27 -------
 ui/console.c         | 165 ++++++++++++++-----------------------------
 2 files changed, 54 insertions(+), 138 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index f590819880b..eefd7e4dc1f 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -108,17 +108,6 @@ struct QemuConsoleClass {
 #define QEMU_ALLOCATED_FLAG     0x01
 #define QEMU_PLACEHOLDER_FLAG   0x02
 
-typedef struct ScanoutTexture {
-    uint32_t backing_id;
-    bool backing_y_0_top;
-    uint32_t backing_width;
-    uint32_t backing_height;
-    uint32_t x;
-    uint32_t y;
-    uint32_t width;
-    uint32_t height;
-} ScanoutTexture;
-
 typedef struct DisplaySurface {
     pixman_format_code_t format;
     pixman_image_t *image;
@@ -189,22 +178,6 @@ typedef struct QemuDmaBuf {
     bool      draw_submitted;
 } QemuDmaBuf;
 
-enum display_scanout {
-    SCANOUT_NONE,
-    SCANOUT_SURFACE,
-    SCANOUT_TEXTURE,
-    SCANOUT_DMABUF,
-};
-
-typedef struct DisplayScanout {
-    enum display_scanout kind;
-    union {
-        /* DisplaySurface *surface; is kept in QemuConsole */
-        ScanoutTexture texture;
-        QemuDmaBuf *dmabuf;
-    };
-} DisplayScanout;
-
 typedef struct DisplayState DisplayState;
 typedef struct DisplayGLCtx DisplayGLCtx;
 
diff --git a/ui/console.c b/ui/console.c
index 40eebb6d2cc..78583df9203 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -77,7 +77,6 @@ struct QemuConsole {
     console_type_t console_type;
     DisplayState *ds;
     DisplaySurface *surface;
-    DisplayScanout scanout;
     int dcls;
     DisplayGLCtx *gl;
     int gl_block;
@@ -147,7 +146,6 @@ static void dpy_refresh(DisplayState *s);
 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 void gui_update(void *opaque)
 {
@@ -483,8 +481,6 @@ static void text_console_resize(QemuConsole *s)
     TextCell *cells, *c, *c1;
     int w1, x, y, last_width;
 
-    assert(s->scanout.kind == SCANOUT_SURFACE);
-
     last_width = s->width;
     s->width = surface_width(s->surface) / FONT_WIDTH;
     s->height = surface_height(s->surface) / FONT_HEIGHT;
@@ -1056,48 +1052,6 @@ static void console_putchar(QemuConsole *s, int ch)
     }
 }
 
-static void displaychangelistener_display_console(DisplayChangeListener *dcl,
-                                                  QemuConsole *con)
-{
-    static const char nodev[] =
-        "This VM has no graphic display device.";
-    static DisplaySurface *dummy;
-
-    if (!con) {
-        if (!dcl->ops->dpy_gfx_switch) {
-            return;
-        }
-        if (!dummy) {
-            dummy = qemu_create_placeholder_surface(640, 480, nodev);
-        }
-        dcl->ops->dpy_gfx_switch(dcl, dummy);
-        return;
-    }
-
-    if (con->scanout.kind == SCANOUT_DMABUF &&
-        displaychangelistener_has_dmabuf(dcl)) {
-        dcl->ops->dpy_gl_scanout_dmabuf(dcl, con->scanout.dmabuf);
-    } else if (con->scanout.kind == SCANOUT_TEXTURE &&
-               dcl->ops->dpy_gl_scanout_texture) {
-        dcl->ops->dpy_gl_scanout_texture(dcl,
-                                         con->scanout.texture.backing_id,
-                                         con->scanout.texture.backing_y_0_top,
-                                         con->scanout.texture.backing_width,
-                                         con->scanout.texture.backing_height,
-                                         con->scanout.texture.x,
-                                         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);
-    }
-
-    dcl->ops->dpy_gfx_update(dcl, 0, 0,
-                             qemu_console_get_width(con, 0),
-                             qemu_console_get_height(con, 0));
-}
-
 void console_select(unsigned int index)
 {
     DisplayChangeListener *dcl;
@@ -1114,7 +1068,13 @@ void console_select(unsigned int index)
                 if (dcl->con != NULL) {
                     continue;
                 }
-                displaychangelistener_display_console(dcl, s);
+                if (dcl->ops->dpy_gfx_switch) {
+                    dcl->ops->dpy_gfx_switch(dcl, s->surface);
+                }
+            }
+            if (s->surface) {
+                dpy_gfx_update(s, 0, 0, surface_width(s->surface),
+                               surface_height(s->surface));
             }
         }
         if (ds->have_text) {
@@ -1520,6 +1480,9 @@ static bool dpy_gl_compatible_with(QemuConsole *con, DisplayChangeListener *dcl)
 
 void register_displaychangelistener(DisplayChangeListener *dcl)
 {
+    static const char nodev[] =
+        "This VM has no graphic display device.";
+    static DisplaySurface *dummy;
     QemuConsole *con;
 
     assert(!dcl->ds);
@@ -1544,7 +1507,16 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
     } else {
         con = active_console;
     }
-    displaychangelistener_display_console(dcl, con);
+    if (dcl->ops->dpy_gfx_switch) {
+        if (con) {
+            dcl->ops->dpy_gfx_switch(dcl, con->surface);
+        } else {
+            if (!dummy) {
+                dummy = qemu_create_placeholder_surface(640, 480, nodev);
+            }
+            dcl->ops->dpy_gfx_switch(dcl, dummy);
+        }
+    }
     text_console_update_cursor(NULL);
 }
 
@@ -1625,9 +1597,13 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
 {
     DisplayState *s = con->ds;
     DisplayChangeListener *dcl;
-    int width = qemu_console_get_width(con, x + w);
-    int height = qemu_console_get_height(con, y + h);
+    int width = w;
+    int height = h;
 
+    if (con->surface) {
+        width = surface_width(con->surface);
+        height = surface_height(con->surface);
+    }
     x = MAX(x, 0);
     y = MAX(y, 0);
     x = MIN(x, width);
@@ -1650,10 +1626,12 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
 
 void dpy_gfx_update_full(QemuConsole *con)
 {
-    int w = qemu_console_get_width(con, 0);
-    int h = qemu_console_get_height(con, 0);
-
-    dpy_gfx_update(con, 0, 0, w, h);
+    if (!con->surface) {
+        return;
+    }
+    dpy_gfx_update(con, 0, 0,
+                   surface_width(con->surface),
+                   surface_height(con->surface));
 }
 
 void dpy_gfx_replace_surface(QemuConsole *con,
@@ -1680,7 +1658,6 @@ void dpy_gfx_replace_surface(QemuConsole *con,
 
     assert(old_surface != surface);
 
-    con->scanout.kind = SCANOUT_SURFACE;
     con->surface = surface;
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (con != (dcl->con ? dcl->con : active_console)) {
@@ -1856,9 +1833,6 @@ void dpy_gl_scanout_disable(QemuConsole *con)
     DisplayState *s = con->ds;
     DisplayChangeListener *dcl;
 
-    if (con->scanout.kind != SCANOUT_SURFACE) {
-        con->scanout.kind = SCANOUT_NONE;
-    }
     QLIST_FOREACH(dcl, &s->listeners, next) {
         dcl->ops->dpy_gl_scanout_disable(dcl);
     }
@@ -1875,11 +1849,6 @@ void dpy_gl_scanout_texture(QemuConsole *con,
     DisplayState *s = con->ds;
     DisplayChangeListener *dcl;
 
-    con->scanout.kind = SCANOUT_TEXTURE;
-    con->scanout.texture = (ScanoutTexture) {
-        backing_id, backing_y_0_top, backing_width, backing_height,
-        x, y, width, height
-    };
     QLIST_FOREACH(dcl, &s->listeners, next) {
         dcl->ops->dpy_gl_scanout_texture(dcl, backing_id,
                                          backing_y_0_top,
@@ -1894,8 +1863,6 @@ void dpy_gl_scanout_dmabuf(QemuConsole *con,
     DisplayState *s = con->ds;
     DisplayChangeListener *dcl;
 
-    con->scanout.kind = SCANOUT_DMABUF;
-    con->scanout.dmabuf = dmabuf;
     QLIST_FOREACH(dcl, &s->listeners, next) {
         dcl->ops->dpy_gl_scanout_dmabuf(dcl, dmabuf);
     }
@@ -2022,8 +1989,10 @@ QemuConsole *graphic_console_init(DeviceState *dev, uint32_t head,
     s = qemu_console_lookup_unused();
     if (s) {
         trace_console_gfx_reuse(s->index);
-        width = qemu_console_get_width(s, 0);
-        height = qemu_console_get_height(s, 0);
+        if (s->surface) {
+            width = surface_width(s->surface);
+            height = surface_height(s->surface);
+        }
     } else {
         trace_console_gfx_new();
         s = new_console(ds, GRAPHIC_CONSOLE, head);
@@ -2052,8 +2021,13 @@ void graphic_console_close(QemuConsole *con)
     static const char unplugged[] =
         "Guest display has been unplugged";
     DisplaySurface *surface;
-    int width = qemu_console_get_width(con, 640);
-    int height = qemu_console_get_height(con, 480);
+    int width = 640;
+    int height = 480;
+
+    if (con->surface) {
+        width = surface_width(con->surface);
+        height = surface_height(con->surface);
+    }
 
     trace_console_gfx_close(con->index);
     object_property_set_link(OBJECT(con), "device", NULL, &error_abort);
@@ -2205,19 +2179,7 @@ int qemu_console_get_width(QemuConsole *con, int fallback)
     if (con == NULL) {
         con = active_console;
     }
-    if (con == NULL) {
-        return fallback;
-    }
-    switch (con->scanout.kind) {
-    case SCANOUT_DMABUF:
-        return con->scanout.dmabuf->width;
-    case SCANOUT_TEXTURE:
-        return con->scanout.texture.width;
-    case SCANOUT_SURFACE:
-        return surface_width(con->surface);
-    default:
-        return fallback;
-    }
+    return con ? surface_width(con->surface) : fallback;
 }
 
 int qemu_console_get_height(QemuConsole *con, int fallback)
@@ -2225,19 +2187,7 @@ int qemu_console_get_height(QemuConsole *con, int fallback)
     if (con == NULL) {
         con = active_console;
     }
-    if (con == NULL) {
-        return fallback;
-    }
-    switch (con->scanout.kind) {
-    case SCANOUT_DMABUF:
-        return con->scanout.dmabuf->height;
-    case SCANOUT_TEXTURE:
-        return con->scanout.texture.height;
-    case SCANOUT_SURFACE:
-        return surface_height(con->surface);
-    default:
-        return fallback;
-    }
+    return con ? surface_height(con->surface) : fallback;
 }
 
 static void vc_chr_accept_input(Chardev *chr)
@@ -2303,13 +2253,12 @@ static void text_console_do_init(Chardev *chr, DisplayState *ds)
     s->total_height = DEFAULT_BACKSCROLL;
     s->x = 0;
     s->y = 0;
-    if (s->scanout.kind != SCANOUT_SURFACE) {
-        if (active_console && active_console->scanout.kind == SCANOUT_SURFACE) {
-            g_width = qemu_console_get_width(active_console, g_width);
-            g_height = qemu_console_get_height(active_console, g_height);
+    if (!s->surface) {
+        if (active_console && active_console->surface) {
+            g_width = surface_width(active_console->surface);
+            g_height = surface_height(active_console->surface);
         }
         s->surface = qemu_create_displaysurface(g_width, g_height);
-        s->scanout.kind = SCANOUT_SURFACE;
     }
 
     s->hw_ops = &text_console_ops;
@@ -2368,7 +2317,6 @@ static void vc_chr_open(Chardev *chr,
         s = new_console(NULL, TEXT_CONSOLE, 0);
     } else {
         s = new_console(NULL, TEXT_CONSOLE_FIXED_SIZE, 0);
-        s->scanout.kind = SCANOUT_SURFACE;
         s->surface = qemu_create_displaysurface(width, height);
     }
 
@@ -2392,13 +2340,13 @@ 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 (s->surface && (s->surface->flags & QEMU_ALLOCATED_FLAG) &&
+        pixman_image_get_width(s->surface->image) == width &&
+        pixman_image_get_height(s->surface->image) == height) {
         return;
     }
 
@@ -2408,12 +2356,7 @@ void qemu_console_resize(QemuConsole *s, int width, int height)
 
 DisplaySurface *qemu_console_surface(QemuConsole *console)
 {
-    switch (console->scanout.kind) {
-    case SCANOUT_SURFACE:
-        return console->surface;
-    default:
-        return NULL;
-    }
+    return console->surface;
 }
 
 PixelFormat qemu_default_pixelformat(int bpp)
-- 
2.32.0 (Apple Git-132)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/6] Revert "ui: split the GL context in a different object"
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 1/6] " Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 2/6] Revert "console: save current scanout details" Akihiko Odaki
@ 2022-02-13  2:42 ` Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 4/6] Revert "ui: dispatch GL events to all listeners" Akihiko Odaki
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

This reverts commit 5e79d516e8ac818d2a90aae9f787775055434ee9.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 include/ui/console.h       | 34 ++++++++++++----------------------
 include/ui/egl-context.h   |  6 +++---
 include/ui/gtk.h           | 11 +++++------
 include/ui/sdl2.h          |  7 +++----
 include/ui/spice-display.h |  1 -
 ui/console.c               | 26 ++++++++++----------------
 ui/dbus-listener.c         | 16 ++++++++++++++--
 ui/dbus.c                  | 22 ----------------------
 ui/dbus.h                  |  4 ----
 ui/egl-context.c           |  6 +++---
 ui/egl-headless.c          | 21 +++++++--------------
 ui/gtk-egl.c               | 10 +++++-----
 ui/gtk-gl-area.c           |  8 ++++----
 ui/gtk.c                   | 24 ++++++++----------------
 ui/sdl2-gl.c               | 10 +++++-----
 ui/sdl2.c                  | 13 ++++---------
 ui/spice-display.c         | 18 +++++++-----------
 17 files changed, 90 insertions(+), 147 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index eefd7e4dc1f..58722312534 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -179,7 +179,6 @@ typedef struct QemuDmaBuf {
 } QemuDmaBuf;
 
 typedef struct DisplayState DisplayState;
-typedef struct DisplayGLCtx DisplayGLCtx;
 
 typedef struct DisplayChangeListenerOps {
     const char *dpy_name;
@@ -214,6 +213,16 @@ typedef struct DisplayChangeListenerOps {
     void (*dpy_cursor_define)(DisplayChangeListener *dcl,
                               QEMUCursor *cursor);
 
+    /* required if GL */
+    QEMUGLContext (*dpy_gl_ctx_create)(DisplayChangeListener *dcl,
+                                       QEMUGLParams *params);
+    /* required if GL */
+    void (*dpy_gl_ctx_destroy)(DisplayChangeListener *dcl,
+                               QEMUGLContext ctx);
+    /* required if GL */
+    int (*dpy_gl_ctx_make_current)(DisplayChangeListener *dcl,
+                                   QEMUGLContext ctx);
+
     /* required if GL */
     void (*dpy_gl_scanout_disable)(DisplayChangeListener *dcl);
     /* required if GL */
@@ -254,26 +263,6 @@ struct DisplayChangeListener {
     QLIST_ENTRY(DisplayChangeListener) next;
 };
 
-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;
-
-    QEMUGLContext (*dpy_gl_ctx_create)(DisplayGLCtx *dgc,
-                                       QEMUGLParams *params);
-    void (*dpy_gl_ctx_destroy)(DisplayGLCtx *dgc,
-                               QEMUGLContext ctx);
-    int (*dpy_gl_ctx_make_current)(DisplayGLCtx *dgc,
-                                   QEMUGLContext ctx);
-} DisplayGLCtxOps;
-
-struct DisplayGLCtx {
-    const DisplayGLCtxOps *ops;
-};
-
 DisplayState *init_displaystate(void);
 DisplaySurface *qemu_create_displaysurface_from(int width, int height,
                                                 pixman_format_code_t format,
@@ -420,7 +409,8 @@ void graphic_hw_gl_block(QemuConsole *con, bool block);
 
 void qemu_console_early_init(void);
 
-void qemu_console_set_display_gl_ctx(QemuConsole *con, DisplayGLCtx *ctx);
+void qemu_console_set_display_gl_ctx(QemuConsole *con,
+                                     DisplayChangeListener *dcl);
 
 QemuConsole *qemu_console_lookup_by_index(unsigned int index);
 QemuConsole *qemu_console_lookup_by_device(DeviceState *dev, uint32_t head);
diff --git a/include/ui/egl-context.h b/include/ui/egl-context.h
index c2761d747a4..9374fe41e32 100644
--- a/include/ui/egl-context.h
+++ b/include/ui/egl-context.h
@@ -4,10 +4,10 @@
 #include "ui/console.h"
 #include "ui/egl-helpers.h"
 
-QEMUGLContext qemu_egl_create_context(DisplayGLCtx *dgc,
+QEMUGLContext qemu_egl_create_context(DisplayChangeListener *dcl,
                                       QEMUGLParams *params);
-void qemu_egl_destroy_context(DisplayGLCtx *dgc, QEMUGLContext ctx);
-int qemu_egl_make_context_current(DisplayGLCtx *dgc,
+void qemu_egl_destroy_context(DisplayChangeListener *dcl, QEMUGLContext ctx);
+int qemu_egl_make_context_current(DisplayChangeListener *dcl,
                                   QEMUGLContext ctx);
 
 #endif /* EGL_CONTEXT_H */
diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index 101b147d1b9..7d22affd381 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -35,7 +35,6 @@ typedef struct GtkDisplayState GtkDisplayState;
 
 typedef struct VirtualGfxConsole {
     GtkWidget *drawing_area;
-    DisplayGLCtx dgc;
     DisplayChangeListener dcl;
     QKbdState *kbd;
     DisplaySurface *ds;
@@ -166,7 +165,7 @@ void gd_egl_update(DisplayChangeListener *dcl,
 void gd_egl_refresh(DisplayChangeListener *dcl);
 void gd_egl_switch(DisplayChangeListener *dcl,
                    DisplaySurface *surface);
-QEMUGLContext gd_egl_create_context(DisplayGLCtx *dgc,
+QEMUGLContext gd_egl_create_context(DisplayChangeListener *dcl,
                                     QEMUGLParams *params);
 void gd_egl_scanout_disable(DisplayChangeListener *dcl);
 void gd_egl_scanout_texture(DisplayChangeListener *dcl,
@@ -188,7 +187,7 @@ void gd_egl_flush(DisplayChangeListener *dcl,
 void gd_egl_scanout_flush(DisplayChangeListener *dcl,
                           uint32_t x, uint32_t y, uint32_t w, uint32_t h);
 void gtk_egl_init(DisplayGLMode mode);
-int gd_egl_make_current(DisplayGLCtx *dgc,
+int gd_egl_make_current(DisplayChangeListener *dcl,
                         QEMUGLContext ctx);
 
 /* ui/gtk-gl-area.c */
@@ -199,9 +198,9 @@ void gd_gl_area_update(DisplayChangeListener *dcl,
 void gd_gl_area_refresh(DisplayChangeListener *dcl);
 void gd_gl_area_switch(DisplayChangeListener *dcl,
                        DisplaySurface *surface);
-QEMUGLContext gd_gl_area_create_context(DisplayGLCtx *dgc,
+QEMUGLContext gd_gl_area_create_context(DisplayChangeListener *dcl,
                                         QEMUGLParams *params);
-void gd_gl_area_destroy_context(DisplayGLCtx *dgc,
+void gd_gl_area_destroy_context(DisplayChangeListener *dcl,
                                 QEMUGLContext ctx);
 void gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl,
                                QemuDmaBuf *dmabuf);
@@ -216,7 +215,7 @@ void gd_gl_area_scanout_disable(DisplayChangeListener *dcl);
 void gd_gl_area_scanout_flush(DisplayChangeListener *dcl,
                               uint32_t x, uint32_t y, uint32_t w, uint32_t h);
 void gtk_gl_area_init(void);
-int gd_gl_area_make_current(DisplayGLCtx *dgc,
+int gd_gl_area_make_current(DisplayChangeListener *dcl,
                             QEMUGLContext ctx);
 
 /* gtk-clipboard.c */
diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
index 71bcf7ebdae..f85c117a78f 100644
--- a/include/ui/sdl2.h
+++ b/include/ui/sdl2.h
@@ -16,7 +16,6 @@
 #endif
 
 struct sdl2_console {
-    DisplayGLCtx dgc;
     DisplayChangeListener dcl;
     DisplaySurface *surface;
     DisplayOptions *opts;
@@ -66,10 +65,10 @@ void sdl2_gl_switch(DisplayChangeListener *dcl,
 void sdl2_gl_refresh(DisplayChangeListener *dcl);
 void sdl2_gl_redraw(struct sdl2_console *scon);
 
-QEMUGLContext sdl2_gl_create_context(DisplayGLCtx *dgc,
+QEMUGLContext sdl2_gl_create_context(DisplayChangeListener *dcl,
                                      QEMUGLParams *params);
-void sdl2_gl_destroy_context(DisplayGLCtx *dgc, QEMUGLContext ctx);
-int sdl2_gl_make_context_current(DisplayGLCtx *dgc,
+void sdl2_gl_destroy_context(DisplayChangeListener *dcl, QEMUGLContext ctx);
+int sdl2_gl_make_context_current(DisplayChangeListener *dcl,
                                  QEMUGLContext ctx);
 
 void sdl2_gl_scanout_disable(DisplayChangeListener *dcl);
diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index e271e011da1..df146773040 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -86,7 +86,6 @@ typedef struct SimpleSpiceCursor SimpleSpiceCursor;
 
 struct SimpleSpiceDisplay {
     DisplaySurface *ds;
-    DisplayGLCtx dgc;
     DisplayChangeListener dcl;
     void *buf;
     int bufsize;
diff --git a/ui/console.c b/ui/console.c
index 78583df9203..13c0d001c09 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -78,7 +78,7 @@ struct QemuConsole {
     DisplayState *ds;
     DisplaySurface *surface;
     int dcls;
-    DisplayGLCtx *gl;
+    DisplayChangeListener *gl;
     int gl_block;
     QEMUTimer *gl_unblock_timer;
     int window_id;
@@ -1458,24 +1458,17 @@ static bool dpy_compatible_with(QemuConsole *con,
     return true;
 }
 
-void qemu_console_set_display_gl_ctx(QemuConsole *con, DisplayGLCtx *gl)
+void qemu_console_set_display_gl_ctx(QemuConsole *con,
+                                     DisplayChangeListener *dcl)
 {
     /* display has opengl support */
-    assert(con);
-    if (con->gl) {
-        error_report("The console already has an OpenGL context.");
+    assert(dcl->con);
+    if (dcl->con->gl) {
+        fprintf(stderr, "can't register two opengl displays (%s, %s)\n",
+                dcl->ops->dpy_name, dcl->con->gl->ops->dpy_name);
         exit(1);
     }
-    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;
+    dcl->con->gl = dcl;
 }
 
 void register_displaychangelistener(DisplayChangeListener *dcl)
@@ -1487,7 +1480,8 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
 
     assert(!dcl->ds);
 
-    if (dcl->con && !dpy_gl_compatible_with(dcl->con, dcl)) {
+    if (dcl->con && dcl->con->gl &&
+        dcl->con->gl != dcl) {
         error_report("Display %s is incompatible with the GL context",
                      dcl->ops->dpy_name);
         exit(1);
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index e4242d69de2..b3ca348c4e5 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -102,6 +102,14 @@ static void dbus_call_update_gl(DBusDisplayListener *ddl,
     }
 }
 
+static QEMUGLContext dbus_create_context(DisplayChangeListener *dcl,
+                                         QEMUGLParams *params)
+{
+    eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
+                   qemu_egl_rn_ctx);
+    return qemu_egl_create_context(dcl, params);
+}
+
 static void dbus_scanout_disable(DisplayChangeListener *dcl)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
@@ -463,7 +471,7 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
     }
 }
 
-const DisplayChangeListenerOps dbus_gl_dcl_ops = {
+static const DisplayChangeListenerOps dbus_gl_dcl_ops = {
     .dpy_name                = "dbus-gl",
     .dpy_gfx_update          = dbus_gl_gfx_update,
     .dpy_gfx_switch          = dbus_gl_gfx_switch,
@@ -472,6 +480,10 @@ const DisplayChangeListenerOps dbus_gl_dcl_ops = {
     .dpy_mouse_set           = dbus_mouse_set,
     .dpy_cursor_define       = dbus_cursor_define,
 
+    .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_scanout_disable  = dbus_scanout_disable,
     .dpy_gl_scanout_texture  = dbus_scanout_texture,
     .dpy_gl_scanout_dmabuf   = dbus_scanout_dmabuf,
@@ -481,7 +493,7 @@ const DisplayChangeListenerOps dbus_gl_dcl_ops = {
     .dpy_gl_update           = dbus_scanout_update,
 };
 
-const DisplayChangeListenerOps dbus_dcl_ops = {
+static const DisplayChangeListenerOps dbus_dcl_ops = {
     .dpy_name                = "dbus",
     .dpy_gfx_update          = dbus_gfx_update,
     .dpy_gfx_switch          = dbus_gfx_switch,
diff --git a/ui/dbus.c b/ui/dbus.c
index 0074424c1fe..040a2a3fd8b 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -30,7 +30,6 @@
 #include "sysemu/sysemu.h"
 #include "ui/dbus-module.h"
 #include "ui/egl-helpers.h"
-#include "ui/egl-context.h"
 #include "audio/audio.h"
 #include "audio/audio_int.h"
 #include "qapi/error.h"
@@ -40,21 +39,6 @@
 
 static DBusDisplay *dbus_display;
 
-static QEMUGLContext dbus_create_context(DisplayGLCtx *dgc,
-                                         QEMUGLParams *params)
-{
-    eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
-                   qemu_egl_rn_ctx);
-    return qemu_egl_create_context(dgc, params);
-}
-
-static const DisplayGLCtxOps dbus_gl_ops = {
-    .compatible_dcl          = &dbus_gl_dcl_ops,
-    .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,
-};
-
 static NotifierList dbus_display_notifiers =
     NOTIFIER_LIST_INITIALIZER(dbus_display_notifiers);
 
@@ -82,7 +66,6 @@ dbus_display_init(Object *o)
     DBusDisplay *dd = DBUS_DISPLAY(o);
     g_autoptr(GDBusObjectSkeleton) vm = NULL;
 
-    dd->glctx.ops = &dbus_gl_ops;
     dd->iface = qemu_dbus_display1_vm_skeleton_new();
     dd->consoles = g_ptr_array_new_with_free_func(g_object_unref);
 
@@ -131,11 +114,6 @@ dbus_display_add_console(DBusDisplay *dd, int idx, Error **errp)
     con = qemu_console_lookup_by_index(idx);
     assert(con);
 
-    if (qemu_console_is_graphic(con) &&
-        dd->gl_mode != DISPLAYGL_MODE_OFF) {
-        qemu_console_set_display_gl_ctx(con, &dd->glctx);
-    }
-
     dbus_console = dbus_display_console_new(dd, con);
     g_ptr_array_insert(dd->consoles, idx, dbus_console);
     g_dbus_object_manager_server_export(dd->server,
diff --git a/ui/dbus.h b/ui/dbus.h
index fd24b299bbf..a6c7dd84287 100644
--- a/ui/dbus.h
+++ b/ui/dbus.h
@@ -45,7 +45,6 @@ struct DBusDisplay {
     bool p2p;
     char *dbus_addr;
     char *audiodev;
-    DisplayGLCtx glctx;
 
     GDBusConnection *bus;
     GDBusObjectManagerServer *server;
@@ -119,9 +118,6 @@ dbus_display_listener_connection_get_console(DBusDisplayListenerConnection *ddlc
 const char *
 dbus_display_listener_connection_get_bus_name(DBusDisplayListenerConnection *ddlc);
 
-extern const DisplayChangeListenerOps dbus_gl_dcl_ops;
-extern const DisplayChangeListenerOps dbus_dcl_ops;
-
 #define TYPE_CHARDEV_DBUS "chardev-dbus"
 
 typedef struct DBusChardevClass {
diff --git a/ui/egl-context.c b/ui/egl-context.c
index eb5f520fc4d..368ffa49d82 100644
--- a/ui/egl-context.c
+++ b/ui/egl-context.c
@@ -1,7 +1,7 @@
 #include "qemu/osdep.h"
 #include "ui/egl-context.h"
 
-QEMUGLContext qemu_egl_create_context(DisplayGLCtx *dgc,
+QEMUGLContext qemu_egl_create_context(DisplayChangeListener *dcl,
                                       QEMUGLParams *params)
 {
    EGLContext ctx;
@@ -24,12 +24,12 @@ QEMUGLContext qemu_egl_create_context(DisplayGLCtx *dgc,
    return ctx;
 }
 
-void qemu_egl_destroy_context(DisplayGLCtx *dgc, QEMUGLContext ctx)
+void qemu_egl_destroy_context(DisplayChangeListener *dcl, QEMUGLContext ctx)
 {
     eglDestroyContext(qemu_egl_display, ctx);
 }
 
-int qemu_egl_make_context_current(DisplayGLCtx *dgc,
+int qemu_egl_make_context_current(DisplayChangeListener *dcl,
                                   QEMUGLContext ctx)
 {
    return eglMakeCurrent(qemu_egl_display,
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index 94082a9da95..08327c40c6e 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -38,12 +38,12 @@ static void egl_gfx_switch(DisplayChangeListener *dcl,
     edpy->ds = new_surface;
 }
 
-static QEMUGLContext egl_create_context(DisplayGLCtx *dgc,
+static QEMUGLContext egl_create_context(DisplayChangeListener *dcl,
                                         QEMUGLParams *params)
 {
     eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
                    qemu_egl_rn_ctx);
-    return qemu_egl_create_context(dgc, params);
+    return qemu_egl_create_context(dcl, params);
 }
 
 static void egl_scanout_disable(DisplayChangeListener *dcl)
@@ -157,6 +157,10 @@ static const DisplayChangeListenerOps egl_ops = {
     .dpy_gfx_update          = egl_gfx_update,
     .dpy_gfx_switch          = egl_gfx_switch,
 
+    .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,
+
     .dpy_gl_scanout_disable  = egl_scanout_disable,
     .dpy_gl_scanout_texture  = egl_scanout_texture,
     .dpy_gl_scanout_dmabuf   = egl_scanout_dmabuf,
@@ -166,13 +170,6 @@ static const DisplayChangeListenerOps egl_ops = {
     .dpy_gl_update           = egl_scanout_flush,
 };
 
-static const DisplayGLCtxOps eglctx_ops = {
-    .compatible_dcl          = &egl_ops,
-    .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,
-};
-
 static void early_egl_headless_init(DisplayOptions *opts)
 {
     display_opengl = 1;
@@ -191,8 +188,6 @@ static void egl_headless_init(DisplayState *ds, DisplayOptions *opts)
     }
 
     for (idx = 0;; idx++) {
-        DisplayGLCtx *ctx;
-
         con = qemu_console_lookup_by_index(idx);
         if (!con || !qemu_console_is_graphic(con)) {
             break;
@@ -202,9 +197,7 @@ static void egl_headless_init(DisplayState *ds, DisplayOptions *opts)
         edpy->dcl.con = con;
         edpy->dcl.ops = &egl_ops;
         edpy->gls = qemu_gl_init_shader();
-        ctx = g_new0(DisplayGLCtx, 1);
-        ctx->ops = &eglctx_ops;
-        qemu_console_set_display_gl_ctx(con, ctx);
+        qemu_console_set_display_gl_ctx(con, &edpy->dcl);
         register_displaychangelistener(&edpy->dcl);
     }
 }
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index e3bd4bc2743..e891b61048a 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -197,14 +197,14 @@ void gd_egl_switch(DisplayChangeListener *dcl,
     }
 }
 
-QEMUGLContext gd_egl_create_context(DisplayGLCtx *dgc,
+QEMUGLContext gd_egl_create_context(DisplayChangeListener *dcl,
                                     QEMUGLParams *params)
 {
-    VirtualConsole *vc = container_of(dgc, VirtualConsole, gfx.dgc);
+    VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
 
     eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
                    vc->gfx.esurface, vc->gfx.ectx);
-    return qemu_egl_create_context(dgc, params);
+    return qemu_egl_create_context(dcl, params);
 }
 
 void gd_egl_scanout_disable(DisplayChangeListener *dcl)
@@ -360,10 +360,10 @@ void gtk_egl_init(DisplayGLMode mode)
     display_opengl = 1;
 }
 
-int gd_egl_make_current(DisplayGLCtx *dgc,
+int gd_egl_make_current(DisplayChangeListener *dcl,
                         QEMUGLContext ctx)
 {
-    VirtualConsole *vc = container_of(dgc, VirtualConsole, gfx.dgc);
+    VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
 
     return eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
                           vc->gfx.esurface, ctx);
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index fc5a082eb84..f1c7636cba9 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -170,10 +170,10 @@ void gd_gl_area_switch(DisplayChangeListener *dcl,
     }
 }
 
-QEMUGLContext gd_gl_area_create_context(DisplayGLCtx *dgc,
+QEMUGLContext gd_gl_area_create_context(DisplayChangeListener *dcl,
                                         QEMUGLParams *params)
 {
-    VirtualConsole *vc = container_of(dgc, VirtualConsole, gfx.dgc);
+    VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
     GdkWindow *window;
     GdkGLContext *ctx;
     GError *err = NULL;
@@ -199,7 +199,7 @@ QEMUGLContext gd_gl_area_create_context(DisplayGLCtx *dgc,
     return ctx;
 }
 
-void gd_gl_area_destroy_context(DisplayGLCtx *dgc, QEMUGLContext ctx)
+void gd_gl_area_destroy_context(DisplayChangeListener *dcl, QEMUGLContext ctx)
 {
     /* FIXME */
 }
@@ -278,7 +278,7 @@ void gtk_gl_area_init(void)
     display_opengl = 1;
 }
 
-int gd_gl_area_make_current(DisplayGLCtx *dgc,
+int gd_gl_area_make_current(DisplayChangeListener *dcl,
                             QEMUGLContext ctx)
 {
     gdk_gl_context_make_current(ctx);
diff --git a/ui/gtk.c b/ui/gtk.c
index a8567b9ddc8..ca29dde6cbd 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -606,6 +606,9 @@ static const DisplayChangeListenerOps dcl_gl_area_ops = {
     .dpy_mouse_set        = gd_mouse_set,
     .dpy_cursor_define    = gd_cursor_define,
 
+    .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,
     .dpy_gl_scanout_texture  = gd_gl_area_scanout_texture,
     .dpy_gl_scanout_disable  = gd_gl_area_scanout_disable,
     .dpy_gl_update           = gd_gl_area_scanout_flush,
@@ -614,14 +617,8 @@ static const DisplayChangeListenerOps dcl_gl_area_ops = {
     .dpy_has_dmabuf          = gd_has_dmabuf,
 };
 
-static const DisplayGLCtxOps gl_area_ctx_ops = {
-    .compatible_dcl          = &dcl_gl_area_ops,
-    .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,
-};
-
 #ifdef CONFIG_X11
+
 static const DisplayChangeListenerOps dcl_egl_ops = {
     .dpy_name             = "gtk-egl",
     .dpy_gfx_update       = gd_egl_update,
@@ -631,6 +628,9 @@ static const DisplayChangeListenerOps dcl_egl_ops = {
     .dpy_mouse_set        = gd_mouse_set,
     .dpy_cursor_define    = gd_cursor_define,
 
+    .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,
     .dpy_gl_scanout_disable  = gd_egl_scanout_disable,
     .dpy_gl_scanout_texture  = gd_egl_scanout_texture,
     .dpy_gl_scanout_dmabuf   = gd_egl_scanout_dmabuf,
@@ -641,12 +641,6 @@ static const DisplayChangeListenerOps dcl_egl_ops = {
     .dpy_has_dmabuf          = gd_has_dmabuf,
 };
 
-static const DisplayGLCtxOps egl_ctx_ops = {
-    .compatible_dcl          = &dcl_egl_ops,
-    .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,
-};
 #endif
 
 #endif /* CONFIG_OPENGL */
@@ -2070,7 +2064,6 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, VirtualConsole *vc,
             g_signal_connect(vc->gfx.drawing_area, "realize",
                              G_CALLBACK(gl_area_realize), vc);
             vc->gfx.dcl.ops = &dcl_gl_area_ops;
-            vc->gfx.dgc.ops = &gl_area_ctx_ops;
         } else {
 #ifdef CONFIG_X11
             vc->gfx.drawing_area = gtk_drawing_area_new();
@@ -2085,7 +2078,6 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, VirtualConsole *vc,
             gtk_widget_set_double_buffered(vc->gfx.drawing_area, FALSE);
 #pragma GCC diagnostic pop
             vc->gfx.dcl.ops = &dcl_egl_ops;
-            vc->gfx.dgc.ops = &egl_ctx_ops;
             vc->gfx.has_dmabuf = qemu_egl_has_dmabuf();
 #else
             abort();
@@ -2121,7 +2113,7 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, VirtualConsole *vc,
     vc->gfx.dcl.con = con;
 
     if (display_opengl) {
-        qemu_console_set_display_gl_ctx(con, &vc->gfx.dgc);
+        qemu_console_set_display_gl_ctx(con, &vc->gfx.dcl);
     }
     register_displaychangelistener(&vc->gfx.dcl);
 
diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index 39cab8cde73..5b950fbbea2 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -132,10 +132,10 @@ void sdl2_gl_redraw(struct sdl2_console *scon)
     }
 }
 
-QEMUGLContext sdl2_gl_create_context(DisplayGLCtx *dgc,
+QEMUGLContext sdl2_gl_create_context(DisplayChangeListener *dcl,
                                      QEMUGLParams *params)
 {
-    struct sdl2_console *scon = container_of(dgc, struct sdl2_console, dgc);
+    struct sdl2_console *scon = container_of(dcl, struct sdl2_console, dcl);
     SDL_GLContext ctx;
 
     assert(scon->opengl);
@@ -167,17 +167,17 @@ QEMUGLContext sdl2_gl_create_context(DisplayGLCtx *dgc,
     return (QEMUGLContext)ctx;
 }
 
-void sdl2_gl_destroy_context(DisplayGLCtx *dgc, QEMUGLContext ctx)
+void sdl2_gl_destroy_context(DisplayChangeListener *dcl, QEMUGLContext ctx)
 {
     SDL_GLContext sdlctx = (SDL_GLContext)ctx;
 
     SDL_GL_DeleteContext(sdlctx);
 }
 
-int sdl2_gl_make_context_current(DisplayGLCtx *dgc,
+int sdl2_gl_make_context_current(DisplayChangeListener *dcl,
                                  QEMUGLContext ctx)
 {
-    struct sdl2_console *scon = container_of(dgc, struct sdl2_console, dgc);
+    struct sdl2_console *scon = container_of(dcl, struct sdl2_console, dcl);
     SDL_GLContext sdlctx = (SDL_GLContext)ctx;
 
     assert(scon->opengl);
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 46a252d7d9d..628ae33245f 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -783,16 +783,12 @@ static const DisplayChangeListenerOps dcl_gl_ops = {
     .dpy_mouse_set           = sdl_mouse_warp,
     .dpy_cursor_define       = sdl_mouse_define,
 
-    .dpy_gl_scanout_disable  = sdl2_gl_scanout_disable,
-    .dpy_gl_scanout_texture  = sdl2_gl_scanout_texture,
-    .dpy_gl_update           = sdl2_gl_scanout_flush,
-};
-
-static const DisplayGLCtxOps gl_ctx_ops = {
-    .compatible_dcl          = &dcl_gl_ops,
     .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,
+    .dpy_gl_scanout_disable  = sdl2_gl_scanout_disable,
+    .dpy_gl_scanout_texture  = sdl2_gl_scanout_texture,
+    .dpy_gl_update           = sdl2_gl_scanout_flush,
 };
 #endif
 
@@ -869,7 +865,6 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
 #ifdef CONFIG_OPENGL
         sdl2_console[i].opengl = display_opengl;
         sdl2_console[i].dcl.ops = display_opengl ? &dcl_gl_ops : &dcl_2d_ops;
-        sdl2_console[i].dgc.ops = display_opengl ? &gl_ctx_ops : NULL;
 #else
         sdl2_console[i].opengl = 0;
         sdl2_console[i].dcl.ops = &dcl_2d_ops;
@@ -877,7 +872,7 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
         sdl2_console[i].dcl.con = con;
         sdl2_console[i].kbd = qkbd_state_init(con);
         if (display_opengl) {
-            qemu_console_set_display_gl_ctx(con, &sdl2_console[i].dgc);
+            qemu_console_set_display_gl_ctx(con, &sdl2_console[i].dcl);
         }
         register_displaychangelistener(&sdl2_console[i].dcl);
 
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 1043f47f945..bf057bc95f5 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -908,12 +908,12 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
     }
 }
 
-static QEMUGLContext qemu_spice_gl_create_context(DisplayGLCtx *dgc,
+static QEMUGLContext qemu_spice_gl_create_context(DisplayChangeListener *dcl,
                                                   QEMUGLParams *params)
 {
     eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
                    qemu_egl_rn_ctx);
-    return qemu_egl_create_context(dgc, params);
+    return qemu_egl_create_context(dcl, params);
 }
 
 static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
@@ -1105,6 +1105,10 @@ static const DisplayChangeListenerOps display_listener_gl_ops = {
     .dpy_mouse_set           = display_mouse_set,
     .dpy_cursor_define       = display_mouse_define,
 
+    .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,
+
     .dpy_gl_scanout_disable  = qemu_spice_gl_scanout_disable,
     .dpy_gl_scanout_texture  = qemu_spice_gl_scanout_texture,
     .dpy_gl_scanout_dmabuf   = qemu_spice_gl_scanout_dmabuf,
@@ -1114,13 +1118,6 @@ static const DisplayChangeListenerOps display_listener_gl_ops = {
     .dpy_gl_update           = qemu_spice_gl_update,
 };
 
-static const DisplayGLCtxOps gl_ctx_ops = {
-    .compatible_dcl          = &display_listener_gl_ops,
-    .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,
-};
-
 #endif /* HAVE_SPICE_GL */
 
 static void qemu_spice_display_init_one(QemuConsole *con)
@@ -1133,7 +1130,6 @@ static void qemu_spice_display_init_one(QemuConsole *con)
 #ifdef HAVE_SPICE_GL
     if (spice_opengl) {
         ssd->dcl.ops = &display_listener_gl_ops;
-        ssd->dgc.ops = &gl_ctx_ops;
         ssd->gl_unblock_bh = qemu_bh_new(qemu_spice_gl_unblock_bh, ssd);
         ssd->gl_unblock_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
                                              qemu_spice_gl_block_timer, ssd);
@@ -1163,7 +1159,7 @@ static void qemu_spice_display_init_one(QemuConsole *con)
     qemu_spice_create_host_memslot(ssd);
 
     if (spice_opengl) {
-        qemu_console_set_display_gl_ctx(con, &ssd->dgc);
+        qemu_console_set_display_gl_ctx(con, &ssd->dcl);
     }
     register_displaychangelistener(&ssd->dcl);
 }
-- 
2.32.0 (Apple Git-132)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/6] Revert "ui: dispatch GL events to all listeners"
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
                   ` (2 preceding siblings ...)
  2022-02-13  2:42 ` [PATCH 3/6] Revert "ui: split the GL context in a different object" Akihiko Odaki
@ 2022-02-13  2:42 ` Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 5/6] Revert "ui: associate GL context outside of display listener registration" Akihiko Odaki
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

This reverts commit 7cc712e9862ffdbe4161dbdf3bbf41bcbe547472.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/console.c | 58 +++++++++++++++-------------------------------------
 1 file changed, 16 insertions(+), 42 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 13c0d001c09..6f21007737e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1824,12 +1824,8 @@ int dpy_gl_ctx_make_current(QemuConsole *con, QEMUGLContext ctx)
 
 void dpy_gl_scanout_disable(QemuConsole *con)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
-
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_scanout_disable(dcl);
-    }
+    assert(con->gl);
+    con->gl->ops->dpy_gl_scanout_disable(con->gl);
 }
 
 void dpy_gl_scanout_texture(QemuConsole *con,
@@ -1840,80 +1836,58 @@ void dpy_gl_scanout_texture(QemuConsole *con,
                             uint32_t x, uint32_t y,
                             uint32_t width, uint32_t height)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
-
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_scanout_texture(dcl, backing_id,
+    assert(con->gl);
+    con->gl->ops->dpy_gl_scanout_texture(con->gl, backing_id,
                                          backing_y_0_top,
                                          backing_width, backing_height,
                                          x, y, width, height);
-    }
 }
 
 void dpy_gl_scanout_dmabuf(QemuConsole *con,
                            QemuDmaBuf *dmabuf)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
-
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_scanout_dmabuf(dcl, dmabuf);
-    }
+    assert(con->gl);
+    con->gl->ops->dpy_gl_scanout_dmabuf(con->gl, dmabuf);
 }
 
 void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf *dmabuf,
                           bool have_hot, uint32_t hot_x, uint32_t hot_y)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
+    assert(con->gl);
 
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (dcl->ops->dpy_gl_cursor_dmabuf) {
-            dcl->ops->dpy_gl_cursor_dmabuf(dcl, dmabuf,
+    if (con->gl->ops->dpy_gl_cursor_dmabuf) {
+        con->gl->ops->dpy_gl_cursor_dmabuf(con->gl, dmabuf,
                                            have_hot, hot_x, hot_y);
-        }
     }
 }
 
 void dpy_gl_cursor_position(QemuConsole *con,
                             uint32_t pos_x, uint32_t pos_y)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
+    assert(con->gl);
 
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (dcl->ops->dpy_gl_cursor_position) {
-            dcl->ops->dpy_gl_cursor_position(dcl, pos_x, pos_y);
-        }
+    if (con->gl->ops->dpy_gl_cursor_position) {
+        con->gl->ops->dpy_gl_cursor_position(con->gl, pos_x, pos_y);
     }
 }
 
 void dpy_gl_release_dmabuf(QemuConsole *con,
                           QemuDmaBuf *dmabuf)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
+    assert(con->gl);
 
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        if (dcl->ops->dpy_gl_release_dmabuf) {
-            dcl->ops->dpy_gl_release_dmabuf(dcl, dmabuf);
-        }
+    if (con->gl->ops->dpy_gl_release_dmabuf) {
+        con->gl->ops->dpy_gl_release_dmabuf(con->gl, dmabuf);
     }
 }
 
 void dpy_gl_update(QemuConsole *con,
                    uint32_t x, uint32_t y, uint32_t w, uint32_t h)
 {
-    DisplayState *s = con->ds;
-    DisplayChangeListener *dcl;
-
     assert(con->gl);
 
     graphic_hw_gl_block(con, true);
-    QLIST_FOREACH(dcl, &s->listeners, next) {
-        dcl->ops->dpy_gl_update(dcl, x, y, w, h);
-    }
+    con->gl->ops->dpy_gl_update(con->gl, x, y, w, h);
     graphic_hw_gl_block(con, false);
 }
 
-- 
2.32.0 (Apple Git-132)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/6] Revert "ui: associate GL context outside of display listener registration"
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
                   ` (3 preceding siblings ...)
  2022-02-13  2:42 ` [PATCH 4/6] Revert "ui: dispatch GL events to all listeners" Akihiko Odaki
@ 2022-02-13  2:42 ` Akihiko Odaki
  2022-02-13  2:42 ` [PATCH 6/6] Revert "ui: factor out qemu_console_set_display_gl_ctx()" Akihiko Odaki
  2022-02-14 12:06 ` [PATCH 0/6] ui/dbus: Share one listener for a console Marc-André Lureau
  6 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

This reverts commit ac32b2fff127843355b4f7e7ac9f93dd4a395adf.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/console.c       | 7 ++-----
 ui/egl-headless.c  | 1 -
 ui/gtk.c           | 3 ---
 ui/sdl2.c          | 3 ---
 ui/spice-display.c | 3 ---
 5 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 6f21007737e..f3d72655bb6 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1480,11 +1480,8 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
 
     assert(!dcl->ds);
 
-    if (dcl->con && dcl->con->gl &&
-        dcl->con->gl != dcl) {
-        error_report("Display %s is incompatible with the GL context",
-                     dcl->ops->dpy_name);
-        exit(1);
+    if (dcl->ops->dpy_gl_ctx_create) {
+        qemu_console_set_display_gl_ctx(dcl->con, dcl);
     }
 
     if (dcl->con) {
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index 08327c40c6e..a26a2520c49 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -197,7 +197,6 @@ static void egl_headless_init(DisplayState *ds, DisplayOptions *opts)
         edpy->dcl.con = con;
         edpy->dcl.ops = &egl_ops;
         edpy->gls = qemu_gl_init_shader();
-        qemu_console_set_display_gl_ctx(con, &edpy->dcl);
         register_displaychangelistener(&edpy->dcl);
     }
 }
diff --git a/ui/gtk.c b/ui/gtk.c
index ca29dde6cbd..b31e900ebda 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2112,9 +2112,6 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, VirtualConsole *vc,
     vc->gfx.kbd = qkbd_state_init(con);
     vc->gfx.dcl.con = con;
 
-    if (display_opengl) {
-        qemu_console_set_display_gl_ctx(con, &vc->gfx.dcl);
-    }
     register_displaychangelistener(&vc->gfx.dcl);
 
     gd_connect_vc_gfx_signals(vc);
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 628ae33245f..4117b4ac6e7 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -871,9 +871,6 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
 #endif
         sdl2_console[i].dcl.con = con;
         sdl2_console[i].kbd = qkbd_state_init(con);
-        if (display_opengl) {
-            qemu_console_set_display_gl_ctx(con, &sdl2_console[i].dcl);
-        }
         register_displaychangelistener(&sdl2_console[i].dcl);
 
 #if defined(SDL_VIDEO_DRIVER_WINDOWS) || defined(SDL_VIDEO_DRIVER_X11)
diff --git a/ui/spice-display.c b/ui/spice-display.c
index bf057bc95f5..07234d49594 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1158,9 +1158,6 @@ static void qemu_spice_display_init_one(QemuConsole *con)
 
     qemu_spice_create_host_memslot(ssd);
 
-    if (spice_opengl) {
-        qemu_console_set_display_gl_ctx(con, &ssd->dcl);
-    }
     register_displaychangelistener(&ssd->dcl);
 }
 
-- 
2.32.0 (Apple Git-132)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/6] Revert "ui: factor out qemu_console_set_display_gl_ctx()"
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
                   ` (4 preceding siblings ...)
  2022-02-13  2:42 ` [PATCH 5/6] Revert "ui: associate GL context outside of display listener registration" Akihiko Odaki
@ 2022-02-13  2:42 ` Akihiko Odaki
  2022-02-14 12:06 ` [PATCH 0/6] ui/dbus: Share one listener for a console Marc-André Lureau
  6 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-13  2:42 UTC (permalink / raw)
  Cc: Marc-André Lureau, qemu-devel, Akihiko Odaki, Gerd Hoffmann

This reverts commit 4f4181499170dcf80182745b319607802ea32896.
This eliminates the situation where a display is registered as a GL
context provider but not as a listener.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 include/ui/console.h |  3 ---
 ui/console.c         | 22 ++++++++--------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 58722312534..760dde770d1 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -409,9 +409,6 @@ void graphic_hw_gl_block(QemuConsole *con, bool block);
 
 void qemu_console_early_init(void);
 
-void qemu_console_set_display_gl_ctx(QemuConsole *con,
-                                     DisplayChangeListener *dcl);
-
 QemuConsole *qemu_console_lookup_by_index(unsigned int index);
 QemuConsole *qemu_console_lookup_by_device(DeviceState *dev, uint32_t head);
 QemuConsole *qemu_console_lookup_by_device_name(const char *device_id,
diff --git a/ui/console.c b/ui/console.c
index f3d72655bb6..dce2ed3e1de 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1458,19 +1458,6 @@ static bool dpy_compatible_with(QemuConsole *con,
     return true;
 }
 
-void qemu_console_set_display_gl_ctx(QemuConsole *con,
-                                     DisplayChangeListener *dcl)
-{
-    /* display has opengl support */
-    assert(dcl->con);
-    if (dcl->con->gl) {
-        fprintf(stderr, "can't register two opengl displays (%s, %s)\n",
-                dcl->ops->dpy_name, dcl->con->gl->ops->dpy_name);
-        exit(1);
-    }
-    dcl->con->gl = dcl;
-}
-
 void register_displaychangelistener(DisplayChangeListener *dcl)
 {
     static const char nodev[] =
@@ -1481,7 +1468,14 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
     assert(!dcl->ds);
 
     if (dcl->ops->dpy_gl_ctx_create) {
-        qemu_console_set_display_gl_ctx(dcl->con, dcl);
+        /* display has opengl support */
+        assert(dcl->con);
+        if (dcl->con->gl) {
+            error_report("can't register two opengl displays (%s, %s)\n",
+                         dcl->ops->dpy_name, dcl->con->gl->ops->dpy_name);
+            exit(1);
+        }
+        dcl->con->gl = dcl;
     }
 
     if (dcl->con) {
-- 
2.32.0 (Apple Git-132)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] ui/dbus: Share one listener for a console
  2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
                   ` (5 preceding siblings ...)
  2022-02-13  2:42 ` [PATCH 6/6] Revert "ui: factor out qemu_console_set_display_gl_ctx()" Akihiko Odaki
@ 2022-02-14 12:06 ` Marc-André Lureau
  2022-02-14 13:15   ` Akihiko Odaki
  6 siblings, 1 reply; 13+ messages in thread
From: Marc-André Lureau @ 2022-02-14 12:06 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: QEMU, Gerd Hoffmann

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

Hi Akihiko

On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> ui/dbus required to have multiple DisplayChangeListeners (possibly with
> OpenGL)
> for a console but that caused several problems:
> - It broke egl-headless, an unusual display which implements OpenGL
> rendering
>   for non-OpenGL displays. The code to support multiple
> DisplayChangeListeners
>   does not consider the case where non-OpenGL displays listens OpenGL
> consoles.
>

Can you provide instructions on what broke? Even better write a test,
please.

"make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py", which
covers egl-headless usage, works.


> - Multiple OpenGL DisplayChangeListeners of dbus shares one DisplaySurface
> and
>   modifies its texture field, causing OpenGL texture leak and
> use-after-free.
>

Again, please provide instructions. I have regularly run -display dbus with
multiple clients and qemu compiled with sanitizers. I don't see any leak or
use after free.


> - Introduced extra code to check the compatibility of OpenGL context
> providers
>   and OpenGL texture renderers where those are often inherently tightly
> coupled
>   since they must share the same hardware.
>

So code checks are meant to prevent misusage. They might be too limited or
broken in some ways, but reverting is likely going to introduce other
regressions I was trying to fix.

- Needed extra work to broadcast the same change to multiple dbus listeners.
>
>
Compared to what?


> This series solve them by implementing the change broadcast in ui/dbus,
> which
> knows exactly what is needed. Changes for the common code to support
> multiple
> OpenGL DisplayChangeListeners were reverted to fix egl-headless and reduce
> the code size.
>

Thanks a lot for your work, I am going to take a look at your approach. But
please help us understand better what the problem actually is, by giving
examples & tests to avoid future regressions and document the expected
behaviour.


> Akihiko Odaki (6):
>   ui/dbus: Share one listener for a console
>   Revert "console: save current scanout details"
>   Revert "ui: split the GL context in a different object"
>   Revert "ui: dispatch GL events to all listeners"
>   Revert "ui: associate GL context outside of display listener
>     registration"
>   Revert "ui: factor out qemu_console_set_display_gl_ctx()"
>
>  include/ui/console.h       |  60 +-----
>  include/ui/egl-context.h   |   6 +-
>  include/ui/gtk.h           |  11 +-
>  include/ui/sdl2.h          |   7 +-
>  include/ui/spice-display.h |   1 -
>  ui/console.c               | 258 +++++++----------------
>  ui/dbus-console.c          | 109 ++--------
>  ui/dbus-listener.c         | 417 +++++++++++++++++++++++++++----------
>  ui/dbus.c                  |  22 --
>  ui/dbus.h                  |  36 +++-
>  ui/egl-context.c           |   6 +-
>  ui/egl-headless.c          |  20 +-
>  ui/gtk-egl.c               |  10 +-
>  ui/gtk-gl-area.c           |   8 +-
>  ui/gtk.c                   |  25 +--
>  ui/sdl2-gl.c               |  10 +-
>  ui/sdl2.c                  |  14 +-
>  ui/spice-display.c         |  19 +-
>  18 files changed, 498 insertions(+), 541 deletions(-)
>
> --
> 2.32.0 (Apple Git-132)
>
>
>

-- 
Marc-André Lureau

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] ui/dbus: Share one listener for a console
  2022-02-14 12:06 ` [PATCH 0/6] ui/dbus: Share one listener for a console Marc-André Lureau
@ 2022-02-14 13:15   ` Akihiko Odaki
  2022-02-14 19:49     ` Marc-André Lureau
  0 siblings, 1 reply; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-14 13:15 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU, Gerd Hoffmann

On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi Akihiko
>
> On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> ui/dbus required to have multiple DisplayChangeListeners (possibly with OpenGL)
>> for a console but that caused several problems:
>> - It broke egl-headless, an unusual display which implements OpenGL rendering
>>   for non-OpenGL displays. The code to support multiple DisplayChangeListeners
>>   does not consider the case where non-OpenGL displays listens OpenGL consoles.
>
>
> Can you provide instructions on what broke? Even better write a test, please.

The following command segfaults. Adding a test would be nice, but it
would need a binary which uses OpenGL.
qemu-system-x86_64 -device virtio-gpu-gl-pci -display egl-headless
-vnc :0 -m 8G -cdrom Fedora-Workstation-Live-x86_64-34-1.2.iso -accel
kvm

>
> "make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py", which covers egl-headless usage, works.
>
>>
>> - Multiple OpenGL DisplayChangeListeners of dbus shares one DisplaySurface and
>>   modifies its texture field, causing OpenGL texture leak and use-after-free.
>
>
> Again, please provide instructions. I have regularly run -display dbus with multiple clients and qemu compiled with sanitizers. I don't see any leak or use after free.

I doubt sanitizers can find this because it is an OpenGL texture. You
may add a probe around surface_gl_create_texture and
surface_gl_destroy_texture.

>
>>
>> - Introduced extra code to check the compatibility of OpenGL context providers
>>   and OpenGL texture renderers where those are often inherently tightly coupled
>>   since they must share the same hardware.
>
>
> So code checks are meant to prevent misusage. They might be too limited or broken in some ways, but reverting is likely going to introduce other regressions I was trying to fix.

The misuse will not occur because DisplayChangeListeners will be
merged with OpenGL context providers.

>
>> - Needed extra work to broadcast the same change to multiple dbus listeners.
>>
>
> Compared to what?

Compared to sharing one DisplayChangeListener for multiple dbus listeners.

>
>>
>> This series solve them by implementing the change broadcast in ui/dbus, which
>> knows exactly what is needed. Changes for the common code to support multiple
>> OpenGL DisplayChangeListeners were reverted to fix egl-headless and reduce
>> the code size.
>
>
> Thanks a lot for your work, I am going to take a look at your approach. But please help us understand better what the problem actually is, by giving examples & tests to avoid future regressions and document the expected behaviour.

The thing is really complicated and I may miss details so please feel
free to ask questions or provide suggestions.

Regards,
Akihiko Odaki


>
>>
>> Akihiko Odaki (6):
>>   ui/dbus: Share one listener for a console
>>   Revert "console: save current scanout details"
>>   Revert "ui: split the GL context in a different object"
>>   Revert "ui: dispatch GL events to all listeners"
>>   Revert "ui: associate GL context outside of display listener
>>     registration"
>>   Revert "ui: factor out qemu_console_set_display_gl_ctx()"
>>
>>  include/ui/console.h       |  60 +-----
>>  include/ui/egl-context.h   |   6 +-
>>  include/ui/gtk.h           |  11 +-
>>  include/ui/sdl2.h          |   7 +-
>>  include/ui/spice-display.h |   1 -
>>  ui/console.c               | 258 +++++++----------------
>>  ui/dbus-console.c          | 109 ++--------
>>  ui/dbus-listener.c         | 417 +++++++++++++++++++++++++++----------
>>  ui/dbus.c                  |  22 --
>>  ui/dbus.h                  |  36 +++-
>>  ui/egl-context.c           |   6 +-
>>  ui/egl-headless.c          |  20 +-
>>  ui/gtk-egl.c               |  10 +-
>>  ui/gtk-gl-area.c           |   8 +-
>>  ui/gtk.c                   |  25 +--
>>  ui/sdl2-gl.c               |  10 +-
>>  ui/sdl2.c                  |  14 +-
>>  ui/spice-display.c         |  19 +-
>>  18 files changed, 498 insertions(+), 541 deletions(-)
>>
>> --
>> 2.32.0 (Apple Git-132)
>>
>>
>
>
> --
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] ui/dbus: Share one listener for a console
  2022-02-14 13:15   ` Akihiko Odaki
@ 2022-02-14 19:49     ` Marc-André Lureau
  2022-02-15  3:08       ` Akihiko Odaki
  0 siblings, 1 reply; 13+ messages in thread
From: Marc-André Lureau @ 2022-02-14 19:49 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: QEMU, Gerd Hoffmann

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

Hi

On Mon, Feb 14, 2022 at 5:15 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi Akihiko
> >
> > On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki <akihiko.odaki@gmail.com>
> wrote:
> >>
> >> ui/dbus required to have multiple DisplayChangeListeners (possibly with
> OpenGL)
> >> for a console but that caused several problems:
> >> - It broke egl-headless, an unusual display which implements OpenGL
> rendering
> >>   for non-OpenGL displays. The code to support multiple
> DisplayChangeListeners
> >>   does not consider the case where non-OpenGL displays listens OpenGL
> consoles.
> >
> >
> > Can you provide instructions on what broke? Even better write a test,
> please.
>
> The following command segfaults. Adding a test would be nice, but it
> would need a binary which uses OpenGL.
> qemu-system-x86_64 -device virtio-gpu-gl-pci -display egl-headless
> -vnc :0 -m 8G -cdrom Fedora-Workstation-Live-x86_64-34-1.2.iso -accel
> kvm
>

Thanks!

This is clearly a mistake from commit 7cc712e98 ("ui: dispatch GL events to
all listener").

It should have taken into account that some listeners do not have GL
callbacks, and guard the call.

We should wrap the missing ops->dpy_gl_call() with a if (ops->dpy_gl_call)
? I'll send a patch for that.

>
> > "make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py", which
> covers egl-headless usage, works.
> >
> >>
> >> - Multiple OpenGL DisplayChangeListeners of dbus shares one
> DisplaySurface and
> >>   modifies its texture field, causing OpenGL texture leak and
> use-after-free.
> >
> >
> > Again, please provide instructions. I have regularly run -display dbus
> with multiple clients and qemu compiled with sanitizers. I don't see any
> leak or use after free.
>
> I doubt sanitizers can find this because it is an OpenGL texture. You
> may add a probe around surface_gl_create_texture and
> surface_gl_destroy_texture.
>

Indeed, a surface is created on each frame, because we create a 2d surface
on each qemu_console_resize(), which is called at each virgl scanout. This
is a regression introduced by commit ebced09185 ("console: save current
scanout details"). I can propose an easy fix, please check it.

And the root of the leak is actually surface_gl_create_texutre(), it should
be idempotent, just like destroy().


> >
> >>
> >> - Introduced extra code to check the compatibility of OpenGL context
> providers
> >>   and OpenGL texture renderers where those are often inherently tightly
> coupled
> >>   since they must share the same hardware.
> >
> >
> > So code checks are meant to prevent misusage. They might be too limited
> or broken in some ways, but reverting is likely going to introduce other
> regressions I was trying to fix.
>
> The misuse will not occur because DisplayChangeListeners will be
> merged with OpenGL context providers.
>

Ok, but aren't the checks enough to prevent it already? I have to check the
use cases and differences of design, but you might be right that we don't
need such a split after all.


> >
> >> - Needed extra work to broadcast the same change to multiple dbus
> listeners.
> >>
> >
> > Compared to what?
>
> Compared to sharing one DisplayChangeListener for multiple dbus listeners.
>

Well, you just moved the problem to the dbus display, not removed any work.

What we have currently is more generic, you should be able to add/remove
various listeners (in theory, we only really do it for dbus at this point).


>
> >
> >>
> >> This series solve them by implementing the change broadcast in ui/dbus,
> which
> >> knows exactly what is needed. Changes for the common code to support
> multiple
> >> OpenGL DisplayChangeListeners were reverted to fix egl-headless and
> reduce
> >> the code size.
> >
> >
> > Thanks a lot for your work, I am going to take a look at your approach.
> But please help us understand better what the problem actually is, by
> giving examples & tests to avoid future regressions and document the
> expected behaviour.
>
> The thing is really complicated and I may miss details so please feel
> free to ask questions or provide suggestions.
>
>
Reverting changes and proposing an alternative implementation requires
detailed explanation and convincing arguments. It may take a while, but we
will try to get through the problems and evaluate the alternative designs.
Thanks again for your help!

Regards,
> Akihiko Odaki
>
>
> >
> >>
> >> Akihiko Odaki (6):
> >>   ui/dbus: Share one listener for a console
> >>   Revert "console: save current scanout details"
> >>   Revert "ui: split the GL context in a different object"
> >>   Revert "ui: dispatch GL events to all listeners"
> >>   Revert "ui: associate GL context outside of display listener
> >>     registration"
> >>   Revert "ui: factor out qemu_console_set_display_gl_ctx()"
> >>
> >>  include/ui/console.h       |  60 +-----
> >>  include/ui/egl-context.h   |   6 +-
> >>  include/ui/gtk.h           |  11 +-
> >>  include/ui/sdl2.h          |   7 +-
> >>  include/ui/spice-display.h |   1 -
> >>  ui/console.c               | 258 +++++++----------------
> >>  ui/dbus-console.c          | 109 ++--------
> >>  ui/dbus-listener.c         | 417 +++++++++++++++++++++++++++----------
> >>  ui/dbus.c                  |  22 --
> >>  ui/dbus.h                  |  36 +++-
> >>  ui/egl-context.c           |   6 +-
> >>  ui/egl-headless.c          |  20 +-
> >>  ui/gtk-egl.c               |  10 +-
> >>  ui/gtk-gl-area.c           |   8 +-
> >>  ui/gtk.c                   |  25 +--
> >>  ui/sdl2-gl.c               |  10 +-
> >>  ui/sdl2.c                  |  14 +-
> >>  ui/spice-display.c         |  19 +-
> >>  18 files changed, 498 insertions(+), 541 deletions(-)
> >>
> >> --
> >> 2.32.0 (Apple Git-132)
> >>
> >>
> >
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] ui/dbus: Share one listener for a console
  2022-02-14 19:49     ` Marc-André Lureau
@ 2022-02-15  3:08       ` Akihiko Odaki
  2022-02-15 13:32         ` Marc-André Lureau
  0 siblings, 1 reply; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-15  3:08 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU, Gerd Hoffmann



On 2022/02/15 4:49, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 14, 2022 at 5:15 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
>     <marcandre.lureau@gmail.com <mailto:marcandre.lureau@gmail.com>> wrote:
>      >
>      > Hi Akihiko
>      >
>      > On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>> wrote:
>      >>
>      >> ui/dbus required to have multiple DisplayChangeListeners
>     (possibly with OpenGL)
>      >> for a console but that caused several problems:
>      >> - It broke egl-headless, an unusual display which implements
>     OpenGL rendering
>      >>   for non-OpenGL displays. The code to support multiple
>     DisplayChangeListeners
>      >>   does not consider the case where non-OpenGL displays listens
>     OpenGL consoles.
>      >
>      >
>      > Can you provide instructions on what broke? Even better write a
>     test, please.
> 
>     The following command segfaults. Adding a test would be nice, but it
>     would need a binary which uses OpenGL.
>     qemu-system-x86_64 -device virtio-gpu-gl-pci -display egl-headless
>     -vnc :0 -m 8G -cdrom Fedora-Workstation-Live-x86_64-34-1.2.iso -accel
>     kvm
> 
> 
> Thanks!
> 
> This is clearly a mistake from commit 7cc712e98 ("ui: dispatch GL events 
> to all listener").
> 
> It should have taken into account that some listeners do not have GL 
> callbacks, and guard the call.
> 
> We should wrap the missing ops->dpy_gl_call() with a if 
> (ops->dpy_gl_call) ? I'll send a patch for that.


The assumption that OpenGL DisplayChangeListener and non-OpenGL 
DisplayChangeListener are exclusive is now broken so we have to examine 
if the whole patch series works correct without the assumption. Other 
problem I have found (and forgot to mention) is:
- 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 and
- that the compatibility check breaks if egl-headless is present and a 
non-OpenGL DisplayChangeListener with con field is being added.

By the way, dbus registers a DisplayChangeListener for the size 
detection, apparently it fails to set "con" field, making it an 
accidental user of console_select.

> 
>      >
>      > "make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py",
>     which covers egl-headless usage, works.
>      >
>      >>
>      >> - Multiple OpenGL DisplayChangeListeners of dbus shares one
>     DisplaySurface and
>      >>   modifies its texture field, causing OpenGL texture leak and
>     use-after-free.
>      >
>      >
>      > Again, please provide instructions. I have regularly run -display
>     dbus with multiple clients and qemu compiled with sanitizers. I
>     don't see any leak or use after free.
> 
>     I doubt sanitizers can find this because it is an OpenGL texture. You
>     may add a probe around surface_gl_create_texture and
>     surface_gl_destroy_texture.
> 
> 
> Indeed, a surface is created on each frame, because we create a 2d 
> surface on each qemu_console_resize(), which is called at each virgl 
> scanout. This is a regression introduced by commit ebced09185 ("console: 
> save current scanout details"). I can propose an easy fix, please check it.
> 
> And the root of the leak is actually surface_gl_create_texutre(), it 
> should be idempotent, just like destroy().

That is not enough since it may leave the texture present after 
unregister_displaychangelistener. And calling 
surface_gl_destroy_texture() before unregister_displaychangelistener may 
break the other listeners.

> 
> 
>      >
>      >>
>      >> - Introduced extra code to check the compatibility of OpenGL
>     context providers
>      >>   and OpenGL texture renderers where those are often inherently
>     tightly coupled
>      >>   since they must share the same hardware.
>      >
>      >
>      > So code checks are meant to prevent misusage. They might be too
>     limited or broken in some ways, but reverting is likely going to
>     introduce other regressions I was trying to fix.
> 
>     The misuse will not occur because DisplayChangeListeners will be
>     merged with OpenGL context providers.
> 
> 
> Ok, but aren't the checks enough to prevent it already? I have to check 
> the use cases and differences of design, but you might be right that we 
> don't need such a split after all.

Yes, the point is that it requires extra code.

> 
> 
>      >
>      >> - Needed extra work to broadcast the same change to multiple
>     dbus listeners.
>      >>
>      >
>      > Compared to what?
> 
>     Compared to sharing one DisplayChangeListener for multiple dbus
>     listeners.
> 
> 
> Well, you just moved the problem to the dbus display, not removed any work.
> 
> What we have currently is more generic, you should be able to add/remove 
> various listeners (in theory, we only really do it for dbus at this point).

The each DisplayChangeListeners have to upload the DisplaySurface to the 
graphics accelerator, create a DMA-BUF file descriptor, and make it 
suitable for D-Bus delivery. The duplicate work can be just done once if 
we have only one DisplayChangeListener for one console.

> 
> 
>      >
>      >>
>      >> This series solve them by implementing the change broadcast in
>     ui/dbus, which
>      >> knows exactly what is needed. Changes for the common code to
>     support multiple
>      >> OpenGL DisplayChangeListeners were reverted to fix egl-headless
>     and reduce
>      >> the code size.
>      >
>      >
>      > Thanks a lot for your work, I am going to take a look at your
>     approach. But please help us understand better what the problem
>     actually is, by giving examples & tests to avoid future regressions
>     and document the expected behaviour.
> 
>     The thing is really complicated and I may miss details so please feel
>     free to ask questions or provide suggestions.
> 
> 
> Reverting changes and proposing an alternative implementation requires 
> detailed explanation and convincing arguments. It may take a while, but 
> we will try to get through the problems and evaluate the alternative 
> designs. Thanks again for your help!

Rather, I think proposing a large change to common console code requires 
thorough examination and it should be reverted before it reaches the 
release if it is doubtful that it is correct and reduces the complexity 
of a few displays (possibly in the future). "No regression" should come 
first before fix and feature. We can always revisit the change land it 
in a proper form even after reverting the change.

Regards,
Akihiko Odaki

> 
>     Regards,
>     Akihiko Odaki
> 
> 
>      >
>      >>
>      >> Akihiko Odaki (6):
>      >>   ui/dbus: Share one listener for a console
>      >>   Revert "console: save current scanout details"
>      >>   Revert "ui: split the GL context in a different object"
>      >>   Revert "ui: dispatch GL events to all listeners"
>      >>   Revert "ui: associate GL context outside of display listener
>      >>     registration"
>      >>   Revert "ui: factor out qemu_console_set_display_gl_ctx()"
>      >>
>      >>  include/ui/console.h       |  60 +-----
>      >>  include/ui/egl-context.h   |   6 +-
>      >>  include/ui/gtk.h           |  11 +-
>      >>  include/ui/sdl2.h          |   7 +-
>      >>  include/ui/spice-display.h |   1 -
>      >>  ui/console.c               | 258 +++++++----------------
>      >>  ui/dbus-console.c          | 109 ++--------
>      >>  ui/dbus-listener.c         | 417
>     +++++++++++++++++++++++++++----------
>      >>  ui/dbus.c                  |  22 --
>      >>  ui/dbus.h                  |  36 +++-
>      >>  ui/egl-context.c           |   6 +-
>      >>  ui/egl-headless.c          |  20 +-
>      >>  ui/gtk-egl.c               |  10 +-
>      >>  ui/gtk-gl-area.c           |   8 +-
>      >>  ui/gtk.c                   |  25 +--
>      >>  ui/sdl2-gl.c               |  10 +-
>      >>  ui/sdl2.c                  |  14 +-
>      >>  ui/spice-display.c         |  19 +-
>      >>  18 files changed, 498 insertions(+), 541 deletions(-)
>      >>
>      >> --
>      >> 2.32.0 (Apple Git-132)
>      >>
>      >>
>      >
>      >
>      > --
>      > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] ui/dbus: Share one listener for a console
  2022-02-15  3:08       ` Akihiko Odaki
@ 2022-02-15 13:32         ` Marc-André Lureau
       [not found]           ` <CAMVc7JWnqucqKeEBDWFES8JYY77gmbMaX-inz+CSzd-bymr5cQ@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Marc-André Lureau @ 2022-02-15 13:32 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: QEMU, Gerd Hoffmann

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

Hi

On Tue, Feb 15, 2022 at 7:09 AM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

>
>
> On 2022/02/15 4:49, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Feb 14, 2022 at 5:15 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >     On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
> >     <marcandre.lureau@gmail.com <mailto:marcandre.lureau@gmail.com>>
> wrote:
> >      >
> >      > Hi Akihiko
> >      >
> >      > On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki
> >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>> wrote:
> >      >>
> >      >> ui/dbus required to have multiple DisplayChangeListeners
> >     (possibly with OpenGL)
> >      >> for a console but that caused several problems:
> >      >> - It broke egl-headless, an unusual display which implements
> >     OpenGL rendering
> >      >>   for non-OpenGL displays. The code to support multiple
> >     DisplayChangeListeners
> >      >>   does not consider the case where non-OpenGL displays listens
> >     OpenGL consoles.
> >      >
> >      >
> >      > Can you provide instructions on what broke? Even better write a
> >     test, please.
> >
> >     The following command segfaults. Adding a test would be nice, but it
> >     would need a binary which uses OpenGL.
> >     qemu-system-x86_64 -device virtio-gpu-gl-pci -display egl-headless
> >     -vnc :0 -m 8G -cdrom Fedora-Workstation-Live-x86_64-34-1.2.iso -accel
> >     kvm
> >
> >
> > Thanks!
> >
> > This is clearly a mistake from commit 7cc712e98 ("ui: dispatch GL events
> > to all listener").
> >
> > It should have taken into account that some listeners do not have GL
> > callbacks, and guard the call.
> >
> > We should wrap the missing ops->dpy_gl_call() with a if
> > (ops->dpy_gl_call) ? I'll send a patch for that.
>
>
> The assumption that OpenGL DisplayChangeListener and non-OpenGL
> DisplayChangeListener are exclusive is now broken so we have to examine
>

Before the changes, there was already such a GL & non-GL listener mixed
situation. It's only because the first listener with GL would be registered
in register_displaychangelistener() that it "worked".

if the whole patch series works correct without the assumption. Other
> problem I have found (and forgot to mention) is:
> - 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 and
>

Can you translate that to a reproducible test with expected outcome?


> - that the compatibility check breaks if egl-headless is present and a
> non-OpenGL DisplayChangeListener with con field is being added.
>
>
Same, thanks


> By the way, dbus registers a DisplayChangeListener for the size
> detection, apparently it fails to set "con" field, making it an
> accidental user of console_select.
>

I need more details, please.


>
> >
> >      >
> >      > "make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py",
> >     which covers egl-headless usage, works.
> >      >
> >      >>
> >      >> - Multiple OpenGL DisplayChangeListeners of dbus shares one
> >     DisplaySurface and
> >      >>   modifies its texture field, causing OpenGL texture leak and
> >     use-after-free.
> >      >
> >      >
> >      > Again, please provide instructions. I have regularly run -display
> >     dbus with multiple clients and qemu compiled with sanitizers. I
> >     don't see any leak or use after free.
> >
> >     I doubt sanitizers can find this because it is an OpenGL texture. You
> >     may add a probe around surface_gl_create_texture and
> >     surface_gl_destroy_texture.
> >
> >
> > Indeed, a surface is created on each frame, because we create a 2d
> > surface on each qemu_console_resize(), which is called at each virgl
> > scanout. This is a regression introduced by commit ebced09185 ("console:
> > save current scanout details"). I can propose an easy fix, please check
> it.
> >
> > And the root of the leak is actually surface_gl_create_texutre(), it
> > should be idempotent, just like destroy().
>
> That is not enough since it may leave the texture present after
> unregister_displaychangelistener. And calling
> surface_gl_destroy_texture() before unregister_displaychangelistener may
> break the other listeners.
>

The texture is shared, but owned by the console. It is not leaked.

However, given that it is shared, it would be indeed better to refcount the
users to avoid destroying the texture by one listener going away.


> >
> >
> >      >
> >      >>
> >      >> - Introduced extra code to check the compatibility of OpenGL
> >     context providers
> >      >>   and OpenGL texture renderers where those are often inherently
> >     tightly coupled
> >      >>   since they must share the same hardware.
> >      >
> >      >
> >      > So code checks are meant to prevent misusage. They might be too
> >     limited or broken in some ways, but reverting is likely going to
> >     introduce other regressions I was trying to fix.
> >
> >     The misuse will not occur because DisplayChangeListeners will be
> >     merged with OpenGL context providers.
> >
> >
> > Ok, but aren't the checks enough to prevent it already? I have to check
> > the use cases and differences of design, but you might be right that we
> > don't need such a split after all.
>
> Yes, the point is that it requires extra code.
>
> >
> >
> >      >
> >      >> - Needed extra work to broadcast the same change to multiple
> >     dbus listeners.
> >      >>
> >      >
> >      > Compared to what?
> >
> >     Compared to sharing one DisplayChangeListener for multiple dbus
> >     listeners.
> >
> >
> > Well, you just moved the problem to the dbus display, not removed any
> work.
> >
> > What we have currently is more generic, you should be able to add/remove
> > various listeners (in theory, we only really do it for dbus at this
> point).
>
> The each DisplayChangeListeners have to upload the DisplaySurface to the
> graphics accelerator, create a DMA-BUF file descriptor, and make it
> suitable for D-Bus delivery. The duplicate work can be just done once if
> we have only one DisplayChangeListener for one console.
>

And we should avoid duplicating the texture/resources if possible. This is
not specific to DBus, it's the reason why I would rather have the logic in
the console code so we avoid code duplication and we can do further
improvement at the lower-level.


>
> >
> >
> >      >
> >      >>
> >      >> This series solve them by implementing the change broadcast in
> >     ui/dbus, which
> >      >> knows exactly what is needed. Changes for the common code to
> >     support multiple
> >      >> OpenGL DisplayChangeListeners were reverted to fix egl-headless
> >     and reduce
> >      >> the code size.
> >      >
> >      >
> >      > Thanks a lot for your work, I am going to take a look at your
> >     approach. But please help us understand better what the problem
> >     actually is, by giving examples & tests to avoid future regressions
> >     and document the expected behaviour.
> >
> >     The thing is really complicated and I may miss details so please feel
> >     free to ask questions or provide suggestions.
> >
> >
> > Reverting changes and proposing an alternative implementation requires
> > detailed explanation and convincing arguments. It may take a while, but
> > we will try to get through the problems and evaluate the alternative
> > designs. Thanks again for your help!
>
> Rather, I think proposing a large change to common console code requires
> thorough examination and it should be reverted before it reaches the
> release if it is doubtful that it is correct and reduces the complexity
> of a few displays (possibly in the future). "No regression" should come
> first before fix and feature. We can always revisit the change land it
> in a proper form even after reverting the change.
>
>
That's not how we usually work, there was a long RFC&PATCH review period
during which testing was done. It's true that Gerd, the maintainer, didn't
do a thorough review though. If we have to revert, we can do it before the
release. However, I would rather fix the current design since it is meant
to be more generic & flexible. We still have some time.

Thanks again for your help

Regards,
> Akihiko Odaki
>
> >
> >     Regards,
> >     Akihiko Odaki
> >
> >
> >      >
> >      >>
> >      >> Akihiko Odaki (6):
> >      >>   ui/dbus: Share one listener for a console
> >      >>   Revert "console: save current scanout details"
> >      >>   Revert "ui: split the GL context in a different object"
> >      >>   Revert "ui: dispatch GL events to all listeners"
> >      >>   Revert "ui: associate GL context outside of display listener
> >      >>     registration"
> >      >>   Revert "ui: factor out qemu_console_set_display_gl_ctx()"
> >      >>
> >      >>  include/ui/console.h       |  60 +-----
> >      >>  include/ui/egl-context.h   |   6 +-
> >      >>  include/ui/gtk.h           |  11 +-
> >      >>  include/ui/sdl2.h          |   7 +-
> >      >>  include/ui/spice-display.h |   1 -
> >      >>  ui/console.c               | 258 +++++++----------------
> >      >>  ui/dbus-console.c          | 109 ++--------
> >      >>  ui/dbus-listener.c         | 417
> >     +++++++++++++++++++++++++++----------
> >      >>  ui/dbus.c                  |  22 --
> >      >>  ui/dbus.h                  |  36 +++-
> >      >>  ui/egl-context.c           |   6 +-
> >      >>  ui/egl-headless.c          |  20 +-
> >      >>  ui/gtk-egl.c               |  10 +-
> >      >>  ui/gtk-gl-area.c           |   8 +-
> >      >>  ui/gtk.c                   |  25 +--
> >      >>  ui/sdl2-gl.c               |  10 +-
> >      >>  ui/sdl2.c                  |  14 +-
> >      >>  ui/spice-display.c         |  19 +-
> >      >>  18 files changed, 498 insertions(+), 541 deletions(-)
> >      >>
> >      >> --
> >      >> 2.32.0 (Apple Git-132)
> >      >>
> >      >>
> >      >
> >      >
> >      > --
> >      > Marc-André Lureau
> >
> >
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/6] ui/dbus: Share one listener for a console
       [not found]             ` <CAJ+F1CJ1ZZV-dv6kHhYmAkg5Pnrb_-q7o3Bndo6cQZsYN+RkgA@mail.gmail.com>
@ 2022-02-16 17:05               ` Akihiko Odaki
  0 siblings, 0 replies; 13+ messages in thread
From: Akihiko Odaki @ 2022-02-16 17:05 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, Gerd Hoffmann


On 2022/02/17 1:51, Marc-André Lureau wrote:
> Hi Akihiko,
> 
> (I suppose you meant to reply-all, feel free to add back qemu-devel)

My bad, restored Ccs.

> 
> On Tue, Feb 15, 2022 at 5:56 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On Tue, Feb 15, 2022 at 10:32 PM Marc-André Lureau
>     <marcandre.lureau@gmail.com <mailto:marcandre.lureau@gmail.com>> wrote:
>      >
>      > Hi
>      >
>      > On Tue, Feb 15, 2022 at 7:09 AM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>> wrote:
>      >>
>      >>
>      >>
>      >> On 2022/02/15 4:49, Marc-André Lureau wrote:
>      >> > Hi
>      >> >
>      >> > On Mon, Feb 14, 2022 at 5:15 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      >> > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >> >
>      >> >     On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
>      >> >     <marcandre.lureau@gmail.com
>     <mailto:marcandre.lureau@gmail.com>
>     <mailto:marcandre.lureau@gmail.com
>     <mailto:marcandre.lureau@gmail.com>>> wrote:
>      >> >      >
>      >> >      > Hi Akihiko
>      >> >      >
>      >> >      > On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki
>      >> >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>>
>     wrote:
>      >> >      >>
>      >> >      >> ui/dbus required to have multiple DisplayChangeListeners
>      >> >     (possibly with OpenGL)
>      >> >      >> for a console but that caused several problems:
>      >> >      >> - It broke egl-headless, an unusual display which
>     implements
>      >> >     OpenGL rendering
>      >> >      >>   for non-OpenGL displays. The code to support multiple
>      >> >     DisplayChangeListeners
>      >> >      >>   does not consider the case where non-OpenGL displays
>     listens
>      >> >     OpenGL consoles.
>      >> >      >
>      >> >      >
>      >> >      > Can you provide instructions on what broke? Even better
>     write a
>      >> >     test, please.
>      >> >
>      >> >     The following command segfaults. Adding a test would be
>     nice, but it
>      >> >     would need a binary which uses OpenGL.
>      >> >     qemu-system-x86_64 -device virtio-gpu-gl-pci -display
>     egl-headless
>      >> >     -vnc :0 -m 8G -cdrom
>     Fedora-Workstation-Live-x86_64-34-1.2.iso -accel
>      >> >     kvm
>      >> >
>      >> >
>      >> > Thanks!
>      >> >
>      >> > This is clearly a mistake from commit 7cc712e98 ("ui: dispatch
>     GL events
>      >> > to all listener").
>      >> >
>      >> > It should have taken into account that some listeners do not
>     have GL
>      >> > callbacks, and guard the call.
>      >> >
>      >> > We should wrap the missing ops->dpy_gl_call() with a if
>      >> > (ops->dpy_gl_call) ? I'll send a patch for that.
>      >>
>      >>
>      >> The assumption that OpenGL DisplayChangeListener and non-OpenGL
>      >> DisplayChangeListener are exclusive is now broken so we have to
>     examine
>      >
>      >
>      > Before the changes, there was already such a GL & non-GL listener
>     mixed situation. It's only because the first listener with GL would
>     be registered in register_displaychangelistener() that it "worked".
> 
>     I mean the dbus patch series should be reexamined since it does not
>     seem to consider the case.
> 
> 
> That's what I am doing, with your very good help.
> 
> 
>      >
>      >> if the whole patch series works correct without the assumption.
>     Other
>      >> problem I have found (and forgot to mention) is:
>      >> - 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 and
>      >
>      >
>      > Can you translate that to a reproducible test with expected outcome?
> 
>     vnc has the feature to switch consoles with Ctrl+Alt+[1-9] if it is
>     not bound to a particular console. Invoking the functionality with
>     egl-headless enabled would trigger the problem.
> 
> 
> That doesn't seem to happen after the fixes ([PATCH 0/3] GL console 
> related fixes) I posted.
> 
>      >
>      >>
>      >> - that the compatibility check breaks if egl-headless is present
>     and a
>      >> non-OpenGL DisplayChangeListener with con field is being added.
>      >>
>      >
>      > Same, thanks
> 
>     The following command should do:
>     qemu-system-x86_64 -device virtio-gpu-gl-pci,id=d -display
>     egl-headless -vnc :0,display=d -m 8G -cdrom
>     Fedora-Workstation-Live-x86_64-34-1.2.iso -accel kvm
> 
> 
> Thanks, that helps! I didn't realize DCL had "dynamic" consoles and the 
> compatibility checks were bypassed without specifying a specific display=d.
> 
> I'll address this in the v2 of my patch fixes series. Please check it.
> 
>      >
>      >>
>      >> By the way, dbus registers a DisplayChangeListener for the size
>      >> detection, apparently it fails to set "con" field, making it an
>      >> accidental user of console_select.
>      >
>      >
>      > I need more details, please.
> 
>     A DisplayChangeListener without con field set is passed to
>     register_displaychangelistener in dbus_display_console_new.
> 
> 
> Good catch, should be fixed too.
> 
> 
>      >
>      >>
>      >>
>      >> >
>      >> >      >
>      >> >      > "make check-avocado
>     AVOCADO_TESTS=tests/avocado/virtio-gpu.py",
>      >> >     which covers egl-headless usage, works.
>      >> >      >
>      >> >      >>
>      >> >      >> - Multiple OpenGL DisplayChangeListeners of dbus
>     shares one
>      >> >     DisplaySurface and
>      >> >      >>   modifies its texture field, causing OpenGL texture
>     leak and
>      >> >     use-after-free.
>      >> >      >
>      >> >      >
>      >> >      > Again, please provide instructions. I have regularly
>     run -display
>      >> >     dbus with multiple clients and qemu compiled with
>     sanitizers. I
>      >> >     don't see any leak or use after free.
>      >> >
>      >> >     I doubt sanitizers can find this because it is an OpenGL
>     texture. You
>      >> >     may add a probe around surface_gl_create_texture and
>      >> >     surface_gl_destroy_texture.
>      >> >
>      >> >
>      >> > Indeed, a surface is created on each frame, because we create a 2d
>      >> > surface on each qemu_console_resize(), which is called at each
>     virgl
>      >> > scanout. This is a regression introduced by commit ebced09185
>     ("console:
>      >> > save current scanout details"). I can propose an easy fix,
>     please check it.
>      >> >
>      >> > And the root of the leak is actually
>     surface_gl_create_texutre(), it
>      >> > should be idempotent, just like destroy().
>      >>
>      >> That is not enough since it may leave the texture present after
>      >> unregister_displaychangelistener. And calling
>      >> surface_gl_destroy_texture() before
>     unregister_displaychangelistener may
>      >> break the other listeners.
>      >
>      >
>      > The texture is shared, but owned by the console. It is not leaked.
>      >
>      > However, given that it is shared, it would be indeed better to
>     refcount the users to avoid destroying the texture by one listener
>     going away.
> 
>     Nobody destroys the texture in the case so it is a leak. Particularly
>     after dpy_gfx_replace_surface, even the reference which remains in
>     DisplaySurface will be gone.
> 
> 
>   We need a QemuConsole finalizer to call qemu_free_displaysurface(). 
> The UI code needs to evolve to become more robust in dynamic situations. 
> Let's do that.

QemuConsole gets never gone (if it does it would also break other 
displays), so adding a code to call surface_gl_destroy_texutre before 
unregister_displaychangelistener should be enough. However, doing so 
would break the other OpenGL DisplayChangeListeners of the same console 
if they exist.

Allowing to add/remove QemuConsole would be nice for hotplugging and it 
should also make the code clear overall by being more explicit about the 
lifetime of the resources, but that needs to update most (probably all) 
displays.

> 
> 
>      >
>      >>
>      >> >
>      >> >
>      >> >      >
>      >> >      >>
>      >> >      >> - Introduced extra code to check the compatibility of
>     OpenGL
>      >> >     context providers
>      >> >      >>   and OpenGL texture renderers where those are often
>     inherently
>      >> >     tightly coupled
>      >> >      >>   since they must share the same hardware.
>      >> >      >
>      >> >      >
>      >> >      > So code checks are meant to prevent misusage. They
>     might be too
>      >> >     limited or broken in some ways, but reverting is likely
>     going to
>      >> >     introduce other regressions I was trying to fix.
>      >> >
>      >> >     The misuse will not occur because DisplayChangeListeners
>     will be
>      >> >     merged with OpenGL context providers.
>      >> >
>      >> >
>      >> > Ok, but aren't the checks enough to prevent it already? I have
>     to check
>      >> > the use cases and differences of design, but you might be
>     right that we
>      >> > don't need such a split after all.
>      >>
>      >> Yes, the point is that it requires extra code.
>      >>
>      >> >
>      >> >
>      >> >      >
>      >> >      >> - Needed extra work to broadcast the same change to
>     multiple
>      >> >     dbus listeners.
>      >> >      >>
>      >> >      >
>      >> >      > Compared to what?
>      >> >
>      >> >     Compared to sharing one DisplayChangeListener for multiple
>     dbus
>      >> >     listeners.
>      >> >
>      >> >
>      >> > Well, you just moved the problem to the dbus display, not
>     removed any work.
>      >> >
>      >> > What we have currently is more generic, you should be able to
>     add/remove
>      >> > various listeners (in theory, we only really do it for dbus at
>     this point).
>      >>
>      >> The each DisplayChangeListeners have to upload the
>     DisplaySurface to the
>      >> graphics accelerator, create a DMA-BUF file descriptor, and make it
>      >> suitable for D-Bus delivery. The duplicate work can be just done
>     once if
>      >> we have only one DisplayChangeListener for one console.
>      >
>      >
>      > And we should avoid duplicating the texture/resources if
>     possible. This is not specific to DBus, it's the reason why I would
>     rather have the logic in the console code so we avoid code
>     duplication and we can do further improvement at the lower-level.
> 
>     The resources created are specific to D-Bus. Not all displays have
>     GUnixFDList, DMA-BUF file descriptors and OpenGL textures.
> 
> 
>   GUnixFDList is in dbus code. dmabuf fd & opengl textures are not 
> specific to dbus, they are used by spice as well, for example.
> 

You have to consider about the case where DMA-BUF is not used or OpenGL 
is not enabled if we try to remove the duplicate work in the common 
code. It would result in much less code if you reuse GUnixFDList in 
dbus, and that is what this patch series do.

> 
>     Regards,
>     Akihiko Odaki
> 
>      >
>      >>
>      >>
>      >> >
>      >> >
>      >> >      >
>      >> >      >>
>      >> >      >> This series solve them by implementing the change
>     broadcast in
>      >> >     ui/dbus, which
>      >> >      >> knows exactly what is needed. Changes for the common
>     code to
>      >> >     support multiple
>      >> >      >> OpenGL DisplayChangeListeners were reverted to fix
>     egl-headless
>      >> >     and reduce
>      >> >      >> the code size.
>      >> >      >
>      >> >      >
>      >> >      > Thanks a lot for your work, I am going to take a look
>     at your
>      >> >     approach. But please help us understand better what the
>     problem
>      >> >     actually is, by giving examples & tests to avoid future
>     regressions
>      >> >     and document the expected behaviour.
>      >> >
>      >> >     The thing is really complicated and I may miss details so
>     please feel
>      >> >     free to ask questions or provide suggestions.
>      >> >
>      >> >
>      >> > Reverting changes and proposing an alternative implementation
>     requires
>      >> > detailed explanation and convincing arguments. It may take a
>     while, but
>      >> > we will try to get through the problems and evaluate the
>     alternative
>      >> > designs. Thanks again for your help!
>      >>
>      >> Rather, I think proposing a large change to common console code
>     requires
>      >> thorough examination and it should be reverted before it reaches the
>      >> release if it is doubtful that it is correct and reduces the
>     complexity
>      >> of a few displays (possibly in the future). "No regression"
>     should come
>      >> first before fix and feature. We can always revisit the change
>     land it
>      >> in a proper form even after reverting the change.
>      >>
>      >
>      > That's not how we usually work, there was a long RFC&PATCH review
>     period during which testing was done. It's true that Gerd, the
>     maintainer, didn't do a thorough review though. If we have to
>     revert, we can do it before the release. However, I would rather fix
>     the current design since it is meant to be more generic & flexible.
>     We still have some time.
>      >
>      > Thanks again for your help
>      >
>      >> Regards,
>      >> Akihiko Odaki
>      >>
>      >> >
>      >> >     Regards,
>      >> >     Akihiko Odaki
>      >> >
>      >> >
>      >> >      >
>      >> >      >>
>      >> >      >> Akihiko Odaki (6):
>      >> >      >>   ui/dbus: Share one listener for a console
>      >> >      >>   Revert "console: save current scanout details"
>      >> >      >>   Revert "ui: split the GL context in a different object"
>      >> >      >>   Revert "ui: dispatch GL events to all listeners"
>      >> >      >>   Revert "ui: associate GL context outside of display
>     listener
>      >> >      >>     registration"
>      >> >      >>   Revert "ui: factor out
>     qemu_console_set_display_gl_ctx()"
>      >> >      >>
>      >> >      >>  include/ui/console.h       |  60 +-----
>      >> >      >>  include/ui/egl-context.h   |   6 +-
>      >> >      >>  include/ui/gtk.h           |  11 +-
>      >> >      >>  include/ui/sdl2.h          |   7 +-
>      >> >      >>  include/ui/spice-display.h |   1 -
>      >> >      >>  ui/console.c               | 258 +++++++----------------
>      >> >      >>  ui/dbus-console.c          | 109 ++--------
>      >> >      >>  ui/dbus-listener.c         | 417
>      >> >     +++++++++++++++++++++++++++----------
>      >> >      >>  ui/dbus.c                  |  22 --
>      >> >      >>  ui/dbus.h                  |  36 +++-
>      >> >      >>  ui/egl-context.c           |   6 +-
>      >> >      >>  ui/egl-headless.c          |  20 +-
>      >> >      >>  ui/gtk-egl.c               |  10 +-
>      >> >      >>  ui/gtk-gl-area.c           |   8 +-
>      >> >      >>  ui/gtk.c                   |  25 +--
>      >> >      >>  ui/sdl2-gl.c               |  10 +-
>      >> >      >>  ui/sdl2.c                  |  14 +-
>      >> >      >>  ui/spice-display.c         |  19 +-
>      >> >      >>  18 files changed, 498 insertions(+), 541 deletions(-)
>      >> >      >>
>      >> >      >> --
>      >> >      >> 2.32.0 (Apple Git-132)
>      >> >      >>
>      >> >      >>
>      >> >      >
>      >> >      >
>      >> >      > --
>      >> >      > Marc-André Lureau
>      >> >
>      >> >
>      >> >
>      >> > --
>      >> > Marc-André Lureau
>      >
>      >
>      >
>      > --
>      > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-02-16 17:27 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-13  2:42 [PATCH 0/6] ui/dbus: Share one listener for a console Akihiko Odaki
2022-02-13  2:42 ` [PATCH 1/6] " Akihiko Odaki
2022-02-13  2:42 ` [PATCH 2/6] Revert "console: save current scanout details" Akihiko Odaki
2022-02-13  2:42 ` [PATCH 3/6] Revert "ui: split the GL context in a different object" Akihiko Odaki
2022-02-13  2:42 ` [PATCH 4/6] Revert "ui: dispatch GL events to all listeners" Akihiko Odaki
2022-02-13  2:42 ` [PATCH 5/6] Revert "ui: associate GL context outside of display listener registration" Akihiko Odaki
2022-02-13  2:42 ` [PATCH 6/6] Revert "ui: factor out qemu_console_set_display_gl_ctx()" Akihiko Odaki
2022-02-14 12:06 ` [PATCH 0/6] ui/dbus: Share one listener for a console Marc-André Lureau
2022-02-14 13:15   ` Akihiko Odaki
2022-02-14 19:49     ` Marc-André Lureau
2022-02-15  3:08       ` Akihiko Odaki
2022-02-15 13:32         ` Marc-André Lureau
     [not found]           ` <CAMVc7JWnqucqKeEBDWFES8JYY77gmbMaX-inz+CSzd-bymr5cQ@mail.gmail.com>
     [not found]             ` <CAJ+F1CJ1ZZV-dv6kHhYmAkg5Pnrb_-q7o3Bndo6cQZsYN+RkgA@mail.gmail.com>
2022-02-16 17:05               ` Akihiko Odaki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.