On 01/20/2014 06:43 PM, Christophe Fergeau wrote: > 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. ACK with two lines removed, see below. > --- > src/vdagent-x11-randr.c | 61 ++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 48 insertions(+), 13 deletions(-) > > diff --git a/src/vdagent-x11-randr.c b/src/vdagent-x11-randr.c > index 5223f88..77b2127 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,40 @@ 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; You can drop this "continue" now. > } > - /* 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: " > + "(%d+%d, %d+%d) < (%d,%d)", > + i, x, width, y, height, primary_w, primary_h); > + > + xrandr_disable_output(x11, i); > + continue; Also here. > } > } > > + /* 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 +807,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; > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel