[PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Vivek Kasireddy]

There is a need (expressed by several customers/users) to assign
ownership of one or more physical monitors/connectors to individual
Guests such that there is a clear notion of which Guest's contents
are being displayed on any given monitor. Given that there is always
a Display Server/Compositor running on the Host, monitor ownership
can never truly be transferred to Guests. However, the closest we
can come to realizing this concept is to request the Host compositor
to fullscreen the Guest's windows on individual monitors. This way,
it would become possible to have 4 different Guests' windows be
displayed on 4 different monitors or a single Guest's windows (or
virtual consoles/outputs) be displayed on 4 monitors or any such
combination.

This patch series attempts to accomplish this by introducing a new
parameter named "connector" to assign the monitors to the GFX VCs
associated with a Guest. If the assigned monitor is not connected,
then the Guest's window would not be displayed anywhere similar to
how a Host compositor would behave when the connectors are not
connected. Once the monitor is hotplugged, the Guest's window(s)
would be fullscreened on the assigned monitor. The first patch is
just a bug fix to destroy context related objects when an associated
window is destroyed. The second patch is a minor refactor and the
third and last patch introduces the new parameter. This patch series
is expected to supersede a similar series from Dongwon Kim here:
https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03214.html

Example Usage: -device virtio-gpu-pci,max_outputs=2,blob=true......
               -display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.....

Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>

Vivek Kasireddy (3):
  ui/gtk: Disable the scanout when a detached tab is closed
  ui/gtk: Factor out tab window creation into a separate function
  ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

 qapi/ui.json     |   9 +-
 qemu-options.hx  |   1 +
 ui/gtk-egl.c     |   2 +
 ui/gtk-gl-area.c |   2 +
 ui/gtk.c         | 220 ++++++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 211 insertions(+), 23 deletions(-)

-- 
2.37.2

[PATCH v1 1/3] ui/gtk: Disable the scanout when a detached tab is closed

[Vivek Kasireddy]

When a detached tab window is closed, the underlying (EGL) context
is destroyed; therefore, disable the scanout which also destroys the
underlying framebuffer (id) and other objects. Also add calls to
make the context current in disable scanout and other missing places.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/gtk-egl.c     | 2 ++
 ui/gtk-gl-area.c | 2 ++
 ui/gtk.c         | 1 +
 3 files changed, 5 insertions(+)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index b5bffbab25..0f9ef11f4c 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -211,6 +211,8 @@ void gd_egl_scanout_disable(DisplayChangeListener *dcl)
 {
     VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
 
+    eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
+                   vc->gfx.esurface, vc->gfx.ectx);
     vc->gfx.w = 0;
     vc->gfx.h = 0;
     gtk_egl_set_scanout_mode(vc, false);
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 682638a197..0b9fd4713a 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -270,6 +270,7 @@ void gd_gl_area_scanout_disable(DisplayChangeListener *dcl)
 {
     VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
 
+    gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
     gtk_gl_area_set_scanout_mode(vc, false);
 }
 
@@ -278,6 +279,7 @@ void gd_gl_area_scanout_flush(DisplayChangeListener *dcl,
 {
     VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
 
+    gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
     if (vc->gfx.guest_fb.dmabuf) {
         graphic_hw_gl_block(vc->gfx.dcl.con, true);
         vc->gfx.guest_fb.dmabuf->draw_submitted = true;
diff --git a/ui/gtk.c b/ui/gtk.c
index 1467b8c7d7..0ff31cb852 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1302,6 +1302,7 @@ static gboolean gd_tab_window_close(GtkWidget *widget, GdkEvent *event,
     VirtualConsole *vc = opaque;
     GtkDisplayState *s = vc->s;
 
+    dpy_gl_scanout_disable(vc->gfx.dcl.con);
     gtk_widget_set_sensitive(vc->menu_item, true);
     gd_widget_reparent(vc->window, s->notebook, vc->tab_item);
     gtk_notebook_set_tab_label_text(GTK_NOTEBOOK(s->notebook),
-- 
2.37.2

[PATCH v1 2/3] ui/gtk: Factor out tab window creation into a separate function

[Vivek Kasireddy]

Pull the code that creates a new window associated with a notebook
tab into a separate function. This new function can be useful not
just when user wants to detach a tab but also in the future when
a new window creation is needed in other scenarios.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/gtk.c | 65 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 0ff31cb852..945c550909 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1335,6 +1335,41 @@ static gboolean gd_win_grab(void *opaque)
     return TRUE;
 }
 
+static void gd_tab_window_create(VirtualConsole *vc)
+{
+    GtkDisplayState *s = vc->s;
+
+    gtk_widget_set_sensitive(vc->menu_item, false);
+    vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
+#if defined(CONFIG_OPENGL)
+    if (vc->gfx.esurface) {
+	eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
+	vc->gfx.esurface = NULL;
+    }
+    if (vc->gfx.esurface) {
+	eglDestroyContext(qemu_egl_display, vc->gfx.ectx);
+	vc->gfx.ectx = NULL;
+    }
+#endif
+    gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
+
+    g_signal_connect(vc->window, "delete-event",
+		     G_CALLBACK(gd_tab_window_close), vc);
+    gtk_widget_show_all(vc->window);
+
+    if (qemu_console_is_graphic(vc->gfx.dcl.con)) {
+	GtkAccelGroup *ag = gtk_accel_group_new();
+	gtk_window_add_accel_group(GTK_WINDOW(vc->window), ag);
+
+	GClosure *cb = g_cclosure_new_swap(G_CALLBACK(gd_win_grab),
+					   vc, NULL);
+	gtk_accel_group_connect(ag, GDK_KEY_g, HOTKEY_MODIFIERS, 0, cb);
+    }
+
+    gd_update_geometry_hints(vc);
+    gd_update_caption(s);
+}
+
 static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
 {
     GtkDisplayState *s = opaque;
@@ -1346,35 +1381,7 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
                                        FALSE);
     }
     if (!vc->window) {
-        gtk_widget_set_sensitive(vc->menu_item, false);
-        vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
-#if defined(CONFIG_OPENGL)
-        if (vc->gfx.esurface) {
-            eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
-            vc->gfx.esurface = NULL;
-        }
-        if (vc->gfx.esurface) {
-            eglDestroyContext(qemu_egl_display, vc->gfx.ectx);
-            vc->gfx.ectx = NULL;
-        }
-#endif
-        gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
-
-        g_signal_connect(vc->window, "delete-event",
-                         G_CALLBACK(gd_tab_window_close), vc);
-        gtk_widget_show_all(vc->window);
-
-        if (qemu_console_is_graphic(vc->gfx.dcl.con)) {
-            GtkAccelGroup *ag = gtk_accel_group_new();
-            gtk_window_add_accel_group(GTK_WINDOW(vc->window), ag);
-
-            GClosure *cb = g_cclosure_new_swap(G_CALLBACK(gd_win_grab),
-                                               vc, NULL);
-            gtk_accel_group_connect(ag, GDK_KEY_g, HOTKEY_MODIFIERS, 0, cb);
-        }
-
-        gd_update_geometry_hints(vc);
-        gd_update_caption(s);
+        gd_tab_window_create(vc);
     }
 }
 
-- 
2.37.2

[PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Vivek Kasireddy]

The new parameter named "connector" can be used to assign physical
monitors/connectors to individual GFX VCs such that when the monitor
is connected or hotplugged, the associated GTK window would be
fullscreened on it. If the monitor is disconnected or unplugged,
the associated GTK window would be destroyed and a relevant
disconnect event would be sent to the Guest.

Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
       -display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.....

Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 qapi/ui.json    |   9 ++-
 qemu-options.hx |   1 +
 ui/gtk.c        | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 286c5731d1..86787a4c95 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1199,13 +1199,20 @@
 #               interfaces (e.g. VGA and virtual console character devices)
 #               by default.
 #               Since 7.1
+# @connector:   List of physical monitor/connector names where the GTK windows
+#               containing the respective graphics virtual consoles (VCs) are
+#               to be placed. If a mapping exists for a VC, it will be
+#               fullscreened on that specific monitor or else it would not be
+#               displayed anywhere and would appear disconnected to the guest.
+#               Since 7.2
 #
 # Since: 2.12
 ##
 { 'struct'  : 'DisplayGTK',
   'data'    : { '*grab-on-hover' : 'bool',
                 '*zoom-to-fit'   : 'bool',
-                '*show-tabs'     : 'bool'  } }
+                '*show-tabs'     : 'bool',
+                '*connector'     : ['str']  } }
 
 ##
 # @DisplayEGLHeadless:
