Re: [PATCH 10/12] HID: input: remove the need for HID_QUIRK_INVERT

[Benjamin Tissoires]

Hi Ping,

On Wed, Feb 2, 2022 at 6:43 AM Ping Cheng <pinglinux@xxxxxxxxx> wrote:
>
> Hi Benjamin,
>
> Thank you for taking care of the issue. The whole set looks good to me, except this one. xf86-input-wacom would not process the events properly if pen and eraser events are reported through the same EV_SYN frame. That's mainly because the tool out of prox will reset all values, while some values the next tool may rely on. Please see my detailed comments inline.
>
> On Wed, Jan 26, 2022 at 8:19 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote:
>>
>> HID_QUIRK_INVERT is kind of complex to deal with and was bogus.
>>
>> Furthermore, it didn't make sense to use a global per struct hid_device
>> quirk for something dynamic as the current state.
>>
>> Store the current tool information in the report itself, and re-order
>> the processing of the fields to enforce having all the tablet "state"
>> fields before getting to In Range and other input fields.
>>
>> This way, we now have all the information whether a tool is present
>> or not while processing In Range.
>>
>> This new behavior enforces that only one tool gets forwarded to userspace
>> at the same time, and that if either eraser or invert is set, we enforce
>>
>> BTN_TOOL_RUBBER.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
>> ---
>>  drivers/hid/hid-input.c | 66 +++++++++++++++++++++++++++++++++++------
>>  include/linux/hid.h     |  6 +++-
>>  2 files changed, 62 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 61d91117f4ae..2d13d3ad9d3c 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -63,8 +63,11 @@ static const struct {
>>   * This still leaves us 65535 individual priority values.
>>   */
>>  static const __u32 hidinput_usages_priorities[] = {
>> +       HID_DG_ERASER,          /* Eraser (eraser touching) must always come before tipswitch */
>>         HID_DG_INVERT,          /* Invert must always come before In Range */
>> -       HID_DG_INRANGE,
>> +       HID_DG_TIPSWITCH,       /* Is the tip of the tool touching? */
>> +       HID_DG_TIPPRESSURE,     /* Tip Pressure might emulate tip switch */
>> +       HID_DG_INRANGE,         /* In Range needs to come after the other tool states */
>>  };
>>
>>  #define map_abs(c)     hid_map_usage(hidinput, usage, &bit, &max, EV_ABS, (c))
>> @@ -1368,6 +1371,7 @@ static void hidinput_handle_scroll(struct hid_usage *usage,
>>  void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value)
>>  {
>>         struct input_dev *input;
>> +       struct hid_report *report = field->report;
>>         unsigned *quirks = &hid->quirks;
>>
>>         if (!usage->type)
>> @@ -1418,25 +1422,69 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>         }
>>
>>         switch (usage->hid) {
>> +       case HID_DG_ERASER:
>> +               report->tool_active |= !!value;
>> +
>> +               /*
>> +                * if eraser is set, we must enforce BTN_TOOL_RUBBER
>> +                * to accommodate for devices not following the spec.
>> +                */
>> +               if (value)
>> +                       report->tool = BTN_TOOL_RUBBER;
>> +
>> +               /* let hid-input set BTN_TOUCH */
>> +               break;
>> +
>>         case HID_DG_INVERT:
>> -               *quirks = value ? (*quirks | HID_QUIRK_INVERT) : (*quirks & ~HID_QUIRK_INVERT);
>> +               report->tool_active |= !!value;
>> +
>> +               /*
>> +                * If invert is set, we store BTN_TOOL_RUBBER.
>> +                */
>> +               if (value)
>> +                       report->tool = BTN_TOOL_RUBBER;
>> +
>> +               /* no further processing */
>>                 return;
>>
>>         case HID_DG_INRANGE:
>> -               if (value) {
>> -                       input_event(input, usage->type, (*quirks & HID_QUIRK_INVERT) ? BTN_TOOL_RUBBER : usage->code, 1);
>> -                       return;
>> -               }
>> -               input_event(input, usage->type, usage->code, 0);
>> -               input_event(input, usage->type, BTN_TOOL_RUBBER, 0);
>> +               report->tool_active |= !!value;
>> +
>> +               /*
>> +                * If the tool is in used (any of TipSwitch, Erase, Invert,
>> +                * InRange), and if tool is not set, store our mapping
>> +                */
>> +               if (report->tool_active && !report->tool)
>> +                       report->tool = usage->code;
>> +
>> +               input_event(input, EV_KEY, usage->code, report->tool == usage->code);
>> +               input_event(input, EV_KEY, BTN_TOOL_RUBBER, report->tool == BTN_TOOL_RUBBER);
>
>
> When usage->code changes value, for userspace clients, such as X driver, one of the tools in the frame would not have its own input events since there is only one set of input event values in the frame. X driver stores the values in different arrays/channels for each tool to post the events for each tool after all the values are processed.
>
> If we can always post the tool that goes out of prox with an EV_SYN first, the other tool would take the coming input events without problem. The tool that goes out of prox doesn't care about any of the other input events since its value will be reset.

OK. Right now, this series ensures we get
ERASER/TIPSWITCH/INVERT/INRANGE first before processing any other
events.

Given that ERASER/TIPSWITCH are responsible for BTN_TOUCH, I'd like to
get some clarifications (sorry dumping my brain here):

In the example of the pen moving from erasing to touching without the
intent to erase:

The HID event sequence would be:
Eraser: 1, Invert: 1, Tipswitch: 0, InRange: 1, X, Y, etc...
then
Eraser: 0, Invert: 0, Tipswitch: 1, InRange: 1, X, Y, etc...

In this current series we would get:
BTN_TOUCH 1, BTN_TOOL_RUBBER 1, X, Y, etc...
then
BTN_TOUCH 0, BTN_TOUCH 1, BTN_TOOL_PEN 1, BTN_TOOL_RUBBER 0, X, Y, etc...

(yeah, oops, there are 2 BTN_TOUCH in this sequence)

A naive change would do (instead of the last evdev event):
BTN_TOUCH 0, BTN_TOUCH 1, BTN_TOOL_RUBBER 0, *EV_SYN*, BTN_TOOL_PEN 1,
X, Y, etc...

However, I wonder if we should not have instead:
BTN_TOUCH 0, BTN_TOOL_RUBBER 0, *EV_SYN*, BTN_TOUCH 1, BTN_TOOL_PEN 1,
X, Y, etc...

That change should be easy to handle actually.

I am a little bit more concerned the other way around:
touching without the intent to erase to erase:
HID events:
Eraser: 0, Invert: 0, Tipswitch: 1, InRange: 1, X, Y, etc...
Eraser: 1, Invert: 1, Tipswitch: 0, InRange: 1, X, Y, etc...

The current series gives:
BTN_TOUCH 1, BTN_TOOL_PEN 1, X, Y, etc...
then
BTN_TOOL_PEN 1, BTN_TOOL_RUBBER 1, X, Y, etc...

(no BTN_TOUCH events)

And I think to get to the "correct" sequence I would have to store more states:
BTN_TOUCH 0, BTN_TOOL_PEN 0, *EV_SYN*, BTN_TOUCH 1, BTN_TOOL_RUBBER 1,
X, Y, etc...

I think the main problem is how to handle BTN_TOUCH correctly (and
PRESSURE is also in the middle). When this event is not set, it should
be easier to add the EV_SYN between the BTN_TOOL_*.

>
> We could update X driver, if libinput is capable of processing two tools in the same EV_SYN frame. This is more like a special multi-pen situation where two styli share the same set of input event values. The regular multi-pen logic would not process it right, I think.

Well, there is already the problem of that BTN_TOUCH 0/1 in one frame
that is not very clean. And if we can not break userspace that would
be best (note that the current kernel is completely sending random
events for those transitions, so in a way it's still slightly better).

Cheers,
Benjamin

>
> Cheers,
> Ping
>
>> +
>> +               /* reset tool and tool_active for the next event */
>> +               report->tool = 0;
>> +               report->tool_active = false;
>> +
>> +               /* no further processing */
>>                 return;
>>
>> +       case HID_DG_TIPSWITCH:
>> +               report->tool_active |= !!value;
>> +
>> +               /* if tool is set we should ignore the current value */
>> +               if (report->tool)
>> +                       return;
>> +
>> +               break;
>> +
>>         case HID_DG_TIPPRESSURE:
>>                 if (*quirks & HID_QUIRK_NOTOUCH) {
>>                         int a = field->logical_minimum;
>>                         int b = field->logical_maximum;
>>
>> -                       input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
>> +                       if (value > a + ((b - a) >> 3)) {
>> +                               input_event(input, EV_KEY, BTN_TOUCH, 1);
>> +                               report->tool_active = true;
>> +                       }
>>                 }
>>                 break;
>>
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index eaad0655b05c..feb8df61168f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -347,7 +347,7 @@ struct hid_item {
>>   */
>>  #define MAX_USBHID_BOOT_QUIRKS 4
>>
>> -#define HID_QUIRK_INVERT                       BIT(0)
>> +/* BIT(0) reserved for backward compatibility, was HID_QUIRK_INVERT */
>>  #define HID_QUIRK_NOTOUCH                      BIT(1)
>>  #define HID_QUIRK_IGNORE                       BIT(2)
>>  #define HID_QUIRK_NOGET                                BIT(3)
>> @@ -515,6 +515,10 @@ struct hid_report {
>>         unsigned maxfield;                              /* maximum valid field index */
>>         unsigned size;                                  /* size of the report (bits) */
>>         struct hid_device *device;                      /* associated device */
>> +
>> +       /* tool related state */
>> +       bool tool_active;                               /* whether the current tool is active */
>> +       unsigned int tool;                              /* BTN_TOOL_* */
>>  };
>>
>>  #define HID_MAX_IDS 256
>> --
>> 2.33.1
>>