Re: [PATCH] display/gtk: get proper refreshrate

[Philippe Mathieu-Daudé]

Hi Nikola,

Cc'ing the qemu-devel list.

On 12/31/19 1:38 AM, Nikola Pavlica wrote:
> On Mon, 2019-12-30 at 23:59 +0100, Philippe Mathieu-Daudé wrote:
> > Hi Nikola,
> >
> > Thanks for your patch!
> >
> > On 12/30/19 6:28 PM, Nikola Pavlica wrote:
> > >   From 70c95b18fa056b2dd0ecc202ab517bc775b986da Mon Sep 17 00:00:00
> > > 2001
> > > From: Nikola Pavlica <address@hidden>
> > > Date: Mon, 30 Dec 2019 18:17:35 +0100
> > > Subject: [PATCH] display/gtk: get proper refreshrate
> >
> > Can you describe here the problem you encountered, and how your
> > patch
> > fixes it?
> >
> > > Signed-off-by: Nikola Pavlica <address@hidden>
> > > ---
> > >    ui/gtk.c | 5 +++++
> > >    1 file changed, 5 insertions(+)
> > >
> > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > index 692ccc7bbb..7a041457f2 100644
> > > --- a/ui/gtk.c
> > > +++ b/ui/gtk.c
> > > @@ -2259,6 +2259,11 @@ static void gtk_display_init(DisplayState
> > > *ds,
> > > DisplayOptions *opts)
> > >            opts->u.gtk.grab_on_hover) {
> > >            gtk_menu_item_activate(GTK_MENU_ITEM(s-
> > > > grab_on_hover_item));
> > >        }
> > > +
> > > +    GdkDisplay *display = gdk_display_get_default();
> >
> > Can we use window_display declared earlier instead?
> >
> >       window_display = gtk_widget_get_display(s->window);
> >
> > If you look at the CODING_STYLE.rst file referenced here:
> > https://wiki.qemu.org/Contribute/SubmitAPatch#Use_the_QEMU_coding_style
> > It states:
> >
> >     Declarations
> >     ============
> >
> >     Mixed declarations (interleaving statements and declarations
> >     within blocks) are generally not allowed; declarations should
> >     be at the beginning of blocks.
> >
> > So you should move the declaration of both display/monitor variables
> > earlier, around line 2192.
> >
> > > +    GdkMonitor *monitor =
> > > gdk_display_get_primary_monitor(display);
> > > +    vc->gfx.dcl.update_interval = 1000000 /
> > > +        gdk_monitor_get_refresh_rate(monitor);
> >
> > Now looking at this line, I think this should be done in the
> > gd_vc_gfx_init() function (line 2029, before the
> > register_displaychangelistener() call).
> >
> > >    }
> > >    
> > >    static void early_gtk_display_init(DisplayOptions *opts)
> >
> > As suggested on IRC, your patch have more chances to get reviewed if
> > you
> > Cc its maintainers. See this help here:
> > https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer
> >
> > In this case we get:
> >
> > $ ./scripts/get_maintainer.pl -f ui/gtk.c
> > Gerd Hoffmann <address@hidden> (odd fixer:Graphics)
> > address@hidden (open list:All patches CC here)
> >
> > I'm Cc'ing Gerd for you.
> >
> > Regards,
> >
> > Phil.

This patch is hard to review because it follows another patch...
See: 
https://wiki.qemu.org/Contribute/SubmitAPatch#Do_not_send_as_an_attachment

This is v2, next time post a v3 as a new thread please.

>  From f4934509ac2da1bbb747422990587433ecc44a0b Mon Sep 17 00:00:00 2001
> From: Nikola Pavlica <address@hidden>
> Date: Tue, 31 Dec 2019 01:12:42 +0100
> Subject: [PATCH v2] display/gtk: get proper refreshrate

<here goes the description of problem and fix>

> Signed-off-by: Nikola Pavlica <address@hidden>
> ---
>   ui/gtk.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 692ccc7bbb..2055dcc03d 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1967,6 +1967,10 @@ static GSList *gd_vc_gfx_init(GtkDisplayState
> *s, VirtualConsole *vc,
>   {
>       bool zoom_to_fit = false;
>   
> +    GdkDisplay *dpy = gtk_widget_get_display(s->window);
> +    GdkWindow *win = gtk_widget_get_window(s->window);
> +    GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
> +
>       vc->label = qemu_console_get_label(con);
>       vc->s = s;
>       vc->gfx.scale_x = 1.0;
> @@ -2026,6 +2030,8 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s,
> VirtualConsole *vc,
>   
>       vc->gfx.kbd = qkbd_state_init(con);
>       vc->gfx.dcl.con = con;
> +    vc->gfx.dcl.update_interval = 1000000 /
> +        gdk_monitor_get_refresh_rate(monitor);

From the documentation of gdk_monitor_get_refresh_rate():

  The value is in milli-Hertz, so a refresh rate of 60Hz is
  returned as 60000.

  Returns

    the refresh rate in milli-Hertz, or 0

Watch out we don't want to crash on division by zero. Maybe we don't 
want to set the update_interval if the refresh rate is null.

Also, I'd try to avoid the magic value and define something more obvious 
to review, such:

  #define MSEC_PER_MILLIHZ 1000000

  ...

  int refresh_rate_millihz;

  ...

  refresh_rate_millihz = gdk_monitor_get_refresh_rate()
  if (refresh_rate_millihz) {
      vc->gfx.dcl.update_interval = MSEC_PER_MILLIHZ / 
refresh_rate_millihz;
  }

>       register_displaychangelistener(&vc->gfx.dcl);
>   
>       gd_connect_vc_gfx_signals(vc);