[Spice-devel] [PATCH v2] randr: Make resolution changing more robust
Christophe Fergeau
cfergeau at redhat.com
Thu Feb 20 06:15:40 PST 2014
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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140220/58a8431e/attachment.pgp>
More information about the Spice-devel
mailing list