diff --git a/qemu-options.hx b/qemu-options.hx
index 31c04f7eea..576b65ef9f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
 #if defined(CONFIG_GTK)
     "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
     "            [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
+    "            [,connector.<id of VC>=<connector name>]\n"
 #endif
 #if defined(CONFIG_VNC)
     "-display vnc=<display>[,<optargs>]\n"
diff --git a/ui/gtk.c b/ui/gtk.c
index 945c550909..651aaaf174 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -37,6 +37,7 @@
 #include "qapi/qapi-commands-misc.h"
 #include "qemu/cutils.h"
 #include "qemu/main-loop.h"
+#include "qemu/option.h"
 
 #include "ui/console.h"
 #include "ui/gtk.h"
@@ -115,6 +116,7 @@
 #endif
 
 #define HOTKEY_MODIFIERS        (GDK_CONTROL_MASK | GDK_MOD1_MASK)
+#define MAX_NUM_ATTEMPTS 5
 
 static const guint16 *keycode_map;
 static size_t keycode_maplen;
@@ -126,6 +128,15 @@ struct VCChardev {
 };
 typedef struct VCChardev VCChardev;
 
+struct gd_monitor_data {
+    GtkDisplayState *s;
+    GdkDisplay *dpy;
+    GdkMonitor *monitor;
+    QEMUTimer *hp_timer;
+    int attempt;
+};
+typedef struct gd_monitor_data gd_monitor_data;
+
 #define TYPE_CHARDEV_VC "chardev-vc"
 DECLARE_INSTANCE_CHECKER(VCChardev, VC_CHARDEV,
                          TYPE_CHARDEV_VC)
@@ -1385,6 +1396,147 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
     }
 }
 
