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

[Hans de Goede]

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.:bvr5.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

Regards,

Hans