Re: [PATCH v3] HID: multitouch: Add quirks for flipped axes

From: Allen Ballway
Date: Tue Dec 13 2022 - 16:47:27 EST

Next message: kernel test robot: "Re: [PATCH] tracing/probe: add a char type to print the character value of traced arguments"
Previous message: Kuogee Hsieh: "[PATCH v12 5/5] drm/msm/dp: add support of max dp link rate"
In reply to: Allen Ballway: "Re: [PATCH v3] HID: multitouch: Add quirks for flipped axes"
Next in thread: Allen Ballway: "[PATCH v4] HID: multitouch: Add quirks for flipped axes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Dec 13, 2022 at 8:45 AM Allen Ballway <ballway@xxxxxxxxxxxx> wrote:
>
> On Tue, Dec 13, 2022 at 1:19 AM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> wrote:
> >
> > On Tue, Dec 13, 2022 at 2:01 AM Allen Ballway <ballway@xxxxxxxxxxxx> wrote:
> > >
> > > Certain touchscreen devices, such as the ELAN9034, are oriented
> > > incorrectly and report touches on opposite points on the X and Y axes.
> > > For example, a 100x200 screen touched at (10,20) would report (90, 180)
> > > and vice versa.
> > >
> > > This changed fixes the issue by adding device quirks to transform
> > > the touch points into the correct spaces, from X -> MAX(X) - X,
> > > and Y -> MAX(Y) - Y. These quirks are added in hid-quirks checking
> > > both DMI information and device vendor and product IDs. The quirk
> > > is handled in hid-multitouch to do the actual transformation.
> > >
> > > Signed-off-by: Allen Ballway <ballway@xxxxxxxxxxxx>
> > > ---
> > > V2 -> V3: Use existing HID_QUIRK_*_INVERT and match the quirk in
> > > hid-quirk, passing down to hid-multitouch through the hid device.
> > >
> > > V1 -> V2: Address review comments, change to use DMI match. Confirmed
> > > MT_TOOL_X/Y require transformation and update orientation based on
> > > flipped axes.
> > >
> > >
> > > drivers/hid/hid-multitouch.c | 43 ++++++++++++++++++++++++++++++++----
> > > drivers/hid/hid-quirks.c | 33 +++++++++++++++++++++++++++
> > > 2 files changed, 72 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > > index 91a4d3fc30e08..1f4c2aa511359 100644
> > > --- a/drivers/hid/hid-multitouch.c
> > > +++ b/drivers/hid/hid-multitouch.c
> > > @@ -71,6 +71,8 @@ MODULE_LICENSE("GPL");
> > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
> > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20)
> > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21)
> > > +#define MT_QUIRK_X_INVERT BIT(22)
> > > +#define MT_QUIRK_Y_INVERT BIT(23)
> >
> > Why duplicate the already available quirks in struct hid_device?
>
> This no longer seems necessary, and will be removed.
>
> >
> > >
> > > #define MT_INPUTMODE_TOUCHSCREEN 0x02
> > > #define MT_INPUTMODE_TOUCHPAD 0x03
> > > @@ -1086,6 +1088,10 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > > int orientation = wide;
> > > int max_azimuth;
> > > int azimuth;
> > > + int x;
> > > + int y;
> > > + int cx;
> > > + int cy;
> > >
> > > if (slot->a != DEFAULT_ZERO) {
> > > /*
> > > @@ -1104,6 +1110,16 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > > if (azimuth > max_azimuth * 2)
> > > azimuth -= max_azimuth * 4;
> > > orientation = -azimuth;
> > > +
> > > + /* Orientation is inverted if the X or Y axes are
> > > + * flipped, but normalized if both are inverted.
> > > + */
> > > + if (quirks & (MT_QUIRK_X_INVERT | MT_QUIRK_Y_INVERT) &&
> > > + !((quirks & MT_QUIRK_X_INVERT)
> > > + && (quirks & MT_QUIRK_Y_INVERT))) {
> >
> > This is done in every finger slot in every report. We should probably
> > cache that information somewhere.
>
> I can do this once in mt_probe and keep it saved as its own quirk.
>
> >
> > > + orientation = -orientation;
> > > + }
> > > +
> > > }
> > >
> > > if (quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
> > > @@ -1115,10 +1131,23 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
> > > minor = minor >> 1;
> > > }
> > >
> > > - input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
> > > - input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
> > > - input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
> > > - input_event(input, EV_ABS, ABS_MT_TOOL_Y, *slot->cy);
> > > + x = quirks & MT_QUIRK_X_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->x :
> > > + *slot->x;
> > > + y = quirks & MT_QUIRK_Y_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->y :
> > > + *slot->y;
> > > + cx = quirks & MT_QUIRK_X_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_X) - *slot->cx :
> > > + *slot->cx;
> > > + cy = quirks & MT_QUIRK_Y_INVERT ?
> > > + input_abs_get_max(input, ABS_MT_POSITION_Y) - *slot->cy :
> > > + *slot->cy;
> >
> > I can't help but think that there must be a better way of doing that.
> > If I didn't postpone HID-BPF to 6.3 I would have asked you to do this
> > as a BPF program.
> >
> > Still, the whole point of the slot pointers was to remove the data
> > copy everywhere, and you are adding it back. Not to mention that the
> > same tests and accesses to variables are called multiple times.
>
> I can move the transformation to mt_touch_input_mapping which has easier access
> to the quirks coming from hid_device rather than using duplicated quirks for
> mt_application/class.

