[PATCH 0/3] ui/gtk: paper over a gtk bug on Windows

[PATCH 0/3] ui/gtk: paper over a gtk bug on Windows

[Volker Rümelin]

Patch dc26435edb "ui/gtk: Update refresh interval after widget
is realized" exposed a bug in gtk on Windows. The monitor refresh
rate reported by gtk may be much smaller than the real refresh
rate leading to an unusable guest screen refresh rate.

Details can be found in [PATCH 3/3] ui/gtk: limit virtual console
max update interval

There will be a merging conflict with Nikola's patches.
https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg02604.html
https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg02633.html
One of us has to do a simple rebase.

Volker Rümelin (3):
  ui/gtk: don't try to redefine SI prefixes
  ui/gtk: rename variable window to widget
  ui/gtk: limit virtual console max update interval

 include/ui/gtk.h |  2 --
 ui/gtk.c         | 25 +++++++++++++------------
 2 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH 1/3] ui/gtk: don't try to redefine SI prefixes

[Volker Rümelin]

Redefining SI prefixes is always wrong. 1s has per definition
1000ms. Remove the misnamed named constant and replace it with
a comment explaining the frequency to period conversion in two
simple steps. Now you can cancel out the unit mHz in the comment
with the implicit unit mHz in refresh_rate_millihz and see why
the implicit unit ms for update_interval remains.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 include/ui/gtk.h | 2 --
 ui/gtk.c         | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index eaeb450f91..80851fb4c7 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -24,8 +24,6 @@
 #include "ui/egl-context.h"
 #endif
 
-#define MILLISEC_PER_SEC 1000000
-
 typedef struct GtkDisplayState GtkDisplayState;
 
 typedef struct VirtualGfxConsole {
diff --git a/ui/gtk.c b/ui/gtk.c
index a752aa22be..86b386a20d 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -798,7 +798,8 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t *cr, void *opaque)
     refresh_rate_millihz = gd_refresh_rate_millihz(vc->window ?
                                                    vc->window : s->window);
     if (refresh_rate_millihz) {
-        vc->gfx.dcl.update_interval = MILLISEC_PER_SEC / refresh_rate_millihz;
+        /* T = 1 / f = 1 [s*Hz] / f = 1000*1000 [ms*mHz] / f */
+        vc->gfx.dcl.update_interval = 1000 * 1000 / refresh_rate_millihz;
     }
 
     fbw = surface_width(vc->gfx.ds);
-- 
2.26.2

[PATCH 2/3] ui/gtk: rename variable window to widget

[Volker Rümelin]

