From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 233EFC43334 for ; Sat, 25 Jun 2022 18:27:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233119AbiFYS1D (ORCPT ); Sat, 25 Jun 2022 14:27:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49228 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231311AbiFYS1B (ORCPT ); Sat, 25 Jun 2022 14:27:01 -0400 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B342A2ADD for ; Sat, 25 Jun 2022 11:26:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1656181618; x=1687717618; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=Y72c1qj3wN/84BifN1x8qLetKh79sRyJzhwL3k9DcsE=; b=nUnyyno1cGDqX2IdGKV2cM3DOxSnhQytbByPxWhij6F39UXOZmZLJDHY 2Oh2Fju5MEPv2QZr2F13vzgpW7iXRff5hg/X0lyTt0bEEx8zBxzWLG/Kv Vi7vE7hlm4aItRecU5JNnu2nP4Uai3oNBfdlmmcbKbWDvW7l0DUli8gho G6U0+A2Rf9TPMY/FWHBBXTv+SQpOhAPcwRNImY75llWO2XLafhwHxnqJA hQ+obOdz4WpdrADIjiv5nnuAjByddNRzcIUalUpNU41yBjRTPKmMYXg+P lrnc4qc6bflEhajXGo6i6jTCGkd2vZsoOTN+ZFUHNbb2eLXBRpSRaovmM Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10389"; a="367526089" X-IronPort-AV: E=Sophos;i="5.92,222,1650956400"; d="scan'208";a="367526089" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jun 2022 11:26:58 -0700 X-IronPort-AV: E=Sophos;i="5.92,222,1650956400"; d="scan'208";a="766106815" Received: from aalleman-mobl2.amr.corp.intel.com (HELO spandruv-desk1.amr.corp.intel.com) ([10.209.81.165]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jun 2022 11:26:57 -0700 Message-ID: <534cde0d461b194e2fa9a91bd987da1cd2aae58a.camel@linux.intel.com> Subject: Re: [PATCH] iio/hid: Add mount_matrix From: srinivas pandruvada To: Hans de Goede , Jonathan Cameron , Gwendal Grignou Cc: jikos@kernel.org, wpsmith@google.com, linux-iio@vger.kernel.org, Bastien Nocera Date: Sat, 25 Jun 2022 11:26:57 -0700 In-Reply-To: <937c3317-91f7-9236-70a8-39ca4c2f6396@redhat.com> References: <20220624223341.2625231-1-gwendal@chromium.org> <20220625120937.24c51ca4@jic23-huawei> <937c3317-91f7-9236-70a8-39ca4c2f6396@redhat.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Sat, 2022-06-25 at 14:33 +0200, Hans de Goede wrote: > Hi, > > Jonathan, thanks for Cc-ing me on this. > > On 6/25/22 13:09, Jonathan Cameron wrote: > > On Fri, 24 Jun 2022 15:33:41 -0700 > > Gwendal Grignou wrote: > > > > > ISH based sensors do not naturally return data in the W3C > > > 'natural' > > > orientation. > > > They returns all data inverted, to match Microsoft Windows > > > requirement: > > > [ > > > https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/senso > > > rs#accelerometer] > > > """ If the device has the SimpleOrientation of FaceUp on a table, > > > then > > > the accelerometer would read -1 G on the Z axis. """ > > > > Probably reference the HID Usage Tables 1.3 spec rather than the MS > > one. > > https://usb.org/sites/default/files/hut1_3_0.pdf > > After some waving around of my left and right hand I'm fairly sure > > that says the same > > thing as the MS spec. Section 4.4 Vector Usages > > > > > While W3C defines > > > [https://www.w3.org/TR/motion-sensors/#accelerometer-sensor] > > > """The Accelerometer sensor is an inertial-frame sensor, this > > > means that > > > when the device is in free fall, the acceleration is 0 m/s2 in > > > the > > > falling direction, and when a device is laying flat on a table, > > > the > > > acceleration in upwards direction will be equal to the Earth > > > gravity, > > > i.e. g ≡ 9.8 m/s2 as it is measuring the force of the table > > > pushing the > > > device upwards.""" > > > > > > Fixes all HID sensors that defines IIO_MOD_[XYZ] attributes. > > > > > > Tested on "HP Spectre x360 Convertible 13" and "Dell XPS 13 > > > 9365". > > > > > > Signed-off-by: Gwendal Grignou > > > > Ah.  Whilst this is a fix, it seems likely to break a whole bunch > > of existing > > users that are compensating for the wrong orientation in > > userspace.  Also, do > > we know how universal this is?  I have a nasty feeling it'll turn > > out some > > HID sensors do it the other way whatever the spec says.  Bastien, > > are you > > carrying a local fix for this in your userspace code? > > > > +CC a few people who are likely to know more on just how bad that > > will be... > > Right, so Linux userspace expects an axis system similar to the > Android one, > which is actually the one which seems to be described here. > > The axis system expect is that when a tablet is layed flat on the > table, > the x and y axis are as one would expect when drawing a mathematics > graph on the surface of the tablet. > > So X-axis goes from left to right, with left side being lower numbers > and > right side higher numbers. > > And Y-axis goes from bottom to top, with the bottom being lower > numbers and > the top higher numbers. > > That leaves the Z-axis which comes out of the screen at a 90° angle > (to both > the X and Y axis) and the vector coming out of the screen towards to > the user / > observer of the screen indicates positive numbers where as imagining > the same > axis pointing down through the table on which the tables is lying > towards > the floor represents negative numbers. > > This means that the accel values of a tablet resting on a table, > expresses > in units of 1G are: [ 0, 0, -1 ] and I've seen quite a few HID > sensors > with accel reporting on various devices and they all adhere to this > without applying any accel matrix. Or in other words, HID sensors > behave > as expected by userspace when applying the norm matrix of: > >         .rotation = { >                 "1", "0", "0", >                 "0", "1", "0", >                 "0", "0", "1" > > And this patch will cause the image to be upside down (no matter what > the > rotation) when using auto-rotation with iio-sensor-proxy. > > So big NACK from me for this patch. > > I'm not sure what this patch is trying to fix but it looks to me like > it > is a bug in the HID sensors implementation of the specific device. > > Again HID-sensors already work fine on tons of existing devices > without > any matrix getting applied. > > Merging this patch would break existing userspace on tons of devices! > Yes, it will. We have devices from 5+ years with this feature. So not practical to test every device. Thanks, Srinivas > Regards, > > Hans > > > > > > > > > One other thing inline.  The mount matrix you've provided isn't a > > rotation matrix. > > > > I'd forgotten the annoyance of graphics folks using the right > > handed sensor > > axis whilst nearly all other uses are left handed. It drove me mad > > many years > > ago - every code base that used sensors and rendered the result > > needed a > > flip of the z axis - it was never well documented, so half the time > > the code ended up with many axis flips based on people debugging > > local > > orientation problems.  *sigh* > > > > > > > --- > > >  drivers/iio/accel/hid-sensor-accel-3d.c       |  3 +++ > > >  .../hid-sensors/hid-sensor-attributes.c       | 21 > > > +++++++++++++++++++ > > >  drivers/iio/gyro/hid-sensor-gyro-3d.c         |  3 +++ > > >  drivers/iio/magnetometer/hid-sensor-magn-3d.c |  3 +++ > > >  include/linux/hid-sensor-hub.h                |  2 ++ > > >  5 files changed, 32 insertions(+) > > > > > > diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c > > > b/drivers/iio/accel/hid-sensor-accel-3d.c > > > index a2def6f9380a3..980bbd7fba502 100644 > > > --- a/drivers/iio/accel/hid-sensor-accel-3d.c > > > +++ b/drivers/iio/accel/hid-sensor-accel-3d.c > > > @@ -59,6 +59,7 @@ static const struct iio_chan_spec > > > accel_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > >                 .scan_index = CHANNEL_SCAN_INDEX_X, > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_ACCEL, > > >                 .modified = 1, > > > @@ -69,6 +70,7 @@ static const struct iio_chan_spec > > > accel_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > >                 .scan_index = CHANNEL_SCAN_INDEX_Y, > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_ACCEL, > > >                 .modified = 1, > > > @@ -79,6 +81,7 @@ static const struct iio_chan_spec > > > accel_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > >                 .scan_index = CHANNEL_SCAN_INDEX_Z, > > > +               .ext_info = hid_sensor_ext_info, > > >         }, > > >         IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP) > > >  }; > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor- > > > attributes.c b/drivers/iio/common/hid-sensors/hid-sensor- > > > attributes.c > > > index 9b279937a24e0..e367e4b482ef0 100644 > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c > > > @@ -585,6 +585,27 @@ int > > > hid_sensor_parse_common_attributes(struct hid_sensor_hub_device > > > *hsdev, > > >  } > > >  EXPORT_SYMBOL_NS(hid_sensor_parse_common_attributes, IIO_HID); > > >   > > > +static const struct iio_mount_matrix hid_sensor_windows_axis = { > > > +       .rotation = { > > > +               "-1", "0", "0", > > > +               "0", "-1", "0", > > > +               "0", "0", "-1" > > > > Unless my memory of rotation matrices serves me wrong, that's not a > > rotation matrix. > > (det(R) != 1) > > > > That's a an axis flip from a right handed set of axis to a left > > handed one. > > So to fix this up, you would need to invert the raw readings of at > > least one axis > > rather than rely on the mount matrix or make the scale negative. > > > > Jonathan > > > > > > > +       } > > > +}; > > > + > > > +static const struct iio_mount_matrix * > > > +hid_sensor_get_mount_matrix(const struct iio_dev *indio_dev, > > > +                               const struct iio_chan_spec *chan) > > > +{ > > > +       return &hid_sensor_windows_axis; > > > +} > > > + > > > +const struct iio_chan_spec_ext_info hid_sensor_ext_info[] = { > > > +       IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, > > > hid_sensor_get_mount_matrix), > > > +       { } > > > +}; > > > +EXPORT_SYMBOL(hid_sensor_ext_info); > > > + > > >  MODULE_AUTHOR("Srinivas Pandruvada > > > "); > > >  MODULE_DESCRIPTION("HID Sensor common attribute processing"); > > >  MODULE_LICENSE("GPL"); > > > diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > b/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > index 8f0ad022c7f1b..b852f5166bb21 100644 > > > --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > @@ -58,6 +58,7 @@ static const struct iio_chan_spec > > > gyro_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > >                 .scan_index = CHANNEL_SCAN_INDEX_X, > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_ANGL_VEL, > > >                 .modified = 1, > > > @@ -68,6 +69,7 @@ static const struct iio_chan_spec > > > gyro_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > >                 .scan_index = CHANNEL_SCAN_INDEX_Y, > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_ANGL_VEL, > > >                 .modified = 1, > > > @@ -78,6 +80,7 @@ static const struct iio_chan_spec > > > gyro_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > >                 .scan_index = CHANNEL_SCAN_INDEX_Z, > > > +               .ext_info = hid_sensor_ext_info, > > >         }, > > >         IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP) > > >  }; > > > diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > b/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > index e85a3a8eea908..aefbdb9b0869a 100644 > > > --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > @@ -74,6 +74,7 @@ static const struct iio_chan_spec > > > magn_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SCALE) | > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_MAGN, > > >                 .modified = 1, > > > @@ -83,6 +84,7 @@ static const struct iio_chan_spec > > > magn_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SCALE) | > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_MAGN, > > >                 .modified = 1, > > > @@ -92,6 +94,7 @@ static const struct iio_chan_spec > > > magn_3d_channels[] = { > > >                 BIT(IIO_CHAN_INFO_SCALE) | > > >                 BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > >                 BIT(IIO_CHAN_INFO_HYSTERESIS), > > > +               .ext_info = hid_sensor_ext_info, > > >         }, { > > >                 .type = IIO_ROT, > > >                 .modified = 1, > > > diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid- > > > sensor-hub.h > > > index c27329e2a5ad5..ee7d5b430a785 100644 > > > --- a/include/linux/hid-sensor-hub.h > > > +++ b/include/linux/hid-sensor-hub.h > > > @@ -236,6 +236,8 @@ struct hid_sensor_common { > > >         struct work_struct work; > > >  }; > > >   > > > +extern const struct iio_chan_spec_ext_info > > > hid_sensor_ext_info[]; > > > + > > >  /* Convert from hid unit expo to regular exponent */ > > >  static inline int hid_sensor_convert_exponent(int unit_expo) > > >  { > > >