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

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

[marcandre.lureau]

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

Hi,

Here are pending fixes related to D-Bus and GL, most of them reported thanks to
Akihiko Odaki.

v3:
 - rebased
 - add "ui/dbus: do not send 2d scanout until gfx_update" & "ui/console: call
   gfx_switch() even if the current scanout is GL"

Marc-André Lureau (12):
  ui/console: move check for compatible GL context
  ui/console: move dcl compatiblity check to a callback
  ui/console: egl-headless is compatible with non-gl listeners
  ui/dbus: associate the DBusDisplayConsole listener with the given
    console
  ui/console: move console compatibility check to dcl_display_console()
  ui/shader: fix potential leak of shader on error
  ui/shader: free associated programs
  ui/console: add a dpy_gfx_switch callback helper
  ui/console: optionally update after gfx switch
  ui/dbus: fix texture sharing
  ui/dbus: do not send 2d scanout until gfx_update
  ui/console: call gfx_switch() even if the current scanout is GL

 include/ui/console.h |  19 +++++---
 ui/dbus.h            |   3 ++
 ui/console.c         | 102 ++++++++++++++++++++++++++-----------------
 ui/dbus-console.c    |  27 ++++++------
 ui/dbus-listener.c   |  48 +++++++++-----------
 ui/dbus.c            |  35 ++++++++++++++-
 ui/egl-headless.c    |  17 +++++++-
 ui/gtk.c             |  18 +++++++-
 ui/sdl2.c            |   9 +++-
 ui/shader.c          |   9 +++-
 ui/spice-display.c   |   9 +++-
 11 files changed, 200 insertions(+), 96 deletions(-)

-- 
2.35.1.273.ge6ebfd0e8cbb

[PATCH v3 01/12] ui/console: move check for compatible GL context

[marcandre.lureau]

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

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

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

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

[PATCH v3 02/12] ui/console: move dcl compatiblity check to a callback

[marcandre.lureau]

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

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

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

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

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

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

[marcandre.lureau]

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

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

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

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

[PATCH v3 04/12] ui/dbus: associate the DBusDisplayConsole listener with the given console

[marcandre.lureau]

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

DBusDisplayConsole is specific to a given QemuConsole.

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

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

[PATCH v3 05/12] ui/console: move console compatibility check to dcl_display_console()

[marcandre.lureau]

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

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

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

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

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

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

[PATCH v3 06/12] ui/shader: fix potential leak of shader on error

[marcandre.lureau]

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

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

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

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

[PATCH v3 07/12] ui/shader: free associated programs

[marcandre.lureau]

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

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

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

[PATCH v3 08/12] ui/console: add a dpy_gfx_switch callback helper

[marcandre.lureau]

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

Slight code improvement.

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

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

[PATCH v3 09/12] ui/console: optionally update after gfx switch

[marcandre.lureau]

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

When switching to the dummy surface, we should also call gfx_update.
But when using GL, we shouldn't call it.

By making it an argument to displaychangelistener_gfx_switch(), it will
be explicit, and cannot be forgotten that easily.

Fixes: commit ebced091 ("console: save current scanout details")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 102fcf0a5068..06ba82db61c9 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1059,11 +1059,18 @@ static void console_putchar(QemuConsole *s, int ch)
 }
 
 static void displaychangelistener_gfx_switch(DisplayChangeListener *dcl,
-                                             struct DisplaySurface *new_surface)
+                                             struct DisplaySurface *new_surface,
+                                             bool update)
 {
     if (dcl->ops->dpy_gfx_switch) {
         dcl->ops->dpy_gfx_switch(dcl, new_surface);
     }
+
+    if (update && dcl->ops->dpy_gfx_update) {
+        dcl->ops->dpy_gfx_update(dcl, 0, 0,
+                                 surface_width(new_surface),
+                                 surface_height(new_surface));
+    }
 }
 
 
@@ -1079,7 +1086,7 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
         if (!dummy) {
             dummy = qemu_create_placeholder_surface(640, 480, nodev);
         }
-        displaychangelistener_gfx_switch(dcl, dummy);
+        displaychangelistener_gfx_switch(dcl, dummy, TRUE);
         return;
     }
 
@@ -1098,12 +1105,8 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
                                          con->scanout.texture.width,
                                          con->scanout.texture.height);
     } else if (con->scanout.kind == SCANOUT_SURFACE) {
-        displaychangelistener_gfx_switch(dcl, con->surface);
+        displaychangelistener_gfx_switch(dcl, con->surface, TRUE);
     }
-
-    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)
@@ -1682,7 +1685,7 @@ void dpy_gfx_replace_surface(QemuConsole *con,
         if (con != (dcl->con ? dcl->con : active_console)) {
             continue;
         }
-        displaychangelistener_gfx_switch(dcl, surface);
+        displaychangelistener_gfx_switch(dcl, surface, FALSE);
     }
     qemu_free_displaysurface(old_surface);
 }
-- 
2.35.1.273.ge6ebfd0e8cbb

[PATCH v3 10/12] ui/dbus: fix texture sharing

[marcandre.lureau]

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

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

Teach DisplayGLCtx to do optionally shared texture handling. This is
only implemented for DBus display at this point, however the same
infrastructure could potentially be used for other future combinations.

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

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

[PATCH v3 11/12] ui/dbus: do not send 2d scanout until gfx_update

[marcandre.lureau]

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

gfx_switch() is called to set the new_surface, not necessarily to
display it. It should be displayed after gfx_update(). Send the whole
scanout only in this case.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index a287edd2fc15..f9fc8eda519a 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -255,6 +255,26 @@ static void dbus_gfx_update(DisplayChangeListener *dcl,
 
     trace_dbus_update(x, y, w, h);
 
+    if (x == 0 && y == 0 && w == surface_width(ddl->ds) && h == surface_height(ddl->ds)) {
+        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(
+            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);
+        return;
+    }
+
     /* make a copy, since gvariant only handles linear data */
     img = pixman_image_create_bits(surface_format(ddl->ds),
                                    w, h, NULL, stride);
@@ -295,29 +315,12 @@ static void dbus_gfx_switch(DisplayChangeListener *dcl,
                             struct DisplaySurface *new_surface)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
-    GVariant *v_data = NULL;
 
     ddl->ds = new_surface;
     if (!ddl->ds) {
         /* why not call disable instead? */
         return;
     }
-
-    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(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);
 }
 
 static void dbus_mouse_set(DisplayChangeListener *dcl,
-- 
2.35.1.273.ge6ebfd0e8cbb

[PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[marcandre.lureau]

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

egl-headless depends on the backing surface to be set before texture are
set and updated. Display it (update=true) iff the current scanout kind
is SURFACE.

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

diff --git a/ui/console.c b/ui/console.c
index 5bfecea4549e..16a0b0909ba2 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1112,6 +1112,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
         return;
     }
 
+    dpy_gfx_create_texture(con, con->surface);
+    displaychangelistener_gfx_switch(dcl, con->surface,
+                                     con->scanout.kind == SCANOUT_SURFACE);
+
     if (con->scanout.kind == SCANOUT_DMABUF &&
         displaychangelistener_has_dmabuf(dcl)) {
         dcl->ops->dpy_gl_scanout_dmabuf(dcl, con->scanout.dmabuf);
@@ -1126,9 +1130,6 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
                                          con->scanout.texture.y,
                                          con->scanout.texture.width,
                                          con->scanout.texture.height);
-    } else if (con->scanout.kind == SCANOUT_SURFACE) {
-        dpy_gfx_create_texture(con, con->surface);
-        displaychangelistener_gfx_switch(dcl, con->surface, TRUE);
     }
 }
 
-- 
2.35.1.273.ge6ebfd0e8cbb

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> egl-headless depends on the backing surface to be set before texture are
> set and updated. Display it (update=true) iff the current scanout kind
> is SURFACE.

egl-headless does not dynamically call register_displaychangelistener 
and has console associated (console_select would not affect egl-headless 
itself) so this should not be necessary.

The remaining problem with egl-headless is that egl-headless renders the 
image to DisplaySurface, and a non-OpenGL display (namely vnc) has to 
consume it instead of texture even when con->scanout.kind is 
SCANOUT_TEXTURE or SCANOUT_DMABUF.

Regards,
Akihiko Odaki

> 
> Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/console.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/console.c b/ui/console.c
> index 5bfecea4549e..16a0b0909ba2 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1112,6 +1112,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>           return;
>       }
>   
> +    dpy_gfx_create_texture(con, con->surface);
> +    displaychangelistener_gfx_switch(dcl, con->surface,
> +                                     con->scanout.kind == SCANOUT_SURFACE);
> +
>       if (con->scanout.kind == SCANOUT_DMABUF &&
>           displaychangelistener_has_dmabuf(dcl)) {
>           dcl->ops->dpy_gl_scanout_dmabuf(dcl, con->scanout.dmabuf);
> @@ -1126,9 +1130,6 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>                                            con->scanout.texture.y,
>                                            con->scanout.texture.width,
>                                            con->scanout.texture.height);
> -    } else if (con->scanout.kind == SCANOUT_SURFACE) {
> -        dpy_gfx_create_texture(con, con->surface);
> -        displaychangelistener_gfx_switch(dcl, con->surface, TRUE);
>       }
>   }
>  

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

Hi Akihiko

On Mon, Mar 7, 2022 at 12:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > egl-headless depends on the backing surface to be set before texture are
> > set and updated. Display it (update=true) iff the current scanout kind
> > is SURFACE.
>
> egl-headless does not dynamically call register_displaychangelistener
> and has console associated (console_select would not affect egl-headless
> itself) so this should not be necessary.

Could you help me understand, what should not be necessary?

> The remaining problem with egl-headless is that egl-headless renders the
> image to DisplaySurface, and a non-OpenGL display (namely vnc) has to
> consume it instead of texture even when con->scanout.kind is
> SCANOUT_TEXTURE or SCANOUT_DMABUF.

This is already happening, because egl-headless calls dpy_gfx_update().

thanks

> Regards,
> Akihiko Odaki
>
> >
> > Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   ui/console.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/ui/console.c b/ui/console.c
> > index 5bfecea4549e..16a0b0909ba2 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -1112,6 +1112,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
> >           return;
> >       }
> >
> > +    dpy_gfx_create_texture(con, con->surface);
> > +    displaychangelistener_gfx_switch(dcl, con->surface,
> > +                                     con->scanout.kind == SCANOUT_SURFACE);
> > +
> >       if (con->scanout.kind == SCANOUT_DMABUF &&
> >           displaychangelistener_has_dmabuf(dcl)) {
> >           dcl->ops->dpy_gl_scanout_dmabuf(dcl, con->scanout.dmabuf);
> > @@ -1126,9 +1130,6 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
> >                                            con->scanout.texture.y,
> >                                            con->scanout.texture.width,
> >                                            con->scanout.texture.height);
> > -    } else if (con->scanout.kind == SCANOUT_SURFACE) {
> > -        dpy_gfx_create_texture(con, con->surface);
> > -        displaychangelistener_gfx_switch(dcl, con->surface, TRUE);
> >       }
> >   }
> >
>

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/07 19:19, Marc-André Lureau wrote:
> Hi Akihiko
> 
> On Mon, Mar 7, 2022 at 12:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> egl-headless depends on the backing surface to be set before texture are
>>> set and updated. Display it (update=true) iff the current scanout kind
>>> is SURFACE.
>>
>> egl-headless does not dynamically call register_displaychangelistener
>> and has console associated (console_select would not affect egl-headless
>> itself) so this should not be necessary.
> 
> Could you help me understand, what should not be necessary?

I read the description as it sets the backing surface for egl-headless 
when register_displaychangelistener or console_select is called. The 
change is not necessary.

> 
>> The remaining problem with egl-headless is that egl-headless renders the
>> image to DisplaySurface, and a non-OpenGL display (namely vnc) has to
>> consume it instead of texture even when con->scanout.kind is
>> SCANOUT_TEXTURE or SCANOUT_DMABUF.
> 
> This is already happening, because egl-headless calls dpy_gfx_update().

It is not called when register_displaychangelistener or console_select 
is called by non-OpenGL display consuming the DisplaySurface.

Regards,
Akihiko Odaki

