diff mbox series

[2/3] ui/gtk: set the ui size to 0 when invisible

Message ID 20240130234840.53122-3-dongwon.kim@intel.com (mailing list archive)
State New, archived
Headers show
Series ui/gtk: introducing vc->visible | expand

Commit Message

Kim, Dongwon Jan. 30, 2024, 11:48 p.m. UTC 
From: Dongwon Kim <dongwon.kim@intel.com>

UI size is set to 0 when the VC is invisible, which will prevent
the further scanout update by notifying the guest that the display
is not in active state. Then it is restored to the original size
whenever the VC becomes visible again.

Cc: Marc-André Lureau <marcandre.lureau@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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Marc-André Lureau Jan. 31, 2024, 7:12 a.m. UTC | #1 
Hi

On Wed, Jan 31, 2024 at 3:50 AM <dongwon.kim@intel.com> wrote:
>
> From: Dongwon Kim <dongwon.kim@intel.com>
>
> UI size is set to 0 when the VC is invisible, which will prevent
> the further scanout update by notifying the guest that the display
> is not in active state. Then it is restored to the original size
> whenever the VC becomes visible again.

This can have unwanted results on multi monitor setups, such as moving
windows or icons around on different monitors.

Switching tabs or minimizing the display window shouldn't cause a
guest display reconfiguration.

What is the benefit of disabling the monitor here? Is it for
performance reasons?