+static void gd_monitor_fullscreen(GdkDisplay *dpy, VirtualConsole *vc,
+                                  gint monitor_num)
+{
+    GtkDisplayState *s = vc->s;
+
+    if (!vc->window) {
+        gd_tab_window_create(vc);
+    }
+    gtk_window_fullscreen_on_monitor(GTK_WINDOW(vc->window),
+                                     gdk_display_get_default_screen(dpy),
+                                     monitor_num);
+    s->full_screen = TRUE;
+    gd_update_cursor(vc);
+}
+
+static int gd_monitor_lookup(GdkDisplay *dpy, char *label)
+{
+    GdkMonitor *monitor;
+    const char *monitor_name;
+    int i, total_monitors;
+
+    total_monitors = gdk_display_get_n_monitors(dpy);
+    for (i = 0; i < total_monitors; i++) {
+        monitor = gdk_display_get_monitor(dpy, i);
+        if (monitor) {
+            monitor_name = gdk_monitor_get_model(monitor);
+            if (monitor_name && !strcmp(monitor_name, label)) {
+                return i;
+            }
+        }
+    }
+    return -1;
+}
+
+static void gd_monitor_check_vcs(GdkDisplay *dpy, GdkMonitor *monitor,
+                                 GtkDisplayState *s)
+{
+    VirtualConsole *vc;
+    const char *monitor_name = gdk_monitor_get_model(monitor);
+    int i;
+
+    for (i = 0; i < s->nb_vcs; i++) {
+        vc = &s->vc[i];
+        if (!strcmp(vc->label, monitor_name)) {
+            gd_monitor_fullscreen(dpy, vc, gd_monitor_lookup(dpy, vc->label));
+            gd_set_ui_size(vc, surface_width(vc->gfx.ds),
+                           surface_height(vc->gfx.ds));
+            break;
+        }
+    }
+}
+
+static void gd_monitor_hotplug_timer(void *opaque)
+{
+    gd_monitor_data *data = opaque;
+    const char *monitor_name = gdk_monitor_get_model(data->monitor);
+
+    if (monitor_name) {
+        gd_monitor_check_vcs(data->dpy, data->monitor, data->s);
+    }
+    if (monitor_name || data->attempt == MAX_NUM_ATTEMPTS) {
+        timer_del(data->hp_timer);
+        g_free(data);
+    } else {
+        data->attempt++;
+        timer_mod(data->hp_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50);
+    }
+}
+
+static void gd_monitor_add(GdkDisplay *dpy, GdkMonitor *monitor,
+                           void *opaque)
+{
+    GtkDisplayState *s = opaque;
+    gd_monitor_data *data;
+    const char *monitor_name = gdk_monitor_get_model(monitor);
+
+    if (!monitor_name) {
+        data = g_malloc0(sizeof(*data));
+        data->s = s;
+        data->dpy = dpy;
+        data->monitor = monitor;
+        data->hp_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
+                                      gd_monitor_hotplug_timer, data);
+        timer_mod(data->hp_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50);
+    } else {
+        gd_monitor_check_vcs(dpy, monitor, s);
+    }
+}
+
+static void gd_monitor_remove(GdkDisplay *dpy, GdkMonitor *monitor,
+                              void *opaque)
+{
+    GtkDisplayState *s = opaque;
+    VirtualConsole *vc;
+    const char *monitor_name = gdk_monitor_get_model(monitor);
+    int i;
+
+    if (!monitor_name) {
+        return;
+    }
+    for (i = 0; i < s->nb_vcs; i++) {
+        vc = &s->vc[i];
+        if (!strcmp(vc->label, monitor_name)) {
+            gd_tab_window_close(NULL, NULL, vc);
+            gd_set_ui_size(vc, 0, 0);
+            break;
+        }
+    }
+}
+
+static void gd_connectors_init(GdkDisplay *dpy, GtkDisplayState *s)
+{
+    VirtualConsole *vc;
+    strList *connector = s->opts->u.gtk.connector;
+    gint page_num = 0, monitor_num;
+
+    gtk_notebook_set_show_tabs(GTK_NOTEBOOK(s->notebook), FALSE);
+    gtk_widget_hide(s->menu_bar);
+    for (; connector; connector = connector->next) {
+        vc = gd_vc_find_by_page(s, page_num);
+        if (!vc) {
+            break;
+        }
+        if (page_num == 0) {
+            vc->window = s->window;
+        }
+
+        g_free(vc->label);
+        vc->label = g_strdup(connector->value);
+        monitor_num = gd_monitor_lookup(dpy, vc->label);
+        if (monitor_num >= 0) {
+            gd_monitor_fullscreen(dpy, vc, monitor_num);
+            gd_set_ui_size(vc, surface_width(vc->gfx.ds),
+                           surface_height(vc->gfx.ds));
+        } else {
+            gd_set_ui_size(vc, 0, 0);
+        }
+        page_num++;
+    }
+}
+
 static void gd_menu_show_menubar(GtkMenuItem *item, void *opaque)
 {
     GtkDisplayState *s = opaque;
@@ -1705,7 +1857,14 @@ static gboolean gd_configure(GtkWidget *widget,
                              GdkEventConfigure *cfg, gpointer opaque)
 {
     VirtualConsole *vc = opaque;
+    GtkDisplayState *s = vc->s;
+    GtkWidget *parent = gtk_widget_get_parent(widget);
 
+    if (s->opts->u.gtk.has_connector) {
+        if (!parent || !GTK_IS_WINDOW(parent)) {
+            return FALSE;
+        }
+    }
     gd_set_ui_size(vc, cfg->width, cfg->height);
     return FALSE;
 }
@@ -2038,6 +2197,12 @@ static void gd_connect_signals(GtkDisplayState *s)
                      G_CALLBACK(gd_menu_grab_input), s);
     g_signal_connect(s->notebook, "switch-page",
                      G_CALLBACK(gd_change_page), s);
+    if (s->opts->u.gtk.has_connector) {
+        g_signal_connect(gtk_widget_get_display(s->window), "monitor-added",
+                         G_CALLBACK(gd_monitor_add), s);
+        g_signal_connect(gtk_widget_get_display(s->window), "monitor-removed",
+                         G_CALLBACK(gd_monitor_remove), s);
+    }
 }
 
 static GtkWidget *gd_create_menu_machine(GtkDisplayState *s)
@@ -2402,6 +2567,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
         opts->u.gtk.show_tabs) {
         gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
     }
+    if (s->opts->u.gtk.has_connector) {
+        gd_connectors_init(window_display, s);
+    }
     gd_clipboard_init(s);
 }
 
-- 
2.37.2

Re: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Markus Armbruster]

Any overlap with Dongwon Kim's "[PATCH v5 0/2] handling guest multiple
displays"?

Message-Id: <20220718233009.18780-1-dongwon.kim@intel.com>
https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03212.html

RE: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Kasireddy, Vivek]

Hi Markus,

> Any overlap with Dongwon Kim's "[PATCH v5 0/2] handling guest multiple
> displays"?
[Kasireddy, Vivek] Yes, there is some overlap but as I mentioned in the cover letter,
this series is intended to replace Dongwon's series dealing with multiple displays.

> 
> Message-Id: <20220718233009.18780-1-dongwon.kim@intel.com>
> https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03212.html
[Kasireddy, Vivek] We felt that using monitor numbers for display/VC assignment
would be cumbersome for users. And, given that his series does not take into account
monitor unplug/hotplug events, it's effectiveness would be limited compared to
this one.

Thanks,
Vivek

Re: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Markus Armbruster]

"Kasireddy, Vivek" <vivek.kasireddy@intel.com> writes:

> Hi Markus,
>
>> Any overlap with Dongwon Kim's "[PATCH v5 0/2] handling guest multiple
>> displays"?
>
> [Kasireddy, Vivek] Yes, there is some overlap but as I mentioned in the cover letter,
> this series is intended to replace Dongwon's series dealing with multiple displays.

I have no idea how I missed that part of your cover letter %-}

Dongwon Kim, would this series work for you?

>> Message-Id: <20220718233009.18780-1-dongwon.kim@intel.com>
>> https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03212.html
>
> [Kasireddy, Vivek] We felt that using monitor numbers for display/VC assignment
> would be cumbersome for users. And, given that his series does not take into account
> monitor unplug/hotplug events, it's effectiveness would be limited compared to
> this one.

Thanks!

Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Markus Armbruster]

Vivek Kasireddy <vivek.kasireddy@intel.com> writes:

> The new parameter named "connector" can be used to assign physical
> monitors/connectors to individual GFX VCs such that when the monitor
> is connected or hotplugged, the associated GTK window would be
> fullscreened on it. If the monitor is disconnected or unplugged,
> the associated GTK window would be destroyed and a relevant
> disconnect event would be sent to the Guest.
>
> Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
>        -display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.....
>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  qapi/ui.json    |   9 ++-
>  qemu-options.hx |   1 +
>  ui/gtk.c        | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 177 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 286c5731d1..86787a4c95 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1199,13 +1199,20 @@
>  #               interfaces (e.g. VGA and virtual console character devices)
>  #               by default.
>  #               Since 7.1
> +# @connector:   List of physical monitor/connector names where the GTK windows
> +#               containing the respective graphics virtual consoles (VCs) are
> +#               to be placed. If a mapping exists for a VC, it will be
> +#               fullscreened on that specific monitor or else it would not be
> +#               displayed anywhere and would appear disconnected to the guest.

Let's see whether I understand this...  We have VCs numbered 0, 1, ...
VC #i is mapped to the i-th element of @connector, counting from zero.
Correct?

Ignorant question: what's a "physical monitor/connector name"?

Would you mind breaking the lines a bit earlier, between column 70 and
75?

> +#               Since 7.2
>  #
>  # Since: 2.12
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
>                  '*zoom-to-fit'   : 'bool',
> -                '*show-tabs'     : 'bool'  } }
> +                '*show-tabs'     : 'bool',
> +                '*connector'     : ['str']  } }
>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 31c04f7eea..576b65ef9f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>  #if defined(CONFIG_GTK)
>      "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
>      "            [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
> +    "            [,connector.<id of VC>=<connector name>]\n"

Is "<id of VC>" a VC number?

>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"

Remainder of my review is on style only.  Style suggestions are not
demands :)

> diff --git a/ui/gtk.c b/ui/gtk.c
> index 945c550909..651aaaf174 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -37,6 +37,7 @@
>  #include "qapi/qapi-commands-misc.h"
>  #include "qemu/cutils.h"
>  #include "qemu/main-loop.h"
> +#include "qemu/option.h"
>  
>  #include "ui/console.h"
>  #include "ui/gtk.h"
> @@ -115,6 +116,7 @@
>  #endif
>  
>  #define HOTKEY_MODIFIERS        (GDK_CONTROL_MASK | GDK_MOD1_MASK)
> +#define MAX_NUM_ATTEMPTS 5

Could use a comment, and maybe a more telling name (this one doesn't
tell me what is being attempted).

>  
>  static const guint16 *keycode_map;
>  static size_t keycode_maplen;
> @@ -126,6 +128,15 @@ struct VCChardev {
>  };
>  typedef struct VCChardev VCChardev;
>  
> +struct gd_monitor_data {
> +    GtkDisplayState *s;
> +    GdkDisplay *dpy;
> +    GdkMonitor *monitor;
> +    QEMUTimer *hp_timer;
> +    int attempt;
> +};
> +typedef struct gd_monitor_data gd_monitor_data;

We usually contract these like

   typedef struct gd_monitor_data {
       ...
   } gd_monitor_data;