> 
> thanks
> 
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    ui/console.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/ui/console.c b/ui/console.c
>>> index 5bfecea4549e..16a0b0909ba2 100644
>>> --- a/ui/console.c
>>> +++ b/ui/console.c
>>> @@ -1112,6 +1112,10 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>>>            return;
>>>        }
>>>
>>> +    dpy_gfx_create_texture(con, con->surface);
>>> +    displaychangelistener_gfx_switch(dcl, con->surface,
>>> +                                     con->scanout.kind == SCANOUT_SURFACE);
>>> +
>>>        if (con->scanout.kind == SCANOUT_DMABUF &&
>>>            displaychangelistener_has_dmabuf(dcl)) {
>>>            dcl->ops->dpy_gl_scanout_dmabuf(dcl, con->scanout.dmabuf);
>>> @@ -1126,9 +1130,6 @@ static void displaychangelistener_display_console(DisplayChangeListener *dcl,
>>>                                             con->scanout.texture.y,
>>>                                             con->scanout.texture.width,
>>>                                             con->scanout.texture.height);
>>> -    } else if (con->scanout.kind == SCANOUT_SURFACE) {
>>> -        dpy_gfx_create_texture(con, con->surface);
>>> -        displaychangelistener_gfx_switch(dcl, con->surface, TRUE);
>>>        }
>>>    }
>>>
>>
> 

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

Hi

On Mon, Mar 7, 2022 at 2:35 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> On 2022/03/07 19:19, Marc-André Lureau wrote:
> > Hi Akihiko
> >
> > On Mon, Mar 7, 2022 at 12:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> >>
> >> On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
> >>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>
> >>> egl-headless depends on the backing surface to be set before texture are
> >>> set and updated. Display it (update=true) iff the current scanout kind
> >>> is SURFACE.
> >>
> >> egl-headless does not dynamically call register_displaychangelistener
> >> and has console associated (console_select would not affect egl-headless
> >> itself) so this should not be necessary.
> >
> > Could you help me understand, what should not be necessary?
>
> I read the description as it sets the backing surface for egl-headless
> when register_displaychangelistener or console_select is called. The
> change is not necessary.

Without it, gfx_switch is not called to set the new surface. Switching
console with VNC would fail (via ctrl+alt+num).

>
> >
> >> The remaining problem with egl-headless is that egl-headless renders the
> >> image to DisplaySurface, and a non-OpenGL display (namely vnc) has to
> >> consume it instead of texture even when con->scanout.kind is
> >> SCANOUT_TEXTURE or SCANOUT_DMABUF.
> >
> > This is already happening, because egl-headless calls dpy_gfx_update().
>
> It is not called when register_displaychangelistener or console_select
> is called by non-OpenGL display consuming the DisplaySurface.

When displaychangelistener_display_console() is called with
con->scanount.kind == SCANOUT_SURFACE, it calls gfx_update(update ==
TRUE), and thus calls gfx_update on the whole surface.

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/07 20:50, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 7, 2022 at 2:35 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On 2022/03/07 19:19, Marc-André Lureau wrote:
>>> Hi Akihiko
>>>
>>> On Mon, Mar 7, 2022 at 12:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>>>
>>>> On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> egl-headless depends on the backing surface to be set before texture are
>>>>> set and updated. Display it (update=true) iff the current scanout kind
>>>>> is SURFACE.
>>>>
>>>> egl-headless does not dynamically call register_displaychangelistener
>>>> and has console associated (console_select would not affect egl-headless
>>>> itself) so this should not be necessary.
>>>
>>> Could you help me understand, what should not be necessary?
>>
>> I read the description as it sets the backing surface for egl-headless
>> when register_displaychangelistener or console_select is called. The
>> change is not necessary.
> 
> Without it, gfx_switch is not called to set the new surface. Switching
> console with VNC would fail (via ctrl+alt+num).

Yes, but that is not because egl-headless requires its dpy_gfx_switch to 
be called. It is because vnc does not receive the surface. The patch 
adds a call of dpy_gfx_switch no matter if the receiver is an OpenGL 
display or not, but an OpenGL display would not need one.

> 
>>
>>>
>>>> The remaining problem with egl-headless is that egl-headless renders the
>>>> image to DisplaySurface, and a non-OpenGL display (namely vnc) has to
>>>> consume it instead of texture even when con->scanout.kind is
>>>> SCANOUT_TEXTURE or SCANOUT_DMABUF.
>>>
>>> This is already happening, because egl-headless calls dpy_gfx_update().
>>
>> It is not called when register_displaychangelistener or console_select
>> is called by non-OpenGL display consuming the DisplaySurface.
> 
> When displaychangelistener_display_console() is called with
> con->scanount.kind == SCANOUT_SURFACE, it calls gfx_update(update ==
> TRUE), and thus calls gfx_update on the whole surface.
> 

con->scanout.kind is SCANOUT_TEXTURE or SCANOUT_DMABUF when egl-headless 
is rendering to surface. It would not call gfx_update in the case.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

Hi

On Mon, Mar 7, 2022 at 4:24 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> On 2022/03/07 20:50, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Mar 7, 2022 at 2:35 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> >>
> >> On 2022/03/07 19:19, Marc-André Lureau wrote:
> >>> Hi Akihiko
> >>>
> >>> On Mon, Mar 7, 2022 at 12:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> >>>>
> >>>> On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
> >>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>
> >>>>> egl-headless depends on the backing surface to be set before texture are
> >>>>> set and updated. Display it (update=true) iff the current scanout kind
> >>>>> is SURFACE.
> >>>>
> >>>> egl-headless does not dynamically call register_displaychangelistener
> >>>> and has console associated (console_select would not affect egl-headless
> >>>> itself) so this should not be necessary.
> >>>
> >>> Could you help me understand, what should not be necessary?
> >>
> >> I read the description as it sets the backing surface for egl-headless
> >> when register_displaychangelistener or console_select is called. The
> >> change is not necessary.
> >
> > Without it, gfx_switch is not called to set the new surface. Switching
> > console with VNC would fail (via ctrl+alt+num).
>
> Yes, but that is not because egl-headless requires its dpy_gfx_switch to
> be called. It is because vnc does not receive the surface. The patch
> adds a call of dpy_gfx_switch no matter if the receiver is an OpenGL
> display or not, but an OpenGL display would not need one.

That's why I added the "update" argument to
displaychangelistener_gfx_switch(). "gfx_switch" merely indicates that
the DisplaySurface has changed, not to show it.

> >
> >>
> >>>
> >>>> The remaining problem with egl-headless is that egl-headless renders the
> >>>> image to DisplaySurface, and a non-OpenGL display (namely vnc) has to
> >>>> consume it instead of texture even when con->scanout.kind is
> >>>> SCANOUT_TEXTURE or SCANOUT_DMABUF.
> >>>
> >>> This is already happening, because egl-headless calls dpy_gfx_update().
> >>
> >> It is not called when register_displaychangelistener or console_select
> >> is called by non-OpenGL display consuming the DisplaySurface.
> >
> > When displaychangelistener_display_console() is called with
> > con->scanount.kind == SCANOUT_SURFACE, it calls gfx_update(update ==
> > TRUE), and thus calls gfx_update on the whole surface.
> >
>
> con->scanout.kind is SCANOUT_TEXTURE or SCANOUT_DMABUF when egl-headless
> is rendering to surface. It would not call gfx_update in the case.

 GL updates go through egl_scanout_flush (dpy_gl_update callback),
which calls dpy_gfx_update() which is in turned handled by
vnc_dpy_update().

Could you provide a failing test case or a more concrete suggestion on
what to do instead? I am all ears :)

thanks

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/07 21:32, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 7, 2022 at 4:24 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On 2022/03/07 20:50, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Mon, Mar 7, 2022 at 2:35 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>>>
>>>> On 2022/03/07 19:19, Marc-André Lureau wrote:
>>>>> Hi Akihiko
>>>>>
>>>>> On Mon, Mar 7, 2022 at 12:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>>>>>
>>>>>> On 2022/03/07 16:46, marcandre.lureau@redhat.com wrote:
>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> egl-headless depends on the backing surface to be set before texture are
>>>>>>> set and updated. Display it (update=true) iff the current scanout kind
>>>>>>> is SURFACE.
>>>>>>
>>>>>> egl-headless does not dynamically call register_displaychangelistener
>>>>>> and has console associated (console_select would not affect egl-headless
>>>>>> itself) so this should not be necessary.
>>>>>
>>>>> Could you help me understand, what should not be necessary?
>>>>
>>>> I read the description as it sets the backing surface for egl-headless
>>>> when register_displaychangelistener or console_select is called. The
>>>> change is not necessary.
>>>
>>> Without it, gfx_switch is not called to set the new surface. Switching
>>> console with VNC would fail (via ctrl+alt+num).
>>
>> Yes, but that is not because egl-headless requires its dpy_gfx_switch to
>> be called. It is because vnc does not receive the surface. The patch
>> adds a call of dpy_gfx_switch no matter if the receiver is an OpenGL
>> display or not, but an OpenGL display would not need one.
> 
> That's why I added the "update" argument to
> displaychangelistener_gfx_switch(). "gfx_switch" merely indicates that
> the DisplaySurface has changed, not to show it.
> 
>>>
>>>>
>>>>>
>>>>>> The remaining problem with egl-headless is that egl-headless renders the
>>>>>> image to DisplaySurface, and a non-OpenGL display (namely vnc) has to
>>>>>> consume it instead of texture even when con->scanout.kind is
>>>>>> SCANOUT_TEXTURE or SCANOUT_DMABUF.
>>>>>
>>>>> This is already happening, because egl-headless calls dpy_gfx_update().
>>>>
>>>> It is not called when register_displaychangelistener or console_select
>>>> is called by non-OpenGL display consuming the DisplaySurface.
>>>
>>> When displaychangelistener_display_console() is called with
>>> con->scanount.kind == SCANOUT_SURFACE, it calls gfx_update(update ==
>>> TRUE), and thus calls gfx_update on the whole surface.
>>>
>>
>> con->scanout.kind is SCANOUT_TEXTURE or SCANOUT_DMABUF when egl-headless
>> is rendering to surface. It would not call gfx_update in the case.
> 
>   GL updates go through egl_scanout_flush (dpy_gl_update callback),
> which calls dpy_gfx_update() which is in turned handled by
> vnc_dpy_update().
> 
> Could you provide a failing test case or a more concrete suggestion on
> what to do instead? I am all ears :)
> 
> thanks
> 

Let me describe a more concrete case. Think that egl-headless and vnc 
are enabled. The guest devices are serial virtio-gpu-gl. vnc selects 
serial at first.

vnc uses register_displaychangelistener and console_select to switch 
consoles.

displaychangelistener_display_console does the actual switching, and 
calls dpy_gfx_switch and dpy_gfx_update if con->scanout.kind is 
SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or 
dpy_gl_scanout_dmabuf if con->scanout.kind is SCANOUT_TEXTURE or 
SCANOUT_DMABUF. It works for a OpenGL display, but it doesn't vnc in 
combination with egl-headless.

virtio-gpu-gl starts scanning out texture. It would happen in the 
following sequence:
1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind will be 
SCANOUT_SURFACE.
2. qemu_console_resize calls dpy_gfx_switch, delivering DisplaySurface 
to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
3. virtio-gpu-gl calls dpy_gl_scanout_texture. egl-headless starts 
rendering the texture to the DisplaySurface.

In the end, the DisplaySurface will have the image rendered, and 
con->scanout.kind will be SCANOUT_TEXTURE.

Now vnc switches to virtio-gpu-gl.

4. vnc calls console_select
5. displaychangelistener_display_console finds con->scanout.kind is 
SCANOUT_TEXTURE so it tries to scanout texture, but vnc is not an OpenGL 
display. It essentially becomes no-op and the display would be blank.

This patch will change it to call dpy_gfx_switch but not to call 
dpy_gfx_update.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

Hi

On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>

(taking notes, mostly for myself)

> > Could you provide a failing test case or a more concrete suggestion on
> > what to do instead? I am all ears :)
> >
> > thanks
> >
>
> Let me describe a more concrete case. Think that egl-headless and vnc
> are enabled. The guest devices are serial virtio-gpu-gl. vnc selects
> serial at first.