The type of the variable window is GtkWidget. Rename the variable
from window to widget, because windows and widgets are different
things.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 ui/gtk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 86b386a20d..7ff9327b9d 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -752,13 +752,13 @@ static void gd_resize_event(GtkGLArea *area,
  * If available, return the refresh rate of the display in milli-Hertz,
  * else return 0.
  */
-static int gd_refresh_rate_millihz(GtkWidget *window)
+static int gd_refresh_rate_millihz(GtkWidget *widget)
 {
 #ifdef GDK_VERSION_3_22
-    GdkWindow *win = gtk_widget_get_window(window);
+    GdkWindow *win = gtk_widget_get_window(widget);
 
     if (win) {
-        GdkDisplay *dpy = gtk_widget_get_display(window);
+        GdkDisplay *dpy = gtk_widget_get_display(widget);
         GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
 
         return gdk_monitor_get_refresh_rate(monitor);
-- 
2.26.2

[PATCH 3/3] ui/gtk: limit virtual console max update interval

[Volker Rümelin]

Limit the virtual console maximum update interval to
GUI_REFRESH_INTERVAL_DEFAULT. This papers over a integer
overflow bug in gtk3 on Windows where the reported monitor
refresh frequency can be much smaller than the real refresh
frequency.

The gtk bug report can be found here:
https://gitlab.gnome.org/GNOME/gtk/-/issues/3394

On my Windows 10 system gtk reports a monitor refresh rate of
1.511Hz instead of 60.031Hz and slows down the screen update
rate in qemu to a crawl. Provided you are affected by the gtk
bug on Windows, these are the steps to reproduce the issue:

Start qemu with -display gtk and activate all qemu virtual
consoles and notice the reduced qemu refresh rate. Activating
all virtual consoles is necessary, because gui_update() in
ui/console.c uses the minimum of all display change listeners
update interval and not yet activated virtual consoles report
the default update interval (30ms).

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 ui/gtk.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 7ff9327b9d..78da5902f4 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -749,10 +749,10 @@ static void gd_resize_event(GtkGLArea *area,
 #endif
 
 /*
- * If available, return the refresh rate of the display in milli-Hertz,
- * else return 0.
+ * If available, return the update interval of the monitor in ms,
+ * else return 0 (the default update interval).
  */
-static int gd_refresh_rate_millihz(GtkWidget *widget)
+static int gd_monitor_update_interval(GtkWidget *widget)
 {
 #ifdef GDK_VERSION_3_22
     GdkWindow *win = gtk_widget_get_window(widget);
@@ -760,8 +760,13 @@ static int gd_refresh_rate_millihz(GtkWidget *widget)
     if (win) {
         GdkDisplay *dpy = gtk_widget_get_display(widget);
         GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
+        int refresh_rate = gdk_monitor_get_refresh_rate(monitor); /* [mHz] */
 
-        return gdk_monitor_get_refresh_rate(monitor);
+        if (refresh_rate) {
+            /* T = 1 / f = 1 [s*Hz] / f = 1000*1000 [ms*mHz] / f */
+            return MIN(1000 * 1000 / refresh_rate,
+                       GUI_REFRESH_INTERVAL_DEFAULT);
+        }
     }
 #endif
     return 0;
@@ -774,7 +779,6 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t *cr, void *opaque)
     int mx, my;
     int ww, wh;
     int fbw, fbh;
-    int refresh_rate_millihz;
 
 #if defined(CONFIG_OPENGL)
     if (vc->gfx.gls) {
@@ -795,12 +799,8 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t *cr, void *opaque)
         return FALSE;
     }
 
-    refresh_rate_millihz = gd_refresh_rate_millihz(vc->window ?
-                                                   vc->window : s->window);
-    if (refresh_rate_millihz) {
-        /* T = 1 / f = 1 [s*Hz] / f = 1000*1000 [ms*mHz] / f */
-        vc->gfx.dcl.update_interval = 1000 * 1000 / refresh_rate_millihz;
-    }
+    vc->gfx.dcl.update_interval =
+        gd_monitor_update_interval(vc->window ? vc->window : s->window);
 
     fbw = surface_width(vc->gfx.ds);
     fbh = surface_height(vc->gfx.ds);
-- 
2.26.2

Re: [PATCH 0/3] ui/gtk: paper over a gtk bug on Windows

[Gerd Hoffmann]

On Sun, Dec 13, 2020 at 05:56:28PM +0100, Volker Rümelin wrote:
> Patch dc26435edb "ui/gtk: Update refresh interval after widget
> is realized" exposed a bug in gtk on Windows. The monitor refresh
> rate reported by gtk may be much smaller than the real refresh
> rate leading to an unusable guest screen refresh rate.

Added to ui patch queue.

thanks,
  Gerd

Re: [PATCH 1/3] ui/gtk: don't try to redefine SI prefixes

[Philippe Mathieu-Daudé]

On 12/13/20 5:57 PM, Volker Rümelin wrote:
> Redefining SI prefixes is always wrong. 1s has per definition
> 1000ms. Remove the misnamed named constant and replace it with
> a comment explaining the frequency to period conversion in two
> simple steps. Now you can cancel out the unit mHz in the comment
> with the implicit unit mHz in refresh_rate_millihz and see why
> the implicit unit ms for update_interval remains.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  include/ui/gtk.h | 2 --
>  ui/gtk.c         | 3 ++-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/ui/gtk.h b/include/ui/gtk.h
> index eaeb450f91..80851fb4c7 100644
> --- a/include/ui/gtk.h
> +++ b/include/ui/gtk.h
> @@ -24,8 +24,6 @@
>  #include "ui/egl-context.h"
>  #endif
>  
> -#define MILLISEC_PER_SEC 1000000

Indeed this is MICROSEC_PER_SEC.

> -
>  typedef struct GtkDisplayState GtkDisplayState;
>  
>  typedef struct VirtualGfxConsole {
> diff --git a/ui/gtk.c b/ui/gtk.c
> index a752aa22be..86b386a20d 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -798,7 +798,8 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t *cr, void *opaque)
>      refresh_rate_millihz = gd_refresh_rate_millihz(vc->window ?
>                                                     vc->window : s->window);
>      if (refresh_rate_millihz) {
> -        vc->gfx.dcl.update_interval = MILLISEC_PER_SEC / refresh_rate_millihz;
> +        /* T = 1 / f = 1 [s*Hz] / f = 1000*1000 [ms*mHz] / f */
> +        vc->gfx.dcl.update_interval = 1000 * 1000 / refresh_rate_millihz;
>      }
>  
>      fbw = surface_width(vc->gfx.ds);
>