ack On Thu, Feb 20, 2014 at 3:15 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > Ping? > > On Tue, Jan 21, 2014 at 10:30:35AM +0100, 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. >> --- >> >> 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 >> >> _______________________________________________ >> Spice-devel mailing list >> Spice-devel@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/spice-devel > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel > -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel