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);
>  }