[PATCH] virtio-gpu: Respect graphics update interval for EDID

[PATCH] virtio-gpu: Respect graphics update interval for EDID

[Akihiko Odaki]

This change introduces an additional member, refresh_rate to
qemu_edid_info in include/hw/display/edid.h.

This change also isolates the graphics update interval from the
display update interval. The guest will update the frame buffer
in the graphics update interval, but displays can be updated in a
dynamic interval, for example to save update costs aggresively
(vnc) or to respond to user-generated events (sdl).
It stabilizes the graphics update interval and prevents the guest
from being confused.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 hw/display/edid-generate.c     |  7 ++++---
 hw/display/virtio-gpu-base.c   | 11 +++++++++++
 hw/display/virtio-gpu.c        |  1 +
 hw/display/xenfb.c             |  2 +-
 include/hw/display/edid.h      |  1 +
 include/hw/virtio/virtio-gpu.h |  2 ++
 include/ui/console.h           |  3 ++-
 ui/console.c                   | 21 ++++++++++++++++-----
 ui/gtk.c                       |  2 +-
 9 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/hw/display/edid-generate.c b/hw/display/edid-generate.c
index 1665b7cbb29..7317e68506b 100644
--- a/hw/display/edid-generate.c
+++ b/hw/display/edid-generate.c
@@ -203,7 +203,7 @@ static void edid_desc_dummy(uint8_t *desc)
     edid_desc_type(desc, 0x10);
 }
 
-static void edid_desc_timing(uint8_t *desc,
+static void edid_desc_timing(uint8_t *desc, uint32_t refresh_rate,
                              uint32_t xres, uint32_t yres,
                              uint32_t xmm, uint32_t ymm)
 {
@@ -216,7 +216,7 @@ static void edid_desc_timing(uint8_t *desc,
     uint32_t ysync  = yres *  5 / 1000;
     uint32_t yblank = yres * 35 / 1000;
 
-    uint32_t clock  = 75 * (xres + xblank) * (yres + yblank);
+    uint32_t clock  = refresh_rate * (xres + xblank) * (yres + yblank);
 
     stl_le_p(desc, clock / 10000);
 
@@ -303,6 +303,7 @@ void qemu_edid_generate(uint8_t *edid, size_t size,
     uint8_t *xtra3 = NULL;
     uint8_t *dta = NULL;
     uint32_t width_mm, height_mm;
+    uint32_t refresh_rate = info->refresh_rate ? info->refresh_rate : 75;
     uint32_t dpi = 100; /* if no width_mm/height_mm */
 
     /* =============== set defaults  =============== */
@@ -400,7 +401,7 @@ void qemu_edid_generate(uint8_t *edid, size_t size,
 
     /* =============== descriptor blocks =============== */
 
-    edid_desc_timing(edid + desc, info->prefx, info->prefy,
+    edid_desc_timing(edid + desc, refresh_rate, info->prefx, info->prefy,
                      width_mm, height_mm);
     desc += 18;
 
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 4a57350917c..41b08b2e944 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -96,6 +96,16 @@ static int virtio_gpu_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
     return 0;
 }
 
+static void virtio_gpu_update_display_interval(void *opaque, uint64_t interval)
+{
+    VirtIOGPUBase *g = opaque;
+
+    g->refresh_rate = 1000 / interval;
+
+    /* send event to guest */
+    virtio_gpu_notify_event(g, VIRTIO_GPU_EVENT_DISPLAY);
+}
+
 static void
 virtio_gpu_gl_flushed(void *opaque)
 {
@@ -142,6 +152,7 @@ static const GraphicHwOps virtio_gpu_ops = {
     .invalidate = virtio_gpu_invalidate_display,
     .gfx_update = virtio_gpu_update_display,
     .text_update = virtio_gpu_text_update,
+    .gfx_update_interval = virtio_gpu_update_display_interval,
     .ui_info = virtio_gpu_ui_info,
     .gl_block = virtio_gpu_gl_block,
     .gl_flushed = virtio_gpu_gl_flushed,
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 2e4a9822b6a..64fdc5a6e89 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -216,6 +216,7 @@ virtio_gpu_generate_edid(VirtIOGPU *g, int scanout,
         .height_mm = b->req_state[scanout].height_mm,
         .prefx = b->req_state[scanout].width,
         .prefy = b->req_state[scanout].height,
+        .refresh_rate = b->refresh_rate,
     };
 
     edid->size = cpu_to_le32(sizeof(edid->edid));
diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 838260b6ad1..4229f9a42df 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -983,5 +983,5 @@ struct XenDevOps xen_framebuffer_ops = {
 static const GraphicHwOps xenfb_ops = {
     .invalidate  = xenfb_invalidate,
     .gfx_update  = xenfb_update,
-    .update_interval = xenfb_update_interval,
+    .gfx_update_interval = xenfb_update_interval,
 };
diff --git a/include/hw/display/edid.h b/include/hw/display/edid.h
index 1f8fc9b3750..9617705cc0a 100644
--- a/include/hw/display/edid.h
+++ b/include/hw/display/edid.h
@@ -11,6 +11,7 @@ typedef struct qemu_edid_info {
     uint32_t    prefy;
     uint32_t    maxx;
     uint32_t    maxy;
+    uint32_t    refresh_rate;
 } qemu_edid_info;
 
 void qemu_edid_generate(uint8_t *edid, size_t size,
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index fae149235c5..9d1a547ba10 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -116,6 +116,8 @@ struct VirtIOGPUBase {
 
     int enabled_output_bitmask;
     struct virtio_gpu_requested_state req_state[VIRTIO_GPU_MAX_SCANOUTS];
+
+    uint32_t refresh_rate;
 };
 
 struct VirtIOGPUBaseClass {
diff --git a/include/ui/console.h b/include/ui/console.h
index d30e972d0b5..0bcb610b80a 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -246,6 +246,7 @@ typedef struct DisplayChangeListenerOps {
 } DisplayChangeListenerOps;
 
 struct DisplayChangeListener {
+    uint64_t gfx_update_interval;
     uint64_t update_interval;
     const DisplayChangeListenerOps *ops;
     DisplayState *ds;
@@ -384,7 +385,7 @@ typedef struct GraphicHwOps {
     void (*gfx_update)(void *opaque);
     bool gfx_update_async; /* if true, calls graphic_hw_update_done() */
     void (*text_update)(void *opaque, console_ch_t *text);
-    void (*update_interval)(void *opaque, uint64_t interval);
+    void (*gfx_update_interval)(void *opaque, uint64_t interval);
     int (*ui_info)(void *opaque, uint32_t head, QemuUIInfo *info);
     void (*gl_block)(void *opaque, bool block);
     void (*gl_flushed)(void *opaque);
diff --git a/ui/console.c b/ui/console.c
index c5d11bc7017..928fb009db1 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -176,6 +176,7 @@ struct QemuConsole {
 struct DisplayState {
     QEMUTimer *gui_timer;
     uint64_t last_update;
+    uint64_t gfx_update_interval;
     uint64_t update_interval;
     bool refreshing;
     bool have_gfx;
@@ -200,6 +201,7 @@ static void text_console_update_cursor(void *opaque);
 static void gui_update(void *opaque)
 {
     uint64_t interval = GUI_REFRESH_INTERVAL_IDLE;
+    uint64_t gfx_interval = GUI_REFRESH_INTERVAL_DEFAULT;
     uint64_t dcl_interval;
     DisplayState *ds = opaque;
     DisplayChangeListener *dcl;
@@ -209,20 +211,29 @@ static void gui_update(void *opaque)
     dpy_refresh(ds);
     ds->refreshing = false;
 
+    QLIST_FOREACH(dcl, &ds->listeners, next) {
+        if (dcl->gfx_update_interval &&
+            gfx_interval > dcl->gfx_update_interval) {
+            gfx_interval = dcl->gfx_update_interval;
+        }
+    }
     QLIST_FOREACH(dcl, &ds->listeners, next) {
         dcl_interval = dcl->update_interval ?
-            dcl->update_interval : GUI_REFRESH_INTERVAL_DEFAULT;
+            dcl->update_interval : gfx_interval;
         if (interval > dcl_interval) {
             interval = dcl_interval;
         }
     }
-    if (ds->update_interval != interval) {
-        ds->update_interval = interval;
+    if (ds->gfx_update_interval != gfx_interval) {
+        ds->gfx_update_interval = gfx_interval;
         QTAILQ_FOREACH(con, &consoles, next) {
-            if (con->hw_ops->update_interval) {
-                con->hw_ops->update_interval(con->hw, interval);
+            if (con->hw_ops->gfx_update_interval) {
+                con->hw_ops->gfx_update_interval(con->hw, gfx_interval);
             }
         }
+    }
+    if (ds->update_interval != interval) {
+        ds->update_interval = interval;
         trace_console_refresh(interval);
     }
     ds->last_update = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/ui/gtk.c b/ui/gtk.c
index 79dc2401203..53f68b0bdaf 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -810,7 +810,7 @@ static gboolean gd_draw_event(GtkWidget *widget, cairo_t *cr, void *opaque)
         return FALSE;
     }
 
-    vc->gfx.dcl.update_interval =
+    vc->gfx.dcl.gfx_update_interval =
         gd_monitor_update_interval(vc->window ? vc->window : s->window);
 
     fbw = surface_width(vc->gfx.ds);
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Gerd Hoffmann]

On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote:
> This change introduces an additional member, refresh_rate to
> qemu_edid_info in include/hw/display/edid.h.
> 
> This change also isolates the graphics update interval from the
> display update interval. The guest will update the frame buffer
> in the graphics update interval, but displays can be updated in a
> dynamic interval, for example to save update costs aggresively
> (vnc) or to respond to user-generated events (sdl).
> It stabilizes the graphics update interval and prevents the guest
> from being confused.

Hmm.  What problem you are trying to solve here?

The update throttle being visible by the guest was done intentionally,
so the guest can throttle the display updates too in case nobody is
watching those display updated anyway.

take care,
  Gerd

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Akihiko Odaki]

2021年2月22日(月) 19:57 Gerd Hoffmann <kraxel@redhat.com>:
>
> On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote:
> > This change introduces an additional member, refresh_rate to
> > qemu_edid_info in include/hw/display/edid.h.
> >
> > This change also isolates the graphics update interval from the
> > display update interval. The guest will update the frame buffer
> > in the graphics update interval, but displays can be updated in a
> > dynamic interval, for example to save update costs aggresively
> > (vnc) or to respond to user-generated events (sdl).
> > It stabilizes the graphics update interval and prevents the guest
> > from being confused.
>
> Hmm.  What problem you are trying to solve here?
>
> The update throttle being visible by the guest was done intentionally,
> so the guest can throttle the display updates too in case nobody is
> watching those display updated anyway.

Indeed, we are throttling the update for vnc to avoid some worthless
work. But typically a guest cannot respond to update interval changes
so often because real display devices the guest is designed for does
not change the update interval in that way. That is why we have to
tell the guest a stable update interval even if it results in wasted
frames.

Regards,
Akihiko Odaki

>
> take care,
>   Gerd
>

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Gerd Hoffmann]

On Tue, Feb 23, 2021 at 01:50:51PM +0900, Akihiko Odaki wrote:
> 2021年2月22日(月) 19:57 Gerd Hoffmann <kraxel@redhat.com>:
> >
> > On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote:
> > > This change introduces an additional member, refresh_rate to
> > > qemu_edid_info in include/hw/display/edid.h.
> > >
> > > This change also isolates the graphics update interval from the
> > > display update interval. The guest will update the frame buffer
> > > in the graphics update interval, but displays can be updated in a
> > > dynamic interval, for example to save update costs aggresively
> > > (vnc) or to respond to user-generated events (sdl).
> > > It stabilizes the graphics update interval and prevents the guest
> > > from being confused.
> >
> > Hmm.  What problem you are trying to solve here?
> >
> > The update throttle being visible by the guest was done intentionally,
> > so the guest can throttle the display updates too in case nobody is
> > watching those display updated anyway.
> 
> Indeed, we are throttling the update for vnc to avoid some worthless
> work. But typically a guest cannot respond to update interval changes
> so often because real display devices the guest is designed for does
> not change the update interval in that way.

What is the problem you are seeing?

Some guest software raising timeout errors when they see only
one vblank irq every 3 seconds?  If so: which software is this?
Any chance we can fix this on the guest side?

> That is why we have to
> tell the guest a stable update interval even if it results in wasted
> frames.

Because of the wasted frames I'd like this to be an option you can
enable when needed.  For the majority of use cases this seems to be
no problem ...

Also: the EDID changes should go to a separate patch.

take care,
  Gerd

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Akihiko Odaki]

2021年2月24日(水) 20:17 Gerd Hoffmann <kraxel@redhat.com>:
>
> On Tue, Feb 23, 2021 at 01:50:51PM +0900, Akihiko Odaki wrote:
> > 2021年2月22日(月) 19:57 Gerd Hoffmann <kraxel@redhat.com>:
> > >
> > > On Sun, Feb 21, 2021 at 10:34:14PM +0900, Akihiko Odaki wrote:
> > > > This change introduces an additional member, refresh_rate to
> > > > qemu_edid_info in include/hw/display/edid.h.
> > > >
> > > > This change also isolates the graphics update interval from the
> > > > display update interval. The guest will update the frame buffer
> > > > in the graphics update interval, but displays can be updated in a
> > > > dynamic interval, for example to save update costs aggresively
> > > > (vnc) or to respond to user-generated events (sdl).
> > > > It stabilizes the graphics update interval and prevents the guest
> > > > from being confused.
> > >
> > > Hmm.  What problem you are trying to solve here?
> > >
> > > The update throttle being visible by the guest was done intentionally,
> > > so the guest can throttle the display updates too in case nobody is
> > > watching those display updated anyway.
> >
> > Indeed, we are throttling the update for vnc to avoid some worthless
> > work. But typically a guest cannot respond to update interval changes
> > so often because real display devices the guest is designed for does
> > not change the update interval in that way.
>
> What is the problem you are seeing?
>
> Some guest software raising timeout errors when they see only
> one vblank irq every 3 seconds?  If so: which software is this?
> Any chance we can fix this on the guest side?
>
> > That is why we have to
> > tell the guest a stable update interval even if it results in wasted
> > frames.
>
> Because of the wasted frames I'd like this to be an option you can
> enable when needed.  For the majority of use cases this seems to be
> no problem ...

I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with
the EDID change included in this patch. The only devices inspecting
the variable, xenfb and modified virtio-gpu, do not yield vblank irq,
but they report the refresh rate to the guest, and the guest
proactively requests them to switch the surface.

I suspect Linux's kernel mode setting causes blinks and other guests
have similar problems.

>
> Also: the EDID changes should go to a separate patch.

That makes sense. I'll isolate it to a seperate patch in a series.

Regards,
Akihiko Odaki

>
> take care,
>   Gerd
>

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Gerd Hoffmann]

  Hi,

> > Because of the wasted frames I'd like this to be an option you can
> > enable when needed.  For the majority of use cases this seems to be
> > no problem ...
> 
> I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with
> the EDID change included in this patch.

/me looks closely at the patch again.

So you update the edid dynamically, each time the refresh rate changes.
Problem with that approach is software doesn't expect edid to change
dynamically because on physical hardware it is static information about
the connected monitor.

So what the virtio-gpu guest driver does is emulate a monitor hotplug
event to notify userspace.  If you resize the qemu window on the host
it'll look like the monitor with the old window size was unplugged and
a new monitor with the new window size got plugged instead, so gnome
shell goes adapt the display resolution to the new virtual monitor size.

The blink you are seeing probably comes from gnome-shell processing the
monitor hotplug event.

We could try to skip generating a monitor hotplug event in case only the
refresh rate did change.  That would fix the blink, but it would also
have the effect that nobody will notice the update.

Bottom line:  I think making the edid refresh rate configurable might be
useful, but changing it dynamically most likely isn't.

take care,
  Gerd

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Akihiko Odaki]

2021年2月25日(木) 20:46 Gerd Hoffmann <kraxel@redhat.com>:
>
>   Hi,
>
> > > Because of the wasted frames I'd like this to be an option you can
> > > enable when needed.  For the majority of use cases this seems to be
> > > no problem ...
> >
> > I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with
> > the EDID change included in this patch.
>
> /me looks closely at the patch again.
>
> So you update the edid dynamically, each time the refresh rate changes.
> Problem with that approach is software doesn't expect edid to change
> dynamically because on physical hardware it is static information about
> the connected monitor.
>
> So what the virtio-gpu guest driver does is emulate a monitor hotplug
> event to notify userspace.  If you resize the qemu window on the host
> it'll look like the monitor with the old window size was unplugged and
> a new monitor with the new window size got plugged instead, so gnome
> shell goes adapt the display resolution to the new virtual monitor size.
>
> The blink you are seeing probably comes from gnome-shell processing the
> monitor hotplug event.
>
> We could try to skip generating a monitor hotplug event in case only the
> refresh rate did change.  That would fix the blink, but it would also
> have the effect that nobody will notice the update.
>
> Bottom line:  I think making the edid refresh rate configurable might be
> useful, but changing it dynamically most likely isn't.
>
> take care,
>   Gerd
>

The "hotplug" implementation is probably what other combinations of
devices and guests will do if they want to respond to the changes of
the refresh rate, or display mode in general. That makes telling the
dynamic refresh rate to guests infeasible.

As you wrote, making the refresh rate configurable should be still
useful, and I think matching it to the backend physical display is
even better. GTK, the sole implementer of gfx_update_interval in my
patch reports the refresh rate of the physical display the window
resides in. It means the value may change when the physical display
changes its refresh rate, which should be rare if it does, or the
window moves to another physical display.

In the former case, there is nothing different from implementing a
physical display driver for guests so there should be no problem. The
latter case is similar to how the changes of the window size, which is
also part of display mode, is delivered to guests and they should be
consistent. The only inconsistency I see in my patch is that the
refresh rate change has no throttling while the window size change
has. I don't think it is problematic because it should be rare to move
the window across physical displays, but I can implement one if you
don't agree or know other cases the refresh rate frequently changes.

Regards,
Akihiko Odaki

Re: [PATCH] virtio-gpu: Respect graphics update interval for EDID

[Gerd Hoffmann]

On Fri, Feb 26, 2021 at 01:47:38PM +0900, Akihiko Odaki wrote:
> 2021年2月25日(木) 20:46 Gerd Hoffmann <kraxel@redhat.com>:
> >
> >   Hi,
> >
> > > > Because of the wasted frames I'd like this to be an option you can
> > > > enable when needed.  For the majority of use cases this seems to be
> > > > no problem ...
> > >
> > > I see blinks with GNOME on Wayland on Ubuntu 20.04 and virtio-gpu with
> > > the EDID change included in this patch.
> >
> > /me looks closely at the patch again.
> >
> > So you update the edid dynamically, each time the refresh rate changes.
> > Problem with that approach is software doesn't expect edid to change
> > dynamically because on physical hardware it is static information about
> > the connected monitor.
> >
> > So what the virtio-gpu guest driver does is emulate a monitor hotplug
> > event to notify userspace.  If you resize the qemu window on the host
> > it'll look like the monitor with the old window size was unplugged and
> > a new monitor with the new window size got plugged instead, so gnome
> > shell goes adapt the display resolution to the new virtual monitor size.
> >
> > The blink you are seeing probably comes from gnome-shell processing the
> > monitor hotplug event.
> >
> > We could try to skip generating a monitor hotplug event in case only the
> > refresh rate did change.  That would fix the blink, but it would also
> > have the effect that nobody will notice the update.
> >
> > Bottom line:  I think making the edid refresh rate configurable might be
> > useful, but changing it dynamically most likely isn't.
> >
> > take care,
> >   Gerd
> 
> The "hotplug" implementation is probably what other combinations of
> devices and guests will do if they want to respond to the changes of
> the refresh rate, or display mode in general. That makes telling the
> dynamic refresh rate to guests infeasible.
> 
> As you wrote, making the refresh rate configurable should be still
> useful, and I think matching it to the backend physical display is
> even better. GTK, the sole implementer of gfx_update_interval in my
> patch reports the refresh rate of the physical display the window
> resides in. It means the value may change when the physical display
> changes its refresh rate, which should be rare if it does, or the
> window moves to another physical display.

Yes.  Added debug printf, can see that it changes exactly twice for me,
once to the default value and then to the real refresh rate.

That rules out the hotplug event as source for the blinks.  I'm
wondering where they are coming from, and I can't reproduce them
on my machine ...

> consistent. The only inconsistency I see in my patch is that the
> refresh rate change has no throttling while the window size change
> has. I don't think it is problematic because it should be rare to move
> the window across physical displays, but I can implement one if you
> don't agree or know other cases the refresh rate frequently changes.

I think it would be best to just add a new field for the refresh rate
to QemuUIInfo.  That avoids a new callback you also get the throttling
for free ;)

take care,
  Gerd