GNOME Bugzilla – Bug 740069
Allow driver to suggest layout order for multiple displays
Last modified: 2014-11-20 18:42:15 UTC
Created attachment 290646 [details] [review] add support for suggested position When running a mutter-based desktop within a vm using spice, it used to be the responsibility of the spice-vdagent to reconfigure multiple displays. Because of this, the vdagent could lay out the displays in the order requested by the client (e.g. place display 2 to the left of display 1, or below display 1, etc). These days, enabling additional displays in spice simply generates a hotplug event and it is mutter's responsibility to respond to this hotplug event and configure the additional displays. Unfortunately, this means that the displays will always be laid out from left-to-right, so we've lost the ability of the client to suggest the order of the displays. To restore the previous behavior and allow the client to lay out the displays in the suggested order, the QXL driver recently gained a couple of connector properties: "suggested X" and "suggested Y". This patch takes advantage of these properties and lays out the displays in the suggested order if it is provided. It degrades gracefully if these properties do not exist. Note that proper testing currently requires a kernel drm patch and a qxl driver patch, but these should be merged upstream very soon.
Review of attachment 290646 [details] [review]: ::: src/backends/meta-monitor-config.c @@ +1122,3 @@ + gboolean is_primary = (&outputs[i] == primary); + + if (outputs[i].suggested_x < 0 || outputs[i].suggested_y < 0) So, we have MetaOutput.has_suggested_position and a meta_monitor_manager_has_suggested_position() API that we already call before getting here. Sounds like duplicated bookkeeping that's likely to go wrong at some point. I'd prefer to have a single way to check this. @@ +1318,3 @@ } + if (meta_monitor_manager_has_suggested_position (manager)) I'd keep it simple, do without this API and unconditionally call make_suggested_config() since it has to loop over the outputs and check the values for sanity anyway.
Review of attachment 290646 [details] [review]: The patch also needs to at least initialize the new MetaOuput fields in the native backend.
Review of attachment 290646 [details] [review]: ::: src/backends/meta-monitor-config.c @@ +1124,3 @@ + if (outputs[i].suggested_x < 0 || outputs[i].suggested_y < 0) + { + g_warning ("Found an output without a suggested position, cannot make suggested config"); If the driver has one monitor with a suggested position and another doesn't, this will go haywire. I'd also much prefer this to be just inside make_default_config() as an extra parameter to the algorithm.
(In reply to comment #3) > Review of attachment 290646 [details] [review]: > > ::: src/backends/meta-monitor-config.c > @@ +1124,3 @@ > + if (outputs[i].suggested_x < 0 || outputs[i].suggested_y < 0) > + { > + g_warning ("Found an output without a suggested position, cannot make > suggested config"); > > If the driver has one monitor with a suggested position and another doesn't, > this will go haywire. It won't go haywire, since it will return NULL and the calling function will then fall back to calling make_default_config() > > I'd also much prefer this to be just inside make_default_config() as an extra > parameter to the algorithm. fair enough, I can re-arrange things if required.
Created attachment 290945 [details] [review] patch v2 This is based on the refactoring done as part of bug 740073. Does this look any better?
Comment on attachment 290945 [details] [review] patch v2 Sorry, this patch is incomplete. new patch to come soon.
Created attachment 290946 [details] [review] patch v3
Review of attachment 290946 [details] [review]: Please prefix the commit subject with monitor-manager: ... The Conflicts: line isn't needed. This is looking better, thanks ::: src/backends/meta-monitor-config.c @@ +1113,3 @@ + MetaOutput *primary; + + g_return_if_fail(config != NULL); style nit, space before ( @@ +1127,3 @@ + + config->outputs[i].rect.x = outputs[i].suggested_x; + config->outputs[i].rect.y = outputs[i].suggested_y; Should we double check that the suggested values won't cause overlapped or disjoint outputs? Probably not worth it. @@ +1273,3 @@ make_linear_config (self, outputs, n_outputs, max_width, max_height, ret); + extra blank line @@ +1280,3 @@ + || (ret->outputs[i].rect.y + ret->outputs[i].rect.height > max_height)) + ret->outputs[i].enabled = FALSE; + } So, this check actually makes sense to generalize and apply to all code paths above. Do you mind including it in a separate patch in the other bug as another refactoring step? In that case, all the return ret; lines above should be a goto here instead.
(In reply to comment #8) > Review of attachment 290946 [details] [review]: > > Please prefix the commit subject with monitor-manager: ... > The Conflicts: line isn't needed. > > This is looking better, thanks > > ::: src/backends/meta-monitor-config.c > @@ +1113,3 @@ > + MetaOutput *primary; > + > + g_return_if_fail(config != NULL); > > style nit, space before ( > > @@ +1127,3 @@ > + > + config->outputs[i].rect.x = outputs[i].suggested_x; > + config->outputs[i].rect.y = outputs[i].suggested_y; > > Should we double check that the suggested values won't cause overlapped or > disjoint outputs? Probably not worth it. Well, I debated whether to do that or not, but I didn't for a couple reasons: A. I wasn't sure what to do in the case of an overlap: disable the output, shift it over? something else? B. In theory an overlap / disjoint output might be desired in some (strange) situations, so I didn't want to prevent it. C. if mutter modifies the suggested configurations, it can make debugging multiscreen issues in spice a bit more difficult since I'd have to look at code in the spice client + server + qxl driver + mutter. But I'm willing to do it if people think it's necessary and have a good ideas for solving issue A above. > @@ +1273,3 @@ > make_linear_config (self, outputs, n_outputs, max_width, max_height, ret); > > + > > extra blank line > > @@ +1280,3 @@ > + || (ret->outputs[i].rect.y + ret->outputs[i].rect.height > > max_height)) > + ret->outputs[i].enabled = FALSE; > + } > > So, this check actually makes sense to generalize and apply to all code paths > above. Do you mind including it in a separate patch in the other bug as another > refactoring step? > > In that case, all the return ret; lines above should be a goto here instead. Good point, I should have split this into a separate commit. Will do.
(In reply to comment #9) > A. I wasn't sure what to do in the case of an overlap: disable the output, > shift it over? something else? Reject the configuration. mutter does not support overlapping outputs.
Created attachment 291032 [details] [review] v4: patch 1/2 also rejects configuration if displays overlap as suggested in comment 10.
Created attachment 291033 [details] [review] v4: patch 2/2 refactor the limit-checking
Review of attachment 291032 [details] [review]: ++
Review of attachment 291033 [details] [review]: looks fine