>
> Cc: Marc-André Lureau <marcandre.lureau@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 | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 02eb667d8a..651ed3492f 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1314,10 +1314,12 @@ static void gd_menu_switch_vc(GtkMenuItem *item, void *opaque)
>      GtkDisplayState *s = opaque;
>      VirtualConsole *vc;
>      GtkNotebook *nb = GTK_NOTEBOOK(s->notebook);
> +    GdkWindow *window;
>      gint page;
>
>      vc = gd_vc_find_current(s);
>      vc->gfx.visible = false;
> +    gd_set_ui_size(vc, 0, 0);
>
>      vc = gd_vc_find_by_menu(s);
>      gtk_release_modifiers(s);
> @@ -1325,6 +1327,9 @@ static void gd_menu_switch_vc(GtkMenuItem *item, void *opaque)
>          page = gtk_notebook_page_num(nb, vc->tab_item);
>          gtk_notebook_set_current_page(nb, page);
>          gtk_widget_grab_focus(vc->focus);
> +        window = gtk_widget_get_window(vc->gfx.drawing_area);
> +        gd_set_ui_size(vc, gdk_window_get_width(window),
> +                       gdk_window_get_height(window));
>          vc->gfx.visible = true;
>      }
>  }
> @@ -1356,6 +1361,7 @@ static gboolean gd_tab_window_close(GtkWidget *widget, GdkEvent *event,
>      GtkDisplayState *s = vc->s;
>
>      vc->gfx.visible = false;
> +    gd_set_ui_size(vc, 0, 0);
>      gtk_widget_set_sensitive(vc->menu_item, true);
>      gd_widget_reparent(vc->window, s->notebook, vc->tab_item);
>      gtk_notebook_set_tab_label_text(GTK_NOTEBOOK(s->notebook),
> @@ -1391,6 +1397,7 @@ static gboolean gd_win_grab(void *opaque)
>  static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
>  {
>      GtkDisplayState *s = opaque;
> +    GdkWindow *window;
>      VirtualConsole *vc = gd_vc_find_current(s);
>
>      if (vc->type == GD_VC_GFX &&
> @@ -1429,6 +1436,10 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
>          gd_update_geometry_hints(vc);
>          gd_update_caption(s);
>      }
> +
> +    window = gtk_widget_get_window(vc->gfx.drawing_area);
> +    gd_set_ui_size(vc, gdk_window_get_width(window),
> +                   gdk_window_get_height(window));
>      vc->gfx.visible = true;
>  }
>
> @@ -1753,7 +1764,9 @@ static gboolean gd_configure(GtkWidget *widget,
>  {
>      VirtualConsole *vc = opaque;
>
> -    gd_set_ui_size(vc, cfg->width, cfg->height);
> +    if (vc->gfx.visible) {
> +        gd_set_ui_size(vc, cfg->width, cfg->height);
> +    }
>      return FALSE;
>  }
>
> --
> 2.34.1
>
>
Kim, Dongwon Jan. 31, 2024, 7:10 p.m. UTC | #2 
Hi Marc-André,

> -----Original Message-----
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> Sent: Tuesday, January 30, 2024 11:13 PM
> To: Kim, Dongwon <dongwon.kim@intel.com>
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 2/3] ui/gtk: set the ui size to 0 when invisible
> 
> Hi
> 
> On Wed, Jan 31, 2024 at 3:50 AM <dongwon.kim@intel.com> wrote:
> >
> > From: Dongwon Kim <dongwon.kim@intel.com>
> >
> > UI size is set to 0 when the VC is invisible, which will prevent the
> > further scanout update by notifying the guest that the display is not
> > in active state. Then it is restored to the original size whenever the
> > VC becomes visible again.
> 
> This can have unwanted results on multi monitor setups, such as moving
> windows or icons around on different monitors.

[Kim, Dongwon]  You are right. This is just a choice we made.
> 
> Switching tabs or minimizing the display window shouldn't cause a guest
> display reconfiguration.
> 
> What is the benefit of disabling the monitor here? Is it for performance reasons?

[Kim, Dongwon] Not sure if you recognized it but this patch series was a part of
our VM display hot-plug feature we submitted a few months ago. There, we added a new
param called connectors to have a way to fix individual VM displays (in multi display env)
on different physical displays there and made the VM display disconnected when
associated physical one is disconnected. We just wanted to make tab switching and
window minimization do the similar to make all of this logic consistent. 

However, if it makes more sense to have those displays all connected even when
those are not shown except for the case of hot-plug in, we could change the logic.
But as you mentioned, there will be some waste of bandwidth and perf since the
guest will keep sending out scan-out frames that would be just dumped.

This might be a minor thing but another concern is about tab-switching. Initially, the guest
will detect only one display even if the max-output is set to N (other than 1). Multi displays
will be detected once you detach or switch to another tab. Then if you move to the original
tab or close the detached window, the guest won't go back to single display setup.
All multi-displays will exist in its setup and this doesn’t look consistent to me.

> 
> >
> > Cc: Marc-André Lureau <marcandre.lureau@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 | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 02eb667d8a..651ed3492f 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1314,10 +1314,12 @@ static void gd_menu_switch_vc(GtkMenuItem
> *item, void *opaque)
> >      GtkDisplayState *s = opaque;
> >      VirtualConsole *vc;
> >      GtkNotebook *nb = GTK_NOTEBOOK(s->notebook);
> > +    GdkWindow *window;
> >      gint page;
> >
> >      vc = gd_vc_find_current(s);
> >      vc->gfx.visible = false;
> > +    gd_set_ui_size(vc, 0, 0);
> >
> >      vc = gd_vc_find_by_menu(s);
> >      gtk_release_modifiers(s);
> > @@ -1325,6 +1327,9 @@ static void gd_menu_switch_vc(GtkMenuItem
> *item, void *opaque)
> >          page = gtk_notebook_page_num(nb, vc->tab_item);
> >          gtk_notebook_set_current_page(nb, page);
> >          gtk_widget_grab_focus(vc->focus);
> > +        window = gtk_widget_get_window(vc->gfx.drawing_area);
> > +        gd_set_ui_size(vc, gdk_window_get_width(window),
> > +                       gdk_window_get_height(window));
> >          vc->gfx.visible = true;
> >      }
> >  }
> > @@ -1356,6 +1361,7 @@ static gboolean gd_tab_window_close(GtkWidget
> *widget, GdkEvent *event,
> >      GtkDisplayState *s = vc->s;
> >
> >      vc->gfx.visible = false;
> > +    gd_set_ui_size(vc, 0, 0);
> >      gtk_widget_set_sensitive(vc->menu_item, true);
> >      gd_widget_reparent(vc->window, s->notebook, vc->tab_item);
> >      gtk_notebook_set_tab_label_text(GTK_NOTEBOOK(s->notebook),
> > @@ -1391,6 +1397,7 @@ static gboolean gd_win_grab(void *opaque)
> > static void gd_menu_untabify(GtkMenuItem *item, void *opaque)  {
> >      GtkDisplayState *s = opaque;
> > +    GdkWindow *window;
> >      VirtualConsole *vc = gd_vc_find_current(s);
> >
> >      if (vc->type == GD_VC_GFX &&
> > @@ -1429,6 +1436,10 @@ static void gd_menu_untabify(GtkMenuItem
> *item, void *opaque)
> >          gd_update_geometry_hints(vc);
> >          gd_update_caption(s);
> >      }
> > +
> > +    window = gtk_widget_get_window(vc->gfx.drawing_area);
> > +    gd_set_ui_size(vc, gdk_window_get_width(window),
> > +                   gdk_window_get_height(window));
> >      vc->gfx.visible = true;
> >  }
> >
> > @@ -1753,7 +1764,9 @@ static gboolean gd_configure(GtkWidget *widget,
> > {
> >      VirtualConsole *vc = opaque;
> >
> > -    gd_set_ui_size(vc, cfg->width, cfg->height);
> > +    if (vc->gfx.visible) {
> > +        gd_set_ui_size(vc, cfg->width, cfg->height);
> > +    }
> >      return FALSE;
> >  }
> >
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau
Daniel P. Berrangé March 7, 2024, 9:47 a.m. UTC | #3 
On Wed, Jan 31, 2024 at 11:12:57AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jan 31, 2024 at 3:50 AM <dongwon.kim@intel.com> wrote:
> >
> > From: Dongwon Kim <dongwon.kim@intel.com>
> >
> > UI size is set to 0 when the VC is invisible, which will prevent
> > the further scanout update by notifying the guest that the display
> > is not in active state. Then it is restored to the original size
> > whenever the VC becomes visible again.
> 
> This can have unwanted results on multi monitor setups, such as moving
> windows or icons around on different monitors.
> 
> Switching tabs or minimizing the display window shouldn't cause a
> guest display reconfiguration.

I agree, changing the size of displays as a side-effect of
something that isn't a guest owner initiated resize operation
is asking for trouble.


With regards,
Daniel
diff mbox series

Patch

diff --git a/ui/gtk.c b/ui/gtk.c
index 02eb667d8a..651ed3492f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1314,10 +1314,12 @@  static void gd_menu_switch_vc(GtkMenuItem *item, void *opaque)
     GtkDisplayState *s = opaque;
     VirtualConsole *vc;
     GtkNotebook *nb = GTK_NOTEBOOK(s->notebook);
+    GdkWindow *window;
     gint page;
 
     vc = gd_vc_find_current(s);
     vc->gfx.visible = false;
+    gd_set_ui_size(vc, 0, 0);
 
     vc = gd_vc_find_by_menu(s);
     gtk_release_modifiers(s);
@@ -1325,6 +1327,9 @@  static void gd_menu_switch_vc(GtkMenuItem *item, void *opaque)
         page = gtk_notebook_page_num(nb, vc->tab_item);
         gtk_notebook_set_current_page(nb, page);
         gtk_widget_grab_focus(vc->focus);
+        window = gtk_widget_get_window(vc->gfx.drawing_area);
+        gd_set_ui_size(vc, gdk_window_get_width(window),
+                       gdk_window_get_height(window));
         vc->gfx.visible = true;
     }
 }
@@ -1356,6 +1361,7 @@  static gboolean gd_tab_window_close(GtkWidget *widget, GdkEvent *event,
     GtkDisplayState *s = vc->s;
 
     vc->gfx.visible = false;
+    gd_set_ui_size(vc, 0, 0);
     gtk_widget_set_sensitive(vc->menu_item, true);
     gd_widget_reparent(vc->window, s->notebook, vc->tab_item);
     gtk_notebook_set_tab_label_text(GTK_NOTEBOOK(s->notebook),
@@ -1391,6 +1397,7 @@  static gboolean gd_win_grab(void *opaque)
 static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
 {
     GtkDisplayState *s = opaque;
+    GdkWindow *window;
     VirtualConsole *vc = gd_vc_find_current(s);
 
     if (vc->type == GD_VC_GFX &&
@@ -1429,6 +1436,10 @@  static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
         gd_update_geometry_hints(vc);
         gd_update_caption(s);
     }
+
+    window = gtk_widget_get_window(vc->gfx.drawing_area);
+    gd_set_ui_size(vc, gdk_window_get_width(window),
+                   gdk_window_get_height(window));
     vc->gfx.visible = true;
 }
 
@@ -1753,7 +1764,9 @@  static gboolean gd_configure(GtkWidget *widget,
 {
     VirtualConsole *vc = opaque;
 
-    gd_set_ui_size(vc, cfg->width, cfg->height);
+    if (vc->gfx.visible) {
+        gd_set_ui_size(vc, cfg->width, cfg->height);
+    }
     return FALSE;
 }