(In reply to Hans de Goede from comment #0)
<snip>
> With the right /lib/udev/hwdb.d/60-sensor.hwdb entry rotation works too and
> the screen can be oriented in the prefered orientation by simply picking the
> device up (so that the accelerometer is which is in the keyboard/base no
> longer sits on a flat surface), and let the accelerometer / iio-sensor-proxy
> pick up the right orientation.
> 
> So with GNOME's orientation support the GPDwin works 99%, there are 2
> problems left, which is what this bug is about:
> 
> 1) The initial orientation where the left side of the device is up is wrong
> and one must tilt the keyboard for the accelerometer + iio-sensor-proxy to
> do their work, this is a minor thing, but still quite annoying
> 
> 2) Disabling rotation results in GNOME restoring things to the wrong initial
> rotation where the left side of the device is up.
> 
> To fix this I would like to propose to add a property in hwdb somewhere
> (exactly where can be decided later). I propose to name this property
> LCD_PANEL_PREFERED_ORIENTATION and use the same left-up / right-up /
> bottom-up values as iio-sensor-proxy uses for it. The goal of this property
> is that when iio-sensor-proxy does not (yet) have any rotation info
> available gnome-shell will use the value from the property (if set) rather
> then defaulting to "normal" thus fixing 1. from above, likewise when
> rotation-locking is enabled gnome-shell should also use the value from the
> property if set, fixing 2.
> 
> I'm willing to write a patch for this but I wanted to discuss the proposed
> method of fixing this first, so any comments ?

You need to forget about iio-sensor-proxy and the accelerometer support in those bugs. It's not the right way to propagate the quirk. 

If the kernel knows that the plane is rotated, but can't handle moving it back to the expected rotation because it can't do this in hardware, couldn't the kernel export this information to user-space via a sysfs entry?

That would mean we could detect this quirk without having an ever expanding list of tablets to which this would apply.

> To fix this I would like to propose to add a property in hwdb somewhere
> (exactly where can be decided later). I propose to name this property
> LCD_PANEL_PREFERED_ORIENTATION and use the same left-up / right-up /
> bottom-up values as iio-sensor-proxy uses for it.