(I am not sure how you would select serial first, but let's assume you did)

>
> vnc uses register_displaychangelistener and console_select to switch
> consoles.
>
> displaychangelistener_display_console does the actual switching, and
> calls dpy_gfx_switch and dpy_gfx_update if con->scanout.kind is
> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
> dpy_gl_scanout_dmabuf if con->scanout.kind is SCANOUT_TEXTURE or
> SCANOUT_DMABUF. It works for a OpenGL display, but it doesn't vnc in
> combination with egl-headless.

(hmm, what doesn't work? With this patch, the DisplaySurface is always
switched, no matter what con->scanout.kind is)

>
> virtio-gpu-gl starts scanning out texture. It would happen in the
> following sequence:
> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind will be
> SCANOUT_SURFACE.
> 2. qemu_console_resize calls dpy_gfx_switch, delivering DisplaySurface
> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.

(qemu_console_resize calls dpy_gfx_replace_surface. con.scanout.kind
is still SCANOUT_SURFACE)

(It calls displaychangelistener_gfx_switch() which will call
egl_gfx_switch() and update the current surface)

> 3. virtio-gpu-gl calls dpy_gl_scanout_texture. egl-headless starts
> rendering the texture to the DisplaySurface.

(now con.scanout.kind becomes SCANOUT_TEXTURE)

>
> In the end, the DisplaySurface will have the image rendered, and
> con->scanout.kind will be SCANOUT_TEXTURE.

(so far it works as expected, right?)



>
> Now vnc switches to virtio-gpu-gl.
>
> 4. vnc calls console_select
> 5. displaychangelistener_display_console finds con->scanout.kind is
> SCANOUT_TEXTURE so it tries to scanout texture, but vnc is not an OpenGL
> display. It essentially becomes no-op and the display would be blank.
>
> This patch will change it to call dpy_gfx_switch but not to call
> dpy_gfx_update.

Alright, I think I see what you mean. egl-headless is associated with
a specific con, and thus is not involved during vnc console switching.

However, dpy_gfx_switch on vnc is not a no-op. It updates the surface,
resize the client and mark the area dirty. Because the con.surface is
kept updated by egl-headless, the client gets the updated content.
Iow, it still works.

If we always called dpy_gfx_update() during
displaychangelistener_display_console(), it would mean for the
listener to display the surface content, even when the scanout kind is
set for a texture. Or we need to change the behaviour of
dpy_gfx_update() to depend on the current scanout kind.

Imho, it's acceptable in the current proposed form, since there is no
apparent bug (unless I miss something). However, I can try to
formulate a FIXME comment and add some documentation around the gfx
callback to improve the situation.

thanks

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/08 23:26, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
> 
> (taking notes, mostly for myself)
> 
>>> Could you provide a failing test case or a more concrete suggestion on
>>> what to do instead? I am all ears :)
>>>
>>> thanks
>>>
>>
>> Let me describe a more concrete case. Think that egl-headless and vnc
>> are enabled. The guest devices are serial virtio-gpu-gl. vnc selects
>> serial at first.
> 
> (I am not sure how you would select serial first, but let's assume you did)
> 
>>
>> vnc uses register_displaychangelistener and console_select to switch
>> consoles.
>>
>> displaychangelistener_display_console does the actual switching, and
>> calls dpy_gfx_switch and dpy_gfx_update if con->scanout.kind is
>> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
>> dpy_gl_scanout_dmabuf if con->scanout.kind is SCANOUT_TEXTURE or
>> SCANOUT_DMABUF. It works for a OpenGL display, but it doesn't vnc in
>> combination with egl-headless.
> 
> (hmm, what doesn't work? With this patch, the DisplaySurface is always
> switched, no matter what con->scanout.kind is)
> 
>>
>> virtio-gpu-gl starts scanning out texture. It would happen in the
>> following sequence:
>> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind will be
>> SCANOUT_SURFACE.
>> 2. qemu_console_resize calls dpy_gfx_switch, delivering DisplaySurface
>> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
> 
> (qemu_console_resize calls dpy_gfx_replace_surface. con.scanout.kind
> is still SCANOUT_SURFACE)
> 
> (It calls displaychangelistener_gfx_switch() which will call
> egl_gfx_switch() and update the current surface)
> 
>> 3. virtio-gpu-gl calls dpy_gl_scanout_texture. egl-headless starts
>> rendering the texture to the DisplaySurface.
> 
> (now con.scanout.kind becomes SCANOUT_TEXTURE)
> 
>>
>> In the end, the DisplaySurface will have the image rendered, and
>> con->scanout.kind will be SCANOUT_TEXTURE.
> 
> (so far it works as expected, right?)
> 
> 
> 
>>
>> Now vnc switches to virtio-gpu-gl.
>>
>> 4. vnc calls console_select
>> 5. displaychangelistener_display_console finds con->scanout.kind is
>> SCANOUT_TEXTURE so it tries to scanout texture, but vnc is not an OpenGL
>> display. It essentially becomes no-op and the display would be blank.
>>
>> This patch will change it to call dpy_gfx_switch but not to call
>> dpy_gfx_update.
> 
> Alright, I think I see what you mean. egl-headless is associated with
> a specific con, and thus is not involved during vnc console switching.
> 
> However, dpy_gfx_switch on vnc is not a no-op. It updates the surface,
> resize the client and mark the area dirty. Because the con.surface is
> kept updated by egl-headless, the client gets the updated content.
> Iow, it still works.
> 
> If we always called dpy_gfx_update() during
> displaychangelistener_display_console(), it would mean for the
> listener to display the surface content, even when the scanout kind is
> set for a texture. Or we need to change the behaviour of
> dpy_gfx_update() to depend on the current scanout kind.

vnc has to display the surface content so dpy_gfx_update should be 
called for vnc, but not for OpenGL displays.

Regards,
Akihiko Odaki

> 
> Imho, it's acceptable in the current proposed form, since there is no
> apparent bug (unless I miss something). However, I can try to
> formulate a FIXME comment and add some documentation around the gfx
> callback to improve the situation.
> 
> thanks
> 

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Tue, Mar 8, 2022 at 6:43 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/08 23:26, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki <akihiko.odaki@gmail.com>
> wrote:
> >>
> >
> > (taking notes, mostly for myself)
> >
> >>> Could you provide a failing test case or a more concrete suggestion on
> >>> what to do instead? I am all ears :)
> >>>
> >>> thanks
> >>>
> >>
> >> Let me describe a more concrete case. Think that egl-headless and vnc
> >> are enabled. The guest devices are serial virtio-gpu-gl. vnc selects
> >> serial at first.
> >
> > (I am not sure how you would select serial first, but let's assume you
> did)
> >
> >>
> >> vnc uses register_displaychangelistener and console_select to switch
> >> consoles.
> >>
> >> displaychangelistener_display_console does the actual switching, and
> >> calls dpy_gfx_switch and dpy_gfx_update if con->scanout.kind is
> >> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
> >> dpy_gl_scanout_dmabuf if con->scanout.kind is SCANOUT_TEXTURE or
> >> SCANOUT_DMABUF. It works for a OpenGL display, but it doesn't vnc in
> >> combination with egl-headless.
> >
> > (hmm, what doesn't work? With this patch, the DisplaySurface is always
> > switched, no matter what con->scanout.kind is)
> >
> >>
> >> virtio-gpu-gl starts scanning out texture. It would happen in the
> >> following sequence:
> >> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind will be
> >> SCANOUT_SURFACE.
> >> 2. qemu_console_resize calls dpy_gfx_switch, delivering DisplaySurface
> >> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
> >
> > (qemu_console_resize calls dpy_gfx_replace_surface. con.scanout.kind
> > is still SCANOUT_SURFACE)
> >
> > (It calls displaychangelistener_gfx_switch() which will call
> > egl_gfx_switch() and update the current surface)
> >
> >> 3. virtio-gpu-gl calls dpy_gl_scanout_texture. egl-headless starts
> >> rendering the texture to the DisplaySurface.
> >
> > (now con.scanout.kind becomes SCANOUT_TEXTURE)
> >
> >>
> >> In the end, the DisplaySurface will have the image rendered, and
> >> con->scanout.kind will be SCANOUT_TEXTURE.
> >
> > (so far it works as expected, right?)
> >
> >
> >
> >>
> >> Now vnc switches to virtio-gpu-gl.
> >>
> >> 4. vnc calls console_select
> >> 5. displaychangelistener_display_console finds con->scanout.kind is
> >> SCANOUT_TEXTURE so it tries to scanout texture, but vnc is not an OpenGL
> >> display. It essentially becomes no-op and the display would be blank.
> >>
> >> This patch will change it to call dpy_gfx_switch but not to call
> >> dpy_gfx_update.
> >
> > Alright, I think I see what you mean. egl-headless is associated with
> > a specific con, and thus is not involved during vnc console switching.
> >
> > However, dpy_gfx_switch on vnc is not a no-op. It updates the surface,
> > resize the client and mark the area dirty. Because the con.surface is
> > kept updated by egl-headless, the client gets the updated content.
> > Iow, it still works.
> >
> > If we always called dpy_gfx_update() during
> > displaychangelistener_display_console(), it would mean for the
> > listener to display the surface content, even when the scanout kind is
> > set for a texture. Or we need to change the behaviour of
> > dpy_gfx_update() to depend on the current scanout kind.
>
> vnc has to display the surface content so dpy_gfx_update should be
> called for vnc, but not for OpenGL displays.
>
>
VNC already marks the whole surface as dirty during vnc_dpy_switch(). This
is like calling dpy_gfx_update().


-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 17:02, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 8, 2022 at 6:43 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/08 23:26, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>> wrote:
>      >>
>      >
>      > (taking notes, mostly for myself)
>      >
>      >>> Could you provide a failing test case or a more concrete
>     suggestion on
>      >>> what to do instead? I am all ears :)
>      >>>
>      >>> thanks
>      >>>
>      >>
>      >> Let me describe a more concrete case. Think that egl-headless
>     and vnc
>      >> are enabled. The guest devices are serial virtio-gpu-gl. vnc selects
>      >> serial at first.
>      >
>      > (I am not sure how you would select serial first, but let's
>     assume you did)
>      >
>      >>
>      >> vnc uses register_displaychangelistener and console_select to switch
>      >> consoles.
>      >>
>      >> displaychangelistener_display_console does the actual switching, and
>      >> calls dpy_gfx_switch and dpy_gfx_update if con->scanout.kind is
>      >> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
>      >> dpy_gl_scanout_dmabuf if con->scanout.kind is SCANOUT_TEXTURE or
>      >> SCANOUT_DMABUF. It works for a OpenGL display, but it doesn't vnc in
>      >> combination with egl-headless.
>      >
>      > (hmm, what doesn't work? With this patch, the DisplaySurface is
>     always
>      > switched, no matter what con->scanout.kind is)
>      >
>      >>
>      >> virtio-gpu-gl starts scanning out texture. It would happen in the
>      >> following sequence:
>      >> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind
>     will be
>      >> SCANOUT_SURFACE.
>      >> 2. qemu_console_resize calls dpy_gfx_switch, delivering
>     DisplaySurface
>      >> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
>      >
>      > (qemu_console_resize calls dpy_gfx_replace_surface. con.scanout.kind
>      > is still SCANOUT_SURFACE)
>      >
>      > (It calls displaychangelistener_gfx_switch() which will call
>      > egl_gfx_switch() and update the current surface)
>      >
>      >> 3. virtio-gpu-gl calls dpy_gl_scanout_texture. egl-headless starts
>      >> rendering the texture to the DisplaySurface.
>      >
>      > (now con.scanout.kind becomes SCANOUT_TEXTURE)
>      >
>      >>
>      >> In the end, the DisplaySurface will have the image rendered, and
>      >> con->scanout.kind will be SCANOUT_TEXTURE.
>      >
>      > (so far it works as expected, right?)
>      >
>      >
>      >
>      >>
>      >> Now vnc switches to virtio-gpu-gl.
>      >>
>      >> 4. vnc calls console_select
>      >> 5. displaychangelistener_display_console finds con->scanout.kind is
>      >> SCANOUT_TEXTURE so it tries to scanout texture, but vnc is not
>     an OpenGL
>      >> display. It essentially becomes no-op and the display would be
>     blank.
>      >>
>      >> This patch will change it to call dpy_gfx_switch but not to call
>      >> dpy_gfx_update.
>      >
>      > Alright, I think I see what you mean. egl-headless is associated with
>      > a specific con, and thus is not involved during vnc console
>     switching.
>      >
>      > However, dpy_gfx_switch on vnc is not a no-op. It updates the
>     surface,
>      > resize the client and mark the area dirty. Because the con.surface is
>      > kept updated by egl-headless, the client gets the updated content.
>      > Iow, it still works.
>      >
>      > If we always called dpy_gfx_update() during
>      > displaychangelistener_display_console(), it would mean for the
>      > listener to display the surface content, even when the scanout
>     kind is
>      > set for a texture. Or we need to change the behaviour of
>      > dpy_gfx_update() to depend on the current scanout kind.
> 
>     vnc has to display the surface content so dpy_gfx_update should be
>     called for vnc, but not for OpenGL displays.
> 
> 
> VNC already marks the whole surface as dirty during vnc_dpy_switch(). 
> This is like calling dpy_gfx_update().

Then why does it call dpy_gfx_update for SCANOUT_SURFACE first place? It 
shouldn't call the function if it is not required to display the surface 
content.

Regards,
Akihiko Odaki

