[Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes

[Christophe Fergeau]

On Mon, Oct 10, 2016 at 08:37:04AM -0400, Marc-André Lureau wrote:
> Imho, it should be fine for the client to send the same monitor
> config, the server should however not notify of changes if none
> happened.

Yup, after quite a lot of digging, this seems to be what is happening,
nothing in the spice-server->QEMU->kernel qxl->mutter is checking
whether the configuration we get is the same as the one which we had.
(the kernel has some checks for that, except that the way the resolution
changes is done in mutter disable these checks)

The way resolution changes happen in mutter are going to trigger a
surface destroy/create. Which gets us into this flicker loop:
- the client sends a MonitorsConfig message
- spice-server gets it, calls a QEMU callback
- QEMU raises an interrupt to inform the guest
- the kernel catches it and sends a udev event to userspace
- mutter catches this, and this triggers a resolution change
- the kernel does the resolution change, which involves a
  surface destroy/create
- the client receives the surface destroy/create, which will cause
  a MonitorsConfig message to be sent, so we are back to step 1

Checking whether we are getting an unchanged MonitorsConfig message
anywehere in qemu/kernel/mutter would solve (avoid ?) this bug, still
not fully sure where the best place is. Maybe best to do it once
host-side, and once guest-side.

Kernel-side, the check would go in qxl_display.c in qxl_display_read_client_monitors_config()
mutter-side, the check would belong in the call chain starting at on_uevent() in
backends/native/meta-monitor-manager-kms.c

In QEMU, the patch below fixes the flickering for me:

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 0e2682d..3530aeb 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1049,6 +1049,7 @@ static int interface_client_monitors_config(QXLInstance *sin,
         rect->right = monitor->x + monitor->width;
         rect->bottom = monitor->y + monitor->height;
     }
+    uint32_t old_crc = rom->client_monitors_config_crc;
     rom->client_monitors_config_crc = qxl_crc32(
             (const uint8_t *)&rom->client_monitors_config,
             sizeof(rom->client_monitors_config));
@@ -1059,7 +1060,9 @@ static int interface_client_monitors_config(QXLInstance *sin,
     trace_qxl_interrupt_client_monitors_config(qxl->id,
                         rom->client_monitors_config.count,
                         rom->client_monitors_config.heads);
-    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+    if (rom->client_monitors_config_crc != old_crc) {
+        qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+    }
     return 1;
 }

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161013/e847e573/attachment.sig>