If you were going for a udev property (hopefully we don't need one), it'd need to be independent of the sensor hwdb. I'd also advise using degrees of rotation rather than any relative value (for example, the number of degrees needed to rotate the actual rotation to the expected one).

Moving to mutter, as that's where crtcs are handled.

(In reply to Bastien Nocera from comment #1)
> (In reply to Hans de Goede from comment #0)

<snip>

> You need to forget about iio-sensor-proxy and the accelerometer support in
> those bugs. It's not the right way to propagate the quirk. 

Right, as hopefully the suggested LCD_PANEL_PREFERED_ORIENTATION property name makes clear the plan is to not involve iio-sensor-proxy in any way, but I do believe that we should be able to use the same code-paths as rotation triggered by iio-sensor-proxy does. Note that everything we care about (lcd-panel and touchscreen) is mounted wrong in the same way, so the unrotated lcd + touchpanel coordinates do match and we need to rotate both, so this really is just like normal rotation caused by the user tilting the device, but then we need to start with a non "normal" orientation and use a non "normal" orientation when rotation-locked.

> If the kernel knows that the plane is rotated, but can't handle moving it
> back to the expected rotation because it can't do this in hardware, couldn't
> the kernel export this information to user-space via a sysfs entry?

Yes it could, but the kernel does not know this, the VBT (video bios tables) do have a field for this, but it contains 0 aka not-rotated, likely because that field can only contain hardware supported values.

> That would mean we could detect this quirk without having an ever expanding
> list of tablets to which this would apply.

These 90 degree rotated cases are very rare, and the 180 degree rotated case can be detected by the kernel and if my i915 kernel patch for that case gets accepted will be handled completely transparently by the kernel.

> > To fix this I would like to propose to add a property in hwdb somewhere
> > (exactly where can be decided later). I propose to name this property
> > LCD_PANEL_PREFERED_ORIENTATION and use the same left-up / right-up /
> > bottom-up values as iio-sensor-proxy uses for it.
> 
> If you were going for a udev property (hopefully we don't need one), it'd
> need to be independent of the sensor hwdb.

Ack.

> I'd also advise using degrees of
> rotation rather than any relative value (for example, the number of degrees
> needed to rotate the actual rotation to the expected one).

As said I plan to use the same code-paths as iio-sensor-proxy triggered rotation, so it will likely be easier to use the same values. All I really want to do is make gnome-shell start with another rotation then "normal" and after that it can just take any rotation info coming from iio-sensor-proxy as is, it does not need to apply a correction.

Actually making this a correction on top of the iio-sensor-proxy provided rotation would mean using different rotation values before and after the fix, which means the gnome version and hwdb 60-sensors.conf need to match up which seems like a bad idea and more in general that just adds unneeded complexity, this really is only about fixing up the initial / rotation locked orientation.

(In reply to Hans de Goede from comment #0)
> from above, likewise when
> rotation-locking is enabled gnome-shell should also use the value from the
> property if set, fixing 2.

FWIW, this is not how rotation locking works in gnome right now. The current semantics are that rotation lock just prevents further orientation changes to cause automatic rotations until the lock is unlocked again. Are you proposing to change this behavior?

Hi,

(In reply to Rui Matos from comment #4)
> (In reply to Hans de Goede from comment #0)
> > from above, likewise when
> > rotation-locking is enabled gnome-shell should also use the value from the
> > property if set, fixing 2.
> 
> FWIW, this is not how rotation locking works in gnome right now. The current
> semantics are that rotation lock just prevents further orientation changes
> to cause automatic rotations until the lock is unlocked again. Are you
> proposing to change this behavior?

No, sorry this was based on a misunderstanding of how rotation-locking works. If it just locks things to the last used rotation then nothing needs to change there.

Regards,

Hans

(In reply to Rui Matos from comment #4)
> FWIW, this is not how rotation locking works in gnome right now. The current
> semantics are that rotation lock just prevents further orientation changes
> to cause automatic rotations until the lock is unlocked again. Are you
> proposing to change this behavior?

Would be nice if the rotation got remembered on locking and would get restored on a reboot, I think that not happening is where my confusion came from. Shall I file a bug for this?

Created attachment 359986 [details] [review]
[PATCH] monitor-manager: Set initial transform based on "panel orientation" drm_connector prop

So I've done some more work on this, including getting kernel support for this in place. The plan is for the kernel to export a "panel orientation" property for this on the LCD panel drm_connector (and the drm maintainers seem to like this I already posted a v1 patchset).

I'm attaching a mutter patch adding support for this, which resolves this bug.

Note that this patch should NOT be merged yet as the drm_property for this is not yet final.

Review of attachment 359986 [details] [review]:

Is there a way to override the default value read from the drm connector if it's either incorrect or not present, at least for testing purposes?

Is the "preferred transformation" exported somehow, so that we can avoid showing things like "Landscape flipped" or "Portrait flipped" as the default/current values in the Displays panel?

Does the code that listens to the accelerometer need at least a comment, explaining how the accelerometer needs to be oriented (after quirks) compared to the screen? For example, if the default drm transform says that the display needs to be rotated 90 degrees to appear as the bezels and case appear to infer, would the sensor output need to say?

(In reply to Bastien Nocera from comment #8)
> Review of attachment 359986 [details] [review] [review]:
> 
> Is there a way to override the default value read from the drm connector if
> it's either incorrect or not present, at least for testing purposes?

ATM not, but I guess we could add an environment variable for that and if
meta_monitor_is_laptop_panel() returns true check that environment variable
and use it instead of the property.

> Is the "preferred transformation" exported somehow, so that we can avoid
> showing things like "Landscape flipped" or "Portrait flipped" as the
> default/current values in the Displays panel?

Again ATM not, I was aiming for a minimal solution at first where at least we show things the right way up. I agree that exporting this and compensating
for it in the Displays panel is a good idea. This can be done in a follow up patch.

> Does the code that listens to the accelerometer need at least a comment,
> explaining how the accelerometer needs to be oriented (after quirks)
> compared to the screen? For example, if the default drm transform says that
> the display needs to be rotated 90 degrees to appear as the bezels and case
> appear to infer, would the sensor output need to say?

My plan here is to have the accelerometer orientation quirk be such that when the accelerometer says normal, the display is unrotated / not compensated for
its mounting so for an upside down lcd panel when holding the tablet normally
the accelerometer will report upside-down. There are 2 reasons for this:

1) KISS, this avoids having to have extra code in mutter to compensate accelerometer orientation values for the monitors preferred_transform /
panel orientation.

2) On some models the kernel info about the display orientation comes from DMI
quirks, so it is possible for the kernel to not have a quirk yet, while we do
have a accel orientation quirk and then we still want things to work.

But I do agree that we should document this somewhere, any suggestion where this would best be documented?

p.s.

Note that also for KISS reasons the kernel patch currently has the following kernel doc about touchscreen orientation:

+ * Connectors for LCD panels may also have one standardized property:
+ *
+ * panel orientation:
+ *	On some devices the LCD panel is mounted in the casing in such a way
+ *	that the up/top side of the panel does not match with the top side of
+ *	the device. Userspace can use this property to check for this.
+ *	Note that input coordinates from touchscreens (input devices with
+ *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
+ *	coordinates, so if userspace rotates the picture to adjust for
+ *	the orientation it must also apply the same transformation to the
+ *	touchscreen input coordinates.


This actually also keeps things on the kernel side KISS of the 5 models I'm aware of which have a 90 degree rotated screen, 4 have a touchscreen for all 4 with a touchscreen, the touchscreen coordinates match the display coordinates before any rotation to compensate for the orientation. IOW with the above
definition on how touchscreen coordinates should actually match the raw
unrotated display coordiantes things just work from a kernel pov, as well as from a gnome-shell pov.

This is also important for older applications (like older versions of gnome shell) which will also expect the unrotated display coordinates to align with the touchscreen coordinates.

This also means that in older version of gnome-shell when manually rotating the display from the control panel, the touchscreen coordinates will match up.

(In reply to Hans de Goede from comment #9)

...

> 
> + * Connectors for LCD panels may also have one standardized property:
> + *
> + * panel orientation:
> + *	On some devices the LCD panel is mounted in the casing in such a way
> + *	that the up/top side of the panel does not match with the top side of
> + *	the device. Userspace can use this property to check for this.
> + *	Note that input coordinates from touchscreens (input devices with
> + *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
> + *	coordinates, so if userspace rotates the picture to adjust for
> + *	the orientation it must also apply the same transformation to the
> + *	touchscreen input coordinates.
> 

Given the new API we provide in 3.26, where we abstract away quirks and unnecessary details (like tiling) to the user(s) (e.g. gnome-control-center, as well as internal mutter/gnome-shell features such as rotate-key-binding), I think it makes more sense if we abstract panel orientation on the same level as we abstract tiling.

Thus, can we just make "normal" transform of a logical monitor (both the logical monitor exposed by org.gnome.Mutter.DisplayConfig and MetaLogicalMonitor) be correct, and then adjust the transform when configuring the crtc/output given any panel orientation exposed by drm/xrandr.

This would be much more "KISS" to me, as otherwise we need to spread out this quirk on several code bases, just as we did with tiling (which was terrible).

Hi,

(In reply to Jonas Ådahl from comment #10)
> (In reply to Hans de Goede from comment #9)
> 
> ...
> 
> > 
> > + * Connectors for LCD panels may also have one standardized property:
> > + *
> > + * panel orientation:
> > + *	On some devices the LCD panel is mounted in the casing in such a way
> > + *	that the up/top side of the panel does not match with the top side of
> > + *	the device. Userspace can use this property to check for this.
> > + *	Note that input coordinates from touchscreens (input devices with
> > + *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
> > + *	coordinates, so if userspace rotates the picture to adjust for
> > + *	the orientation it must also apply the same transformation to the
> > + *	touchscreen input coordinates.
> > 
> 
> Given the new API we provide in 3.26, where we abstract away quirks and
> unnecessary details (like tiling) to the user(s) (e.g. gnome-control-center,
> as well as internal mutter/gnome-shell features such as rotate-key-binding),
> I think it makes more sense if we abstract panel orientation on the same
> level as we abstract tiling.

Ok, note the above kerneldoc-comment is from the kernel and for the kernel we don't really have much choice. Touchscreens have always generated coordinates
which match the native (unrotated) display coordinates, people are already
using Linux on these devices with manually setup display rotation and we cannot
just all of a sudden start the touchscreen drivers to send out coordinates with x/y swapped as that will break existing setups. Also all the hardware also
seems to send out touchscreen coordinates matching the native (unrotated) display coordinates, so things just work this way without the need to add device specific quirks to the kernel.

> Thus, can we just make "normal" transform of a logical monitor (both the
> logical monitor exposed by org.gnome.Mutter.DisplayConfig and
> MetaLogicalMonitor) be correct, and then adjust the transform when
> configuring the crtc/output given any panel orientation exposed by
> drm/xrandr.

Yes I do agree that we should probably hide all this at the mutter level rather then exporting it so that the display-pane can compensate. I will redo the (attached) patch for this, so that it transparently hides the rotation needed to compensate for the panel orientation.

So that does leave the question of what to do with the accelerometer orientation, I do believe that we should follow the way we must deal with the touchscreen there and have the hwdb quirk setup so that normal corresponds with the LCD panel being upright in its native (unrotated) orientation, so that accelerometer based rotation will also do the right thing with older gnome versions and other desktop environments.

Regards,

Hans

(In reply to Hans de Goede from comment #11)
<snip>
> So that does leave the question of what to do with the accelerometer
> orientation, I do believe that we should follow the way we must deal with
> the touchscreen there and have the hwdb quirk setup so that normal
> corresponds with the LCD panel being upright in its native (unrotated)
> orientation, so that accelerometer based rotation will also do the right
> thing with older gnome versions and other desktop environments.

I guess we need to document that somewhere then :)

(In reply to Bastien Nocera from comment #12)
<snip>
> I guess we need to document that somewhere then :)

Agreed, but where?

(In reply to Hans de Goede from comment #13)
> (In reply to Bastien Nocera from comment #12)
> <snip>
> > I guess we need to document that somewhere then :)
> 
> Agreed, but where?

I think it would need to be mentioned in systemd's hwdb documentation for the sensors, as that might be where there would be confusion:
https://github.com/systemd/systemd/blob/master/hwdb/60-sensor.hwdb

Review of attachment 359986 [details] [review]:

(In reply to Hans de Goede from comment #11)

...

> Yes I do agree that we should probably hide all this at the mutter level
> rather then exporting it so that the display-pane can compensate. I will
> redo the (attached) patch for this, so that it transparently hides the
> rotation needed to compensate for the panel orientation.

Marking patch as 'needs-work' given the above quote.

Created attachment 362202 [details] [review]
[PATCH 1/6] monitor: s/meta_monitor_derived_derive_layout/meta_monitor_tiled_derive_layout/

Hi,

Here is a new series addressing the comments against the first attempt.

This series is against the 3.26 branch as gnome-3.26 does not seem to work with the master branch. I've an untested rebase against master here: https://github.com/jwrdegoede/mutter/commits/master

Some bits if the code have moved to different files, but otherwise the rebase was trivial. I will test the version against master once rawhide has a working gnome-3.27.x

Regards,

Hans

Created attachment 362203 [details] [review]
[PATCH 2/6] monitor-manager: Take drm-connector panel-orientation property into account

Created attachment 362204 [details] [review]
[PATCH 3/6] monitor-manager: Take panel orientation into account when getting input matrix

Created attachment 362205 [details] [review]
[PATCH 4/6] cursor-renderer-native: Take panel-orientation into account

Created attachment 362206 [details] [review]
[PATCH 5/6] monitor-manager: Add portrait modes to portrait displays

Created attachment 362207 [details] [review]
[PATCH 6/6] monitor-config-manager: Adjust accelerometer rotation for panel-orientation

Hi,

(In reply to Bastien Nocera from comment #14)
> (In reply to Hans de Goede from comment #13)
> > (In reply to Bastien Nocera from comment #12)
> > <snip>
> > > I guess we need to document that somewhere then :)
> > 
> > Agreed, but where?
> 
> I think it would need to be mentioned in systemd's hwdb documentation for
> the sensors, as that might be where there would be confusion:
> https://github.com/systemd/systemd/blob/master/hwdb/60-sensor.hwdb

Good idea, pull-req submitted:

https://github.com/systemd/systemd/pull/7177

Regards,

Hans

Review of attachment 362202 [details] [review]:

lgtm, except for a style nit

::: src/backends/meta-monitor.c
@@ +1145,1 @@
                                     MetaRectangle *layout)

nit: also fix the now incorrect indentation

Review of attachment 362203 [details] [review]:

Would be nice with unit tests for this. We now have unit tests for most of these types of monitor configuration concepts. See src/tests/monitor-unit-tests.c.

Also, this will most likely not apply on master, since the hybrid-gpu branch has landed there.

Otherwise overall I think this looks good.

::: src/backends/meta-monitor-manager-private.h
@@ +178,2 @@
   MetaConnectorType connector_type;
+  int panel_orientation_transform;

MetaMonitorTransform panel_orientation_transform;

::: src/backends/meta-monitor.c
@@ +417,3 @@
+                          int width,
+                          int height,
+                          MetaCrtcMode *crtc_mode)

style nit: argument names should be aligned

@@ +1337,3 @@
+  flipped = transform / 4;
+  transform = (transform + output->panel_orientation_transform) % 4;
+  transform += flipped * 4;

This is clever and all, but looks like black magic. A suggestion

next to gboolean meta_monitor_transform_is_rotated() add gboolean meta_monitor_transform_is_flipped()

then add a

MetaMonitorTransform
meta_monitor_transform_rotate_by (transform, rotation)
{
  MetaTransform new_transform;

  new_transform = (transform + rotation) % 4;
  if (meta_monitor_transform_is_flipped (transform)
    new_transform += 4;

  return new_transform;
}

(and the reverse for the reverse)

Sure it is more code, but then the next reader doesn't have to go and translate the black magic to something more understandable.

::: src/backends/native/meta-renderer-native.c
@@ +1683,3 @@
   main_monitor = meta_logical_monitor_get_monitors (logical_monitor)->data;
   main_output = meta_monitor_get_main_output (main_monitor);
+  crtc_transform = meta_monitor_logical_to_crtc_transform (main_monitor, logical_monitor->transform);

style nit: avoid too long lines (general "rule" is 80 chars if not too inconvenient)

::: src/backends/x11/nested/meta-renderer-x11-nested.c
@@ +53,3 @@
   main_monitor = meta_logical_monitor_get_monitors (logical_monitor)->data;
   main_output = meta_monitor_get_main_output (main_monitor);
+  crtc_transform = meta_monitor_logical_to_crtc_transform (main_monitor, logical_monitor->transform);

style nit: avoid too long lines (general "rule" is 80 chars if not too inconvenient)

Review of attachment 362204 [details] [review]:

looks good, just cosmetic nits.

::: src/backends/meta-input-settings.c
@@ +799,3 @@
+                                  ClutterInputDevice  *device,
+                                  MetaMonitor        **monitor_ret,
+                                  MetaLogicalMonitor **logical_mon_ret)

consistency nit: The places I know about in the mutter code base that does this, the naming convention is

MetaType **out_blah, not MetaType **blah_ret

@@ +803,3 @@
   MetaInputSettingsPrivate *priv;
   MetaMonitorManager *monitor_manager;
+  MetaMonitor *mon;

for consistency: s/mon/monitor/

::: src/backends/meta-monitor-manager.c
@@ +2824,3 @@
 
+  /* Get transform corrected for LCD panel-orientation. */
+  transform = meta_monitor_logical_to_crtc_transform (monitor, logical_monitor->transform);

nit: split long line

Review of attachment 362205 [details] [review]:

lgtm.

Review of attachment 362206 [details] [review]:

Makes sense.

::: src/backends/native/meta-default-modes.h
@@ +27,2 @@
 { 813000, 4096, 4440, 4888, 5680, 0, 2304, 2307, 2312, 2386, 0, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC, DRM_MODE_TYPE_DEFAULT, "4096x2304_60.00" },
 { 1276500, 5120, 5560, 6128, 7136, 0, 2880, 2883, 2888, 2982, 0, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC, DRM_MODE_TYPE_DEFAULT, "5120x2880_60.00" },

Shouldn't we call the "normal" modes 'landscape' now?

Review of attachment 362207 [details] [review]:

Should we fix this here, or in MetaOrientationManager?

Hi,

Thank you for the quick review.

(In reply to Jonas Ådahl from comment #24)
> Review of attachment 362203 [details] [review] [review]:
> 
> Would be nice with unit tests for this. We now have unit tests for most of
> these types of monitor configuration concepts. See
> src/tests/monitor-unit-tests.c.

Looks doable, I think I will add a test-case for a 90 degree rotated normal
(non-tiled) monitor there, which is the most tricky case.

How do I run these?

> Also, this will most likely not apply on master, since the hybrid-gpu branch
> has landed there.

Right, as mentioned I've a rebased version of the patches available here:

https://github.com/jwrdegoede/mutter/commits/master

But that is untested as I'm waiting for gnome 3.27 to land in rawhide, I guess I could try building all of gnome from source but I would rather wait a bit.

> Otherwise overall I think this looks good.

Great, I'm happy to hear that.

As for all your other comments I agree with all of them except for 2 where I've some questions (see below) and I will fix them all for the next version.

(In reply to Jonas Ådahl from comment #27)
> Review of attachment 362206 [details] [review] [review]:
> 
> Makes sense.
> 
> Shouldn't we call the "normal" modes 'landscape' now?

landscape is the default assumption, as you indicate yourself by calling those modes "normal", so I left it out, but if you want me to prefix the normal mode list with landscape, let me know and I will do so for the next version.

(In reply to Jonas Ådahl from comment #28)
> Review of attachment 362207 [details] [review] [review]:
> 
> Should we fix this here, or in MetaOrientationManager?

MetaOrientationManager is not aware of monitors and I think we should keep it that way, we need a pointer to the LCD-panel monitor, which is why I did this in monitor-config-manager, my initial plan was to get the monitor pointer from the logical-monitor-config, but that required diving 3 levels down IIRC so I ended up just calling meta_monitor_manager_get_laptop_panel(), but even that requires a pointer to the monitor-manager which is not available in MetaOrientationManager AFAICT.

TL;DR: I believe this patch is best kept as is.

Regards,

Hans

(In reply to Hans de Goede from comment #29)
> Hi,
> 
> Thank you for the quick review.
> 
> (In reply to Jonas Ådahl from comment #24)
> > Review of attachment 362203 [details] [review] [review] [review]:
> > 
> > Would be nice with unit tests for this. We now have unit tests for most of
> > these types of monitor configuration concepts. See
> > src/tests/monitor-unit-tests.c.
> 
> Looks doable, I think I will add a test-case for a 90 degree rotated normal
> (non-tiled) monitor there, which is the most tricky case.
> 
> How do I run these?

make -C src run-tests

Note that they are flaky though, I know of three races (two on master):

1. it might segfault if Xwayland didn't start fast enough
2. it might emit two types of warnings (one on master) on the headless tests about stage size and/or about timings. the test runner aborts on warnings.

FWIW, it's on my todo to fix these (at least 2., fixing 1. is more about not relying on Xwayland)

> 
> > Also, this will most likely not apply on master, since the hybrid-gpu branch
> > has landed there.
> 
> Right, as mentioned I've a rebased version of the patches available here:
> 
> https://github.com/jwrdegoede/mutter/commits/master
> 
> But that is untested as I'm waiting for gnome 3.27 to land in rawhide, I
> guess I could try building all of gnome from source but I would rather wait
> a bit.

Yea, I should look at the i686 build errors.

> 
> > Otherwise overall I think this looks good.
> 
> Great, I'm happy to hear that.
> 
> As for all your other comments I agree with all of them except for 2 where
> I've some questions (see below) and I will fix them all for the next version.
> 
> (In reply to Jonas Ådahl from comment #27)
> > Review of attachment 362206 [details] [review] [review] [review]:
> > 
> > Makes sense.
> > 
> > Shouldn't we call the "normal" modes 'landscape' now?
> 
> landscape is the default assumption, as you indicate yourself by calling
> those modes "normal", so I left it out, but if you want me to prefix the
> normal mode list with landscape, let me know and I will do so for the next
> version.

I suppose (I didn't add them myself) they were unnamed that way since portrait modes were not considered. With this changed, it makes sense make the difference between them more obvious I think.

> 
> (In reply to Jonas Ådahl from comment #28)
> > Review of attachment 362207 [details] [review] [review] [review]:
> > 
> > Should we fix this here, or in MetaOrientationManager?
> 
> MetaOrientationManager is not aware of monitors and I think we should keep
> it that way, we need a pointer to the LCD-panel monitor, which is why I did
> this in monitor-config-manager, my initial plan was to get the monitor
> pointer from the logical-monitor-config, but that required diving 3 levels
> down IIRC so I ended up just calling
> meta_monitor_manager_get_laptop_panel(), but even that requires a pointer to
> the monitor-manager which is not available in MetaOrientationManager AFAICT.

Ah, right, it the monitor manager isn't created until after the orientation manager. Lets leave it like it is then.

Just to confirm, the orientation retrieved from the orientation service is the "incorrectly" oriented one right? As in, if it says "right side up" it doesn't mean that the right side of the device when held in its natural position is pointing upwards. FWIW, I don't think it makes sense to do it that way; applications caring about orientation might not be able to correct the orientation if it doesn't get access to the DRM device.

(In reply to Hans de Goede from comment #29)
<snip>
> (In reply to Jonas Ådahl from comment #27)
> > Review of attachment 362206 [details] [review] [review] [review]:
> > 
> > Makes sense.
> > 
> > Shouldn't we call the "normal" modes 'landscape' now?
> 
> landscape is the default assumption, as you indicate yourself by calling
> those modes "normal", so I left it out, but if you want me to prefix the
> normal mode list with landscape, let me know and I will do so for the next
> version.

No, whether normal is "landscape" or "portrait" depends on the device. There are a number of tablets (usually smaller sized ones) where "normal" is portrait. Ditto with even smaller devices such as phones.

As you can see, those names are relative, not absolute, so "landscape" and "portrait" shouldn't be used.

Ok, here is a new version of the patches addressing the review comments, this time against master (and tested against master).

Note that the kernel change for the panel-orientation property has not yet been merged, so if we merge this we may need to adjust it later to match the kernel implementation, or we need to wait a bit. The reception of the panel-orientation property has so far been positive so I don't expect this to change. Also it seems pretty definitive that we are going to use a drm-connector prop for this the only thing which may change is the name, but that seems unlikely.

Created attachment 362314 [details] [review]
[PATCH 1/7] monitor: s/meta_monitor_derived_derive_layout/meta_monitor_tiled_derive_layout/

Created attachment 362315 [details] [review]
[PATCH 2/7] monitor-manager: Take drm-connector panel-orientation property into account

Created attachment 362316 [details] [review]
[PATCH 3/7] monitor-manager: Take panel orientation into account when getting input matrix

Created attachment 362317 [details] [review]
[PATCH 4/7] cursor-renderer-native: Take panel-orientation into account

Created attachment 362318 [details] [review]
[PATCH 5/7] monitor-manager: Add portrait modes to portrait displays

Created attachment 362319 [details] [review]
[PATCH 6/7] monitor-manager: Take panel-orientation into account for physical size

Note new patch in this version of the patch-series.

Created attachment 362320 [details] [review]
[PATCH 7/7] monitor-config-manager: Adjust accelerometer rotation for panel-orientation

Created attachment 362349 [details] [review]
[PATCH 1/2] monitor-unit-tests: Add support for panel-orientation

These 2 patches implement the promised unit-test for this

Created attachment 362350 [details] [review]
[PATCH 2/2] monitor-unit-tests: Add non upright panel test

Is there any documentation I can found describing how iio-sensor-proxy deals with rotated panels? Is every user of the API supposed to query KMS/Xrandr to get the "real" orientation of the device? I can't find anything in https://github.com/hadess/iio-sensor-proxy/ about it.

(In reply to Jonas Ådahl from comment #42)
> Is there any documentation I can found describing how iio-sensor-proxy deals
> with rotated panels? I can't find anything in https://github.com/hadess/iio-sensor-proxy/ about it.

https://github.com/hadess/iio-sensor-proxy/ says:

"Accelerometer orientation

When the accelerometer is not mounted the same way as the screen, we need to modify the readings from the accelerometer to make sure that the computed orientation matches the screen one.

iio-sensor-proxy reads this information from the device's ACCEL_MOUNT_MATRIX udev property. See 60-sensor.hwdb for details."

With 60-sensor.hwdb being a link to:

https://github.com/systemd/systemd/blob/master/hwdb/60-sensor.hwdb#L41

> Is every user of the API supposed to query KMS/Xrandr
> to get the "real" orientation of the device?

No, not needing todo this is exactly why things are as they are, apps / desktop-environments (DEs) which are not aware of panel-orientation don't need to do anything, the orientation as coming out of iio-sensor-proxy is such that apps who don't know about panel-orientation will automatically rotate the screen in such a way that it matches up with the accelerometer.

A simple example, lets take a tablet with an upside-down mounted LCD-panel and a non panel-orientation aware DE. If the user holds the device case normally/upright then iio-sensor-proxy will report upside-down as orientation since the iio-sensor-proxy orientation will match the actual LCD panel orientation (and not the device's casing one). In a response to iio-sensor-proxy reporting upside-down as orientation the DE will rotate the frames send to the LCD by 180 degrees, compensating for the upside-down mounted LCD panel and to the user the DE shows upright while the user is holding the device upright.

We really don't have any other choice here as 60-sensor.hwdb already has a couple of entries for devices with non upright panels doing exactly this, so that existing DEs show up the right way (once the DE checks the iio-sensor-proxy reported orientation, typically they will briefly show the wrong way on login).

So the reason why mutter needs to take panel-orientation into account when dealing with iio-sensor-proxy reported orientation is because it is already panel-orientation aware.

(In reply to Hans de Goede from comment #43)
> (In reply to Jonas Ådahl from comment #42)
> > Is there any documentation I can found describing how iio-sensor-proxy deals
> > with rotated panels? I can't find anything in https://github.com/hadess/iio-sensor-proxy/ about it.
> 
> https://github.com/hadess/iio-sensor-proxy/ says:
> 
> "Accelerometer orientation
> 
> When the accelerometer is not mounted the same way as the screen, we need to
> modify the readings from the accelerometer to make sure that the computed
> orientation matches the screen one.
> 
> iio-sensor-proxy reads this information from the device's ACCEL_MOUNT_MATRIX
> udev property. See 60-sensor.hwdb for details."
> 
> With 60-sensor.hwdb being a link to:
> 
> https://github.com/systemd/systemd/blob/master/hwdb/60-sensor.hwdb#L41

Ah, I was looking in the API documentation.

> 
> > Is every user of the API supposed to query KMS/Xrandr
> > to get the "real" orientation of the device?
> 
> No, not needing todo this is exactly why things are as they are, apps /
> desktop-environments (DEs) which are not aware of panel-orientation don't
> need to do anything, the orientation as coming out of iio-sensor-proxy is
> such that apps who don't know about panel-orientation will automatically
> rotate the screen in such a way that it matches up with the accelerometer.
> 
> A simple example, lets take a tablet with an upside-down mounted LCD-panel
> and a non panel-orientation aware DE. If the user holds the device case
> normally/upright then iio-sensor-proxy will report upside-down as
> orientation since the iio-sensor-proxy orientation will match the actual LCD
> panel orientation (and not the device's casing one). In a response to
> iio-sensor-proxy reporting upside-down as orientation the DE will rotate the
> frames send to the LCD by 180 degrees, compensating for the upside-down
> mounted LCD panel and to the user the DE shows upright while the user is
> holding the device upright.
> 
> We really don't have any other choice here as 60-sensor.hwdb already has a
> couple of entries for devices with non upright panels doing exactly this, so
> that existing DEs show up the right way (once the DE checks the
> iio-sensor-proxy reported orientation, typically they will briefly show the
> wrong way on login).
> 
> So the reason why mutter needs to take panel-orientation into account when
> dealing with iio-sensor-proxy reported orientation is because it is already
> panel-orientation aware.

So in other words, use cases which doesn't directly affect the CRTC/connector configuration like a desktop environment/compositor, that actually cares about the real orientation would still need to do what mutter now does, to get the "correct" orientation right (as in, how the device is physically oriented, not caring about panel mount details)? I guess this means that this API is not meant for arbitrary applications that wont be able to access such information (not that I can think of any that really cares about this I guess, just an observation)?

(In reply to Jonas Ådahl from comment #44)
> 
> So in other words, use cases which doesn't directly affect the
> CRTC/connector configuration like a desktop environment/compositor, that
> actually cares about the real orientation would still need to do what mutter
> now does, to get the "correct" orientation right (as in, how the device is
> physically oriented, not caring about panel mount details)? I guess this
> means that this API is not meant for arbitrary applications that wont be
> able to access such information (not that I can think of any that really
> cares about this I guess, just an observation)?

Correct.

Hi, good news the kernel side of this has been queued for merging into 4.16, so we're good to go to merge these patches.

There has been 1 change from the kernel side, the DRM_MODE_PANEL_ORIENTATION_ #defines I was using are not to be added to the userspace API, instead userspace should check the string values of the enum.

I've updated "monitor-manager: Take drm-connector panel-orientation property into account" accordingly and I've rebased (and re-tested) the entire set.

I'll go and attach the new versions.

Created attachment 365039 [details] [review]
[PATCH 1/9] monitor: s/meta_monitor_derived_derive_layout/meta_monitor_tiled_derive_layout

Created attachment 365040 [details] [review]
[PATCH 2/9] monitor-manager: Take drm-connector panel-orientation property into account

Created attachment 365041 [details] [review]
[PATCH 3/9] monitor-manager: Take panel orientation into account when getting input matrix

Created attachment 365042 [details] [review]
[PATCH 4/9] cursor-renderer-native: Take panel-orientation into account

Created attachment 365044 [details] [review]
[PATCH 5/9] monitor-manager: Add portrait modes to portrait displays

Created attachment 365045 [details] [review]
[PATCH 6/9] monitor-manager: Take panel-orientation into account for physical size

Created attachment 365046 [details] [review]
[PATCH 7/9] monitor-config-manager: Adjust accelerometer rotation for panel-orientation

Created attachment 365047 [details] [review]
[PATCH 8/9] monitor-unit-tests: Add support for panel-orientation

Created attachment 365048 [details] [review]
[PATCH 9/9] monitor-unit-tests: Add non upright panel test

Pushed to master.

(In reply to Jonas Ådahl from comment #56)
> Pushed to master.

Great, thank you! And Merry Christmas :)