It looks like I won't be able to make the change in
mt_touch_input_mapping because
the slot values I need to transform are pointers into the hid_report
coming through the device.
The other use of HID_QUIRK_*_INVERT is in hid-input.c and is
transforming the value
before calling input_event. I see some changes to the field values happening in
mt_need_to_apply_feature and doing a hid_hw_raw_request, but those
seem to happen
once with a fixed value and seems more expensive than just doing the
transform every time,
if that would even work for this.

>
> >
> > > +
> > > + input_event(input, EV_ABS, ABS_MT_POSITION_X, x);
> > > + input_event(input, EV_ABS, ABS_MT_POSITION_Y, y);
> > > + input_event(input, EV_ABS, ABS_MT_TOOL_X, cx);
> > > + input_event(input, EV_ABS, ABS_MT_TOOL_Y, cy);
> > > input_event(input, EV_ABS, ABS_MT_DISTANCE, !*slot->tip_state);
> > > input_event(input, EV_ABS, ABS_MT_ORIENTATION, orientation);
> > > input_event(input, EV_ABS, ABS_MT_PRESSURE, *slot->p);
> > > @@ -1735,6 +1764,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> > > td->serial_maybe = true;
> > >
> > > + if (hdev->quirks & HID_QUIRK_X_INVERT)
> > > + td->mtclass.quirks |= MT_QUIRK_X_INVERT;
> > > +
> > > + if (hdev->quirks & HID_QUIRK_Y_INVERT)
> > > + td->mtclass.quirks |= MT_QUIRK_Y_INVERT;
> >
> > As mentioned above, I don't see the point in doing that duplication of quirks.
>
> Will remove
>
> >
> > > +
> > > /* This allows the driver to correctly support devices
> > > * that emit events over several HID messages.
> > > */
> > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> >
> > AFAICT, Dmitry told you twice to use i2c-hid, not hid-quirks.c.
> >
> > We already have i2c-hid-dmi-quirks.c that you could extend. The
> > rationale is that this way, the dmi check will be done only for
> > internal peripherals, leaving aside the USB ones.
>
> I had added the quirks to hid_device to i2c-hid-dmi-quirks before
> trying this but they were getting stomped by the quirks coming from
> hid-quirks. I will move these back to i2c-hid-dmi-quirks and change
> hid-device to add them to whatever quirks come from hid-quirks.

I can get the quirks to come from i2c-hid-dmi-quirks.c, but the hid_device
quirks reset on probe so I will need to get them again in hid_lookup_quirk.
I can at least put it behind an if (bus == I2C) to keep the DMI checks to
a minimum but I don't see a way to get the quirk from i2c-hid-dmi-quirks
without going through hid-quirks.

>
> >
> > Cheers,
> > Benjamin
> >
> > > index 0e9702c7f7d6c..47c6cd62f019a 100644
> > > --- a/drivers/hid/hid-quirks.c
> > > +++ b/drivers/hid/hid-quirks.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/export.h>
> > > #include <linux/slab.h>
> > > #include <linux/mutex.h>
> > > +#include <linux/dmi.h>
> > > #include <linux/input/elan-i2c-ids.h>
> > >
> > > #include "hid-ids.h"
> > > @@ -957,6 +958,29 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
> > > { }
> > > };
> > >
> > > +static const struct hid_device_id elan_flipped_quirks[] = {
> > > + { HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, USB_VENDOR_ID_ELAN, 0x2dcd),
> > > + HID_QUIRK_X_INVERT | HID_QUIRK_Y_INVERT },
> > > + { }
> > > +};
> > > +
> > > +/*
> > > + * This list contains devices which have specific issues based on the system
> > > + * they're on and not just the device itself. The driver_data will have a
> > > + * specific hid device to match against.
> > > + */
> > > +static const struct dmi_system_id dmi_override_table[] = {
> > > + {
> > > + .ident = "DynaBook K50/FR",
> > > + .matches = {
> > > + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
> > > + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
> > > + },
> > > + .driver_data = (void *)elan_flipped_quirks,
> > > + },
> > > + { } /* Terminate list */
> > > +};
> > > +
> > > bool hid_ignore(struct hid_device *hdev)
> > > {
> > > int i;
> > > @@ -1238,6 +1262,7 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> > > {
> > > const struct hid_device_id *bl_entry;
> > > unsigned long quirks = 0;
> > > + const struct dmi_system_id *system_id;
> > >
> > > if (hid_match_id(hdev, hid_ignore_list))
> > > quirks |= HID_QUIRK_IGNORE;are
> > > @@ -1249,6 +1274,14 @@ static unsigned long hid_gets_squirk(const struct hid_device *hdev)
> > > if (bl_entry != NULL)
> > > quirks |= bl_entry->driver_data;
> > >
> > > + system_id = dmi_first_match(dmi_override_table);
> > > + if (system_id != NULL) {
> > > + bl_entry = hid_match_id(hdev, system_id->driver_data);
> > > + if (bl_entry != NULL)
> > > + quirks |= bl_entry->driver_data;
> > > + }
> > > +
> > > +
> > > if (quirks)
> > > dbg_hid("Found squirk 0x%lx for HID device 0x%04x:0x%04x\n",
> > > quirks, hdev->vendor, hdev->product);
> > > --
> > > 2.39.0.rc1.256.g54fd8350bd-goog
> > >
> >
>
> Thanks for the comments,
> Allen

Thanks,
Allen

Next message: kernel test robot: "Re: [PATCH] tracing/probe: add a char type to print the character value of traced arguments"
Previous message: Kuogee Hsieh: "[PATCH v12 5/5] drm/msm/dp: add support of max dp link rate"
In reply to: Allen Ballway: "Re: [PATCH v3] HID: multitouch: Add quirks for flipped axes"
Next in thread: Allen Ballway: "[PATCH v4] HID: multitouch: Add quirks for flipped axes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]