> +
>  #define TYPE_CHARDEV_VC "chardev-vc"
>  DECLARE_INSTANCE_CHECKER(VCChardev, VC_CHARDEV,
>                           TYPE_CHARDEV_VC)
> @@ -1385,6 +1396,147 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
>      }
>  }
>  
> +static void gd_monitor_fullscreen(GdkDisplay *dpy, VirtualConsole *vc,
> +                                  gint monitor_num)
> +{
> +    GtkDisplayState *s = vc->s;
> +
> +    if (!vc->window) {
> +        gd_tab_window_create(vc);
> +    }
> +    gtk_window_fullscreen_on_monitor(GTK_WINDOW(vc->window),
> +                                     gdk_display_get_default_screen(dpy),
> +                                     monitor_num);
> +    s->full_screen = TRUE;
> +    gd_update_cursor(vc);
> +}
> +
> +static int gd_monitor_lookup(GdkDisplay *dpy, char *label)
> +{
> +    GdkMonitor *monitor;
> +    const char *monitor_name;
> +    int i, total_monitors;
> +
> +    total_monitors = gdk_display_get_n_monitors(dpy);
> +    for (i = 0; i < total_monitors; i++) {

Suggest to format like this:

       int total_monitors = gdk_display_get_n_monitors(dpy);
       GdkMonitor *monitor;
       const char *monitor_name;
       int i;

       for (i = 0; i < total_monitors; i++) {

> +        monitor = gdk_display_get_monitor(dpy, i);
> +        if (monitor) {
> +            monitor_name = gdk_monitor_get_model(monitor);
> +            if (monitor_name && !strcmp(monitor_name, label)) {

Would

           if (monitor && !g_strcmp0(gdk_monitor_get_model(monitor), label)) {

do?

> +                return i;
> +            }
> +        }
> +    }
> +    return -1;
> +}
> +
> +static void gd_monitor_check_vcs(GdkDisplay *dpy, GdkMonitor *monitor,
> +                                 GtkDisplayState *s)
> +{
> +    VirtualConsole *vc;
> +    const char *monitor_name = gdk_monitor_get_model(monitor);
> +    int i;
> +
> +    for (i = 0; i < s->nb_vcs; i++) {
> +        vc = &s->vc[i];
> +        if (!strcmp(vc->label, monitor_name)) {
> +            gd_monitor_fullscreen(dpy, vc, gd_monitor_lookup(dpy, vc->label));
> +            gd_set_ui_size(vc, surface_width(vc->gfx.ds),
> +                           surface_height(vc->gfx.ds));
> +            break;
> +        }
> +    }
> +}
> +
> +static void gd_monitor_hotplug_timer(void *opaque)
> +{
> +    gd_monitor_data *data = opaque;
> +    const char *monitor_name = gdk_monitor_get_model(data->monitor);
> +
> +    if (monitor_name) {
> +        gd_monitor_check_vcs(data->dpy, data->monitor, data->s);
> +    }
> +    if (monitor_name || data->attempt == MAX_NUM_ATTEMPTS) {
> +        timer_del(data->hp_timer);
> +        g_free(data);
> +    } else {
> +        data->attempt++;
> +        timer_mod(data->hp_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50);

Suggest to break the line like

           timer_mod(data->hp_timer,
                     qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50);

for readability.

> +    }
> +}
> +
> +static void gd_monitor_add(GdkDisplay *dpy, GdkMonitor *monitor,
> +                           void *opaque)
> +{
> +    GtkDisplayState *s = opaque;
> +    gd_monitor_data *data;
> +    const char *monitor_name = gdk_monitor_get_model(monitor);
> +
> +    if (!monitor_name) {
> +        data = g_malloc0(sizeof(*data));
> +        data->s = s;
> +        data->dpy = dpy;
> +        data->monitor = monitor;
> +        data->hp_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> +                                      gd_monitor_hotplug_timer, data);
> +        timer_mod(data->hp_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50);
> +    } else {
> +        gd_monitor_check_vcs(dpy, monitor, s);
> +    }

Often

       if (cond) {
           do stuff when cond
       } else {
           do stuff when !cond
       }

is easier to read than

       if (!cond) {
           do stuff when !cond
       } else {
           do stuff when !!cond
       }

Give it a thought.

> +}
> +
> +static void gd_monitor_remove(GdkDisplay *dpy, GdkMonitor *monitor,
> +                              void *opaque)
> +{
> +    GtkDisplayState *s = opaque;
> +    VirtualConsole *vc;
> +    const char *monitor_name = gdk_monitor_get_model(monitor);
> +    int i;
> +
> +    if (!monitor_name) {
> +        return;
> +    }
> +    for (i = 0; i < s->nb_vcs; i++) {
> +        vc = &s->vc[i];
> +        if (!strcmp(vc->label, monitor_name)) {
> +            gd_tab_window_close(NULL, NULL, vc);
> +            gd_set_ui_size(vc, 0, 0);
> +            break;
> +        }
> +    }
> +}
> +
> +static void gd_connectors_init(GdkDisplay *dpy, GtkDisplayState *s)
> +{
> +    VirtualConsole *vc;
> +    strList *connector = s->opts->u.gtk.connector;
> +    gint page_num = 0, monitor_num;
> +
> +    gtk_notebook_set_show_tabs(GTK_NOTEBOOK(s->notebook), FALSE);
> +    gtk_widget_hide(s->menu_bar);
> +    for (; connector; connector = connector->next) {

Please don't split off part of the loop control.  What about

       for (conn = s->opts->u.gtk.connector; conn; conn = conn->next) {

?

> +        vc = gd_vc_find_by_page(s, page_num);
> +        if (!vc) {
> +            break;
> +        }
> +        if (page_num == 0) {
> +            vc->window = s->window;
> +        }
> +
> +        g_free(vc->label);
> +        vc->label = g_strdup(connector->value);
> +        monitor_num = gd_monitor_lookup(dpy, vc->label);
> +        if (monitor_num >= 0) {
> +            gd_monitor_fullscreen(dpy, vc, monitor_num);
> +            gd_set_ui_size(vc, surface_width(vc->gfx.ds),
> +                           surface_height(vc->gfx.ds));
> +        } else {
> +            gd_set_ui_size(vc, 0, 0);
> +        }
> +        page_num++;
> +    }
> +}
> +
>  static void gd_menu_show_menubar(GtkMenuItem *item, void *opaque)
>  {
>      GtkDisplayState *s = opaque;
> @@ -1705,7 +1857,14 @@ static gboolean gd_configure(GtkWidget *widget,
>                               GdkEventConfigure *cfg, gpointer opaque)
>  {
>      VirtualConsole *vc = opaque;
> +    GtkDisplayState *s = vc->s;
> +    GtkWidget *parent = gtk_widget_get_parent(widget);
>  
> +    if (s->opts->u.gtk.has_connector) {
> +        if (!parent || !GTK_IS_WINDOW(parent)) {
> +            return FALSE;
> +        }
> +    }
>      gd_set_ui_size(vc, cfg->width, cfg->height);
>      return FALSE;
>  }
> @@ -2038,6 +2197,12 @@ static void gd_connect_signals(GtkDisplayState *s)
>                       G_CALLBACK(gd_menu_grab_input), s);
>      g_signal_connect(s->notebook, "switch-page",
>                       G_CALLBACK(gd_change_page), s);
> +    if (s->opts->u.gtk.has_connector) {
> +        g_signal_connect(gtk_widget_get_display(s->window), "monitor-added",
> +                         G_CALLBACK(gd_monitor_add), s);
> +        g_signal_connect(gtk_widget_get_display(s->window), "monitor-removed",
> +                         G_CALLBACK(gd_monitor_remove), s);
> +    }
>  }
>  
>  static GtkWidget *gd_create_menu_machine(GtkDisplayState *s)
> @@ -2402,6 +2567,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>          opts->u.gtk.show_tabs) {
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
>      }
> +    if (s->opts->u.gtk.has_connector) {
> +        gd_connectors_init(window_display, s);
> +    }
>      gd_clipboard_init(s);
>  }

RE: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Kasireddy, Vivek]

Hi Markus,

Thank you for the review.

> Vivek Kasireddy <vivek.kasireddy@intel.com> writes:
> 
> > The new parameter named "connector" can be used to assign physical
> > monitors/connectors to individual GFX VCs such that when the monitor
> > is connected or hotplugged, the associated GTK window would be
> > fullscreened on it. If the monitor is disconnected or unplugged,
> > the associated GTK window would be destroyed and a relevant
> > disconnect event would be sent to the Guest.
> >
> > Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
> >        -display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.....
> >
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  qapi/ui.json    |   9 ++-
> >  qemu-options.hx |   1 +
> >  ui/gtk.c        | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 177 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 286c5731d1..86787a4c95 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1199,13 +1199,20 @@
> >  #               interfaces (e.g. VGA and virtual console character devices)
> >  #               by default.
> >  #               Since 7.1
> > +# @connector:   List of physical monitor/connector names where the GTK windows
> > +#               containing the respective graphics virtual consoles (VCs) are
> > +#               to be placed. If a mapping exists for a VC, it will be
> > +#               fullscreened on that specific monitor or else it would not be
> > +#               displayed anywhere and would appear disconnected to the guest.
> 
> Let's see whether I understand this...  We have VCs numbered 0, 1, ...
> VC #i is mapped to the i-th element of @connector, counting from zero.
> Correct?
[Kasireddy, Vivek] Yes, correct.