> 
> 
> -- 
> Marc-André Lureau

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 12:05 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 17:02, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Mar 8, 2022 at 6:43 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >     On 2022/03/08 23:26, Marc-André Lureau wrote:
> >      > Hi
> >      >
> >      > On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki
> >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>> wrote:
> >      >>
> >      >
> >      > (taking notes, mostly for myself)
> >      >
> >      >>> Could you provide a failing test case or a more concrete
> >     suggestion on
> >      >>> what to do instead? I am all ears :)
> >      >>>
> >      >>> thanks
> >      >>>
> >      >>
> >      >> Let me describe a more concrete case. Think that egl-headless
> >     and vnc
> >      >> are enabled. The guest devices are serial virtio-gpu-gl. vnc
> selects
> >      >> serial at first.
> >      >
> >      > (I am not sure how you would select serial first, but let's
> >     assume you did)
> >      >
> >      >>
> >      >> vnc uses register_displaychangelistener and console_select to
> switch
> >      >> consoles.
> >      >>
> >      >> displaychangelistener_display_console does the actual switching,
> and
> >      >> calls dpy_gfx_switch and dpy_gfx_update if con->scanout.kind is
> >      >> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
> >      >> dpy_gl_scanout_dmabuf if con->scanout.kind is SCANOUT_TEXTURE or
> >      >> SCANOUT_DMABUF. It works for a OpenGL display, but it doesn't
> vnc in
> >      >> combination with egl-headless.
> >      >
> >      > (hmm, what doesn't work? With this patch, the DisplaySurface is
> >     always
> >      > switched, no matter what con->scanout.kind is)
> >      >
> >      >>
> >      >> virtio-gpu-gl starts scanning out texture. It would happen in the
> >      >> following sequence:
> >      >> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind
> >     will be
> >      >> SCANOUT_SURFACE.
> >      >> 2. qemu_console_resize calls dpy_gfx_switch, delivering
> >     DisplaySurface
> >      >> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
> >      >
> >      > (qemu_console_resize calls dpy_gfx_replace_surface.
> con.scanout.kind
> >      > is still SCANOUT_SURFACE)
> >      >
> >      > (It calls displaychangelistener_gfx_switch() which will call
> >      > egl_gfx_switch() and update the current surface)
> >      >
> >      >> 3. virtio-gpu-gl calls dpy_gl_scanout_texture. egl-headless
> starts
> >      >> rendering the texture to the DisplaySurface.
> >      >
> >      > (now con.scanout.kind becomes SCANOUT_TEXTURE)
> >      >
> >      >>
> >      >> In the end, the DisplaySurface will have the image rendered, and
> >      >> con->scanout.kind will be SCANOUT_TEXTURE.
> >      >
> >      > (so far it works as expected, right?)
> >      >
> >      >
> >      >
> >      >>
> >      >> Now vnc switches to virtio-gpu-gl.
> >      >>
> >      >> 4. vnc calls console_select
> >      >> 5. displaychangelistener_display_console finds con->scanout.kind
> is
> >      >> SCANOUT_TEXTURE so it tries to scanout texture, but vnc is not
> >     an OpenGL
> >      >> display. It essentially becomes no-op and the display would be
> >     blank.
> >      >>
> >      >> This patch will change it to call dpy_gfx_switch but not to call
> >      >> dpy_gfx_update.
> >      >
> >      > Alright, I think I see what you mean. egl-headless is associated
> with
> >      > a specific con, and thus is not involved during vnc console
> >     switching.
> >      >
> >      > However, dpy_gfx_switch on vnc is not a no-op. It updates the
> >     surface,
> >      > resize the client and mark the area dirty. Because the
> con.surface is
> >      > kept updated by egl-headless, the client gets the updated content.
> >      > Iow, it still works.
> >      >
> >      > If we always called dpy_gfx_update() during
> >      > displaychangelistener_display_console(), it would mean for the
> >      > listener to display the surface content, even when the scanout
> >     kind is
> >      > set for a texture. Or we need to change the behaviour of
> >      > dpy_gfx_update() to depend on the current scanout kind.
> >
> >     vnc has to display the surface content so dpy_gfx_update should be
> >     called for vnc, but not for OpenGL displays.
> >
> >
> > VNC already marks the whole surface as dirty during vnc_dpy_switch().
> > This is like calling dpy_gfx_update().
>
> Then why does it call dpy_gfx_update for SCANOUT_SURFACE first place? It
> shouldn't call the function if it is not required to display the surface
> content.
>

This is a pre-existing discrepancy. vnc handles switch with an implicit
update. Imho, we need to improve the code so that updates/display become
explicit.

But this is extra, not necessary to fix the regressions you pointed out.
I'd rather not mix concerns, and proceed step by step.

-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 17:11, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 12:05 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 17:02, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Tue, Mar 8, 2022 at 6:43 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >     On 2022/03/08 23:26, Marc-André Lureau wrote:
>      >      > Hi
>      >      >
>      >      > On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki
>      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>>
>     wrote:
>      >      >>
>      >      >
>      >      > (taking notes, mostly for myself)
>      >      >
>      >      >>> Could you provide a failing test case or a more concrete
>      >     suggestion on
>      >      >>> what to do instead? I am all ears :)
>      >      >>>
>      >      >>> thanks
>      >      >>>
>      >      >>
>      >      >> Let me describe a more concrete case. Think that egl-headless
>      >     and vnc
>      >      >> are enabled. The guest devices are serial virtio-gpu-gl.
>     vnc selects
>      >      >> serial at first.
>      >      >
>      >      > (I am not sure how you would select serial first, but let's
>      >     assume you did)
>      >      >
>      >      >>
>      >      >> vnc uses register_displaychangelistener and
>     console_select to switch
>      >      >> consoles.
>      >      >>
>      >      >> displaychangelistener_display_console does the actual
>     switching, and
>      >      >> calls dpy_gfx_switch and dpy_gfx_update if
>     con->scanout.kind is
>      >      >> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
>      >      >> dpy_gl_scanout_dmabuf if con->scanout.kind is
>     SCANOUT_TEXTURE or
>      >      >> SCANOUT_DMABUF. It works for a OpenGL display, but it
>     doesn't vnc in
>      >      >> combination with egl-headless.
>      >      >
>      >      > (hmm, what doesn't work? With this patch, the
>     DisplaySurface is
>      >     always
>      >      > switched, no matter what con->scanout.kind is)
>      >      >
>      >      >>
>      >      >> virtio-gpu-gl starts scanning out texture. It would
>     happen in the
>      >      >> following sequence:
>      >      >> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind
>      >     will be
>      >      >> SCANOUT_SURFACE.
>      >      >> 2. qemu_console_resize calls dpy_gfx_switch, delivering
>      >     DisplaySurface
>      >      >> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
>      >      >
>      >      > (qemu_console_resize calls dpy_gfx_replace_surface.
>     con.scanout.kind
>      >      > is still SCANOUT_SURFACE)
>      >      >
>      >      > (It calls displaychangelistener_gfx_switch() which will call
>      >      > egl_gfx_switch() and update the current surface)
>      >      >
>      >      >> 3. virtio-gpu-gl calls dpy_gl_scanout_texture.
>     egl-headless starts
>      >      >> rendering the texture to the DisplaySurface.
>      >      >
>      >      > (now con.scanout.kind becomes SCANOUT_TEXTURE)
>      >      >
>      >      >>
>      >      >> In the end, the DisplaySurface will have the image
>     rendered, and
>      >      >> con->scanout.kind will be SCANOUT_TEXTURE.
>      >      >
>      >      > (so far it works as expected, right?)
>      >      >
>      >      >
>      >      >
>      >      >>
>      >      >> Now vnc switches to virtio-gpu-gl.
>      >      >>
>      >      >> 4. vnc calls console_select
>      >      >> 5. displaychangelistener_display_console finds
>     con->scanout.kind is
>      >      >> SCANOUT_TEXTURE so it tries to scanout texture, but vnc
>     is not
>      >     an OpenGL
>      >      >> display. It essentially becomes no-op and the display
>     would be
>      >     blank.
>      >      >>
>      >      >> This patch will change it to call dpy_gfx_switch but not
>     to call
>      >      >> dpy_gfx_update.
>      >      >
>      >      > Alright, I think I see what you mean. egl-headless is
>     associated with
>      >      > a specific con, and thus is not involved during vnc console
>      >     switching.
>      >      >
>      >      > However, dpy_gfx_switch on vnc is not a no-op. It updates the
>      >     surface,
>      >      > resize the client and mark the area dirty. Because the
>     con.surface is
>      >      > kept updated by egl-headless, the client gets the updated
>     content.
>      >      > Iow, it still works.
>      >      >
>      >      > If we always called dpy_gfx_update() during
>      >      > displaychangelistener_display_console(), it would mean for the
>      >      > listener to display the surface content, even when the scanout
>      >     kind is
>      >      > set for a texture. Or we need to change the behaviour of
>      >      > dpy_gfx_update() to depend on the current scanout kind.
>      >
>      >     vnc has to display the surface content so dpy_gfx_update
>     should be
>      >     called for vnc, but not for OpenGL displays.
>      >
>      >
>      > VNC already marks the whole surface as dirty during
>     vnc_dpy_switch().
>      > This is like calling dpy_gfx_update().
> 
>     Then why does it call dpy_gfx_update for SCANOUT_SURFACE first
>     place? It
>     shouldn't call the function if it is not required to display the
>     surface
>     content.
> 
> 
> This is a pre-existing discrepancy. vnc handles switch with an implicit 
> update. Imho, we need to improve the code so that updates/display become 
> explicit.
> 
> But this is extra, not necessary to fix the regressions you pointed out. 
> I'd rather not mix concerns, and proceed step by step.
> 
> -- 
> Marc-André Lureau

If it is expected that dpy_gfx_update is required, it should call 
dpy_gfx_update. I agree it is not a right timing to fix vnc to remove 
the implicit update as it is pre-existing.
However the lack of dpy_gfx_update call is a regression and should be fixed.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 12:21 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

>
> If it is expected that dpy_gfx_update is required, it should call
> dpy_gfx_update. I agree it is not a right timing to fix vnc to remove
> the implicit update as it is pre-existing.
> However the lack of dpy_gfx_update call is a regression and should be
> fixed.
>
>
Calling dpy_gfx_update is done when the scanount.kind is SURFACE.

dpy_gfx_update is specific to SURFACE, GL uses dpy_gl_update.

-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 17:33, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 12:21 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
> 
>     If it is expected that dpy_gfx_update is required, it should call
>     dpy_gfx_update. I agree it is not a right timing to fix vnc to remove
>     the implicit update as it is pre-existing.
>     However the lack of dpy_gfx_update call is a regression and should
>     be fixed.
> 
> 
> Calling dpy_gfx_update is done when the scanount.kind is SURFACE.
> 
> dpy_gfx_update is specific to SURFACE, GL uses dpy_gl_update.
> 
> -- 
> Marc-André Lureau

egl-headless requires non-OpenGL to display the surface content even if 
scanout.kind is not SURFACE. Calling dpy_gfx_update is done when the 
scanount.kind is SURFACE is not enough.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 12:34 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 17:33, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 9, 2022 at 12:21 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >
> >     If it is expected that dpy_gfx_update is required, it should call
> >     dpy_gfx_update. I agree it is not a right timing to fix vnc to remove
> >     the implicit update as it is pre-existing.
> >     However the lack of dpy_gfx_update call is a regression and should
> >     be fixed.
> >
> >
> > Calling dpy_gfx_update is done when the scanount.kind is SURFACE.
> >
> > dpy_gfx_update is specific to SURFACE, GL uses dpy_gl_update.
> >
> > --
> > Marc-André Lureau
>
> egl-headless requires non-OpenGL to display the surface content even if
> scanout.kind is not SURFACE. Calling dpy_gfx_update is done when the
> scanount.kind is SURFACE is not enough.
>
>
We are going in circles... egl-headless call dpy_gfx_update on
dpy_gl_update.

-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 17:40, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 12:34 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 17:33, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Wed, Mar 9, 2022 at 12:21 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >
>      >     If it is expected that dpy_gfx_update is required, it should call
>      >     dpy_gfx_update. I agree it is not a right timing to fix vnc
>     to remove
>      >     the implicit update as it is pre-existing.
>      >     However the lack of dpy_gfx_update call is a regression and
>     should
>      >     be fixed.
>      >
>      >
>      > Calling dpy_gfx_update is done when the scanount.kind is SURFACE.
>      >
>      > dpy_gfx_update is specific to SURFACE, GL uses dpy_gl_update.
>      >
>      > --
>      > Marc-André Lureau
> 
>     egl-headless requires non-OpenGL to display the surface content even if
>     scanout.kind is not SURFACE. Calling dpy_gfx_update is done when the
>     scanount.kind is SURFACE is not enough.
> 
> 
> We are going in circles... egl-headless call dpy_gfx_update on 
> dpy_gl_update.
> -- 
> Marc-André Lureau

