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