Message-ID: <CAO-hwJ+tc_nFYiGMNfnFkpGyB7P+R+fH2SnZntq7MQ1CAoSdpQ@mail.gmail.com>
Date:   Tue, 5 Jun 2018 00:14:38 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Henrik Rydberg <rydberg@...math.org>,
        Jiri Kosina <jikos@...nel.org>,
        Jason Gerecke <killertofu@...il.com>,
        Dennis Kempin <denniskempin@...gle.com>,
        Andrew de los Reyes <adlr@...gle.com>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for
 non-confident touches

On Mon, Jun 4, 2018 at 11:32 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
>> On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
>> <dmitry.torokhov@...il.com> wrote:
>> > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
>> >> Hi Dmitry,
>> >>
>> >> > > > Logically, the confidence state is a property of a contact, not a new type
>> >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
>> >> > > >
>> >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
>> >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
>> >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
>> >> > > > I can't find any other users in the current upstream tree, but those
>> >> > > > two are already making a precedent and changing the semantic is a
>> >> > > > little bit late :/
>> >> > I am sorry I did not respond and lost track of this issue back then, but
>> >> > I disagree with Henrik here. While confidence is a property of contact,
>> >> > so is the type of contact and it can and will change throughout life of
>> >> > a contact, especially if we will continue adding new types, such as, for
>> >> > example, thumb. In this case the firmware can transition through
>> >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
>> >> > contact becomes better understood. Still it is the same contact and we
>> >> > should not attempt to signal userspace differently.
>> >> We agree that the contact should stay the same, but the fear, and I think
>> >> somewhere along the blurry history of this thread, the problem was that
>> >> userspace interpreted the property change as a new contact (lift up/double
>> >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
>> >> Pen? It would be hard for an application to consider a switch from finger to
>> >> pen as the same contact, which is where the natural implementation starts to
>> >> diverge from the intention.
>> >
>> > I think the userspace has to trust our tracking ID to decide whether it
>> > is a same contact or not. The current issue is that kernel is forcing
>> > tracking ID change on tool type change, and one of the 2 patches that I
>> > posted fixed that, allowing us to keep the tracking ID for finger->palm
>> > transitions.
>>
>> I think I missed those 2 patches, can you point a LKML link?
>
> Sorry, I thought I sent it out with the patch we are talking about here,
> but I did not. See below. Note that it doe snot have any protections on
> finger->pen transitions and I am not sure any are needed at the moment.
> We can add them wither to MT core or to drivers if we see issues with
> devices.

For what it worth, after a quick reading of the patch, it would be:
Acked-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>

I can't remember exactly why we checked for the tool in the first place.

>
>> Also, note that libevdev discards the tracking ID change now (it
>> shouts at the user in the logs). So that means that it will now be
>> hard to force libevdev to trust the kernel again for the tracking ID.
>> The current rule is:
>> - tracking ID >= 0 -> new touch
>> - any subsequent tracking ID >= 0 -> discarded
>> - tracking ID == -1 -> end of touch
>
> Well, I guess it is like synaptics driver that used to dump core
> whenever it saw tracking ID change for the same slot (not going though
> -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> us having to produce weird results when users would use fancy gestures
> with 3+ fingers.
>
> It probably does not matter with devices with 5+ slots. We should pretty
> much always have free slot for new contact.

On Win 8 devices, we can safely expect the firmware to have a correct
maximum contact number information (there is no point reporting 5
fingers and saying you can support only 2). So if we try to add a new
slot and we are out of them, it's pretty much down to a firmware bug
or a touch we missed to release.

>
>>
>> >
>> > I think it is kernel task to not signal transitions that do not make
>> > sense, such as finger->pen or palm->pen etc.
>>
>> I fully agree, though there is currently no such guard in the kernel
>> (maybe it's part of your series). I am worried about the RMI4 F12
>> driver that automatically forward the info from the firmware, so if
>> the firmware does something crazy, it will be exported to user space.
>> But I guess it might be better to treat that on a per driver basis.
>
> Yeah, I think so.
>
>>
>> >
>> >>
>> >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
>> >> > complement ABS_MT_TOOL, but that would not really solve the issue with
>> >> > Wacom firmware (declaring contact non-confident and releasing it right
>> >> > away) and given MS explanation of the confidence as "contact is too big"
>> >> > MT_TOOL_PALM fits it perfectly.
>> >> Indeed, the Wacom firmware seems to need some special handling, which should
>> >> be fine by everyone. I do think it would make sense to add
>> >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
>>
>> Except we are already running out of ABS_* axes.
>
> Sorry, meants MT_TOOL_TO_BIG, not a new axis.
>
>>
>> >> also to a pen lying down on a touchpad, for instance.
>> >
>> > OK, I can see that for Pens, if we have firmware that would recognize
>> > such condition, it would be weird to report PALM. We could indeed have
>> > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
>> > the hardware can recognize it as such). Maybe we'd be better off just
>> > having userspace going by contact size for pens. Peter, any suggestions
>> > here?
>>
>> I don't think we have size handling in the tablet implementation in
>> libinput. I do not see it as a big issue to add such axes from a
>> libinput point of view. However, there is no existing hardware that
>> would provide such information, so I guess this will be a 'no' until
>> actual hardware comes in.
>>
>> Also note that the MT_TOOL_PEN implementation is limited (even
>> non-existant if I remember correctly). Peter and I do not have access
>> to any device that support such multi pen, so AFAICT, there is no code
>> to handle this in libinput.
>>
>> One last point from libinput, the pen device would need to be on its
>> separate kernel node for the protocol to be smoothly handled. So
>> basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
>> would not be handled properly right now. The Pen event will be treated
>> as a touch.
>
> I think normally pen and touch a separate controllers, so we have that
> going for us...

The problem is for devices like the N-trig DualSense or the Wacom AES
where only one sensor handles both pen and touch. For now, given that
MS says it supports only one pen we are on the safe side, but things
might gets scarier in the future :)

Cheers,
Benjamin

>
> Thanks.
>
> --
> Dmitry
>
>
> Input: do not assign new tracking ID when changing tool type
>
> From: Dmitry Torokhov <dmitry.torokhov@...il.com>
>
> We allow changing tool type (from MT_TOOL_FINGER to MT_TOOL_PALM) so we
> should not be forcing new tracking ID for the slot.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
>  drivers/input/input-mt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9cda8d4..7ca4b318ed419 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -151,7 +151,7 @@ void input_mt_report_slot_state(struct input_dev *dev,
>         }
>
>         id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
> -       if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
> +       if (id < 0)
>                 id = input_mt_new_trkid(mt);
>
>         input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);
>

Powered by blists - more mailing lists