Ok, let me summarize the situation.

The problem occurs in the following condition.
1. register_displaychangelistener of console_select is called for a 
non-OpenGL display.
2. scanout.kind is SURFACE_TEXTURE or SURFACE_DMABUF.
3. egl-headless is employed.

dpy_gfx_switch and dpy_gfx_update need to be called to finish the 
initialization or switching of the non-OpenGL display. However, the 
proposed patch only calls dpy_gfx_switch.

vnc actually does not need dpy_gfx_update because the vnc implementation 
of dpy_gfx_switch implicitly does the work for dpy_gfx_update, but the 
model of ui/console expects the two of dpy_gfx_switch and dpy_gfx_update 
is separated and only calling dpy_gfx_switch violates the model. 
dpy_gfx_update used to be called even in such a case before and it is a 
regression.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 12:49 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 17:40, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 9, 2022 at 12:34 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >     On 2022/03/09 17:33, Marc-André Lureau wrote:
> >      > Hi
> >      >
> >      > On Wed, Mar 9, 2022 at 12:21 PM Akihiko Odaki
> >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
> >      > <mailto:akihiko.odaki@gmail.com
> >     <mailto:akihiko.odaki@gmail.com>>> wrote:
> >      >
> >      >
> >      >     If it is expected that dpy_gfx_update is required, it should
> call
> >      >     dpy_gfx_update. I agree it is not a right timing to fix vnc
> >     to remove
> >      >     the implicit update as it is pre-existing.
> >      >     However the lack of dpy_gfx_update call is a regression and
> >     should
> >      >     be fixed.
> >      >
> >      >
> >      > Calling dpy_gfx_update is done when the scanount.kind is SURFACE.
> >      >
> >      > dpy_gfx_update is specific to SURFACE, GL uses dpy_gl_update.
> >      >
> >      > --
> >      > Marc-André Lureau
> >
> >     egl-headless requires non-OpenGL to display the surface content even
> if
> >     scanout.kind is not SURFACE. Calling dpy_gfx_update is done when the
> >     scanount.kind is SURFACE is not enough.
> >
> >
> > We are going in circles... egl-headless call dpy_gfx_update on
> > dpy_gl_update.
> > --
> > Marc-André Lureau
>
> Ok, let me summarize the situation.
>
> The problem occurs in the following condition.
> 1. register_displaychangelistener of console_select is called for a
> non-OpenGL display.
> 2. scanout.kind is SURFACE_TEXTURE or SURFACE_DMABUF.
>

You mean SCANOUT_TEXTURE or SCANOUT_DMABUF.


> 3. egl-headless is employed.
>
> dpy_gfx_switch and dpy_gfx_update need to be called to finish the
> initialization or switching of the non-OpenGL display. However, the
> proposed patch only calls dpy_gfx_switch.
>

dpy_gfx_update  is called for non-opengl (SCANOUT_SURFACE)


> vnc actually does not need dpy_gfx_update because the vnc implementation
> of dpy_gfx_switch implicitly does the work for dpy_gfx_update, but the
> model of ui/console expects the two of dpy_gfx_switch and dpy_gfx_update
> is separated and only calling dpy_gfx_switch violates the model.
> dpy_gfx_update used to be called even in such a case before and it is a
> regression.
>

Sorry, I don't see a regression. Can you translate that to an actually user
visible regression?


-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 17:54, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 12:49 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 17:40, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Wed, Mar 9, 2022 at 12:34 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >     On 2022/03/09 17:33, Marc-André Lureau wrote:
>      >      > Hi
>      >      >
>      >      > On Wed, Mar 9, 2022 at 12:21 PM Akihiko Odaki
>      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
>      >      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>> wrote:
>      >      >
>      >      >
>      >      >     If it is expected that dpy_gfx_update is required, it
>     should call
>      >      >     dpy_gfx_update. I agree it is not a right timing to
>     fix vnc
>      >     to remove
>      >      >     the implicit update as it is pre-existing.
>      >      >     However the lack of dpy_gfx_update call is a
>     regression and
>      >     should
>      >      >     be fixed.
>      >      >
>      >      >
>      >      > Calling dpy_gfx_update is done when the scanount.kind is
>     SURFACE.
>      >      >
>      >      > dpy_gfx_update is specific to SURFACE, GL uses dpy_gl_update.
>      >      >
>      >      > --
>      >      > Marc-André Lureau
>      >
>      >     egl-headless requires non-OpenGL to display the surface
>     content even if
>      >     scanout.kind is not SURFACE. Calling dpy_gfx_update is done
>     when the
>      >     scanount.kind is SURFACE is not enough.
>      >
>      >
>      > We are going in circles... egl-headless call dpy_gfx_update on
>      > dpy_gl_update.
>      > --
>      > Marc-André Lureau
> 
>     Ok, let me summarize the situation.
> 
>     The problem occurs in the following condition.
>     1. register_displaychangelistener of console_select is called for a
>     non-OpenGL display.
>     2. scanout.kind is SURFACE_TEXTURE or SURFACE_DMABUF.
> 
> 
> You mean SCANOUT_TEXTURE or SCANOUT_DMABUF.

Ah, yes, of course. My bad.

> 
>     3. egl-headless is employed.
> 
>     dpy_gfx_switch and dpy_gfx_update need to be called to finish the
>     initialization or switching of the non-OpenGL display. However, the
>     proposed patch only calls dpy_gfx_switch.
> 
> 
> dpy_gfx_update  is called for non-opengl (SCANOUT_SURFACE)

I meant non-OpenGL "display" (e.g. vnc). scanout.kind is SCANOUT_TEXTURE 
or SCANOUT_SURFACE as I described.

> 
> 
>     vnc actually does not need dpy_gfx_update because the vnc
>     implementation
>     of dpy_gfx_switch implicitly does the work for dpy_gfx_update, but the
>     model of ui/console expects the two of dpy_gfx_switch and
>     dpy_gfx_update
>     is separated and only calling dpy_gfx_switch violates the model.
>     dpy_gfx_update used to be called even in such a case before and it is a
>     regression.
> 
> 
> Sorry, I don't see a regression. Can you translate that to an actually 
> user visible regression?

It shouldn't be visible to user because vnc is the only display which 
uses the code path and vnc does the implicit update. However it still 
violates the model common for all displays and it would break once 
egl-headless gets working on macOS. It is a regression if something is 
getting worse whether it is immediately visible for user or not.

Regards,
Akihiko Odaki

> 
> -- 
> Marc-André Lureau

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Gerd Hoffmann]

  Hi,
 
> dpy_gfx_switch and dpy_gfx_update need to be called to finish the
> initialization or switching of the non-OpenGL display. However, the proposed
> patch only calls dpy_gfx_switch.
> 
> vnc actually does not need dpy_gfx_update because the vnc implementation of
> dpy_gfx_switch implicitly does the work for dpy_gfx_update, but the model of
> ui/console expects the two of dpy_gfx_switch and dpy_gfx_update is separated
> and only calling dpy_gfx_switch violates the model. dpy_gfx_update used to
> be called even in such a case before and it is a regression.

Well, no, the ->dpy_gfx_switch() callback is supposed to do everything
needed to bring the new surface to the screen.  vnc isn't alone here,
gtk for example does the same (see gd_switch()).

Yes, typically this is roughly the same an explicit dpy_gfx_update call
would do.  So this could be changed if it helps making the opengl code
paths less confusing, but that should be a separate patch series and
separate discussion.

take care,
  Gerd

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 18:26, Gerd Hoffmann wrote:
>    Hi,
>   
>> dpy_gfx_switch and dpy_gfx_update need to be called to finish the
>> initialization or switching of the non-OpenGL display. However, the proposed
>> patch only calls dpy_gfx_switch.
>>
>> vnc actually does not need dpy_gfx_update because the vnc implementation of
>> dpy_gfx_switch implicitly does the work for dpy_gfx_update, but the model of
>> ui/console expects the two of dpy_gfx_switch and dpy_gfx_update is separated
>> and only calling dpy_gfx_switch violates the model. dpy_gfx_update used to
>> be called even in such a case before and it is a regression.
> 
> Well, no, the ->dpy_gfx_switch() callback is supposed to do everything
> needed to bring the new surface to the screen.  vnc isn't alone here,
> gtk for example does the same (see gd_switch()).
> 
> Yes, typically this is roughly the same an explicit dpy_gfx_update call
> would do.  So this could be changed if it helps making the opengl code
> paths less confusing, but that should be a separate patch series and
> separate discussion.
> 
> take care,
>    Gerd
> 

Then ui/cocoa is probably wrong. I don't think it does the update when 
dpy_gfx_switch is called.

Please tell me if you think dpy_gfx_switch shouldn't do the implicit 
update in the future. I'll write a patch to do the update in cocoa's 
dpy_gfx_switch implementation otherwise.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 18:26, Gerd Hoffmann wrote:
> >    Hi,
> >
> >> dpy_gfx_switch and dpy_gfx_update need to be called to finish the
> >> initialization or switching of the non-OpenGL display. However, the
> proposed
> >> patch only calls dpy_gfx_switch.
> >>
> >> vnc actually does not need dpy_gfx_update because the vnc
> implementation of
> >> dpy_gfx_switch implicitly does the work for dpy_gfx_update, but the
> model of
> >> ui/console expects the two of dpy_gfx_switch and dpy_gfx_update is
> separated
> >> and only calling dpy_gfx_switch violates the model. dpy_gfx_update used
> to
> >> be called even in such a case before and it is a regression.
> >
> > Well, no, the ->dpy_gfx_switch() callback is supposed to do everything
> > needed to bring the new surface to the screen.  vnc isn't alone here,
> > gtk for example does the same (see gd_switch()).
> >
>

If dpy_gfx_switch() implies a full dpy_gfx_update(), then we would need
another callback to just set the new surface. This would avoid intermediary
and useless switches to 2d/surface when the scanout is GL.

For consistency, we should also declare that gl_scanout_texture and
gl_scanout_dmabuf imply full update as well.


> > Yes, typically this is roughly the same an explicit dpy_gfx_update call
> > would do.  So this could be changed if it helps making the opengl code
> > paths less confusing, but that should be a separate patch series and
> > separate discussion.
> >
> > take care,
> >    Gerd
> >
>
> Then ui/cocoa is probably wrong. I don't think it does the update when
> dpy_gfx_switch is called.
>
> Please tell me if you think dpy_gfx_switch shouldn't do the implicit
> update in the future. I'll write a patch to do the update in cocoa's
> dpy_gfx_switch implementation otherwise.
>
>
Can we ack this series first and iterate on top? It solves a number of
issues already and is a better starting point.

thanks

-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 18:53, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 18:26, Gerd Hoffmann wrote:
>      >    Hi,
>      >
>      >> dpy_gfx_switch and dpy_gfx_update need to be called to finish the
>      >> initialization or switching of the non-OpenGL display. However,
>     the proposed
>      >> patch only calls dpy_gfx_switch.
>      >>
>      >> vnc actually does not need dpy_gfx_update because the vnc
>     implementation of
>      >> dpy_gfx_switch implicitly does the work for dpy_gfx_update, but
>     the model of
>      >> ui/console expects the two of dpy_gfx_switch and dpy_gfx_update
>     is separated
>      >> and only calling dpy_gfx_switch violates the model.
>     dpy_gfx_update used to
>      >> be called even in such a case before and it is a regression.
>      >
>      > Well, no, the ->dpy_gfx_switch() callback is supposed to do
>     everything
>      > needed to bring the new surface to the screen.  vnc isn't alone here,
>      > gtk for example does the same (see gd_switch()).
>      >
> 
> 
> If dpy_gfx_switch() implies a full dpy_gfx_update(), then we would need 
> another callback to just set the new surface. This would avoid 
> intermediary and useless switches to 2d/surface when the scanout is GL.
> 
> For consistency, we should also declare that gl_scanout_texture and 
> gl_scanout_dmabuf imply full update as well.
> 
>      > Yes, typically this is roughly the same an explicit
>     dpy_gfx_update call
>      > would do.  So this could be changed if it helps making the opengl
>     code
>      > paths less confusing, but that should be a separate patch series and
>      > separate discussion.
>      >
>      > take care,
>      >    Gerd
>      >
> 
>     Then ui/cocoa is probably wrong. I don't think it does the update when
>     dpy_gfx_switch is called.
> 
>     Please tell me if you think dpy_gfx_switch shouldn't do the implicit
>     update in the future. I'll write a patch to do the update in cocoa's
>     dpy_gfx_switch implementation otherwise.
> 
> 
> Can we ack this series first and iterate on top? It solves a number of 
> issues already and is a better starting point.
> 
> thanks
> 
> -- 
> Marc-André Lureau

The call of dpy_gfx_update in displaychangelistener_display_console 
should be removed. It would simplify the patch.

Also it is still not shown that the series is a better alternative to:
https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/

The series "ui/dbus: Share one listener for a console" has significantly 
less code than this series and therefore needs some reasoning for that.

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 18:53, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
> >      >    Hi,
> >      >
> >      >> dpy_gfx_switch and dpy_gfx_update need to be called to finish the
> >      >> initialization or switching of the non-OpenGL display. However,
> >     the proposed
> >      >> patch only calls dpy_gfx_switch.
> >      >>
> >      >> vnc actually does not need dpy_gfx_update because the vnc
> >     implementation of
> >      >> dpy_gfx_switch implicitly does the work for dpy_gfx_update, but
> >     the model of
> >      >> ui/console expects the two of dpy_gfx_switch and dpy_gfx_update
> >     is separated
> >      >> and only calling dpy_gfx_switch violates the model.
> >     dpy_gfx_update used to
> >      >> be called even in such a case before and it is a regression.
> >      >
> >      > Well, no, the ->dpy_gfx_switch() callback is supposed to do
> >     everything
> >      > needed to bring the new surface to the screen.  vnc isn't alone
> here,
> >      > gtk for example does the same (see gd_switch()).
> >      >
> >
> >
> > If dpy_gfx_switch() implies a full dpy_gfx_update(), then we would need
> > another callback to just set the new surface. This would avoid
> > intermediary and useless switches to 2d/surface when the scanout is GL.
> >
> > For consistency, we should also declare that gl_scanout_texture and
> > gl_scanout_dmabuf imply full update as well.
> >
> >      > Yes, typically this is roughly the same an explicit
> >     dpy_gfx_update call
> >      > would do.  So this could be changed if it helps making the opengl
> >     code
> >      > paths less confusing, but that should be a separate patch series
> and
> >      > separate discussion.
> >      >
> >      > take care,
> >      >    Gerd
> >      >
> >
> >     Then ui/cocoa is probably wrong. I don't think it does the update
> when
> >     dpy_gfx_switch is called.
> >
> >     Please tell me if you think dpy_gfx_switch shouldn't do the implicit
> >     update in the future. I'll write a patch to do the update in cocoa's
> >     dpy_gfx_switch implementation otherwise.
> >
> >
> > Can we ack this series first and iterate on top? It solves a number of
> > issues already and is a better starting point.
> >
> > thanks
> >
> > --
> > Marc-André Lureau
>
> The call of dpy_gfx_update in displaychangelistener_display_console
> should be removed. It would simplify the patch.
>
> Also it is still not shown that the series is a better alternative to:
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
>
> The series "ui/dbus: Share one listener for a console" has significantly
> less code than this series and therefore needs some reasoning for that.
>

At this point, your change is much larger than the proposed fixes.

I already discussed the rationale for the current design. To summarize:
- dispatching DCL in the common code allows for greater reuse if an
alternative to dbus emerges, and should help making the code more dynamic
- the GL context split also is a separation of concerns and should help for
alternatives to EGL
- dbus code only handles dbus specifics

My understanding of your proposal is that you would rather see all this
done within the dbus code. I disagree for the reasons above. I may be
proven wrong, but so far, this works as expected minor the left-over and
regressions you pointed out that should be fixed. Going back to a different
design should be done in a next release if sufficiently motivated.
-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Gerd Hoffmann]

  Hi,

> If dpy_gfx_switch() implies a full dpy_gfx_update(), then we would need
> another callback to just set the new surface. This would avoid intermediary
> and useless switches to 2d/surface when the scanout is GL.

We can certainly change what dpy_gfx_switch() is supposed to do.

Current behavior dates back to the days where opengl support didn't
exist, so changing things and doing an explit update (when needed)
instead of an implicit automatic update makes sense to me.

When doing that the current ui implementations need a review though,
that's why I think it would be best to do such a change as separate
patch series.

take care,
  Gerd

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

[Gerd Hoffmann]

On Mon, Mar 07, 2022 at 11:46:20AM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Here are pending fixes related to D-Bus and GL, most of them reported thanks to
> Akihiko Odaki.
> 

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 19:07, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 18:53, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
>      >      >    Hi,
>      >      >
>      >      >> dpy_gfx_switch and dpy_gfx_update need to be called to
>     finish the
>      >      >> initialization or switching of the non-OpenGL display.
>     However,
>      >     the proposed
>      >      >> patch only calls dpy_gfx_switch.
>      >      >>
>      >      >> vnc actually does not need dpy_gfx_update because the vnc
>      >     implementation of
>      >      >> dpy_gfx_switch implicitly does the work for
>     dpy_gfx_update, but
>      >     the model of
>      >      >> ui/console expects the two of dpy_gfx_switch and
>     dpy_gfx_update
>      >     is separated
>      >      >> and only calling dpy_gfx_switch violates the model.
>      >     dpy_gfx_update used to
>      >      >> be called even in such a case before and it is a regression.
>      >      >
>      >      > Well, no, the ->dpy_gfx_switch() callback is supposed to do
>      >     everything
>      >      > needed to bring the new surface to the screen.  vnc isn't
>     alone here,
>      >      > gtk for example does the same (see gd_switch()).
>      >      >
>      >
>      >
>      > If dpy_gfx_switch() implies a full dpy_gfx_update(), then we
>     would need
>      > another callback to just set the new surface. This would avoid
>      > intermediary and useless switches to 2d/surface when the scanout
>     is GL.
>      >
>      > For consistency, we should also declare that gl_scanout_texture and
>      > gl_scanout_dmabuf imply full update as well.
>      >
>      >      > Yes, typically this is roughly the same an explicit
>      >     dpy_gfx_update call
>      >      > would do.  So this could be changed if it helps making the
>     opengl
>      >     code
>      >      > paths less confusing, but that should be a separate patch
>     series and
>      >      > separate discussion.
>      >      >
>      >      > take care,
>      >      >    Gerd
>      >      >
>      >
>      >     Then ui/cocoa is probably wrong. I don't think it does the
>     update when
>      >     dpy_gfx_switch is called.
>      >
>      >     Please tell me if you think dpy_gfx_switch shouldn't do the
>     implicit
>      >     update in the future. I'll write a patch to do the update in
>     cocoa's
>      >     dpy_gfx_switch implementation otherwise.
>      >
>      >
>      > Can we ack this series first and iterate on top? It solves a
>     number of
>      > issues already and is a better starting point.
>      >
>      > thanks
>      >
>      > --
>      > Marc-André Lureau
> 
>     The call of dpy_gfx_update in displaychangelistener_display_console
>     should be removed. It would simplify the patch.
> 
>     Also it is still not shown that the series is a better alternative to:
>     https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
>     <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>
> 
>     The series "ui/dbus: Share one listener for a console" has
>     significantly
>     less code than this series and therefore needs some reasoning for that.
> 
> 
> At this point, your change is much larger than the proposed fixes.

My change does not touch the common code except reverting and minimizes 
the risk of regression. It also results in the less code when applied to 
the tree.

> 
> I already discussed the rationale for the current design. To summarize:
> - dispatching DCL in the common code allows for greater reuse if an 
> alternative to dbus emerges, and should help making the code more dynamic
> - the GL context split also is a separation of concerns and should help 
> for alternatives to EGL
> - dbus code only handles dbus specifics

Let me summarize my counterargument:
- The suggested reuse case is not emerged yet.
- The GL context split is not aligned with the reality where the display 
knows the graphics accelerator where the window resides and the context 
should be created. The alternative to EGL can be introduced in a similar 
manner with ui/egl-context.c and ui/egl-helpers.c. If several context 
providers need to be supported, the selection should be passed as a 
parameter, just as the current code does for EGL rendernode.
- implementing the dispatching would allow dbus to share more things 
like e.g. textures converted from DisplaySurface and GunixFDList for 
DMA-BUF. They are not present in all displays and some are completely 
specific to dbus.

> 
> My understanding of your proposal is that you would rather see all this 
> done within the dbus code. I disagree for the reasons above. I may be 
> proven wrong, but so far, this works as expected minor the left-over and 
> regressions you pointed out that should be fixed. Going back to a 
> different design should be done in a next release if sufficiently motivated.

Reverting the dbus change is the safest option if it does not settle.

Regards,
Akuhiko Odaki

 > --
 > Marc-André Lureau

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 2:20 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 19:07, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >     On 2022/03/09 18:53, Marc-André Lureau wrote:
> >      > Hi
> >      >
> >      > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki
> >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
> >      > <mailto:akihiko.odaki@gmail.com
> >     <mailto:akihiko.odaki@gmail.com>>> wrote:
> >      >
> >      >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
> >      >      >    Hi,
> >      >      >
> >      >      >> dpy_gfx_switch and dpy_gfx_update need to be called to
> >     finish the
> >      >      >> initialization or switching of the non-OpenGL display.
> >     However,
> >      >     the proposed
> >      >      >> patch only calls dpy_gfx_switch.
> >      >      >>
> >      >      >> vnc actually does not need dpy_gfx_update because the vnc
> >      >     implementation of
> >      >      >> dpy_gfx_switch implicitly does the work for
> >     dpy_gfx_update, but
> >      >     the model of
> >      >      >> ui/console expects the two of dpy_gfx_switch and
> >     dpy_gfx_update
> >      >     is separated
> >      >      >> and only calling dpy_gfx_switch violates the model.
> >      >     dpy_gfx_update used to
> >      >      >> be called even in such a case before and it is a
> regression.
> >      >      >
> >      >      > Well, no, the ->dpy_gfx_switch() callback is supposed to do
> >      >     everything
> >      >      > needed to bring the new surface to the screen.  vnc isn't
> >     alone here,
> >      >      > gtk for example does the same (see gd_switch()).
> >      >      >
> >      >
> >      >
> >      > If dpy_gfx_switch() implies a full dpy_gfx_update(), then we
> >     would need
> >      > another callback to just set the new surface. This would avoid
> >      > intermediary and useless switches to 2d/surface when the scanout
> >     is GL.
> >      >
> >      > For consistency, we should also declare that gl_scanout_texture
> and
> >      > gl_scanout_dmabuf imply full update as well.
> >      >
> >      >      > Yes, typically this is roughly the same an explicit
> >      >     dpy_gfx_update call
> >      >      > would do.  So this could be changed if it helps making the
> >     opengl
> >      >     code
> >      >      > paths less confusing, but that should be a separate patch
> >     series and
> >      >      > separate discussion.
> >      >      >
> >      >      > take care,
> >      >      >    Gerd
> >      >      >
> >      >
> >      >     Then ui/cocoa is probably wrong. I don't think it does the
> >     update when
> >      >     dpy_gfx_switch is called.
> >      >
> >      >     Please tell me if you think dpy_gfx_switch shouldn't do the
> >     implicit
> >      >     update in the future. I'll write a patch to do the update in
> >     cocoa's
> >      >     dpy_gfx_switch implementation otherwise.
> >      >
> >      >
> >      > Can we ack this series first and iterate on top? It solves a
> >     number of
> >      > issues already and is a better starting point.
> >      >
> >      > thanks
> >      >
> >      > --
> >      > Marc-André Lureau
> >
> >     The call of dpy_gfx_update in displaychangelistener_display_console
> >     should be removed. It would simplify the patch.
> >
> >     Also it is still not shown that the series is a better alternative
> to:
> >
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
> >     <
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>
> >
> >     The series "ui/dbus: Share one listener for a console" has
> >     significantly
> >     less code than this series and therefore needs some reasoning for
> that.
> >
> >
> > At this point, your change is much larger than the proposed fixes.
>
> My change does not touch the common code except reverting and minimizes
> the risk of regression. It also results in the less code when applied to
> the tree.
>

The risk of regressions is proportional to the amount of code change. Your
change is larger (and may be even larger when updated and reviewed
properly). At this point in Qemu schedule, this is a greater risk.


> >
> > I already discussed the rationale for the current design. To summarize:
> > - dispatching DCL in the common code allows for greater reuse if an
> > alternative to dbus emerges, and should help making the code more dynamic
> > - the GL context split also is a separation of concerns and should help
> > for alternatives to EGL
> > - dbus code only handles dbus specifics
>
> Let me summarize my counterargument:
> - The suggested reuse case is not emerged yet.
>

It doesn't mean the design isn't superior and wanted.


> - The GL context split is not aligned with the reality where the display
> knows the graphics accelerator where the window resides and the context
> should be created. The alternative to EGL can be introduced in a similar
>

A GL context is not necessarily associated with a window.


> manner with ui/egl-context.c and ui/egl-helpers.c. If several context
> providers need to be supported, the selection should be passed as a
> parameter, just as the current code does for EGL rendernode.
>

It's not just about where the code resides, but also about the type design.
It's cleaner to separate DisplayGLCtxt from DisplayChangeListener.


> - implementing the dispatching would allow dbus to share more things
> like e.g. textures converted from DisplaySurface and GunixFDList for
> DMA-BUF. They are not present in all displays and some are completely
> specific to dbus.
>

And the dbus specific code is within dbus modules.

>
> >
> > My understanding of your proposal is that you would rather see all this
> > done within the dbus code. I disagree for the reasons above. I may be
> > proven wrong, but so far, this works as expected minor the left-over and
> > regressions you pointed out that should be fixed. Going back to a
> > different design should be done in a next release if sufficiently
> motivated.
>
> Reverting the dbus change is the safest option if it does not settle.


We have a different sense of safety.

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 19:27, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 2:20 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 19:07, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >     On 2022/03/09 18:53, Marc-André Lureau wrote:
>      >      > Hi
>      >      >
>      >      > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki
>      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
>      >      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>> wrote:
>      >      >
>      >      >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
>      >      >      >    Hi,
>      >      >      >
>      >      >      >> dpy_gfx_switch and dpy_gfx_update need to be called to
>      >     finish the
>      >      >      >> initialization or switching of the non-OpenGL display.
>      >     However,
>      >      >     the proposed
>      >      >      >> patch only calls dpy_gfx_switch.
>      >      >      >>
>      >      >      >> vnc actually does not need dpy_gfx_update because
>     the vnc
>      >      >     implementation of
>      >      >      >> dpy_gfx_switch implicitly does the work for
>      >     dpy_gfx_update, but
>      >      >     the model of
>      >      >      >> ui/console expects the two of dpy_gfx_switch and
>      >     dpy_gfx_update
>      >      >     is separated
>      >      >      >> and only calling dpy_gfx_switch violates the model.
>      >      >     dpy_gfx_update used to
>      >      >      >> be called even in such a case before and it is a
>     regression.
>      >      >      >
>      >      >      > Well, no, the ->dpy_gfx_switch() callback is
>     supposed to do
>      >      >     everything
>      >      >      > needed to bring the new surface to the screen.  vnc
>     isn't
>      >     alone here,
>      >      >      > gtk for example does the same (see gd_switch()).
>      >      >      >
>      >      >
>      >      >
>      >      > If dpy_gfx_switch() implies a full dpy_gfx_update(), then we
>      >     would need
>      >      > another callback to just set the new surface. This would avoid
>      >      > intermediary and useless switches to 2d/surface when the
>     scanout
>      >     is GL.
>      >      >
>      >      > For consistency, we should also declare that
>     gl_scanout_texture and
>      >      > gl_scanout_dmabuf imply full update as well.
>      >      >
>      >      >      > Yes, typically this is roughly the same an explicit
>      >      >     dpy_gfx_update call
>      >      >      > would do.  So this could be changed if it helps
>     making the
>      >     opengl
>      >      >     code
>      >      >      > paths less confusing, but that should be a separate
>     patch
>      >     series and
>      >      >      > separate discussion.
>      >      >      >
>      >      >      > take care,
>      >      >      >    Gerd
>      >      >      >
>      >      >
>      >      >     Then ui/cocoa is probably wrong. I don't think it does the
>      >     update when
>      >      >     dpy_gfx_switch is called.
>      >      >
>      >      >     Please tell me if you think dpy_gfx_switch shouldn't
>     do the
>      >     implicit
>      >      >     update in the future. I'll write a patch to do the
>     update in
>      >     cocoa's
>      >      >     dpy_gfx_switch implementation otherwise.
>      >      >
>      >      >
>      >      > Can we ack this series first and iterate on top? It solves a
>      >     number of
>      >      > issues already and is a better starting point.
>      >      >
>      >      > thanks
>      >      >
>      >      > --
>      >      > Marc-André Lureau
>      >
>      >     The call of dpy_gfx_update in
>     displaychangelistener_display_console
>      >     should be removed. It would simplify the patch.
>      >
>      >     Also it is still not shown that the series is a better
>     alternative to:
>      >
>     https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
>     <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>
>      >   
>       <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>>
>      >
>      >     The series "ui/dbus: Share one listener for a console" has
>      >     significantly
>      >     less code than this series and therefore needs some reasoning
>     for that.
>      >
>      >
>      > At this point, your change is much larger than the proposed fixes.
> 
>     My change does not touch the common code except reverting and minimizes
>     the risk of regression. It also results in the less code when
>     applied to
>     the tree.
> 
> 
> The risk of regressions is proportional to the amount of code change. 
> Your change is larger (and may be even larger when updated and reviewed 
> properly). At this point in Qemu schedule, this is a greater risk.

Possibly it can make dbus buggy, but it is not a regression as it is a 
new feature.

> 
> 
>      >
>      > I already discussed the rationale for the current design. To
>     summarize:
>      > - dispatching DCL in the common code allows for greater reuse if an
>      > alternative to dbus emerges, and should help making the code more
>     dynamic
>      > - the GL context split also is a separation of concerns and
>     should help
>      > for alternatives to EGL
>      > - dbus code only handles dbus specifics
> 
>     Let me summarize my counterargument:
>     - The suggested reuse case is not emerged yet.
> 
> 
> It doesn't mean the design isn't superior and wanted.

It doesn't, but it does not mean the design is superior and wanted either.

> 
>     - The GL context split is not aligned with the reality where the
>     display
>     knows the graphics accelerator where the window resides and the context
>     should be created. The alternative to EGL can be introduced in a
>     similar
> 
> 
> A GL context is not necessarily associated with a window.

It still can happen. Even if it is not associated with a window, it 
still requires some code to know that and the user must be aware of that.

> 
>     manner with ui/egl-context.c and ui/egl-helpers.c. If several context
>     providers need to be supported, the selection should be passed as a
>     parameter, just as the current code does for EGL rendernode.
> 
> 
> It's not just about where the code resides, but also about the type 
> design. It's cleaner to separate DisplayGLCtxt from DisplayChangeListener.

It would add a new failure possibility where the compatibility check 
between DisplayGLCtx and DisplayChangeListener is flawed, which happened 
with egl-headless. Unified DisplayChangeListener is a cleaner approach 
to describe the compatibility.

> 
>     - implementing the dispatching would allow dbus to share more things
>     like e.g. textures converted from DisplaySurface and GunixFDList for
>     DMA-BUF. They are not present in all displays and some are completely
>     specific to dbus.
> 
> 
> And the dbus specific code is within dbus modules.

The code to share e.g. GunixFDList are not yet.

> 
> 
>      >
>      > My understanding of your proposal is that you would rather see
>     all this
>      > done within the dbus code. I disagree for the reasons above. I
>     may be
>      > proven wrong, but so far, this works as expected minor the
>     left-over and
>      > regressions you pointed out that should be fixed. Going back to a
>      > different design should be done in a next release if sufficiently
>     motivated.
> 
>     Reverting the dbus change is the safest option if it does not settle.
> 
> 
> We have a different sense of safety.

Can you elaborate?

Regards,
Akihiko Odaki

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Marc-André Lureau]

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

Hi

On Wed, Mar 9, 2022 at 2:38 PM Akihiko Odaki <akihiko.odaki@gmail.com>
wrote:

> On 2022/03/09 19:27, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 9, 2022 at 2:20 PM Akihiko Odaki <akihiko.odaki@gmail.com
> > <mailto:akihiko.odaki@gmail.com>> wrote:
> >
> >     On 2022/03/09 19:07, Marc-André Lureau wrote:
> >      > Hi
> >      >
> >      > On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki
> >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
> >      > <mailto:akihiko.odaki@gmail.com
> >     <mailto:akihiko.odaki@gmail.com>>> wrote:
> >      >
> >      >     On 2022/03/09 18:53, Marc-André Lureau wrote:
> >      >      > Hi
> >      >      >
> >      >      > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki
> >      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
> >     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
> >      >      > <mailto:akihiko.odaki@gmail.com
> >     <mailto:akihiko.odaki@gmail.com>
> >      >     <mailto:akihiko.odaki@gmail.com
> >     <mailto:akihiko.odaki@gmail.com>>>> wrote:
> >      >      >
> >      >      >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
> >      >      >      >    Hi,
> >      >      >      >
> >      >      >      >> dpy_gfx_switch and dpy_gfx_update need to be
> called to
> >      >     finish the
> >      >      >      >> initialization or switching of the non-OpenGL
> display.
> >      >     However,
> >      >      >     the proposed
> >      >      >      >> patch only calls dpy_gfx_switch.
> >      >      >      >>
> >      >      >      >> vnc actually does not need dpy_gfx_update because
> >     the vnc
> >      >      >     implementation of
> >      >      >      >> dpy_gfx_switch implicitly does the work for
> >      >     dpy_gfx_update, but
> >      >      >     the model of
> >      >      >      >> ui/console expects the two of dpy_gfx_switch and
> >      >     dpy_gfx_update
> >      >      >     is separated
> >      >      >      >> and only calling dpy_gfx_switch violates the model.
> >      >      >     dpy_gfx_update used to
> >      >      >      >> be called even in such a case before and it is a
> >     regression.
> >      >      >      >
> >      >      >      > Well, no, the ->dpy_gfx_switch() callback is
> >     supposed to do
> >      >      >     everything
> >      >      >      > needed to bring the new surface to the screen.  vnc
> >     isn't
> >      >     alone here,
> >      >      >      > gtk for example does the same (see gd_switch()).
> >      >      >      >
> >      >      >
> >      >      >
> >      >      > If dpy_gfx_switch() implies a full dpy_gfx_update(), then
> we
> >      >     would need
> >      >      > another callback to just set the new surface. This would
> avoid
> >      >      > intermediary and useless switches to 2d/surface when the
> >     scanout
> >      >     is GL.
> >      >      >
> >      >      > For consistency, we should also declare that
> >     gl_scanout_texture and
> >      >      > gl_scanout_dmabuf imply full update as well.
> >      >      >
> >      >      >      > Yes, typically this is roughly the same an explicit
> >      >      >     dpy_gfx_update call
> >      >      >      > would do.  So this could be changed if it helps
> >     making the
> >      >     opengl
> >      >      >     code
> >      >      >      > paths less confusing, but that should be a separate
> >     patch
> >      >     series and
> >      >      >      > separate discussion.
> >      >      >      >
> >      >      >      > take care,
> >      >      >      >    Gerd
> >      >      >      >
> >      >      >
> >      >      >     Then ui/cocoa is probably wrong. I don't think it does
> the
> >      >     update when
> >      >      >     dpy_gfx_switch is called.
> >      >      >
> >      >      >     Please tell me if you think dpy_gfx_switch shouldn't
> >     do the
> >      >     implicit
> >      >      >     update in the future. I'll write a patch to do the
> >     update in
> >      >     cocoa's
> >      >      >     dpy_gfx_switch implementation otherwise.
> >      >      >
> >      >      >
> >      >      > Can we ack this series first and iterate on top? It solves
> a
> >      >     number of
> >      >      > issues already and is a better starting point.
> >      >      >
> >      >      > thanks
> >      >      >
> >      >      > --
> >      >      > Marc-André Lureau
> >      >
> >      >     The call of dpy_gfx_update in
> >     displaychangelistener_display_console
> >      >     should be removed. It would simplify the patch.
> >      >
> >      >     Also it is still not shown that the series is a better
> >     alternative to:
> >      >
> >
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
> >     <
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>
> >      >
> >       <
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <
> https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>>
> >      >
> >      >     The series "ui/dbus: Share one listener for a console" has
> >      >     significantly
> >      >     less code than this series and therefore needs some reasoning
> >     for that.
> >      >
> >      >
> >      > At this point, your change is much larger than the proposed fixes.
> >
> >     My change does not touch the common code except reverting and
> minimizes
> >     the risk of regression. It also results in the less code when
> >     applied to
> >     the tree.
> >
> >
> > The risk of regressions is proportional to the amount of code change.
> > Your change is larger (and may be even larger when updated and reviewed
> > properly). At this point in Qemu schedule, this is a greater risk.
>
> Possibly it can make dbus buggy, but it is not a regression as it is a
> new feature.
>

A regression is not necessarily against the last stable, but also on the
current master which is freezing as we speak.