> 
> Ignorant question: what's a "physical monitor/connector name"?
[Kasireddy, Vivek] IIUC, while the HDMI/DP protocols refer to a receiver (RX)
as a "sink", the DRM Graphics subsystem in the kernel uses the term "connector"
and the GTK library prefers the term "monitor". All of these terms can be
used interchangeably but I chose the term connector for the new parameter
after debating between connector, monitor, output, etc. 
The connector name (e.g. DP-1, HDMI-A-2, etc) uniquely identifies a connector
or a monitor on any given system:
https://elixir.bootlin.com/linux/v6.0-rc6/source/include/drm/drm_connector.h#L1328
If you are on Intel hardware, you can find more info related to connectors by doing:
cat /sys/kernel/debug/dri/0/i915_display_info

> 
> Would you mind breaking the lines a bit earlier, between column 70 and
> 75?
[Kasireddy, Vivek] Np, will do that.

> 
> > +#               Since 7.2
> >  #
> >  # Since: 2.12
> >  ##
> >  { 'struct'  : 'DisplayGTK',
> >    'data'    : { '*grab-on-hover' : 'bool',
> >                  '*zoom-to-fit'   : 'bool',
> > -                '*show-tabs'     : 'bool'  } }
> > +                '*show-tabs'     : 'bool',
> > +                '*connector'     : ['str']  } }
> >
> >  ##
> >  # @DisplayEGLHeadless:
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 31c04f7eea..576b65ef9f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
> >  #if defined(CONFIG_GTK)
> >      "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> >      "            [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
> > +    "            [,connector.<id of VC>=<connector name>]\n"
> 
> Is "<id of VC>" a VC number?
[Kasireddy, Vivek] Yes.

> 
> >  #endif
> >  #if defined(CONFIG_VNC)
> >      "-display vnc=<display>[,<optargs>]\n"
> 
> Remainder of my review is on style only.  Style suggestions are not
> demands :)
[Kasireddy, Vivek] No problem; most of your suggestions are sensible
and I'll include them all in v2. 

> 
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 945c550909..651aaaf174 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -37,6 +37,7 @@
> >  #include "qapi/qapi-commands-misc.h"
> >  #include "qemu/cutils.h"
> >  #include "qemu/main-loop.h"
> > +#include "qemu/option.h"
> >
> >  #include "ui/console.h"
> >  #include "ui/gtk.h"
> > @@ -115,6 +116,7 @@
> >  #endif
> >
> >  #define HOTKEY_MODIFIERS        (GDK_CONTROL_MASK | GDK_MOD1_MASK)
> > +#define MAX_NUM_ATTEMPTS 5
> 
> Could use a comment, and maybe a more telling name (this one doesn't
> tell me what is being attempted).
[Kasireddy, Vivek] How about MAX_NUM_RETRIES?
gdk_monitor_get_model() can return NULL but if I wait for sometime 
(few milliseconds) and try again it may give a valid value. So, I need a 
macro to limit the number of attempts or retries. 

> 
> >
> >  static const guint16 *keycode_map;
> >  static size_t keycode_maplen;
> > @@ -126,6 +128,15 @@ struct VCChardev {
> >  };
> >  typedef struct VCChardev VCChardev;
> >
> > +struct gd_monitor_data {
> > +    GtkDisplayState *s;
> > +    GdkDisplay *dpy;
> > +    GdkMonitor *monitor;
> > +    QEMUTimer *hp_timer;
> > +    int attempt;
> > +};
> > +typedef struct gd_monitor_data gd_monitor_data;
> 
> We usually contract these like
> 
>    typedef struct gd_monitor_data {
>        ...
>    } gd_monitor_data;
[Kasireddy, Vivek] I was following the style in this file but sure I can
do that.

