Hi Markus, Thank you for the review.
> Vivek Kasireddy <vivek.kasire...@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....@intel.com> > > Cc: Gerd Hoffmann <kra...@redhat.com> > > Cc: Daniel P. Berrangé <berra...@redhat.com> > > Cc: Markus Armbruster <arm...@redhat.com> > > Cc: Philippe Mathieu-Daudé <f4...@amsat.org> > > Cc: Marc-André Lureau <marcandre.lur...@redhat.com> > > Cc: Thomas Huth <th...@redhat.com> > > Signed-off-by: Vivek Kasireddy <vivek.kasire...@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); > > }