[systemd-devel] Bad accelerometer values cause incorrect screen rotation

Thu Sep 5 13:00:29 UTC 2019

On Thu, 2019-09-05 at 14:36 +0200, Hans de Goede wrote:
> Hi,
> 
> On 05-09-19 14:11, Hans de Goede wrote:
> > Hi,
> > 
> > On 05-09-19 13:28, Bastien Nocera wrote:
> > > On Thu, 2019-09-05 at 13:13 +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 05-09-19 12:49, Bastien Nocera wrote:
> > > > > On Thu, 2019-09-05 at 18:38 +0800, Daniel Drake wrote:
> > > > > > On Thu, Sep 5, 2019 at 6:07 PM Bastien Nocera <
> > > > > > hadess at hadess.net>
> > > > > > wrote:
> > > > > > > I've read through this, and I'm happy blacklisting the
> > > > > > > hp_accel
> > > > > > > driver
> > > > > > > in code. For the other devices, I'd rather leave it as-
> > > > > > > is.
> > > > > > 
> > > > > > That would indeed avoid most problem cases that I've seen,
> > > > > > and
> > > > > > the
> > > > > > current case, probably enough to stop me grumbling for
> > > > > > another
> > > > > > year
> > > > > > or
> > > > > > so until this happens again in some other context :)
> > > > > > So I support that idea. Do you have any preference on where
> > > > > > we
> > > > > > blacklist it?
> > > > > > 
> > > > > > In the hwdb it's quite easy to match DMI vendor HP & driver
> > > > > > lis3lv02d.
> > > > > > But we'd really want a new way of saying "ignore the
> > > > > > accelerometer"
> > > > > > as
> > > > > > ACCEL_POSITION=base doesn't seem like the right way to
> > > > > > express
> > > > > > that.
> > > > > > 
> > > > > > Or we could blacklist it in iio-sensor-proxy but since
> > > > > > there's no
> > > > > > mention of hp_accel in the udev properties for the device
> > > > > > (you
> > > > > > just
> > > > > > get the driver as li3lv02d) then you'd need to grab the DMI
> > > > > > vendor
> > > > > > name from /sys/class/dmi/id or something like that.
> > > > > > Maybe making this driver export enough data so we can
> > > > > > blacklist
> > > > > > it
> > > > > > would be the best way to go about it, along with a new udev
> > > > > > property.
> > > > > 
> > > > > We should make this driver export enough data so we can
> > > > > differentiate
> > > > > it, then we can install a udev property private to iio-
> > > > > sensor-proxy
> > > > > about ignoring specific accelerometers such as this one. This
> > > > > way,
> > > > > the
> > > > > sensor hwdb only contains quirks, not policy decisions about
> > > > > whether
> > > > > the hp_accel driver is worthy.
> > > > 
> > > > I think a good question to ask here is, why do we want to
> > > > blacklist
> > > > the lis3lv02d when used in HP laptops and I think the answer is
> > > > because it usually sits in the base of the device. So a simpler
> > > > answer
> > > > here might be to just do this:
> > > > 
> > > > diff --git a/hwdb/60-sensor.hwdb b/hwdb/60-sensor.hwdb
> > > > index 3976d9a76a..de5e1b95a2 100644
> > > > --- a/hwdb/60-sensor.hwdb
> > > > +++ b/hwdb/60-sensor.hwdb
> > > > @@ -288,13 +288,6 @@
> > > > sensor:modalias:acpi:KIOX000A*:dmi:bvnAmericanMegatrendsInc.:bv
> > > > r5.11:
> > > > bd05/25/201
> > > >    # is then applied below.
> > > >    sensor:modalias:platform:lis3lv02d:dmi:*svn*Hewlett-
> > > > Packard*:*
> > > >     ACCEL_MOUNT_MATRIX=1, 0, 0; 0, 0, -1; 0, 1, 0
> > > > -
> > > > -# HP laptops which have the lis3lv02d device in the base, tell
> > > > iio-
> > > > sensor-proxy
> > > > -# about this so that the sensor is not used for display
> > > > orientation
> > > > -sensor:modalias:platform:lis3lv02d:dmi:*svn*Hewlett-
> > > > Packard*:*pnHPProBook4535s*
> > > > - ACCEL_LOCATION=base
> > > > -
> > > > -sensor:modalias:platform:lis3lv02d:dmi:*:svnHewlett-
> > > > Packard:pnHPENVY17NotebookPC:*
> > > >     ACCEL_LOCATION=base
> > > > 
> > > >    sensor:modalias:acpi:SMO8500*:dmi:*:svnHewlett-
> > > > Packard:pnHPStream7Tablet:*
> > > > 
> > > > 
> > > > So in the default hwdb rule for HP laptops with the lis3lv02d
> > > > sensor
> > > > mark
> > > > the sensor as being in the base.
> > > > 
> > > > This will have the same result as marking it as hp_accel +
> > > > blacklisting
> > > > hp_accel in iio-sensor-proxy. With the added advantage that we
> > > > can
> > > > whitelist it if we encounter a HP tablet or 2-in-1 where we do
> > > > actually
> > > > want to use it for auto-rotation.
> > > > 
> > > > And this would not require adding any extra code anywhere, so I
> > > > believe
> > > > this would be a nice KISS solution.
> > > 
> > > That works for me, although the mount matrix would now be wrong
> > > and
> > > should probably be removed (or moved).
> > 
> > True, I will remove it.
> 
> So looking at the mount-matrix closer, the comment describes it
> as mapping "from "can play neverball" to "matches Windows 8
> orientation""
> but what it really does translates base-coordinates to display-
> coordinates
> assuming the display is at an angle of exact 90 degrees to the base
> (swap Y and Z axis).
> 
> So dropping the MOUNT_MATRIX very much is the right thing, as this
> will
> leave us with sensor readings in base-coordinates matching the
> ACCEL_LOCATION=base udev attribute.
> 
> I think that this will greatly help with the problem Daniel
> described,
> as least for HP laptops which seem to be a major source of bug
> reports
> about this problem.
> 
> I've submitted a pullreq with the discussed changes here:
> https://github.com/systemd/systemd/pull/13479

Daniel, if you run into many more problems, there's also the
possibility of adding a boot argument to disable the accelerometer (or
maybe its effects?), either in iio-sensor-proxy or gnome-shell.

Locking the rotation by default wouldn't be so great, as unlocking it
is likely to break those affected systems, with no way to reset it
easily. Rebooting with the boot argument would be easier.