Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Tue, Jun 05, 2018 at 09:51:14AM +1000, Peter Hutterer wrote:
> On Mon, Jun 04, 2018 at 04:28:01PM -0700, Dmitry Torokhov wrote:
> > On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> > > On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov 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@xxxxxxxxx> 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.
> > > > 
> > > > > 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.
> > > 
> > > yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> > > never M to N. This assumption made its way into libevdev (where the tracking
> > > ID is transparently discarded, albeit with a warning). There are libevdev
> > > patches to get rid of that but whatever device needed it got fixed in some
> > > other way, so the patch didn't get pushed.
> > > 
> > > fwiw, dump core was just "print the backtrace to the log" here, there was no
> > > actual core dump.
> > 
> > Hmm, I do not recall what version I was playing with, but I tried
> > changing Synaptics kernel driver to not insert the fake -1 tracking ID
> > for a slot when rolling 3 fingers on a 2-slot device (so removing finger
> > 1 while holding finger 2 and adding finger 3 does not appear to
> > userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
> > and xf86-input-synaptics-1.7.8 would scream about too many slots and
> > stop working.
> > 
> > That was a while ago though, before libinput I think.
> > 
> > > 
> > > > It probably does not matter with devices with 5+ slots. We should pretty
> > > > much always have free slot for new contact.
> > > > 
> > > > > 
> > > > > >
> > > > > > 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.
> > > 
> > > bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> > > pen that's lying on its side doesn't have a size but should still be
> > > ignored.
> > 
> > OK, when we start seeing this for non finger/thumb/palm objects we can
> > add this tool type. For current devices we are dealing with palms.
> > 
> > > 
> > > > > 
> > > > > >> 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.
> > > 
> > > correct on all counts :)
> > > 
> > > 
> > > > > 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.
> > > 
> > > Yep, correct. On this note: libinput very much follows a "no hardware, no
> > > implementation" rule. I played the game of trying to support everything in a
> > > generic manner with the X drivers and it's a nightmare to maintain. libinput
> > > instead takes a use case and tries to make it sensible - but for that to
> > > work we need to know both the hardware and the use-cases. That's why tablet
> > > handling coming out of libinput is very different to the handling we have in
> > > X but, afaict, everyone is better off for it so far.
> > > 
> > > This means that if you give me a MT_TOOL_FINGER â MT_TOOL_PEN transition,
> > > I'll handle it, but only after you also give me the use-case for it and the
> > > promise of real devices that need it. 
> > > 
> > > > > 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...
> > > 
> > > Side-effect of this is: the tablet interface doesn't handle touch at all
> > > because it didn't need to yet. So while technically possible, it requires a
> > > fair bit of re-arranging.
> > 
> > What about things like Bamboo touch? It is a Wacom tablet with both
> > multitouch finger and stylus.
> 
> these are on two different event nodes though, isn't it? If not, then no-one
> has tested them with libinput so far...

I'll have to plug it in and see... It was a while since I used it.

Thanks.

-- 
Dmitry