Re: Bug#1029850: linux: Driver not loaded for ST Microelectronics LSM6DS3TR-C accelerometer (acpi:SMO8B30:SMO8B30:)

[Darrell Kavanagh]

OK, pull request has been submitted.
https://github.com/systemd/systemd/pull/26317

On Sun, 5 Feb 2023 at 18:06, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 2/5/23 17:06, Darrell Kavanagh wrote:
> > So does this mean that the least worst (only?) option is to get my
> > hwdb mount matrix entry added to systemd? I can raise a bug as
> > suggested in hwdb.d/60-sensor.hwdb if so.
>
> Yes you should add a hwdb entry for this, note just submitting
> a pull-req with the fix is better then filing an issue for this.
>
> Regards,
>
> Hans
>
>
>
> > On Sun, 5 Feb 2023 at 14:22, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >>
> >> On Sun, 5 Feb 2023 09:50:51 +0100
> >> Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >>> Hi,
> >>>
> >>> On 2/4/23 23:15, Darrell Kavanagh wrote:
> >>>> Yes, I understand that.
> >>>>
> >>>> What I mean is that the matrix read from the DSDT by Jonathan's
> >>>> amended driver is
> >>>>
> >>>>  0 -1  0
> >>>>  1  0  0
> >>>>  0  0  1
> >>>>
> >>>> and the (correct) matrix created with my new hwdb entry is
> >>>>
> >>>>  0  1  0
> >>>> -1  0  0
> >>>>  0  0  1
> >>
> >> May be concidence, but I think that's the inverse of the one we are reading
> >> from ROTM - so represents the transform in the other direction.
> >>
> >> The way ROTM is defined is that first row represents the direction of
> >> the x axis in device coordinates - so it's the transform from sensor
> >> to device space.
> >>
> >> I wonder if the hwdb matrix is defined from world space to sensor?  Seems
> >> unlikely.
> >>
> >> The IIO ABI docs describe mount matrix as being what you apply to data to
> >> tranform into device space (oh for a diagram in the docs).  Anyhow my reading
> >> is that matches with ROTM definition but maybe I'm reading that wrong...
> >>
> >> For extra annoyance, the ROTM matrix on this device isn't a rotation matrix.
> >> It's flipping the handedness of the sensor.  Determinant isn't -1 which it
> >> should be.  I guess the sensor itself might have an axis backwards from
> >> windows convention though *sigh*  I think windows uses left handed convention
> >> and looks like sensor is using right handed (which I think is what Android and
> >> similar use).
> >>
> >>>>
> >>>> which is the algebraic transposition (ie reflection in the diagonal)
> >>>> of the DSDT one.
> >>>>
> >>>> In other words, though the DST matrix is wrong, it is wrong in a
> >>>> specific way - the rows should be the columns, and vv. I was just
> >>>> wondering if this was a DSDT bug that might have been seen elsewhere
> >>>> before.
> >>>
> >>> No this does not ring a bell, but the x and y axis being swapped
> >>> does seem related to the LCD panel being 90° rotated.
> >>>
> >>>> BTW, there is another matrix in the DSTD, but I can't find the
> >>>> associated HID (10EC5280) anywhere (Linux sysfs or Windows Powershell
> >>>> system data extract). It's not a correct matrix, though - could it be
> >>>> just a bit of redundant code in the DST?
> >>>
> >>> Yes that is likely there often is a bunch of dead stuff DSDT leftover
> >>> from other device models.
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>>
> >>>> Darrell
> >>>>
> >>>> On Sat, 4 Feb 2023 at 21:31, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On 2/4/23 18:09, Darrell Kavanagh wrote:
> >>>>>> I've just noticed that the working mount matrix that I added to my
> >>>>>> hwdb is the matrix retrieved from the ACPI ROTM call in the amended
> >>>>>> driver, transposed.
> >>>>>
> >>>>> An other word for the mount matrix would be a rotation matrix,
> >>>>> since it defines how the physical sensor is mounted on the PCB
> >>>>> in a rotated fashion compared to its standard orientation.
> >>>>>
> >>>>> The x, y, z axis relationship underling of course does
> >>>>> not change by the rotation, so yes all mount matrices
> >>>>> are a transposition of the standard:
> >>>>>
> >>>>> 1, 0, 0 : 0, 1, 0 : 0, 0, 1
> >>>>>
> >>>>> matrix, that is expected. Where that to not be the case
> >>>>> then there would be a bug in the accelerometer driver itself
> >>>>> where the driver itself is swapping or inverting axis.
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> Hans
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>> On Fri, 3 Feb 2023 at 18:23, Darrell Kavanagh
> >>>>>> <darrell.kavanagh@xxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>> Finally got a 6.2.0-rc6 kernel built and installed, with the following
> >>>>>>> patch, and everything is working as expected.
> >>>>>>>
> >>>>>>> Moving on now to look at Bastien's suggestion.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Darrell
> >>>>>>>
> >>>>>>> diff --git a/kernel/drm_panel_orientation_quirks.c
> >>>>>>> b/kernel/linux-6.2-rc6/drivers/gpu/drm/drm_panel_orientation_quirks.c
> >>>>>>> index 3659f04..590bb7b 100644
> >>>>>>> --- a/kernel/drm_panel_orientation_quirks.c
> >>>>>>> +++ b/kernel/linux-6.2-rc6/drivers/gpu/drm/drm_panel_orientation_quirks.c
> >>>>>>> @@ -304,6 +304,12 @@ static const struct dmi_system_id orientation_data[] = {
> >>>>>>>                   DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "Lenovo ideapad
> >>>>>>> D330-10IGM"),
> >>>>>>>                 },
> >>>>>>>                 .driver_data = (void *)&lcd1200x1920_rightside_up,
> >>>>>>> +       }, {    /* Lenovo IdeaPad Duet 3 10IGL5 */
> >>>>>>> +               .matches = {
> >>>>>>> +                 DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> >>>>>>> +                 DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "IdeaPad Duet 3 10IGL5"),
> >>>>>>> +               },
> >>>>>>> +               .driver_data = (void *)&lcd1200x1920_rightside_up,
> >>>>>>>         }, {    /* Lenovo Ideapad D330-10IGL (HD) */
> >>>>>>>                 .matches = {
> >>>>>>>                   DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> >>>>>>>
> >>>>>>> On Wed, 1 Feb 2023 at 17:55, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> On 2/1/23 18:50, Darrell Kavanagh wrote:
> >>>>>>>>> Thank you. I don't have anything that could be called a big machine.
> >>>>>>>>> The fastest processor I have access to is a Core m3-8100Y - that's in
> >>>>>>>>> a Chromebook with 4GB memory - it can run Linux in a chroot or
> >>>>>>>>> officially in Google's VM. I also have an ancient gen 2 core i5-2410M
> >>>>>>>>> machine which is slower than the m3 in theory, but that has 6GB of
> >>>>>>>>> memory.
> >>>>>>>>>
> >>>>>>>>> Is the kernel build more processor or memory bound?
> >>>>>>>>
> >>>>>>>> It is mostly processor bound, esp. wtih something like make -j4,
> >>>>>>>> make -j16 will start taking some RAM, but with make -j4 I expect you
> >>>>>>>> to be fully CPU bound.
> >>>>>>>>
> >>>>>>>> Regards,
> >>>>>>>>
> >>>>>>>> Hans
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> On Wed, 1 Feb 2023 at 16:12, Bastien Nocera <hadess@xxxxxxxxxx> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Wed, 2023-02-01 at 12:00 +0100, Hans de Goede wrote:
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> On 2/1/23 11:28, Jonathan Cameron wrote:
> >>>>>>>>>>>> On Wed, 1 Feb 2023 01:40:49 +0000
> >>>>>>>>>>>> Darrell Kavanagh <darrell.kavanagh@xxxxxxxxx> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Hello, all.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I've finally reached a conclusion on this, after testing all the
> >>>>>>>>>>>>> combinations of the patches (with and without reading the acpi
> >>>>>>>>>>>>> mounting matrix), window managers (wayland, xorg) and the
> >>>>>>>>>>>>> presence or
> >>>>>>>>>>>>> not of my custom kernel parms.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What works well is the full set of patches with the custom kernel
> >>>>>>>>>>>>> parms and a new hwdb entry for the sensor:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> sensor:modalias:acpi:SMO8B30*:dmi:*:svnLENOVO*:pn82AT:*
> >>>>>>>>>>>>>  ACCEL_MOUNT_MATRIX=0, 1, 0; -1, 0, 0; 0, 0, 1
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> The autorotate then works correctly in wayland and xorg, but for
> >>>>>>>>>>>>> xorg,
> >>>>>>>>>>>>> the settings say the screen is "portrait left" when in actual
> >>>>>>>>>>>>> fact it
> >>>>>>>>>>>>> is in standard laptop landscape orientation. Wayland does not
> >>>>>>>>>>>>> have
> >>>>>>>>>>>>> this problem (I guess because wayland's view of the screen is
> >>>>>>>>>>>>> straight
> >>>>>>>>>>>>> from the kernel).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Without the hwdb entry, the orientation is 90 degrees out without
> >>>>>>>>>>>>> using the acpi matrix and 180 degrees out when using it. I could
> >>>>>>>>>>>>> have
> >>>>>>>>>>>>> gone either way here with appropriate hwdb entries, but my view
> >>>>>>>>>>>>> is
> >>>>>>>>>>>>> that we *should* be using the matrix.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Added Hans de Goede as he has probably run into more of this mess
> >>>>>>>>>>>> than anyone else.  Hans, any thoughts on if we are doing something
> >>>>>>>>>>>> wrong on kernel side?  Or is the matrix just wrong *sigh*
> >>>>>>>>>>>
> >>>>>>>>>>> I see below that this laptop has a panel which is mounted 90 degrees
> >>>>>>>>>>> rotated, that likely explains why the ACPI matrix does not work.
> >>>>>>>>>>> So the best thing to do here is to just override it with a hwdb
> >>>>>>>>>>> entries.
> >>>>>>>>>>>
> >>>>>>>>>>> IIRC there are already 1 or 2 other hwdb entries which actually
> >>>>>>>>>>> override the ACPI provided matrix because of similar issues.
> >>>>>>>>>>>
> >>>>>>>>>>> Linux userspace expects the matrix in this case to be set so that
> >>>>>>>>>>> it causes e.g. gnome's auto-rotation to put the image upright
> >>>>>>>>>>> even with older gnome versions / mate / xfce which don't know about
> >>>>>>>>>>> the panel being mounted 90 degrees.
> >>>>>>>>>>>
> >>>>>>>>>>> So e.g. "monitor-sensor" will report left-side-up or right-side-up
> >>>>>>>>>>> while the device is actually in normal clamshell mode with the
> >>>>>>>>>>> display up-right.
> >>>>>>>>>>>
> >>>>>>>>>>> This reporting of left-side-up or right-side-up is actually "correct"
> >>>>>>>>>>> looking from the native LCD panel orientation and as mentioned is
> >>>>>>>>>>> done for backward compatibility. This is documented here:
> >>>>>>>>>>>
> >>>>>>>>>>> https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb#L54
> >>>>>>>>>>>
> >>>>>>>>>>> The way we are handling this is likely incompatible with how Windows
> >>>>>>>>>>> handles this special case of 90° rotated screen + ROTM. Or the
> >>>>>>>>>>> matrix in the ACPI tables could be just wrong...
> >>>>>>>>>>>
> >>>>>>>>>>>> I think 'ROTM' is defined by MS.
> >>>>>>>>>>>> https://learn.microsoft.com/en-us/windows-hardware/drivers/sensors/sensors-acpi-entries
> >>>>>>>>>>>
> >>>>>>>>>>> Right and as such it would be good if we can still add support to
> >>>>>>>>>>> it to the sensor driver in question. Because the ROTM info usually
> >>>>>>>>>>> is correct and avoids the need for adding more and more hwdb entries.
> >>>>>>>>>>>
> >>>>>>>>>>> Note there already is existing support in some other sensor drivers.
> >>>>>>>>>>>
> >>>>>>>>>>> So we probably need to factor out some helper code for this and share
> >>>>>>>>>>> that between sensor drivers.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>> The only thing that concerns me is the need for custom kernel
> >>>>>>>>>>>>> parms.
> >>>>>>>>>>>>> It would be better if there was a way to avoid this, so that the
> >>>>>>>>>>>>> user
> >>>>>>>>>>>>> didn't have to mess around with their grub config. Though having
> >>>>>>>>>>>>> said
> >>>>>>>>>>>>> that, the sensors fix as we have it doesn't make things worse -
> >>>>>>>>>>>>> under
> >>>>>>>>>>>>> currently released kernels the screen always starts up sideways
> >>>>>>>>>>>>> unless
> >>>>>>>>>>>>> custom parms are added in grub.
> >>>>>>>>>>>
> >>>>>>>>>>> We actually have a quirk mechanism in the kernel for specifying
> >>>>>>>>>>> the need for: video=DSI-1:panel_orientation=right_side_up  and this
> >>>>>>>>>>> will also automatically fix the fbcon orientation, see:
> >>>>>>>>>>>
> >>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_panel_orientation_quirks.c
> >>>>>>>>>>>
> >>>>>>>>>>> If you submit a patch for this upstream please Cc me.
> >>>>>>>>>>
> >>>>>>>>>> And if after that change, and copy/pasting the orientation from the
> >>>>>>>>>> DSDT into hwdb the sensor and screen move in the expected ways, then
> >>>>>>>>>> maybe stealing the BMC150 driver's
> >>>>>>>>>> bmc150_apply_bosc0200_acpi_orientation() might be a good idea.
> >>>>>>>>>>
> >>>>>>>>>> Once exported through "mount_matrix", iio-sensor-proxy should see it
> >>>>>>>>>> and read it without the need for a hwdb entry.
> >>>>>>>>>>
> >>>>>>>>>> Cheers
> >>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>