GNOME Bugzilla – Bug 740073
with QXL driver, additional displays are regularly left unconfigured
Last modified: 2014-11-20 18:41:13 UTC
Created attachment 290649 [details] [review] ignore stored config when hotplug_mode_update is set When running a recent mutter in a spice VM (with a QXL device), enabling additional displays often fails, and the new display window simply shows a "Waiting for display N..." message forever. The user must then manually configure it via the control center in order for it to be useable. Alternately, sometimes it gets configured at the 'wrong' resolution. The reason that this happens is because QXL (and more generally, any device with 'hotplug_mode_update') violates some assumptions made in the make_default_config() function. When a new display is added (for a total of N displays), mutter looks through its list of stored configurations (monitors.xml) and looks for a configuration that has N-1 displays that match the the currently-connected displays. It then tries to extend the desktop by placing the new display to the right of the last display. This generally works find for displays that have a fixed 'preferred' mode (e.g. physical monitors). But it doesn't work so well for devices that have 'hotplug_mode_update'. These types of devices generally have a preferred mode that matches the current size of the client window (e.g. an arbitrary geometry such as 963x851), and then a list of standard modes (e.g. 800x600, 1280x720, 1440x900, etc). As an example, consider the following cases that result in slightly different misbehaviors. Case #1 (monitors configured with a non-preferred resolution): -------------------------------------------------------------- - monitors.xml contains a configuration with one display (Virtual-0) that has a width of 1280 and a height of 720) - user resizes Virtual-0 via a hotplug event (not via control center) to a non-standard mode: 1121x689. The stored config is not updated. - user enables a new display (Virtual-1), with a resolution of 800x600. There are now 2 displays enabled (N=2) - mutter iterates through its list of stored configs for a configuration with N-1 displays. It finds the config mentioned above (1280x720). make_default_config() uses this configuration and appends Virtual-1 to the right of Virtual-0 with a size of 800x600. Since 1280x720 is a standard mode supported by Virtual-0, and 800x600 is the preferred mode of Virtual-1, this configuration will apply successfully. -> As a result of enabling a second display, Virtual-0 suddenly and unexpectedly changes size from 1121x689 to 1280x720. Case #2 (new monitor is left unconfigured): ------------------------------------------- - monitors.xml contains a stored configuration with one display (Virtual-0) that has a non-standard width of 1121 and a height of 689. - user resizes Virtual-0 via a hotplug event (not via control center) to a new arbitrary mode: 989x521. The stored config is not updated. - user enables a new display (Virtual-1), with a resolution of 800x600. There are now 2 displays enabled (N=2) - mutter iterates through its list of stored configs for a configuration with N-1 displays. It finds the config mentioned above (1121x689). make_default_config() uses this configuration and appends Virtual-1 to the right of Virtual-0 with a size of 800x600. Since 1121x689 is no longer a mode that is supported by Virtual-0 this configuration will *NOT* apply successfully. -> Since the configuration that mutter tried to apply was an invalid configuration, the new display is left disabled. The attached patch solves this issue by simply ignoring the saved configuration for devices that have the 'hotplug_mode_update' property set.
By the way, that this patch might not apply cleanly unless the patch from Bug 740069 is already applied. But that's just a matter of the order I worked on them. There's no functional dependency between the patches.
Review of attachment 290649 [details] [review]: This is getting a bit unwieldy. Do you mind refactoring make_default_config() ? I think the make_suggested_config() call you introduced in the other patch could be a subfunction of make_default_config() and then we could also have subfunctions for make_expanded_existing_config() and make_linear_config() (likely with better names) . This way the special case of one single output would also apply for make_suggested_config(). ::: src/backends/meta-monitor-config.c @@ +1333,3 @@ + * resulting config will fail. Just use the 'really-default' + * configuration in this situation to avoid leaving thay unconfigured. */ + gboolean use_stored_config = !meta_monitor_manager_has_hotplug_mode_update(manager); style nit: space before (
Created attachment 290943 [details] [review] refactor make_default_config()
Created attachment 290944 [details] [review] ignore stored config when hotplug_mode_udpate is set.
Is this any better?
Review of attachment 290943 [details] [review]: commit subject should be "monitor-config: ..." With that and the comments below fixed, looks good, thanks. ::: src/backends/meta-monitor-config.c @@ +1097,3 @@ } +static gboolean return type here should be void @@ +1136,3 @@ } + return TRUE; see above
Review of attachment 290944 [details] [review]: s/hotplug/monitor-config/ in the subject. Also, we generally try to keep commit messages under 80 characters wide and the the Conflicts: line isn't needed. Code looks fine, thanks