> 
> > +
> >  #define TYPE_CHARDEV_VC "chardev-vc"
> >  DECLARE_INSTANCE_CHECKER(VCChardev, VC_CHARDEV,
> >                           TYPE_CHARDEV_VC)
> > @@ -1385,6 +1396,147 @@ static void gd_menu_untabify(GtkMenuItem *item, void
> *opaque)
> >      }
> >  }
> >
> > +static void gd_monitor_fullscreen(GdkDisplay *dpy, VirtualConsole *vc,
> > +                                  gint monitor_num)
> > +{
> > +    GtkDisplayState *s = vc->s;
> > +
> > +    if (!vc->window) {
> > +        gd_tab_window_create(vc);
> > +    }
> > +    gtk_window_fullscreen_on_monitor(GTK_WINDOW(vc->window),
> > +                                     gdk_display_get_default_screen(dpy),
> > +                                     monitor_num);
> > +    s->full_screen = TRUE;
> > +    gd_update_cursor(vc);
> > +}
> > +
> > +static int gd_monitor_lookup(GdkDisplay *dpy, char *label)
> > +{
> > +    GdkMonitor *monitor;
> > +    const char *monitor_name;
> > +    int i, total_monitors;
> > +
> > +    total_monitors = gdk_display_get_n_monitors(dpy);
> > +    for (i = 0; i < total_monitors; i++) {
> 
> Suggest to format like this:
> 
>        int total_monitors = gdk_display_get_n_monitors(dpy);
>        GdkMonitor *monitor;
>        const char *monitor_name;
>        int i;
> 
>        for (i = 0; i < total_monitors; i++) {
[Kasireddy, Vivek] Makes sense; will do that.

> 
> > +        monitor = gdk_display_get_monitor(dpy, i);
> > +        if (monitor) {
> > +            monitor_name = gdk_monitor_get_model(monitor);
> > +            if (monitor_name && !strcmp(monitor_name, label)) {
> 
> Would
> 
>            if (monitor && !g_strcmp0(gdk_monitor_get_model(monitor), label)) {
> 
> do?
[Kasireddy, Vivek] Yes, that should work; didn't realize there was a Glib version
of a string compare function that can take NULL as well.

> 
> > +                return i;
> > +            }
> > +        }
> > +    }
> > +    return -1;
> > +}
> > +
> > +static void gd_monitor_check_vcs(GdkDisplay *dpy, GdkMonitor *monitor,
> > +                                 GtkDisplayState *s)
> > +{
> > +    VirtualConsole *vc;
> > +    const char *monitor_name = gdk_monitor_get_model(monitor);
> > +    int i;
> > +
> > +    for (i = 0; i < s->nb_vcs; i++) {
> > +        vc = &s->vc[i];
> > +        if (!strcmp(vc->label, monitor_name)) {
> > +            gd_monitor_fullscreen(dpy, vc, gd_monitor_lookup(dpy, vc->label));
> > +            gd_set_ui_size(vc, surface_width(vc->gfx.ds),
> > +                           surface_height(vc->gfx.ds));
> > +            break;
> > +        }
> > +    }
> > +}
> > +
> > +static void gd_monitor_hotplug_timer(void *opaque)
> > +{
> > +    gd_monitor_data *data = opaque;
> > +    const char *monitor_name = gdk_monitor_get_model(data->monitor);
> > +
> > +    if (monitor_name) {
> > +        gd_monitor_check_vcs(data->dpy, data->monitor, data->s);
> > +    }
> > +    if (monitor_name || data->attempt == MAX_NUM_ATTEMPTS) {
> > +        timer_del(data->hp_timer);
> > +        g_free(data);
> > +    } else {
> > +        data->attempt++;
> > +        timer_mod(data->hp_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> + 50);
> 
> Suggest to break the line like
> 
>            timer_mod(data->hp_timer,
>                      qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 50);
> 
> for readability.
[Kasireddy, Vivek] Ok.

> 
> > +    }
> > +}
> > +
> > +static void gd_monitor_add(GdkDisplay *dpy, GdkMonitor *monitor,
> > +                           void *opaque)
> > +{
> > +    GtkDisplayState *s = opaque;
> > +    gd_monitor_data *data;
> > +    const char *monitor_name = gdk_monitor_get_model(monitor);
> > +
> > +    if (!monitor_name) {
> > +        data = g_malloc0(sizeof(*data));
> > +        data->s = s;
> > +        data->dpy = dpy;
> > +        data->monitor = monitor;
> > +        data->hp_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> > +                                      gd_monitor_hotplug_timer, data);
> > +        timer_mod(data->hp_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> + 50);
> > +    } else {
> > +        gd_monitor_check_vcs(dpy, monitor, s);
> > +    }
> 
> Often
> 
>        if (cond) {
>            do stuff when cond
>        } else {
>            do stuff when !cond
>        }
> 
> is easier to read than
> 
>        if (!cond) {
>            do stuff when !cond
>        } else {
>            do stuff when !!cond
>        }
> 
> Give it a thought.
[Kasireddy, Vivek] Ok, makes sense; will do what you are suggesting.

> 
> > +}
> > +
> > +static void gd_monitor_remove(GdkDisplay *dpy, GdkMonitor *monitor,
> > +                              void *opaque)
> > +{
> > +    GtkDisplayState *s = opaque;
> > +    VirtualConsole *vc;
> > +    const char *monitor_name = gdk_monitor_get_model(monitor);
> > +    int i;
> > +
> > +    if (!monitor_name) {
> > +        return;
> > +    }
> > +    for (i = 0; i < s->nb_vcs; i++) {
> > +        vc = &s->vc[i];
> > +        if (!strcmp(vc->label, monitor_name)) {
> > +            gd_tab_window_close(NULL, NULL, vc);
> > +            gd_set_ui_size(vc, 0, 0);
> > +            break;
> > +        }
> > +    }
> > +}
> > +
> > +static void gd_connectors_init(GdkDisplay *dpy, GtkDisplayState *s)
> > +{
> > +    VirtualConsole *vc;
> > +    strList *connector = s->opts->u.gtk.connector;
> > +    gint page_num = 0, monitor_num;
> > +
> > +    gtk_notebook_set_show_tabs(GTK_NOTEBOOK(s->notebook), FALSE);
> > +    gtk_widget_hide(s->menu_bar);
> > +    for (; connector; connector = connector->next) {
> 
> Please don't split off part of the loop control.  What about
> 
>        for (conn = s->opts->u.gtk.connector; conn; conn = conn->next) {
> 
> ?
[Kasireddy, Vivek] Sure, I am ok with shortening the variable name.

Thanks,
Vivek

> 
> > +        vc = gd_vc_find_by_page(s, page_num);
> > +        if (!vc) {
> > +            break;
> > +        }
> > +        if (page_num == 0) {
> > +            vc->window = s->window;
> > +        }
> > +
> > +        g_free(vc->label);
> > +        vc->label = g_strdup(connector->value);
> > +        monitor_num = gd_monitor_lookup(dpy, vc->label);
> > +        if (monitor_num >= 0) {
> > +            gd_monitor_fullscreen(dpy, vc, monitor_num);
> > +            gd_set_ui_size(vc, surface_width(vc->gfx.ds),
> > +                           surface_height(vc->gfx.ds));
> > +        } else {
> > +            gd_set_ui_size(vc, 0, 0);
> > +        }
> > +        page_num++;
> > +    }
> > +}
> > +
> >  static void gd_menu_show_menubar(GtkMenuItem *item, void *opaque)
> >  {
> >      GtkDisplayState *s = opaque;
> > @@ -1705,7 +1857,14 @@ static gboolean gd_configure(GtkWidget *widget,
> >                               GdkEventConfigure *cfg, gpointer opaque)
> >  {
> >      VirtualConsole *vc = opaque;
> > +    GtkDisplayState *s = vc->s;
> > +    GtkWidget *parent = gtk_widget_get_parent(widget);
> >
> > +    if (s->opts->u.gtk.has_connector) {
> > +        if (!parent || !GTK_IS_WINDOW(parent)) {
> > +            return FALSE;
> > +        }
> > +    }
> >      gd_set_ui_size(vc, cfg->width, cfg->height);
> >      return FALSE;
> >  }
> > @@ -2038,6 +2197,12 @@ static void gd_connect_signals(GtkDisplayState *s)
> >                       G_CALLBACK(gd_menu_grab_input), s);
> >      g_signal_connect(s->notebook, "switch-page",
> >                       G_CALLBACK(gd_change_page), s);
> > +    if (s->opts->u.gtk.has_connector) {
> > +        g_signal_connect(gtk_widget_get_display(s->window), "monitor-added",
> > +                         G_CALLBACK(gd_monitor_add), s);
> > +        g_signal_connect(gtk_widget_get_display(s->window), "monitor-removed",
> > +                         G_CALLBACK(gd_monitor_remove), s);
> > +    }
> >  }
> >
> >  static GtkWidget *gd_create_menu_machine(GtkDisplayState *s)
> > @@ -2402,6 +2567,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions
> *opts)
> >          opts->u.gtk.show_tabs) {
> >          gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
> >      }
> > +    if (s->opts->u.gtk.has_connector) {
> > +        gd_connectors_init(window_display, s);
> > +    }
> >      gd_clipboard_init(s);
> >  }

Re: [PATCH v1 3/3] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Markus Armbruster]

"Kasireddy, Vivek" <vivek.kasireddy@intel.com> writes:

> Hi Markus,
>
> Thank you for the review.
>
>> Vivek Kasireddy <vivek.kasireddy@intel.com> writes:
>> 
>> > The new parameter named "connector" can be used to assign physical
>> > monitors/connectors to individual GFX VCs such that when the monitor
>> > is connected or hotplugged, the associated GTK window would be
>> > fullscreened on it. If the monitor is disconnected or unplugged,
>> > the associated GTK window would be destroyed and a relevant
>> > disconnect event would be sent to the Guest.
>> >
>> > Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,xres=1920,yres=1080...
>> >        -display gtk,gl=on,connector.0=eDP-1,connector.1=DP-1.....
>> >
>> > Cc: Dongwon Kim <dongwon.kim@intel.com>
>> > Cc: Gerd Hoffmann <kraxel@redhat.com>
>> > Cc: Daniel P. Berrangé <berrange@redhat.com>
>> > Cc: Markus Armbruster <armbru@redhat.com>
>> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Cc: Thomas Huth <thuth@redhat.com>
>> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> > ---
>> >  qapi/ui.json    |   9 ++-
>> >  qemu-options.hx |   1 +
>> >  ui/gtk.c        | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 177 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index 286c5731d1..86787a4c95 100644
>> > --- a/qapi/ui.json
>> > +++ b/qapi/ui.json
>> > @@ -1199,13 +1199,20 @@
>> >  #               interfaces (e.g. VGA and virtual console character devices)
>> >  #               by default.
>> >  #               Since 7.1
>> > +# @connector:   List of physical monitor/connector names where the GTK windows
>> > +#               containing the respective graphics virtual consoles (VCs) are
>> > +#               to be placed. If a mapping exists for a VC, it will be
>> > +#               fullscreened on that specific monitor or else it would not be
>> > +#               displayed anywhere and would appear disconnected to the guest.
>> 
>> Let's see whether I understand this...  We have VCs numbered 0, 1, ...
>> VC #i is mapped to the i-th element of @connector, counting from zero.
>> Correct?
>
> [Kasireddy, Vivek] Yes, correct.
>
>> Ignorant question: what's a "physical monitor/connector name"?
>
> [Kasireddy, Vivek] IIUC, while the HDMI/DP protocols refer to a receiver (RX)
> as a "sink", the DRM Graphics subsystem in the kernel uses the term "connector"
> and the GTK library prefers the term "monitor".

Awesome.

>                                                 All of these terms can be
> used interchangeably but I chose the term connector for the new parameter
> after debating between connector, monitor, output, etc. 

Okay.

> The connector name (e.g. DP-1, HDMI-A-2, etc) uniquely identifies a connector
> or a monitor on any given system:
> https://elixir.bootlin.com/linux/v6.0-rc6/source/include/drm/drm_connector.h#L1328
> If you are on Intel hardware, you can find more info related to connectors by doing:
> cat /sys/kernel/debug/dri/0/i915_display_info

Thanks!

>> Would you mind breaking the lines a bit earlier, between column 70 and
>> 75?
>
> [Kasireddy, Vivek] Np, will do that.
>
>> 
>> > +#               Since 7.2
>> >  #
>> >  # Since: 2.12
>> >  ##
>> >  { 'struct'  : 'DisplayGTK',
>> >    'data'    : { '*grab-on-hover' : 'bool',
>> >                  '*zoom-to-fit'   : 'bool',
>> > -                '*show-tabs'     : 'bool'  } }
>> > +                '*show-tabs'     : 'bool',
>> > +                '*connector'     : ['str']  } }
>> >
>> >  ##
>> >  # @DisplayEGLHeadless:

Elsewhere in ui.json, names of list-valued members use plural: channels,
clients, keys, addresses.  Let's name this one connectors for
consistency.

With that, QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>

>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 31c04f7eea..576b65ef9f 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -1945,6 +1945,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>> >  #if defined(CONFIG_GTK)
>> >      "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
>> >      "            [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
>> > +    "            [,connector.<id of VC>=<connector name>]\n"
>> 
>> Is "<id of VC>" a VC number?
>
> [Kasireddy, Vivek] Yes.

An "id" is commonly a name.  Suggest connector.<index>.


>> >  #endif
>> >  #if defined(CONFIG_VNC)
>> >      "-display vnc=<display>[,<optargs>]\n"

A bit of documentation is missing:

              ``show-cursor=on|off`` :  Force showing the mouse cursor

              ``window-close=on|off`` : Allow to quit qemu with window close button
     +        ``connector.<index>`` : <explanation>

          ``curses[,charset=<encoding>]``

>> 
>> Remainder of my review is on style only.  Style suggestions are not
>> demands :)
>
> [Kasireddy, Vivek] No problem; most of your suggestions are sensible
> and I'll include them all in v2. 

Thanks!

Re: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Kim, Dongwon]

Hi Markus,

Vivek and I have discussed this already. I am fine with replacing my old 
series with this.

Thanks,

DW

On 9/20/2022 11:06 PM, Markus Armbruster wrote:
> "Kasireddy, Vivek" <vivek.kasireddy@intel.com> writes:
>
>> Hi Markus,
>>
>>> Any overlap with Dongwon Kim's "[PATCH v5 0/2] handling guest multiple
>>> displays"?
>> [Kasireddy, Vivek] Yes, there is some overlap but as I mentioned in the cover letter,
>> this series is intended to replace Dongwon's series dealing with multiple displays.
> I have no idea how I missed that part of your cover letter %-}
>
> Dongwon Kim, would this series work for you?
>
>>> Message-Id: <20220718233009.18780-1-dongwon.kim@intel.com>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03212.html
>> [Kasireddy, Vivek] We felt that using monitor numbers for display/VC assignment
>> would be cumbersome for users. And, given that his series does not take into account
>> monitor unplug/hotplug events, it's effectiveness would be limited compared to
>> this one.
> Thanks!
>

Re: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Markus Armbruster]

"Kim, Dongwon" <dongwon.kim@intel.com> writes:

> Hi Markus,
>
> Vivek and I have discussed this already. I am fine with replacing my old series with this.

Good to know, thank you!