>
> >
> >
> >      >
> >      > I already discussed the rationale for the current design. To
> >     summarize:
> >      > - dispatching DCL in the common code allows for greater reuse if
> an
> >      > alternative to dbus emerges, and should help making the code more
> >     dynamic
> >      > - the GL context split also is a separation of concerns and
> >     should help
> >      > for alternatives to EGL
> >      > - dbus code only handles dbus specifics
> >
> >     Let me summarize my counterargument:
> >     - The suggested reuse case is not emerged yet.
> >
> >
> > It doesn't mean the design isn't superior and wanted.
>
> It doesn't, but it does not mean the design is superior and wanted either.
>

But your suggestion would not help in this regard.


>
> >
> >     - The GL context split is not aligned with the reality where the
> >     display
> >     knows the graphics accelerator where the window resides and the
> context
> >     should be created. The alternative to EGL can be introduced in a
> >     similar
> >
> >
> > A GL context is not necessarily associated with a window.
>
> It still can happen. Even if it is not associated with a window, it
> still requires some code to know that and the user must be aware of that.
>
>
That's why we have compatibility checks now (which also help in other cases)


> >
> >     manner with ui/egl-context.c and ui/egl-helpers.c. If several context
> >     providers need to be supported, the selection should be passed as a
> >     parameter, just as the current code does for EGL rendernode.
> >
> >
> > It's not just about where the code resides, but also about the type
> > design. It's cleaner to separate DisplayGLCtxt from
> DisplayChangeListener.
>
> It would add a new failure possibility where the compatibility check
> between DisplayGLCtx and DisplayChangeListener is flawed, which happened
> with egl-headless. Unified DisplayChangeListener is a cleaner approach
> to describe the compatibility.
>

Care to describe the flaw in detail?


>
> >
> >     - implementing the dispatching would allow dbus to share more things
> >     like e.g. textures converted from DisplaySurface and GunixFDList for
> >     DMA-BUF. They are not present in all displays and some are completely
> >     specific to dbus.
> >
> >
> > And the dbus specific code is within dbus modules.
>
> The code to share e.g. GunixFDList are not yet.
>
>
 ~/src/qemu   master  git grep UnixFD
audio/dbusaudio.c:                             GUnixFDList *fd_list,
audio/dbusaudio.c:                                 GUnixFDList *fd_list,
audio/dbusaudio.c:                                GUnixFDList *fd_list,
tests/qtest/dbus-display-test.c:    g_autoptr(GUnixFDList) fd_list = NULL;
ui/dbus-chardev.c:    GUnixFDList *fd_list,
ui/dbus-console.c:                               GUnixFDList *fd_list,
ui/dbus-listener.c:    g_autoptr(GUnixFDList) fd_list = NULL;
..

>
> >
> >      >
> >      > My understanding of your proposal is that you would rather see
> >     all this
> >      > done within the dbus code. I disagree for the reasons above. I
> >     may be
> >      > proven wrong, but so far, this works as expected minor the
> >     left-over and
> >      > regressions you pointed out that should be fixed. Going back to a
> >      > different design should be done in a next release if sufficiently
> >     motivated.
> >
> >     Reverting the dbus change is the safest option if it does not settle.
> >
> >
> > We have a different sense of safety.
>
> Can you elaborate?
>

See above.

Sorry, I'll slow down my reply, as I think we have made enough rumble and
not much progress.

thanks again for helping so far


-- 
Marc-André Lureau

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

Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki]

On 2022/03/09 19:45, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 2:38 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 19:27, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Wed, Mar 9, 2022 at 2:20 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >     On 2022/03/09 19:07, Marc-André Lureau wrote:
>      >      > Hi
>      >      >
>      >      > On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki
>      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
>      >      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>> wrote:
>      >      >
>      >      >     On 2022/03/09 18:53, Marc-André Lureau wrote:
>      >      >      > Hi
>      >      >      >
>      >      >      > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki
>      >      >     <akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com> <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com> <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>
>      >      >      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
>      >      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>>> wrote:
>      >      >      >
>      >      >      >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
>      >      >      >      >    Hi,
>      >      >      >      >
>      >      >      >      >> dpy_gfx_switch and dpy_gfx_update need to
>     be called to
>      >      >     finish the
>      >      >      >      >> initialization or switching of the
>     non-OpenGL display.
>      >      >     However,
>      >      >      >     the proposed
>      >      >      >      >> patch only calls dpy_gfx_switch.
>      >      >      >      >>
>      >      >      >      >> vnc actually does not need dpy_gfx_update
>     because
>      >     the vnc
>      >      >      >     implementation of
>      >      >      >      >> dpy_gfx_switch implicitly does the work for
>      >      >     dpy_gfx_update, but
>      >      >      >     the model of
>      >      >      >      >> ui/console expects the two of
>     dpy_gfx_switch and
>      >      >     dpy_gfx_update
>      >      >      >     is separated
>      >      >      >      >> and only calling dpy_gfx_switch violates
>     the model.
>      >      >      >     dpy_gfx_update used to
>      >      >      >      >> be called even in such a case before and it
>     is a
>      >     regression.
>      >      >      >      >
>      >      >      >      > Well, no, the ->dpy_gfx_switch() callback is
>      >     supposed to do
>      >      >      >     everything
>      >      >      >      > needed to bring the new surface to the
>     screen.  vnc
>      >     isn't
>      >      >     alone here,
>      >      >      >      > gtk for example does the same (see gd_switch()).
>      >      >      >      >
>      >      >      >
>      >      >      >
>      >      >      > If dpy_gfx_switch() implies a full
>     dpy_gfx_update(), then we
>      >      >     would need
>      >      >      > another callback to just set the new surface. This
>     would avoid
>      >      >      > intermediary and useless switches to 2d/surface
>     when the
>      >     scanout
>      >      >     is GL.
>      >      >      >
>      >      >      > For consistency, we should also declare that
>      >     gl_scanout_texture and
>      >      >      > gl_scanout_dmabuf imply full update as well.
>      >      >      >
>      >      >      >      > Yes, typically this is roughly the same an
>     explicit
>      >      >      >     dpy_gfx_update call
>      >      >      >      > would do.  So this could be changed if it helps
>      >     making the
>      >      >     opengl
>      >      >      >     code
>      >      >      >      > paths less confusing, but that should be a
>     separate
>      >     patch
>      >      >     series and
>      >      >      >      > separate discussion.
>      >      >      >      >
>      >      >      >      > take care,
>      >      >      >      >    Gerd
>      >      >      >      >
>      >      >      >
>      >      >      >     Then ui/cocoa is probably wrong. I don't think
>     it does the
>      >      >     update when
>      >      >      >     dpy_gfx_switch is called.
>      >      >      >
>      >      >      >     Please tell me if you think dpy_gfx_switch
>     shouldn't
>      >     do the
>      >      >     implicit
>      >      >      >     update in the future. I'll write a patch to do the
>      >     update in
>      >      >     cocoa's
>      >      >      >     dpy_gfx_switch implementation otherwise.
>      >      >      >
>      >      >      >
>      >      >      > Can we ack this series first and iterate on top? It
>     solves a
>      >      >     number of
>      >      >      > issues already and is a better starting point.
>      >      >      >
>      >      >      > thanks
>      >      >      >
>      >      >      > --
>      >      >      > Marc-André Lureau
>      >      >
>      >      >     The call of dpy_gfx_update in
>      >     displaychangelistener_display_console
>      >      >     should be removed. It would simplify the patch.
>      >      >
>      >      >     Also it is still not shown that the series is a better
>      >     alternative to:
>      >      >
>      >
>     https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
>     <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>
>      >   
>       <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>>
>      >      >
>      >     
>       <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/> <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>>>
>      >      >
>      >      >     The series "ui/dbus: Share one listener for a console" has
>      >      >     significantly
>      >      >     less code than this series and therefore needs some
>     reasoning
>      >     for that.
>      >      >
>      >      >
>      >      > At this point, your change is much larger than the
>     proposed fixes.
>      >
>      >     My change does not touch the common code except reverting and
>     minimizes
>      >     the risk of regression. It also results in the less code when
>      >     applied to
>      >     the tree.
>      >
>      >
>      > The risk of regressions is proportional to the amount of code
>     change.
>      > Your change is larger (and may be even larger when updated and
>     reviewed
>      > properly). At this point in Qemu schedule, this is a greater risk.
> 
>     Possibly it can make dbus buggy, but it is not a regression as it is a
>     new feature.
> 
> 
> A regression is not necessarily against the last stable, but also on the 
> current master which is freezing as we speak.

In that sense, yes, my series could have more regressions. The premise 
of the less regression only applies to the difference between releases.

> 
> 
>      >
>      >
>      >      >
>      >      > I already discussed the rationale for the current design. To
>      >     summarize:
>      >      > - dispatching DCL in the common code allows for greater
>     reuse if an
>      >      > alternative to dbus emerges, and should help making the
>     code more
>      >     dynamic
>      >      > - the GL context split also is a separation of concerns and
>      >     should help
>      >      > for alternatives to EGL
>      >      > - dbus code only handles dbus specifics
>      >
>      >     Let me summarize my counterargument:
>      >     - The suggested reuse case is not emerged yet.
>      >
>      >
>      > It doesn't mean the design isn't superior and wanted.
> 
>     It doesn't, but it does not mean the design is superior and wanted
>     either.
> 
> 
> But your suggestion would not help in this regard.

It doesn't, but it is not shown that allowing another display to 
dispatch the GL output to multiple listeners in the same manner as dbus 
does would help in the future.

> 
> 
>      >
>      >     - The GL context split is not aligned with the reality where the
>      >     display
>      >     knows the graphics accelerator where the window resides and
>     the context
>      >     should be created. The alternative to EGL can be introduced in a
>      >     similar
>      >
>      >
>      > A GL context is not necessarily associated with a window.
> 
>     It still can happen. Even if it is not associated with a window, it
>     still requires some code to know that and the user must be aware of
>     that.
> 
> 
> That's why we have compatibility checks now (which also help in other cases)

Can you elaborate the other cases?

> 
>      >
>      >     manner with ui/egl-context.c and ui/egl-helpers.c. If several
>     context
>      >     providers need to be supported, the selection should be
>     passed as a
>      >     parameter, just as the current code does for EGL rendernode.
>      >
>      >
>      > It's not just about where the code resides, but also about the type
>      > design. It's cleaner to separate DisplayGLCtxt from
>     DisplayChangeListener.
> 
>     It would add a new failure possibility where the compatibility check
>     between DisplayGLCtx and DisplayChangeListener is flawed, which
>     happened
>     with egl-headless. Unified DisplayChangeListener is a cleaner approach
>     to describe the compatibility.
> 
> 
> Care to describe the flaw in detail?

egl-headless requires to be compatible with any displays without GL 
handlers, but that case was not considered and required a patch.

Regards,
Akihiko Odaki

> 
> 
>      >
>      >     - implementing the dispatching would allow dbus to share more
>     things
>      >     like e.g. textures converted from DisplaySurface and
>     GunixFDList for
>      >     DMA-BUF. They are not present in all displays and some are
>     completely
>      >     specific to dbus.
>      >
>      >
>      > And the dbus specific code is within dbus modules.
> 
>     The code to share e.g. GunixFDList are not yet.
> 
> 
>   ~/src/qemu   master  git grep UnixFD
> audio/dbusaudio.c:                             GUnixFDList *fd_list,
> audio/dbusaudio.c:                                 GUnixFDList *fd_list,
> audio/dbusaudio.c:                                GUnixFDList *fd_list,
> tests/qtest/dbus-display-test.c:    g_autoptr(GUnixFDList) fd_list = NULL;
> ui/dbus-chardev.c:    GUnixFDList *fd_list,
> ui/dbus-console.c:                               GUnixFDList *fd_list,
> ui/dbus-listener.c:    g_autoptr(GUnixFDList) fd_list = NULL; > ..

I meant "shared code" but "code to share GunixFDList". GunixFDLists are 
created for each listeners and not shared.

Regards,
Akihiko Odaki

> 
>      >
>      >
>      >      >
>      >      > My understanding of your proposal is that you would rather see
>      >     all this
>      >      > done within the dbus code. I disagree for the reasons above. I
>      >     may be
>      >      > proven wrong, but so far, this works as expected minor the
>      >     left-over and
>      >      > regressions you pointed out that should be fixed. Going
>     back to a
>      >      > different design should be done in a next release if
>     sufficiently
>      >     motivated.
>      >
>      >     Reverting the dbus change is the safest option if it does not
>     settle.
>      >
>      >
>      > We have a different sense of safety.
> 
>     Can you elaborate?
> 
> 
> See above.
> 
> Sorry, I'll slow down my reply, as I think we have made enough rumble 
> and not much progress.
> 
> thanks again for helping so far
> 
> 
> -- 
> Marc-André Lureau