[Spice-devel] [PATCH v2] randr: Make resolution changing more robust

[Christophe Fergeau]

When running a guest using a KMS QXL driver on a RHEL6 hypervisor,
spice_vdagent would crash on resolution changes with:
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  139 (RANDR)
  Minor opcode of failed request:  21 (RRSetCrtcConfig)
  Value in failed request:  0x0
  Serial number of failed request:  75
  Current serial number in output stream:  75

(if using a newer QEMU, this will not crash as the resolution codepath is
totally different, see red_dispatcher_use_client_monitors_config() in
spice-server, when using the UMS QXL driver, the crash will not happen
because of
http://cgit.freedesktop.org/xorg/driver/xf86-video-qxl/commit/?id=907d0ff0
).

This crash happens in vdagent_x11_set_monitor_config() because the X server
rejects the request because we are trying to set a CRTC to a size which is
bigger than the current screen (using 'screen' and 'CRTC' as in the RandR
spec at http://www.x.org/releases/X11R7.5/doc/randrproto/randrproto.txt ).

If we resize the screen before changing the CRTCs resolution, then this
error will no longer happen. However, if we first call XRRSetScreenSize()
and then XRRSetCrtcConfig(), the call to XRRSetScreeSize() will fail when
we try to make the screen smaller as there will be active CRTCs which would
be bigger than the screen.

In order to solve these issues, we follow the same process as what mutter
does in meta_monitor_manager_xrandr_apply_configuration()
https://git.gnome.org/browse/mutter/tree/src/core/monitor-xrandr.c#n689:

1. we disable the CRTCs which are off and the ones which are bigger than the
   size we want to set the screen to.
2. we resize the screen with a call to XRRSetScreenSize().
3. we set each CRTCs to their new resolution.
---

Changes since v1:
- remove 2 useless 'continue;' statements
- fix debug log when disabling a monitor, and make it closer to xrandr
  output (width x height + x + y)

 src/vdagent-x11-randr.c | 61 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/src/vdagent-x11-randr.c b/src/vdagent-x11-randr.c
index 5223f88..5fcfcc8 100644
--- a/src/vdagent-x11-randr.c
+++ b/src/vdagent-x11-randr.c
@@ -688,8 +688,6 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
                                     VDAgentMonitorsConfig *mon_config,
                                     int fallback)
 {
-    int width, height;
-    int x, y;
     int primary_w, primary_h;
     int i, real_num_of_monitors = 0;
     VDAgentMonitorsConfig *curr = NULL;
@@ -748,24 +746,38 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
     for (i = mon_config->num_of_monitors; i < x11->randr.res->noutput; i++)
         xrandr_disable_output(x11, i);
 
+    /* First, disable disabled CRTCs... */
     for (i = 0; i < mon_config->num_of_monitors; ++i) {
         if (!monitor_enabled(&mon_config->monitors[i])) {
             xrandr_disable_output(x11, i);
-            continue;
         }
-        /* Try to create the requested resolution */
-        width = mon_config->monitors[i].width;
-        height = mon_config->monitors[i].height;
-        x = mon_config->monitors[i].x;
-        y = mon_config->monitors[i].y;
-        if (!xrandr_add_and_set(x11, i, x, y, width, height) &&
-                enabled_monitors(mon_config) == 1) {
-            set_screen_to_best_size(x11, width, height,
-                                    &primary_w, &primary_h);
-            goto update;
+    }
+
+    /* ... and disable the ones that would be bigger than
+     * the new RandR screen once it is resized. If they are enabled the
+     * XRRSetScreenSize call will fail with BadMatch. They will be
+     * reenabled after hanging the screen size.
+     */
+    for (i = 0; i < curr->num_of_monitors; ++i) {
+        int width, height;
+        int x, y;
+
+        width = curr->monitors[i].width;
+        height = curr->monitors[i].height;
+        x = curr->monitors[i].x;
+        y = curr->monitors[i].y;
+
+        if ((x + width > primary_w) || (y + height > primary_h)) {
+            if (x11->debug)
+                syslog(LOG_DEBUG, "Disabling monitor %d: "
+                       "%dx%d+%d+%d > (%d,%d)",
+                       i, width, height, x, y, primary_w, primary_h);
+
+            xrandr_disable_output(x11, i);
         }
     }
 
+    /* Then we can resize the RandR screen. */
     if (primary_w != x11->width[0] || primary_h != x11->height[0]) {
         if (x11->debug)
             syslog(LOG_DEBUG, "Changing screen size to %dx%d",
@@ -793,7 +805,28 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
         }
     }
 
-update:
+    /* Finally, we set the new resolutions on RandR CRTCs now that the
+     * RandR screen is big enough to hold these.  */
+    for (i = 0; i < mon_config->num_of_monitors; ++i) {
+        int width, height;
+        int x, y;
+
+        if (!monitor_enabled(&mon_config->monitors[i])) {
+            continue;
+        }
+        /* Try to create the requested resolution */
+        width = mon_config->monitors[i].width;
+        height = mon_config->monitors[i].height;
+        x = mon_config->monitors[i].x;
+        y = mon_config->monitors[i].y;
+        if (!xrandr_add_and_set(x11, i, x, y, width, height) &&
+                enabled_monitors(mon_config) == 1) {
+            set_screen_to_best_size(x11, width, height,
+                                    &primary_w, &primary_h);
+            break;
+        }
+    }
+
     update_randr_res(x11,
         x11->randr.num_monitors != enabled_monitors(mon_config));
     x11->width[0] = primary_w;
-- 
1.8.4.2