Message-ID: <CAMxBKG3-f5+j+=5mXQccBkWfa3cwUTY=mSb8n88A1e0rXk8cnQ@mail.gmail.com>
Date:   Sun, 5 Feb 2023 16:06:10 +0000
From:   Darrell Kavanagh <darrell.kavanagh@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Bastien Nocera <hadess@...ess.net>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Bug#1029850: linux: Driver not loaded for ST Microelectronics
 LSM6DS3TR-C accelerometer (acpi:SMO8B30:SMO8B30:)

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.

Darrell

On Sun, 5 Feb 2023 at 14:22, Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sun, 5 Feb 2023 09:50:51 +0100
> Hans de Goede <hdegoede@...hat.com> 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@...hat.com> 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@...il.com> 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@...hat.com> 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@...ess.net> 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@...il.com> 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
> > >>>>>>
> > >>>>>
> > >>>
> > >>
> > >
> >
>

Powered by blists - more mailing lists