After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740069 - Allow driver to suggest layout order for multiple displays
Allow driver to suggest layout order for multiple displays
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2014-11-13 15:46 UTC by Jonathon Jongsma
Modified: 2014-11-20 18:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add support for suggested position (8.55 KB, patch)
2014-11-13 15:46 UTC, Jonathon Jongsma
needs-work Details | Review
patch v2 (9.21 KB, patch)
2014-11-18 23:09 UTC, Jonathon Jongsma
none Details | Review
patch v3 (7.75 KB, patch)
2014-11-18 23:26 UTC, Jonathon Jongsma
needs-work Details | Review
v4: patch 1/2 (7.24 KB, patch)
2014-11-19 20:17 UTC, Jonathon Jongsma
committed Details | Review
v4: patch 2/2 (2.35 KB, patch)
2014-11-19 20:19 UTC, Jonathon Jongsma
committed Details | Review

Description Jonathon Jongsma 2014-11-13 15:46:22 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.
Comment 1 Rui Matos 2014-11-18 13:55:20 UTC
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.
Comment 2 Rui Matos 2014-11-18 14:00:47 UTC
Review of attachment 290646 [details] [review]:

The patch also needs to at least initialize the new MetaOuput fields in the native backend.
Comment 3 Jasper St. Pierre (not reading bugmail) 2014-11-18 15:28:55 UTC
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.
Comment 4 Jonathon Jongsma 2014-11-18 20:38:53 UTC
(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.
Comment 5 Jonathon Jongsma 2014-11-18 23:09:45 UTC
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 6 Jonathon Jongsma 2014-11-18 23:13:40 UTC
Comment on attachment 290945 [details] [review]
patch v2

Sorry, this patch is incomplete. new patch to come soon.
Comment 7 Jonathon Jongsma 2014-11-18 23:26:47 UTC
Created attachment 290946 [details] [review]
patch v3
Comment 8 Rui Matos 2014-11-19 15:47:42 UTC
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.
Comment 9 Jonathon Jongsma 2014-11-19 16:46:02 UTC
(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.
Comment 10 Jasper St. Pierre (not reading bugmail) 2014-11-19 16:48:04 UTC
(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.
Comment 11 Jonathon Jongsma 2014-11-19 20:17:41 UTC
Created attachment 291032 [details] [review]
v4: patch 1/2

also rejects configuration if displays overlap as suggested in comment 10.
Comment 12 Jonathon Jongsma 2014-11-19 20:19:03 UTC
Created attachment 291033 [details] [review]
v4: patch 2/2

refactor the limit-checking
Comment 13 Rui Matos 2014-11-20 11:28:01 UTC
Review of attachment 291032 [details] [review]:

++
Comment 14 Rui Matos 2014-11-20 11:28:16 UTC
Review of attachment 291033 [details] [review]:

looks fine