| Message ID | 20220917000731.465003-4-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows | expand |
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); > }
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); > > }
"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!
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); }
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(-)