Re: [PATCH] iio/hid: Add mount_matrix

[Hans de Goede <hdegoede@redhat.com>]

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 <gwendal@chromium.org> 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/sensors#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 <gwendal@chromium.org>
> 
> 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!

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 <srinivas.pandruvada@intel.com>");
>>  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)
>>  {
>