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 745079 - Mutter & Wayland doesn't support Screen Rotation
Mutter & Wayland doesn't support Screen Rotation
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
3.20.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 758403 (view as bug list)
Depends on: 765007
Blocks: WaylandRelated
 
 
Reported: 2015-02-24 11:15 UTC by D.S. (Spider) Ljungmark
Modified: 2018-09-20 13:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
WIP: Support display rotation as a native compositor (7.03 KB, patch)
2015-12-18 11:37 UTC, Carlos Garnacho
none Details | Review
native: Implement DRM-based crtc rotation (8.83 KB, patch)
2015-12-22 18:54 UTC, Carlos Garnacho
none Details | Review
Patch updated to check all CRTCs that overlap with the pointer position, as pointed out in IRC review by Jasper. (8.92 KB, patch)
2015-12-23 13:08 UTC, Carlos Garnacho
committed Details | Review
backends: Mark MetaRendererView:info as G_PARAM_STATIC_STRINGS (976 bytes, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
backends/native: Split hw supported CRTC rotation modes (4.75 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
backends/native: Remove unneeded call (1.64 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
backends/native: Refactor g_object_set() call (2.54 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
clutter: Add infrastructure to render ClutterStageViews to offscreen (10.64 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
clutter/cogl: Use the "onscreen" on ClutterStageCogl (1.02 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
backends: Use clutter_stage_view_get_onscreen() on native stage/view (3.24 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
clutter: Hook up ClutterStageView render-to-texture (1.11 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
clutter/cogl: Transform swap buffers regions into onscreen coordinates (2.59 KB, patch)
2016-08-08 11:28 UTC, Carlos Garnacho
committed Details | Review
backends: Add MetaStageView::transform property (5.22 KB, patch)
2016-08-08 11:29 UTC, Carlos Garnacho
committed Details | Review
backends/native: Set transform on MetaRendererViews (2.81 KB, patch)
2016-08-08 11:29 UTC, Carlos Garnacho
committed Details | Review
backends/native: Use framebuffer size on swap_buffers implementation (2.12 KB, patch)
2016-08-08 11:29 UTC, Carlos Garnacho
committed Details | Review
backends/native: Refactor onscreen creation into separate function (6.36 KB, patch)
2016-08-08 11:29 UTC, Carlos Garnacho
committed Details | Review
backends/native: Set offscreen on the transformed MetaRendererViews (4.42 KB, patch)
2016-08-08 11:29 UTC, Carlos Garnacho
committed Details | Review
backends/native: Expose all transform modes in CRTCs (1.55 KB, patch)
2016-08-08 11:29 UTC, Carlos Garnacho
committed Details | Review

Description D.S. (Spider) Ljungmark 2015-02-24 11:15:18 UTC
Mutter wayland doesn't support having a screen in Portrait mode. A config for how the screen should look from Xorg doesn't apply in Wayland/mutter, and there's no option in the screen setup to rotate screens.

This will be a killer in case GDM goes to default to wayland, as most users find it very very difficult to log in on a screen with the wrong rotation.
Comment 1 porjo38 2015-05-28 03:04:55 UTC
Any progress? I'm seeing this issue on Fedora 22.
Comment 2 Andrew 2015-08-26 05:17:44 UTC
I would also like to ascertain the progress on this project. Will 3.18 solve this issue?  Wayland is a fundamental step forward in performance and smoothness with my only xorg reservation being a need for a portrait screen for coding. Any update would be appreciated!
Comment 3 Rui Matos 2015-11-20 14:49:19 UTC
*** Bug 758403 has been marked as a duplicate of this bug. ***
Comment 4 Jeroen Bollen 2015-11-22 18:04:14 UTC
This feature is also bugging me. GDM is now using Wayland, so I have to fallback to X in order to login.
Comment 5 Carlos Garnacho 2015-12-18 11:37:19 UTC
I'm adding a WIP patch that uses KMS to rotate the primary planes associated with the crtcs. Although, given inconsistent support in drivers, I don't think we can (only) go with this at the moment, IMO we do still need a generic approach that works regardless of driver support.
Comment 6 Carlos Garnacho 2015-12-18 11:37:42 UTC
Created attachment 317625 [details] [review]
WIP: Support display rotation as a native compositor
Comment 7 Carlos Garnacho 2015-12-22 18:54:09 UTC
Created attachment 317793 [details] [review]
native: Implement DRM-based crtc rotation

We can know the rotation modes supported by the driver, so
export these as our supported modes, and ensure these modes
are honored on the CRTC primary plane upon apply_configuration().

It is worth noting however that not all hardware will be
capable of supporting all rotation modes (in fact, most of
them won't). A driver independent solution should be in
place to back up the rotation modes unsupported by the
drivers, so this is still a partial solution.

The cursor renderer has also been changed to default to
software-based rendering anytime the cursor enters a
rotated CRTC. Another solution would be actually rotating
the DRM cursor planes, but then it requires applying rotation on
these per-CRTC, and actually transforming the pointer position by
the output matrix. This brings marginal gains, so we use the
"sw" rendered cursor, which will be transformed together with
the primary plane.
Comment 8 Carlos Garnacho 2015-12-23 13:08:58 UTC
Created attachment 317813 [details] [review]
Patch updated to check all CRTCs that overlap with the pointer position, as pointed out in IRC review by Jasper.

native: Implement DRM-based crtc rotation

We can know the rotation modes supported by the driver, so
export these as our supported modes, and ensure these modes
are honored on the CRTC primary plane upon apply_configuration().

It is worth noting however that not all hardware will be
capable of supporting all rotation modes (in fact, most of
them won't). A driver independent solution should be in
place to back up the rotation modes unsupported by the
drivers, so this is still a partial solution.

The cursor renderer has also been changed to default to
software-based rendering anytime the cursor enters a
rotated CRTC. Another solution would be actually rotating
the DRM cursor planes, but then it requires applying rotation on
these per-CRTC, and actually transforming the pointer position by
the output matrix. This brings marginal gains, so we use the
"sw" rendered cursor, which will be transformed together with
the primary plane.
Comment 9 Carlos Garnacho 2016-01-07 16:14:29 UTC
Comment on attachment 317813 [details] [review]
Patch updated to check all CRTCs that overlap with the pointer position, as pointed out in IRC review by Jasper.

Pushed the patch (with minor updates), leaving the bug opened as this is not an integral solution yet.
Comment 10 Jasper St. Pierre (not reading bugmail) 2016-01-07 16:21:06 UTC
Yeah, I don't know how many CRTCs support rotation, tbh. A full solution would need a shadow buffer.
Comment 11 Adam Williamson 2016-03-03 23:20:58 UTC
FWIW, the partial solution - assuming it's in current F24 - doesn't work for me (on a desktop with a couple of monitors attached to a GeForce 9600 GT running nouveau). I see no rotation option in Displays.
Comment 12 Carlos Garnacho 2016-03-04 00:01:16 UTC
(In reply to Adam Williamson from comment #11)
> FWIW, the partial solution - assuming it's in current F24 - doesn't work for
> me (on a desktop with a couple of monitors attached to a GeForce 9600 GT
> running nouveau). I see no rotation option in Displays.

I don't find it entirely surprising. Current support relies on exclusively on what the driver offers, probably somes down to noveau not offering any DRM plane rotation, or (less likely probably, but I can't tell) the HW actually supporting none.

FWIW, I started a couple weeks ago some work to support rotation modes not directly supported by the driver at https://git.gnome.org/browse/clutter/log/?h=wip/garnacho/egl-emulated-screen-rotation. It's very much wip at the moment though.
Comment 13 Carlos Garnacho 2016-03-04 11:17:00 UTC
(In reply to Carlos Garnacho from comment #12)
> (In reply to Adam Williamson from comment #11)
> > FWIW, the partial solution - assuming it's in current F24 - doesn't work for
> > me (on a desktop with a couple of monitors attached to a GeForce 9600 GT
> > running nouveau). I see no rotation option in Displays.
> 
> I don't find it entirely surprising. Current support relies on exclusively
> on what the driver offers, probably somes down to noveau not offering any
> DRM plane rotation

From a look in the kernel sources, this is the case. I see no trace of hw plane rotation support for any model...
Comment 14 Carlos Garnacho 2016-03-04 11:21:35 UTC
And FWIW, the only video drivers I see supporting some rotation are:

drivers/gpu/drm/atmel-hlcdc/
drivers/gpu/drm/i915/
drivers/gpu/drm/omapdrm/

Most relevant to us, there's just i915 there, so the situation is admittedly not brilliant.
Comment 15 Guillaume Ayoub 2016-03-30 01:15:21 UTC
I'm currently using Gnome 3.20 with Intel drivers (Haswell card) and I don't have the "rotate" buttons that I have with X. Did I miss something?
Comment 16 Jonathan Briggs 2016-04-06 19:22:15 UTC
I am sure that everyone has already thought of this, but am going to mention it anyway.

If the hardware does not support rotating the hardware output then the compositor (Mutter w/Clutter w/OpenGL) can do it. It can apply a rotation to the texture. It doesn't necessarily need a shadow buffer of the entire screen if it does a coordinate transform and rotates every individual surface.

Actually, reading back through all the comments carefully I see Carlos is already working on this.

And now I'm thinking, why not just ignore the hardware CRTCs and always do it with GL/EGL.
Comment 17 Bastien Nocera 2016-04-07 12:09:07 UTC
(In reply to Jonathan Briggs from comment #16)
<snip>
> And now I'm thinking, why not just ignore the hardware CRTCs and always do
> it with GL/EGL.

Because depending on the hardware, there are accelerations that won't be available, for example, cursor plane acceleration, or video decoding acceleration.
Comment 18 Carlos Garnacho 2016-04-14 10:04:41 UTC
(In reply to Jonathan Briggs from comment #16)
> I am sure that everyone has already thought of this, but am going to mention
> it anyway.
> 
> If the hardware does not support rotating the hardware output then the
> compositor (Mutter w/Clutter w/OpenGL) can do it. It can apply a rotation to
> the texture. It doesn't necessarily need a shadow buffer of the entire
> screen if it does a coordinate transform and rotates every individual
> surface.

Just to mention, it's not that simple :). Things complicate real soon when you have more than one monitor with arbitrary rotations: the outputs don't connect anymore as you expect, windows may lay across several outputs, ... With these scenarios we can only:

- Render to an intermediate buffer with the right post-rotation(s) geometry, and/or
- Control what is being drawn to each output.

And this hits the limits of Clutter rendering, where the stage is just one big rectangular onscreen surface, not a "view" of anything.

The current plans of refactoring Clutter into Mutter will certainly help here, because we'll be able to adapt rendering to Mutter's purposes without having to largely refactor Clutter itself for something that has absolutely no interest outside Mutter.
Comment 19 Scott Palmer 2016-06-27 23:39:09 UTC
It's been a few months.. what is the status of things now?
Comment 20 Jonas Ådahl 2016-06-27 23:56:39 UTC
We have since made great progress on doing the necessary code refactorizations. The rotation part is not done yet, but its happening.
Comment 21 Leho Kraav (@lkraav :macmaN) 2016-06-28 06:24:26 UTC
Thanks a ton. Is the landing target going to be 3.22+ only, or can 3.20 benefit as well?
Comment 22 Jonas Ådahl 2016-06-28 06:29:21 UTC
3.22 at the earliest.
Comment 23 Carlos Garnacho 2016-08-08 11:28:02 UTC
Created attachment 332920 [details] [review]
backends: Mark MetaRendererView:info as G_PARAM_STATIC_STRINGS
Comment 24 Carlos Garnacho 2016-08-08 11:28:08 UTC
Created attachment 332921 [details] [review]
backends/native: Split hw supported CRTC rotation modes

Those will need a separate treatment from the modes that we eventually
support through "software", so split those into a separate enum so we
can can do the right thing when applying the configuration.

Also, add a helper function that returns the transform that the software
fallbacks should perform, which should be "normal" if the rotation is
already handled via hw.
Comment 25 Carlos Garnacho 2016-08-08 11:28:15 UTC
Created attachment 332922 [details] [review]
backends/native: Remove unneeded call

The call to _cogl_framebuffer_winsys_update_size() results in no-op here,
as the framebuffer has already the right size when rebuilding the views.
Comment 26 Carlos Garnacho 2016-08-08 11:28:22 UTC
Created attachment 332923 [details] [review]
backends/native: Refactor g_object_set() call

Makes sense to update ::layout inside meta_renderer_native_set_legacy_view_size().
Comment 27 Carlos Garnacho 2016-08-08 11:28:28 UTC
Created attachment 332924 [details] [review]
clutter: Add infrastructure to render ClutterStageViews to offscreen

The offscreen is given through the ::back-buffer property, the ClutterStageView
will set up the the CoglPipeline used to render it back to the "onscreen"
framebuffer.

The pipeline can be altered through the setup_pipeline() vfunc, so ClutterStageView
implementations can alter the default behavior of blitting from offscreen to
onscreen with no transformations.
Comment 28 Carlos Garnacho 2016-08-08 11:28:35 UTC
Created attachment 332925 [details] [review]
clutter/cogl: Use the "onscreen" on ClutterStageCogl

It is where buffer swapping happens.
Comment 29 Carlos Garnacho 2016-08-08 11:28:41 UTC
Created attachment 332926 [details] [review]
backends: Use clutter_stage_view_get_onscreen() on native stage/view

All their relevant calls affect onscreen data, so we should ensure we
get that framebuffer.
Comment 30 Carlos Garnacho 2016-08-08 11:28:47 UTC
Created attachment 332927 [details] [review]
clutter: Hook up ClutterStageView render-to-texture

"Blit" the result on the framebuffer after each view is painted.
This of course only applies if there is an offscreen buffer to
perform any blitting. Otherwise the onscreen framebuffer is rendered
to directly.
Comment 31 Carlos Garnacho 2016-08-08 11:28:54 UTC
Created attachment 332928 [details] [review]
clutter/cogl: Transform swap buffers regions into onscreen coordinates

Those are given in view coordinates, so we must transform into onscreen
coordinates in order to perform swap_buffers on the right damage regions.
Comment 32 Carlos Garnacho 2016-08-08 11:29:01 UTC
Created attachment 332929 [details] [review]
backends: Add MetaStageView::transform property

This property updates the ClutterStageView pipeline, so the texture is applied
with the corresponding transform.
Comment 33 Carlos Garnacho 2016-08-08 11:29:08 UTC
Created attachment 332930 [details] [review]
backends/native: Set transform on MetaRendererViews

Only do this if mutter uses the multiple stage views feature. This
is uneffective at the moment because no back texture is set yet.
Comment 34 Carlos Garnacho 2016-08-08 11:29:15 UTC
Created attachment 332931 [details] [review]
backends/native: Use framebuffer size on swap_buffers implementation

Instead of ClutterStageView layout, which may be affected by transformations.
Comment 35 Carlos Garnacho 2016-08-08 11:29:21 UTC
Created attachment 332932 [details] [review]
backends/native: Refactor onscreen creation into separate function

And use the right size, regarless of view transform.
Comment 36 Carlos Garnacho 2016-08-08 11:29:27 UTC
Created attachment 332933 [details] [review]
backends/native: Set offscreen on the transformed MetaRendererViews

The texture is only created if the view is transformed at the software level,
otherwise the texture is NULL, and rendering happens on the onscreen.
Comment 37 Carlos Garnacho 2016-08-08 11:29:33 UTC
Created attachment 332934 [details] [review]
backends/native: Expose all transform modes in CRTCs

We can only honor this properly in the MUTTER_STAGE_VIEWS=1 case. When using
the legacy view, software implemented transforms are only exposed if there is
only one output, as we can only transform the entire stage there.
Comment 38 Carlos Garnacho 2016-08-08 11:38:56 UTC
These patches (also on wip/garnacho/wayland-emulated-output-rotation) implement all output transforms, provided that the stage views feature is enabled (still needs the MUTTER_STAGE_VIEWS envvar, but will be turned on by default at some point soon). If the feature is disabled, only HW rotation modes are handled, as per the first patch in this bug pushed time ago.
Comment 39 Jonas Ådahl 2016-08-23 14:21:40 UTC
Review of attachment 332920 [details] [review]:

LGTM.
Comment 40 Jonas Ådahl 2016-08-23 14:24:32 UTC
Review of attachment 332921 [details] [review]:

Looks good; with one nit.

::: src/backends/native/meta-monitor-manager-kms.c
@@ +1077,3 @@
       MetaCRTC *crtc = crtc_info->crtc;
       MetaCRTCKms *crtc_kms = crtc->driver_private;
+      MetaMonitorTransform hw_transform = META_MONITOR_TRANSFORM_NORMAL;

nit: Init this in the else branch where its set to crtc->transform; and it'll be more obvious whats going on.

@@ +1134,3 @@
+                                DRM_MODE_OBJECT_PLANE,
+                                crtc_kms->rotation_prop_id,
+                                crtc_kms->rotation_map[hw_transform]);

This is now always called, but it'll be called with the transform NORMAL, so I guess its harmless.
Comment 41 Jonas Ådahl 2016-08-23 14:25:12 UTC
Review of attachment 332922 [details] [review]:

Makes sense.
Comment 42 Jonas Ådahl 2016-08-23 14:25:42 UTC
Review of attachment 332923 [details] [review]:

Sure.
Comment 43 Jonas Ådahl 2016-08-23 14:46:52 UTC
Review of attachment 332924 [details] [review]:

::: clutter/clutter/clutter-stage-view.c
@@ +81,3 @@
 
+static void
+clutter_stage_view_ensure_pipeline (ClutterStageView *view)

nit: Naming could be better, to make it obvious this is the offscreen blit pipeline, not some generic one. For example

clutter_stage_view_ensure_offscreen_blit_pipeline

@@ +89,3 @@
+
+  if (!priv->offscreen)
+    return;

Remove this or replace it with g_assert(). It makes no sense to have "ensure" not ensure given some condition. It seems to have that check before being called as well.

@@ +108,3 @@
+
+void
+clutter_stage_view_invalidate_pipeline (ClutterStageView *view)

This could also use some more descriptive name, not making it sound like its a generic pipeline for general drawing.

@@ +126,3 @@
+  priv->offscreen = offscreen ? cogl_object_ref (offscreen) : NULL;
+
+  clutter_stage_view_invalidate_pipeline (view);

By making the "offscreen" parameter CONSTRUCT_ONLY, we can get rid of this function and always do just

priv->offscreen = cogl_obect_ref (offscreen) in the set_property vfunc for the "offscreen" property.

We only ever set the "offscreen" on construction anyway.

@@ +134,3 @@
+                            cairo_rectangle_t       *rect_out)
+{
+  gfloat x1, y1, x2, y2;

Do we really need to use glib types in new code? I see no reason not using just double/float etc.

@@ +147,3 @@
+  rect_out->y = MIN (y1, y2);
+  rect_out->width = ABS (x2 - x1);
+  rect_out->height = ABS (y2 - y1);

nit: Could be made declarative:

*rect_out = (cairo_rectangle_t) {
  .x = MIN (x1, x2),
  .y = MIN (y1, y2),
  .width = ABS (x2 - x1),
  .height = ABS (y2 - y2)
};

@@ +160,3 @@
+
+  if (!priv->offscreen)
+    return;

Why would this be called when there is no offscreen. I think the caller should rather know what its doing instead.

@@ +177,3 @@
+    (gdouble) rect->y / cogl_framebuffer_get_height (priv->offscreen),
+    (gdouble) rect->width / cogl_framebuffer_get_width (priv->offscreen),
+    (gdouble) rect->height / cogl_framebuffer_get_height (priv->offscreen)

nit: Can change to not rely on remembered struct field order? I.e. .x = ..., .y = ... etc.

::: clutter/clutter/clutter-stage-view.h
@@ +39,3 @@
+
+  void (* get_transformation_matrix) (ClutterStageView *view,
+                                      CoglMatrix       *matrix);

Naming of these two could be improved by making it obvious its about offscreen_blit and not generic for all types of views.
Comment 44 Jonas Ådahl 2016-08-23 14:48:08 UTC
Review of attachment 332925 [details] [review]:

Really think it'd make more sense if this was part of the one where the onscreen vs offscreen was introduced. This one is by definition incorrect now that _get_framebuffer() is defined to potentially not return the onscreen.
Comment 45 Jonas Ådahl 2016-08-23 14:48:48 UTC
Review of attachment 332926 [details] [review]:

Same with this one and introducing onscreen/offscreen split and adapting callers.
Comment 46 Jonas Ådahl 2016-08-23 14:51:26 UTC
Review of attachment 332927 [details] [review]:

I don't thi

::: clutter/clutter/clutter-stage.c
@@ +678,3 @@
   clutter_actor_paint (CLUTTER_ACTOR (stage));
+
+  clutter_stage_view_blit_offscreen (view, clip);

I don't think ClutterStage should do this. By calling it here, we'd blit and swap-buffers for no reason when picking and doing screen capture, and we're leaking the details about the drawing out of ClutterStageCogl and friends.

The fewer places that know about offscreen blitting, the better.
Comment 47 Jonas Ådahl 2016-08-23 14:55:38 UTC
Review of attachment 332928 [details] [review]:

::: clutter/clutter/cogl/clutter-stage-cogl.c
@@ +455,3 @@
+  y1 = (gdouble) swap_region->y / layout.height;
+  x2 = (gdouble) (swap_region->x + swap_region->width) / layout.width;
+  y2 = (gdouble) (swap_region->y + swap_region->height) / layout.height;

Same here about using glib types versus regular ones. Whats the point?

@@ +471,3 @@
+  swap_region->y = y1;
+  swap_region->width = x2 - x1;
+  swap_region->height = y2 - y1;

This could be made declarative:

  *swap_region = (cairo_rectangle_int_t) {
    .x = x1,
    .y = y1,
    .width = x2 - x1,
    .height = y2 - y1
  };

Also, shouldn't you type cast explicitly from float to int? Or maybe thats just double to int.

@@ +744,3 @@
   if (do_swap_buffer)
     {
+      transform_swap_region_to_onscreen (view, &swap_region);

nit: Should we maybe only do this if we have an offscreen? Not really for less computational burdon, but because it'll be obvious why there is any transformation going on to begin with.
Comment 48 Jonas Ådahl 2016-08-23 14:56:48 UTC
Review of attachment 332929 [details] [review]:

::: src/backends/meta-renderer-view.c
@@ +194,3 @@
+                       META_MONITOR_TRANSFORM_NORMAL,
+                       G_PARAM_READWRITE |
+                       G_PARAM_STATIC_STRINGS);

G_PARAM_CONSTRUCT_ONLY?
Comment 49 Jonas Ådahl 2016-08-23 14:58:43 UTC
Review of attachment 332930 [details] [review]:

::: src/backends/native/meta-renderer-native.c
@@ +850,3 @@
+  /* The monitor info has no outputs, what to do? */
+  if (monitor_info->n_outputs == 0)
+    return META_MONITOR_TRANSFORM_NORMAL;

I don't think this can happen (i.e. g_assert (monitor_info->n_outputs > 0) or g_return_val_if_fail (monitor_info->n_outputs > 0, META_MONITOR_TRANSFORM_NORMAL);

The reason would be that, if the monitor didn't have any outputs (with crtcs connected) the MetaMonitorInfo wouldn't exist to begin with.
Comment 50 Jonas Ådahl 2016-08-23 14:59:09 UTC
Review of attachment 332931 [details] [review]:

Makes sense.
Comment 51 Jonas Ådahl 2016-08-23 15:00:37 UTC
Review of attachment 332932 [details] [review]:

::: src/backends/native/meta-renderer-native.c
@@ +858,3 @@
+                                      gint                   view_height,
+                                      CoglOnscreen         **onscreen,
+                                      GError               **error)

Why don't you just make this function return the onscreen, and NULL on error? It seems unnecessarily complex to deal with parameter writing, when returning the created object and NULL as an error indicator is common practice.
Comment 52 Jonas Ådahl 2016-08-23 15:02:46 UTC
Review of attachment 332933 [details] [review]:

::: src/backends/native/meta-renderer-native.c
@@ +895,3 @@
+                                             gint                   view_height,
+                                             CoglOffscreen        **offscreen,
+                                             GError               **error)

I think an improvement would be to make this one just meta_renderer_native_create_offscreen() returning the new CoglOffscreen and NULL on error, moving the "if (transform ...)" to the call site. That way, when reading meta_renderer_create_view() its obvious why there sometimes is an offscreen created.
Comment 53 Jonas Ådahl 2016-08-23 15:03:13 UTC
Review of attachment 332934 [details] [review]:

\o/
Comment 54 Carlos Garnacho 2016-08-23 18:09:51 UTC
Thanks jonas for the review! I've fixed most of your comments in the branch. one reply and one question below before I push.

(In reply to Jonas Ådahl from comment #40)
> @@ +1134,3 @@
> +                                DRM_MODE_OBJECT_PLANE,
> +                                crtc_kms->rotation_prop_id,
> +                                crtc_kms->rotation_map[hw_transform]);
> 
> This is now always called, but it'll be called with the transform NORMAL, so
> I guess its harmless.

This is actually on purpose, when switching from a hw-managed transform mode to a sw-managed one, we want the hw transform to be restored for the sw transform to come out alright.

(In reply to Jonas Ådahl from comment #46)
> Review of attachment 332927 [details] [review] [review]:
> 
> I don't thi
> 
> ::: clutter/clutter/clutter-stage.c
> @@ +678,3 @@
>    clutter_actor_paint (CLUTTER_ACTOR (stage));
> +
> +  clutter_stage_view_blit_offscreen (view, clip);
> 
> I don't think ClutterStage should do this. By calling it here, we'd blit and
> swap-buffers for no reason when picking and doing screen capture, and we're
> leaking the details about the drawing out of ClutterStageCogl and friends.
> 
> The fewer places that know about offscreen blitting, the better.

Where do you suggest to do this? It IMHO should happen as close to having rendered the view as possible, doing this right before doing swap_buffers sounds a bit too late, and is actually the only other place where we could be doing this outside the stage.
Comment 55 Jonas Ådahl 2016-08-23 23:35:53 UTC
(In reply to Carlos Garnacho from comment #54)
> Thanks jonas for the review! I've fixed most of your comments in the branch.
> one reply and one question below before I push.
> 
> (In reply to Jonas Ådahl from comment #40)
> > @@ +1134,3 @@
> > +                                DRM_MODE_OBJECT_PLANE,
> > +                                crtc_kms->rotation_prop_id,
> > +                                crtc_kms->rotation_map[hw_transform]);
> > 
> > This is now always called, but it'll be called with the transform NORMAL, so
> > I guess its harmless.
> 
> This is actually on purpose, when switching from a hw-managed transform mode
> to a sw-managed one, we want the hw transform to be restored for the sw
> transform to come out alright.

Ah, right. That could be added to the commit message.

> 
> (In reply to Jonas Ådahl from comment #46)
> > Review of attachment 332927 [details] [review] [review] [review]:
> > 
> > I don't thi
> > 
> > ::: clutter/clutter/clutter-stage.c
> > @@ +678,3 @@
> >    clutter_actor_paint (CLUTTER_ACTOR (stage));
> > +
> > +  clutter_stage_view_blit_offscreen (view, clip);
> > 
> > I don't think ClutterStage should do this. By calling it here, we'd blit and
> > swap-buffers for no reason when picking and doing screen capture, and we're
> > leaking the details about the drawing out of ClutterStageCogl and friends.
> > 
> > The fewer places that know about offscreen blitting, the better.
> 
> Where do you suggest to do this? It IMHO should happen as close to having
> rendered the view as possible, doing this right before doing swap_buffers
> sounds a bit too late, and is actually the only other place where we could
> be doing this outside the stage.

Why is before swap-buffers too late? To me the offscreen -> onscreen blitting is conceptually kind of part of the buffer swapping. Otherwise in paint_stage() in clutter-stage-cogl.c seems like a reasonable place.
Comment 56 Carlos Garnacho 2016-08-24 00:25:20 UTC
(In reply to Jonas Ådahl from comment #55)
> (In reply to Carlos Garnacho from comment #54)
> > Thanks jonas for the review! I've fixed most of your comments in the branch.
> > one reply and one question below before I push.
> > 
> > (In reply to Jonas Ådahl from comment #40)
> > > @@ +1134,3 @@
> > > +                                DRM_MODE_OBJECT_PLANE,
> > > +                                crtc_kms->rotation_prop_id,
> > > +                                crtc_kms->rotation_map[hw_transform]);
> > > 
> > > This is now always called, but it'll be called with the transform NORMAL, so
> > > I guess its harmless.
> > 
> > This is actually on purpose, when switching from a hw-managed transform mode
> > to a sw-managed one, we want the hw transform to be restored for the sw
> > transform to come out alright.
> 
> Ah, right. That could be added to the commit message.

Just did :).

> 
> > 
> > (In reply to Jonas Ådahl from comment #46)
> > > Review of attachment 332927 [details] [review] [review] [review] [review]:
> > > 
> > > I don't thi
> > > 
> > > ::: clutter/clutter/clutter-stage.c
> > > @@ +678,3 @@
> > >    clutter_actor_paint (CLUTTER_ACTOR (stage));
> > > +
> > > +  clutter_stage_view_blit_offscreen (view, clip);
> > > 
> > > I don't think ClutterStage should do this. By calling it here, we'd blit and
> > > swap-buffers for no reason when picking and doing screen capture, and we're
> > > leaking the details about the drawing out of ClutterStageCogl and friends.
> > > 
> > > The fewer places that know about offscreen blitting, the better.
> > 
> > Where do you suggest to do this? It IMHO should happen as close to having
> > rendered the view as possible, doing this right before doing swap_buffers
> > sounds a bit too late, and is actually the only other place where we could
> > be doing this outside the stage.
> 
> Why is before swap-buffers too late? To me the offscreen -> onscreen
> blitting is conceptually kind of part of the buffer swapping. Otherwise in
> paint_stage() in clutter-stage-cogl.c seems like a reasonable place.

Perhaps this is all overengineered by now and my knowledge outdated :), but I understand swap_buffers as one operation where one memory address is replaced by another memory address. This is about pixel transfers, thus closer to the draw phase IMHO.

Anyway I tried doing this before the real eglSwapBuffers* in meta_onscreen_native_swap_buffers_with_damage() and I just managed to get corrupted content... thus went for the paint_stage() option.
Comment 57 Carlos Garnacho 2016-08-24 00:41:42 UTC
Pushed with all the suggested fixes.

Attachment 332920 [details] pushed as a53ca0d - backends: Mark MetaRendererView:info as G_PARAM_STATIC_STRINGS
Attachment 332921 [details] pushed as 92c03e8 - backends/native: Split hw supported CRTC rotation modes
Attachment 332922 [details] pushed as 4b4eb3a - backends/native: Remove unneeded call
Attachment 332923 [details] pushed as 7baa1d8 - backends/native: Refactor g_object_set() call
Attachment 332924 [details] pushed as 54dc10f - clutter: Add infrastructure to render ClutterStageViews to offscreen
Attachment 332928 [details] pushed as a6e15e8 - clutter/cogl: Transform swap buffers regions into onscreen coordinates
Attachment 332929 [details] pushed as 89854f9 - backends: Add MetaStageView::transform property
Attachment 332930 [details] pushed as a72bd1b - backends/native: Set transform on MetaRendererViews
Attachment 332931 [details] pushed as 6ce9186 - backends/native: Use framebuffer size on swap_buffers implementation
Attachment 332932 [details] pushed as 8065ff5 - backends/native: Refactor onscreen creation into separate function
Attachment 332933 [details] pushed as 9e641f6 - backends/native: Set offscreen on the transformed MetaRendererViews
Attachment 332934 [details] pushed as 9fb4783 - backends/native: Expose all transform modes in CRTCs
Comment 58 Carlos Garnacho 2016-08-24 00:46:44 UTC
Finally closing this bug. If anyone tries this, please remember that at the moment you will need the MUTTER_STAGE_VIEWS=1 envvar defined somewhere gnome-shell can pick it up.
Comment 59 Nathaniel McCallum 2016-08-31 14:19:48 UTC
I've tried the feature in Fedora's latest build. My setup is a 4k monitor (not rotated) in the middle and two 1080p monitors (rotated to vertical position) on the outside.

I can rotate the left monitor (#2) without any problem. But as soon as I rotate the right monitor (#3), I get strange flashing patterns. If I try to rotate both monitors, the distortion appears on both monitors.

I have not experienced any problems with rotation on Xorg.
Comment 60 Guillaume Ayoub 2016-08-31 16:18:21 UTC
(In reply to Nathaniel McCallum from comment #59)
> I've tried the feature in Fedora's latest build. My setup is a 4k monitor
> (not rotated) in the middle and two 1080p monitors (rotated to vertical
> position) on the outside.
> 
> I can rotate the left monitor (#2) without any problem. But as soon as I
> rotate the right monitor (#3), I get strange flashing patterns. If I try to
> rotate both monitors, the distortion appears on both monitors.
> 
> I have not experienced any problems with rotation on Xorg.

Almost same problem here. Rotating monitor #1 (internal laptop screen) works well, but rotating #2 or #3 (external screens) give strange flashing patterns. For example, when #3 is rotated, it seems to display altertatively garbage content from screen #1 and screen #2, switching each time the screen is redrawn (ie. each time I move the cursor).

Everything works fine with Xorg.

I've tried with Linux 4.6.2 and 4.7.2, and I get different results: with 4.6.2, even the colors are random and the content just looks like total garbage. With 4.7.2, I colors are OK and I can easily see that the content drawn on the rotated screen comes from the other screens. Could it be a bug in the kernel drivers for some Intel cards?

My config: Gentoo Linux, Intel Haswell graphics, latest gnome-3.21.x packages.
Comment 61 Bastien Nocera 2016-08-31 16:42:18 UTC
Unless you passed MUTTER_STAGE_VIEWS=1, it's likely not the same code path. Please file new bugs for each one of your problems.
Comment 62 Guillaume Ayoub 2016-08-31 17:13:20 UTC
(In reply to Bastien Nocera from comment #61)
> Unless you passed MUTTER_STAGE_VIEWS=1, it's likely not the same code path.

I have applied commit 262e184 ("Default to using stage views") to mutter. Without that, I don't even have the buttons to rotate the screens on Wayland.


I think that I've found what's going on: a rotated screen only works if its top-left corner is at the top-left corner of the whole displayed area. I'm pretty sure that it's exactly the problem reported by Nathaniel, as he can only rotate his left screen.

For example:


OK
--

-----------------
|   |  2  |  3  |
| 1 |------------
|   |
-----

-----
|   |
| 1 |------------
|   |  2  |  3  |
-----------------

-----------
|   |  3  |
| 1 |------
|   |  2  |
-----------

-----
|   |
| 1 |------
|   |  2  |
-----------
    |  3  |
    -------


Broken
------

-----------------
|  3  |   |  2  |
------| 1 |------
      |   |
      -----

      -----
------|   |------
|  3  | 1 |  2  |
------|   |------
      -----

    -------
----|  3  |
|   |------
| 1 |------
|   |  2  |
-----------

  -------
  |  3  |  
---------
|   |
| 1 |------
|   |  2  |
-----------



For the record, contrary to what I've written above:
- I get similar results with Linux 4.6.2 and 4.7.2.
- Screen #1 is not different from the other screens.
Comment 63 Nathaniel McCallum 2016-08-31 17:30:05 UTC
This is my setup:

-----------------------
|   |             |   |
| 2 |             | 3 |
|   |      1      |   |
-----             -----
    |             |
    ---------------

#1 is not rotated. #2 and #3 are rotated. #2 works. #3 does not.

I used MUTTER_STAGE_VIEWS=1 in order to get the buttons.

Should we reopen this issue? Or should I create a new issue?
Comment 64 Nathaniel McCallum 2016-08-31 19:46:55 UTC
I have opened https://bugzilla.gnome.org/show_bug.cgi?id=770672
Comment 65 antistress 2016-10-01 20:10:09 UTC
Hi,

Sorry but with X I used to key xrandr -o normal or xrandr -o left
What are the commands with Mutter & Wayland on GNOME 3.22 ?
Thanks !
Comment 66 Kamil Páral 2016-10-04 12:34:00 UTC
(In reply to antistress from comment #65)
There is no cmdline tool to control this under mutter+wayland that I know of.
Comment 67 Johannes Raggam 2017-10-13 09:42:11 UTC
I had the problem with no screen rotation buttons in the monitor settings in my Lenovo Yoga 910 laptop, which is a convertible.

After randomly flipping the screen I disabled automatic screen rotation via:
```gsettings set org.gnome.settings-daemon.plugins.orientation active false```

and suddenly, I've got these buttons!

IMO they should always be visible and not hidden on convertible laptops with orientation sensors, where the orientation plugin obviously gets the setting wrong anyways.
Comment 68 Carlos Garnacho 2017-10-13 11:09:49 UTC
Please realize that you are complaining about gnome-settings-daemon and gnome-control-center behavior on a (closed) Mutter bug. This bug was about implementing output rotation itself for Wayland, any UI issues on top of the feature are out of Mutter scope, thus don't belong to this bug.
Comment 69 Scott Palmer 2017-10-22 22:53:30 UTC
It is possible this is not completely resolved.  I am having rotation issues in gnome 3.26 with Ubuntu 17.10.  Comment #62 here might be on to something.

After manually setting a monitors.xml file (because of https://bugs.launchpad.net/ubuntu/+source/xorg/+bug/1724955) I had weird issues.  If the rotated monitor was left to the right (instead of the left) it worked fine.

https://bugs.launchpad.net/ubuntu/+source/xorg/+bug/1724977
Comment 70 David 2018-09-19 20:43:32 UTC
Not sure if I should reopen this bug or file a new one. I'm using Ryzen 5 2400g on Wayland am unable to flip one of my monitors. It claims that the setting is flipped but it's not flipped :)
Comment 71 Adam Williamson 2018-09-19 21:03:57 UTC
File a new bug (but at gitlab.gnome.org, not here). The basic capacity was added years ago, which is what this bug was about. What you're running into is certainly something different.
Comment 72 David 2018-09-20 13:08:02 UTC
Thank you!