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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
