[PATCH v2 0/2] handling guest multiple displays

[PATCH v2 0/2] handling guest multiple displays

[Dongwon Kim]

This patch seires is for adding some useful features for the guest os with
multi-displays. First patch is to make all of guest displays visible
when guest os is launched using "detach". Second patch is for providing
a method to assign each guest display to specific physical monitor,
which would be useful if someone wants to directly full-screen individual
guest scanouts to host's physical displays.

Changes in v2:

* ui/gtk: detach VCS for additional guest displays

  - check if the type of VC is GD_VC_GFX before qemu_console_is_graphic
    (Gerd Hoffman)
  - vc[0] is always primary guest display so we won't need n_gfx_vcs
    (Gerd Hoffmann)
  - making sure detached window's size same as original surface size
    (Daniel P. Berrangé)

Dongwon Kim (2):
  ui/gtk: detach VCS for additional guest displays (v2)
  ui/gtk: a new array param monitor to specify the target displays (v2)

 qapi/ui.json    |  7 ++++++-
 qemu-options.hx |  2 +-
 ui/gtk.c        | 43 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 48 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] ui/gtk: detach VCS for additional guest displays

[Dongwon Kim]

Detaching any addtional guest displays in case there are multiple
displays assigned to the guest OS (e.g. max_outputs=n) so that
all of them are visible upon lauching.

v2: - check if the type of VC is GD_VC_GFX before qemu_console_is_graphic
      (Gerd Hoffman)
    - vc[0] is always primary guest display so we won't need n_gfx_vcs
      (Gerd Hoffmann)
    - making sure detached window's size same as original surface size
      (Daniel P. Berrangé)

Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 ui/gtk.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 2a791dd2aa..e6878c3209 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1361,6 +1361,11 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
 
         g_signal_connect(vc->window, "delete-event",
                          G_CALLBACK(gd_tab_window_close), vc);
+
+        gtk_window_set_default_size(GTK_WINDOW(vc->window),
+                                    surface_width(vc->gfx.ds),
+                                    surface_height(vc->gfx.ds));
+
         gtk_widget_show_all(vc->window);
 
         if (qemu_console_is_graphic(vc->gfx.dcl.con)) {
@@ -2311,6 +2316,7 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
     GtkDisplayState *s = g_malloc0(sizeof(*s));
     GdkDisplay *window_display;
     GtkIconTheme *theme;
+    int i;
     char *dir;
 
     if (!gtkinit) {
@@ -2381,7 +2387,12 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
     gtk_widget_set_sensitive(s->copy_item,
                              vc && vc->type == GD_VC_VTE);
 #endif
-
+    for (i = 1; i < s->nb_vcs; i++) {
+        if (vc->type == GD_VC_GFX &&
+            qemu_console_is_graphic(s->vc[i].gfx.dcl.con)) {
+            gtk_menu_item_activate(GTK_MENU_ITEM(s->untabify_item));
+        }
+    }
     if (opts->has_full_screen &&
         opts->full_screen) {
         gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
-- 
2.30.2

[PATCH v2 2/2] ui/gtk: a new array param monitor to specify the target displays

[Dongwon Kim]

New integer array parameter, 'monitor' is for specifying the target
displays where individual QEMU windows are placed upon launching.

The array contains a series of numbers representing the monitor where
QEMU windows are placed.

Numbers in the array are mapped to QEMU windows like,

[1st detached window, 2nd detached window,.... Main window]

Usage example: -display gtk,monitor.0=0,monitor.1=1.....

Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 qapi/ui.json    |  7 ++++++-
 qemu-options.hx |  2 +-
 ui/gtk.c        | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 413371d5e8..5980f30c7f 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1195,12 +1195,17 @@
 #               assuming the guest will resize the display to match
 #               the window size then.  Otherwise it defaults to "off".
 #               Since 3.1
+# @monitor:     Array of numbers, each of which represents physical
+#               monitor where individual QEMU window is placed in case
+#               there are multiple of them
+#               since 7.1
 #
 # Since: 2.12
 ##
 { 'struct'  : 'DisplayGTK',
   'data'    : { '*grab-on-hover' : 'bool',
-                '*zoom-to-fit'   : 'bool'  } }
+                '*zoom-to-fit'   : 'bool',
+                '*monitor'       : ['uint16']  } }
 
 ##
 # @DisplayEGLHeadless:
diff --git a/qemu-options.hx b/qemu-options.hx
index 377d22fbd8..f79f533e9d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1938,7 +1938,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
 #endif
 #if defined(CONFIG_GTK)
     "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
-    "            [,show-cursor=on|off][,window-close=on|off]\n"
+    "            [,monitor.<order>=<value>][,show-cursor=on|off][,window-close=on|off]\n"
 #endif
 #if defined(CONFIG_VNC)
     "-display vnc=<display>[,<optargs>]\n"
diff --git a/ui/gtk.c b/ui/gtk.c
index e6878c3209..fc9bf04680 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2316,6 +2316,10 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
     GtkDisplayState *s = g_malloc0(sizeof(*s));
     GdkDisplay *window_display;
     GtkIconTheme *theme;
+    GtkWidget *win;
+    GdkRectangle dest;
+    uint16List *mon;
+    int n_mon;
     int i;
     char *dir;
 
@@ -2393,7 +2397,33 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
             gtk_menu_item_activate(GTK_MENU_ITEM(s->untabify_item));
         }
     }
-    if (opts->has_full_screen &&
+    if (opts->u.gtk.has_monitor) {
+        i = 0;
+        n_mon = gdk_display_get_n_monitors(window_display);
+        for (mon = opts->u.gtk.monitor; mon; mon = mon->next) {
+            if (mon->value < n_mon) {
+                for (; i < s->nb_vcs; i++) {
+                    win = s->vc[i].window ? s->vc[i].window : s->window;
+                    if (opts->has_full_screen && opts->full_screen) {
+                        gtk_window_fullscreen_on_monitor(
+                            GTK_WINDOW(win),
+                            gdk_display_get_default_screen(window_display),
+                            mon->value);
+                    } else {
+                        gdk_monitor_get_geometry(
+                            gdk_display_get_monitor(window_display, mon->value),
+                            &dest);
+                        gtk_window_move(GTK_WINDOW(win),
+                                        dest.x, dest.y);
+                    }
+                    i++;
+                    break;
+                }
+            }
+        }
+    }
+    if (!opts->u.gtk.has_monitor &&
+        opts->has_full_screen &&
         opts->full_screen) {
         gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
     }
-- 
2.30.2

Re: [PATCH v2 2/2] ui/gtk: a new array param monitor to specify the target displays

[Markus Armbruster]

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

> New integer array parameter, 'monitor' is for specifying the target
> displays where individual QEMU windows are placed upon launching.
>
> The array contains a series of numbers representing the monitor where
> QEMU windows are placed.
>
> Numbers in the array are mapped to QEMU windows like,
>
> [1st detached window, 2nd detached window,.... Main window]
>
> Usage example: -display gtk,monitor.0=0,monitor.1=1.....
>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  qapi/ui.json    |  7 ++++++-
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 32 +++++++++++++++++++++++++++++++-
>  3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 413371d5e8..5980f30c7f 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1195,12 +1195,17 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> +# @monitor:     Array of numbers, each of which represents physical
> +#               monitor where individual QEMU window is placed in case
> +#               there are multiple of them

End you sentence with a period, and ...

> +#               since 7.1

... start the next phrase with a capital letter.

The documentation text feels vague.  Possibly because I lack familiarity
with this part of the user interface.  What are the "individual QEMU
windows"?  How are they numbered?

>  #
>  # Since: 2.12
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*monitor'       : ['uint16']  } }
>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 377d22fbd8..f79f533e9d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1938,7 +1938,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>  #endif
>  #if defined(CONFIG_GTK)
>      "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> -    "            [,show-cursor=on|off][,window-close=on|off]\n"
> +    "            [,monitor.<order>=<value>][,show-cursor=on|off][,window-close=on|off]\n"
>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"
> diff --git a/ui/gtk.c b/ui/gtk.c
> index e6878c3209..fc9bf04680 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2316,6 +2316,10 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>      GtkDisplayState *s = g_malloc0(sizeof(*s));
>      GdkDisplay *window_display;
>      GtkIconTheme *theme;
> +    GtkWidget *win;
> +    GdkRectangle dest;
> +    uint16List *mon;
> +    int n_mon;
>      int i;
>      char *dir;
>  
> @@ -2393,7 +2397,33 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>              gtk_menu_item_activate(GTK_MENU_ITEM(s->untabify_item));
>          }
>      }
> -    if (opts->has_full_screen &&
> +    if (opts->u.gtk.has_monitor) {
> +        i = 0;
> +        n_mon = gdk_display_get_n_monitors(window_display);
> +        for (mon = opts->u.gtk.monitor; mon; mon = mon->next) {
> +            if (mon->value < n_mon) {
> +                for (; i < s->nb_vcs; i++) {
> +                    win = s->vc[i].window ? s->vc[i].window : s->window;
> +                    if (opts->has_full_screen && opts->full_screen) {
> +                        gtk_window_fullscreen_on_monitor(
> +                            GTK_WINDOW(win),
> +                            gdk_display_get_default_screen(window_display),
> +                            mon->value);
> +                    } else {
> +                        gdk_monitor_get_geometry(
> +                            gdk_display_get_monitor(window_display, mon->value),
> +                            &dest);
> +                        gtk_window_move(GTK_WINDOW(win),
> +                                        dest.x, dest.y);
> +                    }
> +                    i++;
> +                    break;
> +                }

This loop is odd.  It's of the form

                   for (; COND; STEP) {
                       ...
                       break;
                   }

STEP is unreachable.  The whole thing boils down to

                   if (COND) {
                       ....
                   }

doesn't it?

> +            }
> +        }
> +    }
> +    if (!opts->u.gtk.has_monitor &&
> +        opts->has_full_screen &&
>          opts->full_screen) {
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
>      }


This is

       if (COND1) {
           ...
       }
       if (!COND1 && COND2) {
           ...
       }

I'd prefer

       if (COND1) {
           ...
       } else if (COND2) {
           ...
       }

Re: [PATCH v2 2/2] ui/gtk: a new array param monitor to specify the target displays

[Dongwon Kim]

Hi Markus,

On Mon, Jun 20, 2022 at 09:07:04AM +0200, Markus Armbruster wrote:
> Dongwon Kim <dongwon.kim@intel.com> writes:
> 
> > New integer array parameter, 'monitor' is for specifying the target
> > displays where individual QEMU windows are placed upon launching.
> >
> > The array contains a series of numbers representing the monitor where
> > QEMU windows are placed.
> >
> > Numbers in the array are mapped to QEMU windows like,
> >
> > [1st detached window, 2nd detached window,.... Main window]
> >
> > Usage example: -display gtk,monitor.0=0,monitor.1=1.....
> >
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> > ---
> >  qapi/ui.json    |  7 ++++++-
> >  qemu-options.hx |  2 +-
> >  ui/gtk.c        | 32 +++++++++++++++++++++++++++++++-
> >  3 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 413371d5e8..5980f30c7f 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1195,12 +1195,17 @@
> >  #               assuming the guest will resize the display to match
> >  #               the window size then.  Otherwise it defaults to "off".
> >  #               Since 3.1
> > +# @monitor:     Array of numbers, each of which represents physical
> > +#               monitor where individual QEMU window is placed in case
> > +#               there are multiple of them
> 
> End you sentence with a period, and ...
> 
> > +#               since 7.1
> 
> ... start the next phrase with a capital letter.
> 
> The documentation text feels vague.  Possibly because I lack familiarity
> with this part of the user interface.  What are the "individual QEMU
> windows"?  How are they numbered?
> 

Will rework on the phrase. So there is only one QEMU window normally
when you start the guest os. And this window usually contains multiple
virtual consoles by default. You can detach any of them by clicking detach
menu item from UI. In this case, a new window will be created for the
detached VC. By doing this, we could have primary window and newly
created n number of windows. Individual windows means these windows.

In this patch, VC number is being used as windows number. The primary
one - guest display is '0'. The guest's secondary display will be '1'
and so on.

Thanks for the feedback.

> >  #
> >  # Since: 2.12
> >  ##
> >  { 'struct'  : 'DisplayGTK',
> >    'data'    : { '*grab-on-hover' : 'bool',
> > -                '*zoom-to-fit'   : 'bool'  } }
> > +                '*zoom-to-fit'   : 'bool',
> > +                '*monitor'       : ['uint16']  } }
> >  
> >  ##
> >  # @DisplayEGLHeadless:
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 377d22fbd8..f79f533e9d 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1938,7 +1938,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
> >  #endif
> >  #if defined(CONFIG_GTK)
> >      "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> > -    "            [,show-cursor=on|off][,window-close=on|off]\n"
> > +    "            [,monitor.<order>=<value>][,show-cursor=on|off][,window-close=on|off]\n"
> >  #endif
> >  #if defined(CONFIG_VNC)
> >      "-display vnc=<display>[,<optargs>]\n"
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index e6878c3209..fc9bf04680 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -2316,6 +2316,10 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
> >      GtkDisplayState *s = g_malloc0(sizeof(*s));
> >      GdkDisplay *window_display;
> >      GtkIconTheme *theme;
> > +    GtkWidget *win;
> > +    GdkRectangle dest;
> > +    uint16List *mon;
> > +    int n_mon;
> >      int i;
> >      char *dir;
> >  
> > @@ -2393,7 +2397,33 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
> >              gtk_menu_item_activate(GTK_MENU_ITEM(s->untabify_item));
> >          }
> >      }
> > -    if (opts->has_full_screen &&
> > +    if (opts->u.gtk.has_monitor) {
> > +        i = 0;
> > +        n_mon = gdk_display_get_n_monitors(window_display);
> > +        for (mon = opts->u.gtk.monitor; mon; mon = mon->next) {
> > +            if (mon->value < n_mon) {
> > +                for (; i < s->nb_vcs; i++) {
> > +                    win = s->vc[i].window ? s->vc[i].window : s->window;
> > +                    if (opts->has_full_screen && opts->full_screen) {
> > +                        gtk_window_fullscreen_on_monitor(
> > +                            GTK_WINDOW(win),
> > +                            gdk_display_get_default_screen(window_display),
> > +                            mon->value);
> > +                    } else {
> > +                        gdk_monitor_get_geometry(
> > +                            gdk_display_get_monitor(window_display, mon->value),
> > +                            &dest);
> > +                        gtk_window_move(GTK_WINDOW(win),
> > +                                        dest.x, dest.y);
> > +                    }
> > +                    i++;
> > +                    break;
> > +                }
> 
> This loop is odd.  It's of the form
> 
>                    for (; COND; STEP) {
>                        ...
>                        break;
>                    }
> 
> STEP is unreachable.  The whole thing boils down to
> 
>                    if (COND) {
>                        ....
>                    }
> 
> doesn't it?

You are definitely right. if (COND) should be the one here. I will fix this in the
next version.

> 
> > +            }
> > +        }
> > +    }
> > +    if (!opts->u.gtk.has_monitor &&
> > +        opts->has_full_screen &&
> >          opts->full_screen) {
> >          gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
> >      }
> 
> 
> This is
> 
>        if (COND1) {
>            ...
>        }
>        if (!COND1 && COND2) {
>            ...
>        }
> 
> I'd prefer
> 
>        if (COND1) {
>            ...
>        } else if (COND2) {
>            ...
>        }
> 

I will take a look at this as well.