| Message ID | 1429717509-27396-3-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 22, 2015 at 11:45:09AM -0400, Benjamin Tissoires wrote: > Synaptics PS/2 touchpad can send only 2 touches in a report. They can > detect 4 or 5 and this information is valuable. > > In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep > stability in image sensors"), we allocate 3 slots, but we still continue > to report the 2 available fingers. That means that the client sees 2 used > slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. > > For old kernels this is not a problem because max_slots was 2 and libinput/ > xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > information, and goes wild. > > We can pin the 3 slots until we get a total number of fingers below 2. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 Benjamin, I do not quite like it. It seems that original patch was not quite right and we are adding more workarounds. Synaptics can only track 2 contacts, correct? Why 2 slots to track them is not enough? > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > drivers/input/mouse/synaptics.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > index 630af73..c69b308 100644 > --- a/drivers/input/mouse/synaptics.c > +++ b/drivers/input/mouse/synaptics.c > @@ -935,6 +935,14 @@ static void synaptics_report_mt_data(struct psmouse *psmouse, > input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z); > } > > + /* keep (slot count <= num_fingers) by pinning all slots */ > + if (num_fingers >= 3) { > + for (i = 0; i < 3; i++) { > + input_mt_slot(dev, i); > + input_mt_report_slot_state(dev, MT_TOOL_FINGER, true); > + } > + } > + > input_mt_drop_unused(dev); > > /* Don't use active slot count to generate BTN_TOOL events. */ > -- > 2.1.0 >
On Apr 23 2015 or thereabouts, Dmitry Torokhov wrote: > On Wed, Apr 22, 2015 at 11:45:09AM -0400, Benjamin Tissoires wrote: > > Synaptics PS/2 touchpad can send only 2 touches in a report. They can > > detect 4 or 5 and this information is valuable. > > > > In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep > > stability in image sensors"), we allocate 3 slots, but we still continue > > to report the 2 available fingers. That means that the client sees 2 used > > slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. > > > > For old kernels this is not a problem because max_slots was 2 and libinput/ > > xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > > clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > > It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > > information, and goes wild. > > > > We can pin the 3 slots until we get a total number of fingers below 2. > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 > > Benjamin, I do not quite like it. It seems that original patch was not > quite right and we are adding more workarounds. Agree. And I am starting to hate more and more the synaptics PS/2 and all the PS/2 drivers to be honest :) - trying to fit a heavy load data like multitouch in a simple and lightweight protocol like PS/2 is insane... We are internally trying to figure out if we can finally take advantage of the SMBus/RMI4 protocol, but we tried for one year without much success. > > Synaptics can only track 2 contacts, correct? Why 2 slots to track them > is not enough? IIRC, the problem was that upon a third finger down, with only 2 slots, the fingers were silently inverted in most cases. The thing is that the firmware forwards 2 fingers, but not necessarily the two first. So you generally get fingers 1+3 so the slot 2 needs to be removed. And that means the kernel tracking has to track 3 fingers upon transitions. This may be completely bullshit and we might not need to use 3 slots at all. I'll need to do further experiments to validate which one is best then. I am perfectly fine holding this one up for a little bit more testings and then we can decide which one needs to be done (revert or an other band-aid). Cheers, Benjamin > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > drivers/input/mouse/synaptics.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > > index 630af73..c69b308 100644 > > --- a/drivers/input/mouse/synaptics.c > > +++ b/drivers/input/mouse/synaptics.c > > @@ -935,6 +935,14 @@ static void synaptics_report_mt_data(struct psmouse *psmouse, > > input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z); > > } > > > > + /* keep (slot count <= num_fingers) by pinning all slots */ > > + if (num_fingers >= 3) { > > + for (i = 0; i < 3; i++) { > > + input_mt_slot(dev, i); > > + input_mt_report_slot_state(dev, MT_TOOL_FINGER, true); > > + } > > + } > > + > > input_mt_drop_unused(dev); > > > > /* Don't use active slot count to generate BTN_TOOL events. */ > > -- > > 2.1.0 > > > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dmitry, [ adding more relevant people to the discussion ] On Apr 23 2015 or thereabouts, Benjamin Tissoires wrote: > On Apr 23 2015 or thereabouts, Dmitry Torokhov wrote: > > On Wed, Apr 22, 2015 at 11:45:09AM -0400, Benjamin Tissoires wrote: > > > Synaptics PS/2 touchpad can send only 2 touches in a report. They can > > > detect 4 or 5 and this information is valuable. > > > > > > In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep > > > stability in image sensors"), we allocate 3 slots, but we still continue > > > to report the 2 available fingers. That means that the client sees 2 used > > > slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. > > > > > > For old kernels this is not a problem because max_slots was 2 and libinput/ > > > xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > > > clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > > > It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > > > information, and goes wild. > > > > > > We can pin the 3 slots until we get a total number of fingers below 2. > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 > > > > Benjamin, I do not quite like it. It seems that original patch was not > > quite right and we are adding more workarounds. > > Agree. And I am starting to hate more and more the synaptics PS/2 and all > the PS/2 drivers to be honest :) - trying to fit a heavy load data like > multitouch in a simple and lightweight protocol like PS/2 is insane... > > We are internally trying to figure out if we can finally take advantage > of the SMBus/RMI4 protocol, but we tried for one year without much > success. > > > > > Synaptics can only track 2 contacts, correct? Why 2 slots to track them > > is not enough? > > IIRC, the problem was that upon a third finger down, with only 2 slots, > the fingers were silently inverted in most cases. The thing is that the > firmware forwards 2 fingers, but not necessarily the two first. So you > generally get fingers 1+3 so the slot 2 needs to be removed. And that > means the kernel tracking has to track 3 fingers upon transitions. > > This may be completely bullshit and we might not need to use 3 slots at > all. I'll need to do further experiments to validate which one is best > then. > > I am perfectly fine holding this one up for a little bit more testings > and then we can decide which one needs to be done (revert or an other > band-aid). > So I carefully recorded each situation (initial with 2 slots, 2 slots and then with the pinning in this patch*), and I am now convinced that the pinning is the best sequence that we forward to the user space (best among the 3). With 2 slots declared, there are 2 problems: - the first finger jumps to the position of the 3rd when it lands - the transition between 2 to 3 fingers goes to a state where the kernel removes the second finger (while jumping the first to the position of the 3rd finger), send a sync and then reallocate the first finger position as the second slot in use -> that means that user space sees a small transition where the slots count is 1 while the BTN_TOOL advertise triple tap :/ With 3 slots, we have the problem reported in the rhbz bug #1212230: - during the transition, the fingers are stable, but we have at most 2 active slots in one frame, which confuses libinput/xorg-synaptics. With the pinning, the user space is no more confused because BTN_TOOL is always greater or equal than the active slots. So I think for now we have 3 possibilities: 1. Just carry this patch, and hope that we will be able to switch the synaptics device in the non-PS/2 mode 2. Revert to 2 patches and fix the kernel tracking to accept 3 fingers and return the 2 best matches 3. Revert the use of the kernel tracking at all and re-introduce the spaghetti code that was here before and hope that all cases where properly handled. IMO that the solution 2. is the best, but I can not do it because I don't understand what the code does. I can guess things but I can not accurately change it because it is not readable IMO. (yes, there is also the solution 4: "screw up and let the user space deal with it", but I'd rather not do that given the history of the multitouch protocol) Cheers, Benjamin * I tried to put side by side the 3 test cases in the following logs: (init slots 2, no pinning) | (init slots 3, no pinning) | (init slots 3, pinning) ----------------------------- | ----------------------------- | --------------------------- ------------------------------|----- one finger down -------|---------------------------- | | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 ABS_MT_TRACKING_ID 53 | ABS_MT_TRACKING_ID 53 | ABS_MT_TRACKING_ID 53 ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 ABS_MT_PRESSURE 68 | ABS_MT_PRESSURE 68 | ABS_MT_PRESSURE 68 BTN_TOUCH 1 | BTN_TOUCH 1 | BTN_TOUCH 1 ABS_X 2000 | ABS_X 2000 | ABS_X 2000 ABS_Y 2000 | ABS_Y 2000 | ABS_Y 2000 ABS_PRESSURE 68 | ABS_PRESSURE 68 | ABS_PRESSURE 68 BTN_TOOL_FINGER 1 | BTN_TOOL_FINGER 1 | BTN_TOOL_FINGER 1 --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | | ... | ... | ... | | ------------------------------------ second finger down ------------------------------------ | | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 ABS_MT_SLOT 1 | ABS_MT_SLOT 1 | ABS_MT_SLOT 1 ABS_MT_TRACKING_ID 54 | ABS_MT_TRACKING_ID 54 | ABS_MT_TRACKING_ID 54 ABS_MT_POSITION_X 3000 | ABS_MT_POSITION_X 3000 | ABS_MT_POSITION_X 3000 ABS_MT_POSITION_Y 3000 | ABS_MT_POSITION_Y 3000 | ABS_MT_POSITION_Y 3000 ABS_MT_PRESSURE 64 | ABS_MT_PRESSURE 64 | ABS_MT_PRESSURE 64 ABS_X 2000 | ABS_X 2000 | ABS_X 2000 ABS_Y 2000 | ABS_Y 2000 | ABS_Y 2000 ABS_PRESSURE 78 | ABS_PRESSURE 78 | ABS_PRESSURE 78 BTN_TOOL_FINGER 0 | BTN_TOOL_FINGER 0 | BTN_TOOL_FINGER 0 BTN_TOOL_DOUBLETAP 1 | BTN_TOOL_DOUBLETAP 1 | BTN_TOOL_DOUBLETAP 1 --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | | ... | ... | ... | | ------------------------------------ third finger down ------------------------------------ | | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- ABS_MT_SLOT 0 | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 ABS_MT_POSITION_X 4000 | ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 ABS_MT_POSITION_Y 4000 | ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 ABS_MT_SLOT 1 | ABS_MT_SLOT 2 | ABS_MT_SLOT 2 ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID 55 | ABS_MT_TRACKING_ID 55 | ABS_MT_POSITION_X 4000 | ABS_MT_POSITION_X 4000 | ABS_MT_POSITION_Y 4000 | ABS_MT_POSITION_Y 4000 | ABS_MT_PRESSURE 72 | ABS_MT_PRESSURE 72 | ABS_MT_SLOT 1 | | ABS_MT_TRACKING_ID -1 | ABS_X 4000 | ABS_X 2000 | ABS_X 2000 ABS_Y 4000 | ABS_Y 2000 | ABS_Y 2000 ABS_PRESSURE 78 | ABS_PRESSURE 78 | ABS_PRESSURE 78 BTN_TOOL_DOUBLETAP 0 | BTN_TOOL_DOUBLETAP 0 | BTN_TOOL_DOUBLETAP 0 BTN_TOOL_TRIPLETAP 1 | BTN_TOOL_TRIPLETAP 1 | BTN_TOOL_TRIPLETAP 1 --- SYN_REPORT (0) ---------- | | ABS_MT_SLOT 0 | | ABS_MT_POSITION_X 4000 | | ABS_MT_POSITION_Y 4000 | | ABS_MT_PRESSURE 78 | | ABS_MT_SLOT 1 | | ABS_MT_TRACKING_ID 55 | | ABS_MT_POSITION_X 2000 | | ABS_MT_POSITION_Y 2000 | | ABS_MT_PRESSURE 72 | | ABS_X 4000 | | ABS_Y 4000 | | ABS_PRESSURE 78 | | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | | ... | ... | ... | | ------------------------------------ 3 fingers release ------------------------------------ | | | | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | | ABS_MT_SLOT 2 | | ABS_MT_POSITION_X 4000 | | ABS_MT_POSITION_Y 4000 | | ABS_MT_PRESSURE 45 ABS_MT_SLOT 0 | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 ABS_MT_SLOT 1 | ABS_MT_SLOT 2 | ABS_MT_SLOT 1 ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 | | ABS_X 4000 | | ABS_Y 4000 BTN_TOUCH 0 | BTN_TOUCH 0 | ABS_PRESSURE 45 ABS_PRESSURE 0 | ABS_PRESSURE 0 | BTN_TOOL_FINGER 1 BTN_TOOL_TRIPLETAP 0 | BTN_TOOL_TRIPLETAP 0 | BTN_TOOL_TRIPLETAP 0 --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | | ABS_MT_SLOT 2 | | ABS_MT_TRACKING_ID -1 | | BTN_TOUCH 0 | | ABS_PRESSURE 0 | | BTN_TOOL_FINGER 0 | | --- SYN_REPORT (0) ---------- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Benjamin, >>>> For old kernels this is not a problem because max_slots was 2 and libinput/ >>>> xorg-synaptics knew how to deal with that. Now that max_slot is 3, the >>>> clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). >>>> It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP >>>> information, and goes wild. Maybe the cr48 sensor should be classified as MT_SEMI instead. Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Henrik, On Apr 25 2015 or thereabouts, Henrik Rydberg wrote: > Benjamin, > > >>>> For old kernels this is not a problem because max_slots was 2 and libinput/ > >>>> xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > >>>> clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > >>>> It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > >>>> information, and goes wild. > > Maybe the cr48 sensor should be classified as MT_SEMI instead. > That's not a cr48 issue (actually the cr48 is also affected given that it allocates 2 slots). To be able to fix the cursors jumps that we see with any regular image sensor touchpad, I cleaned up all the code and relied on the kernel tracking to provide an actual tracking. So now, either the touchpad is old and it does forwards SEMI_MT, either it is new enough and uses the in-kernel tracking. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Apr 24 2015 or thereabouts, Benjamin Tissoires wrote: > Hi Dmitry, > > [ adding more relevant people to the discussion ] > > On Apr 23 2015 or thereabouts, Benjamin Tissoires wrote: > > On Apr 23 2015 or thereabouts, Dmitry Torokhov wrote: > > > On Wed, Apr 22, 2015 at 11:45:09AM -0400, Benjamin Tissoires wrote: > > > > Synaptics PS/2 touchpad can send only 2 touches in a report. They can > > > > detect 4 or 5 and this information is valuable. > > > > > > > > In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep > > > > stability in image sensors"), we allocate 3 slots, but we still continue > > > > to report the 2 available fingers. That means that the client sees 2 used > > > > slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. > > > > > > > > For old kernels this is not a problem because max_slots was 2 and libinput/ > > > > xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > > > > clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > > > > It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > > > > information, and goes wild. > > > > > > > > We can pin the 3 slots until we get a total number of fingers below 2. > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 > > > > > > Benjamin, I do not quite like it. It seems that original patch was not > > > quite right and we are adding more workarounds. > > > > Agree. And I am starting to hate more and more the synaptics PS/2 and all > > the PS/2 drivers to be honest :) - trying to fit a heavy load data like > > multitouch in a simple and lightweight protocol like PS/2 is insane... > > > > We are internally trying to figure out if we can finally take advantage > > of the SMBus/RMI4 protocol, but we tried for one year without much > > success. > > > > > > > > Synaptics can only track 2 contacts, correct? Why 2 slots to track them > > > is not enough? > > > > IIRC, the problem was that upon a third finger down, with only 2 slots, > > the fingers were silently inverted in most cases. The thing is that the > > firmware forwards 2 fingers, but not necessarily the two first. So you > > generally get fingers 1+3 so the slot 2 needs to be removed. And that > > means the kernel tracking has to track 3 fingers upon transitions. > > > > This may be completely bullshit and we might not need to use 3 slots at > > all. I'll need to do further experiments to validate which one is best > > then. > > > > I am perfectly fine holding this one up for a little bit more testings > > and then we can decide which one needs to be done (revert or an other > > band-aid). > > > > So I carefully recorded each situation (initial with 2 slots, 2 slots > and then with the pinning in this patch*), and I am now convinced that > the pinning is the best sequence that we forward to the user space (best > among the 3). > > With 2 slots declared, there are 2 problems: > - the first finger jumps to the position of the 3rd when it lands > - the transition between 2 to 3 fingers goes to a state where the kernel > removes the second finger (while jumping the first to the position of > the 3rd finger), send a sync and then reallocate the first finger > position as the second slot in use > > -> that means that user space sees a small transition where the slots > count is 1 while the BTN_TOOL advertise triple tap :/ > > With 3 slots, we have the problem reported in the rhbz bug #1212230: > - during the transition, the fingers are stable, but we have at most 2 > active slots in one frame, which confuses libinput/xorg-synaptics. > > With the pinning, the user space is no more confused because BTN_TOOL is > always greater or equal than the active slots. > > So I think for now we have 3 possibilities: > 1. Just carry this patch, and hope that we will be able to switch the > synaptics device in the non-PS/2 mode > 2. Revert to 2 patches and fix the kernel tracking to accept 3 fingers > and return the 2 best matches > 3. Revert the use of the kernel tracking at all and re-introduce the > spaghetti code that was here before and hope that all cases where > properly handled. > > IMO that the solution 2. is the best, but I can not do it because I > don't understand what the code does. I can guess things but I can not > accurately change it because it is not readable IMO. > > (yes, there is also the solution 4: "screw up and let the user space deal > with it", but I'd rather not do that given the history of the multitouch > protocol) Dmitry, I feel like this discussion fell a little bit between the cracks and that we all forgot about it. I still believe that the patch is needed (even if it is not the best solution), so I am sending a gently ping on this one :) Cheers, Benjamin > * I tried to put side by side the 3 test cases in the following logs: > > > (init slots 2, no pinning) | (init slots 3, no pinning) | (init slots 3, pinning) > ----------------------------- | ----------------------------- | --------------------------- > ------------------------------|----- one finger down -------|---------------------------- > | | > ABS_MT_SLOT 0 | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 > ABS_MT_TRACKING_ID 53 | ABS_MT_TRACKING_ID 53 | ABS_MT_TRACKING_ID 53 > ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 > ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 > ABS_MT_PRESSURE 68 | ABS_MT_PRESSURE 68 | ABS_MT_PRESSURE 68 > BTN_TOUCH 1 | BTN_TOUCH 1 | BTN_TOUCH 1 > ABS_X 2000 | ABS_X 2000 | ABS_X 2000 > ABS_Y 2000 | ABS_Y 2000 | ABS_Y 2000 > ABS_PRESSURE 68 | ABS_PRESSURE 68 | ABS_PRESSURE 68 > BTN_TOOL_FINGER 1 | BTN_TOOL_FINGER 1 | BTN_TOOL_FINGER 1 > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > | | > ... | ... | ... > | | > ------------------------------------ second finger down ------------------------------------ > | | > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 > ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 > ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 > ABS_MT_SLOT 1 | ABS_MT_SLOT 1 | ABS_MT_SLOT 1 > ABS_MT_TRACKING_ID 54 | ABS_MT_TRACKING_ID 54 | ABS_MT_TRACKING_ID 54 > ABS_MT_POSITION_X 3000 | ABS_MT_POSITION_X 3000 | ABS_MT_POSITION_X 3000 > ABS_MT_POSITION_Y 3000 | ABS_MT_POSITION_Y 3000 | ABS_MT_POSITION_Y 3000 > ABS_MT_PRESSURE 64 | ABS_MT_PRESSURE 64 | ABS_MT_PRESSURE 64 > ABS_X 2000 | ABS_X 2000 | ABS_X 2000 > ABS_Y 2000 | ABS_Y 2000 | ABS_Y 2000 > ABS_PRESSURE 78 | ABS_PRESSURE 78 | ABS_PRESSURE 78 > BTN_TOOL_FINGER 0 | BTN_TOOL_FINGER 0 | BTN_TOOL_FINGER 0 > BTN_TOOL_DOUBLETAP 1 | BTN_TOOL_DOUBLETAP 1 | BTN_TOOL_DOUBLETAP 1 > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > | | > ... | ... | ... > | | > ------------------------------------ third finger down ------------------------------------ > | | > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > ABS_MT_SLOT 0 | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 > ABS_MT_POSITION_X 4000 | ABS_MT_POSITION_X 2000 | ABS_MT_POSITION_X 2000 > ABS_MT_POSITION_Y 4000 | ABS_MT_POSITION_Y 2000 | ABS_MT_POSITION_Y 2000 > ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 | ABS_MT_PRESSURE 78 > ABS_MT_SLOT 1 | ABS_MT_SLOT 2 | ABS_MT_SLOT 2 > ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID 55 | ABS_MT_TRACKING_ID 55 > | ABS_MT_POSITION_X 4000 | ABS_MT_POSITION_X 4000 > | ABS_MT_POSITION_Y 4000 | ABS_MT_POSITION_Y 4000 > | ABS_MT_PRESSURE 72 | ABS_MT_PRESSURE 72 > | ABS_MT_SLOT 1 | > | ABS_MT_TRACKING_ID -1 | > ABS_X 4000 | ABS_X 2000 | ABS_X 2000 > ABS_Y 4000 | ABS_Y 2000 | ABS_Y 2000 > ABS_PRESSURE 78 | ABS_PRESSURE 78 | ABS_PRESSURE 78 > BTN_TOOL_DOUBLETAP 0 | BTN_TOOL_DOUBLETAP 0 | BTN_TOOL_DOUBLETAP 0 > BTN_TOOL_TRIPLETAP 1 | BTN_TOOL_TRIPLETAP 1 | BTN_TOOL_TRIPLETAP 1 > --- SYN_REPORT (0) ---------- | | > ABS_MT_SLOT 0 | | > ABS_MT_POSITION_X 4000 | | > ABS_MT_POSITION_Y 4000 | | > ABS_MT_PRESSURE 78 | | > ABS_MT_SLOT 1 | | > ABS_MT_TRACKING_ID 55 | | > ABS_MT_POSITION_X 2000 | | > ABS_MT_POSITION_Y 2000 | | > ABS_MT_PRESSURE 72 | | > ABS_X 4000 | | > ABS_Y 4000 | | > ABS_PRESSURE 78 | | > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > | | > ... | ... | ... > | | > ------------------------------------ 3 fingers release ------------------------------------ > | | > | | > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > | | ABS_MT_SLOT 2 > | | ABS_MT_POSITION_X 4000 > | | ABS_MT_POSITION_Y 4000 > | | ABS_MT_PRESSURE 45 > ABS_MT_SLOT 0 | ABS_MT_SLOT 0 | ABS_MT_SLOT 0 > ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 > ABS_MT_SLOT 1 | ABS_MT_SLOT 2 | ABS_MT_SLOT 1 > ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 | ABS_MT_TRACKING_ID -1 > | | ABS_X 4000 > | | ABS_Y 4000 > BTN_TOUCH 0 | BTN_TOUCH 0 | ABS_PRESSURE 45 > ABS_PRESSURE 0 | ABS_PRESSURE 0 | BTN_TOOL_FINGER 1 > BTN_TOOL_TRIPLETAP 0 | BTN_TOOL_TRIPLETAP 0 | BTN_TOOL_TRIPLETAP 0 > --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- | --- SYN_REPORT (0) ---------- > | | ABS_MT_SLOT 2 > | | ABS_MT_TRACKING_ID -1 > | | BTN_TOUCH 0 > | | ABS_PRESSURE 0 > | | BTN_TOOL_FINGER 0 > | | --- SYN_REPORT (0) ---------- > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, On Thu, Jun 11, 2015 at 01:29:09PM -0400, Benjamin Tissoires wrote: > On Apr 24 2015 or thereabouts, Benjamin Tissoires wrote: > > Hi Dmitry, > > > > [ adding more relevant people to the discussion ] > > > > On Apr 23 2015 or thereabouts, Benjamin Tissoires wrote: > > > On Apr 23 2015 or thereabouts, Dmitry Torokhov wrote: > > > > On Wed, Apr 22, 2015 at 11:45:09AM -0400, Benjamin Tissoires wrote: > > > > > Synaptics PS/2 touchpad can send only 2 touches in a report. They can > > > > > detect 4 or 5 and this information is valuable. > > > > > > > > > > In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep > > > > > stability in image sensors"), we allocate 3 slots, but we still continue > > > > > to report the 2 available fingers. That means that the client sees 2 used > > > > > slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. > > > > > > > > > > For old kernels this is not a problem because max_slots was 2 and libinput/ > > > > > xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > > > > > clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > > > > > It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > > > > > information, and goes wild. > > > > > > > > > > We can pin the 3 slots until we get a total number of fingers below 2. > > > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 > > > > > > > > Benjamin, I do not quite like it. It seems that original patch was not > > > > quite right and we are adding more workarounds. > > > > > > Agree. And I am starting to hate more and more the synaptics PS/2 and all > > > the PS/2 drivers to be honest :) - trying to fit a heavy load data like > > > multitouch in a simple and lightweight protocol like PS/2 is insane... > > > > > > We are internally trying to figure out if we can finally take advantage > > > of the SMBus/RMI4 protocol, but we tried for one year without much > > > success. > > > > > > > > > > > Synaptics can only track 2 contacts, correct? Why 2 slots to track them > > > > is not enough? > > > > > > IIRC, the problem was that upon a third finger down, with only 2 slots, > > > the fingers were silently inverted in most cases. The thing is that the > > > firmware forwards 2 fingers, but not necessarily the two first. So you > > > generally get fingers 1+3 so the slot 2 needs to be removed. And that > > > means the kernel tracking has to track 3 fingers upon transitions. > > > > > > This may be completely bullshit and we might not need to use 3 slots at > > > all. I'll need to do further experiments to validate which one is best > > > then. > > > > > > I am perfectly fine holding this one up for a little bit more testings > > > and then we can decide which one needs to be done (revert or an other > > > band-aid). > > > > > > > So I carefully recorded each situation (initial with 2 slots, 2 slots > > and then with the pinning in this patch*), and I am now convinced that > > the pinning is the best sequence that we forward to the user space (best > > among the 3). > > > > With 2 slots declared, there are 2 problems: > > - the first finger jumps to the position of the 3rd when it lands > > - the transition between 2 to 3 fingers goes to a state where the kernel > > removes the second finger (while jumping the first to the position of > > the 3rd finger), send a sync and then reallocate the first finger > > position as the second slot in use > > > > -> that means that user space sees a small transition where the slots > > count is 1 while the BTN_TOOL advertise triple tap :/ > > > > With 3 slots, we have the problem reported in the rhbz bug #1212230: > > - during the transition, the fingers are stable, but we have at most 2 > > active slots in one frame, which confuses libinput/xorg-synaptics. > > > > With the pinning, the user space is no more confused because BTN_TOOL is > > always greater or equal than the active slots. > > > > So I think for now we have 3 possibilities: > > 1. Just carry this patch, and hope that we will be able to switch the > > synaptics device in the non-PS/2 mode > > 2. Revert to 2 patches and fix the kernel tracking to accept 3 fingers > > and return the 2 best matches > > 3. Revert the use of the kernel tracking at all and re-introduce the > > spaghetti code that was here before and hope that all cases where > > properly handled. > > > > IMO that the solution 2. is the best, but I can not do it because I > > don't understand what the code does. I can guess things but I can not > > accurately change it because it is not readable IMO. > > > > (yes, there is also the solution 4: "screw up and let the user space deal > > with it", but I'd rather not do that given the history of the multitouch > > protocol) > > Dmitry, I feel like this discussion fell a little bit between the cracks > and that we all forgot about it. > > I still believe that the patch is needed (even if it is not the best > solution), so I am sending a gently ping on this one :) Sorry I lost track of this, but I still believe that introducing the 3rd slot is not the right solution as is evidenced by the need of more workarounds. If the hardware is only capable of tracking the 2 contacts then we should be using 2 slots. It seems that userspace (and maybe the kernel as well?) is not quite prepared to handle change of contact's identity in a slot (i.e. assigning new tracking id to a slot without transitioning through -1), but that is what we need to fix then. I think we should revert 63c4fda3c0bb. Thanks.
On Jun 30 2015 or thereabouts, Dmitry Torokhov wrote: > Hi Benjamin, > > On Thu, Jun 11, 2015 at 01:29:09PM -0400, Benjamin Tissoires wrote: > > On Apr 24 2015 or thereabouts, Benjamin Tissoires wrote: > > > Hi Dmitry, > > > > > > [ adding more relevant people to the discussion ] > > > > > > On Apr 23 2015 or thereabouts, Benjamin Tissoires wrote: > > > > On Apr 23 2015 or thereabouts, Dmitry Torokhov wrote: > > > > > On Wed, Apr 22, 2015 at 11:45:09AM -0400, Benjamin Tissoires wrote: > > > > > > Synaptics PS/2 touchpad can send only 2 touches in a report. They can > > > > > > detect 4 or 5 and this information is valuable. > > > > > > > > > > > > In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep > > > > > > stability in image sensors"), we allocate 3 slots, but we still continue > > > > > > to report the 2 available fingers. That means that the client sees 2 used > > > > > > slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. > > > > > > > > > > > > For old kernels this is not a problem because max_slots was 2 and libinput/ > > > > > > xorg-synaptics knew how to deal with that. Now that max_slot is 3, the > > > > > > clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). > > > > > > It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP > > > > > > information, and goes wild. > > > > > > > > > > > > We can pin the 3 slots until we get a total number of fingers below 2. > > > > > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 > > > > > > > > > > Benjamin, I do not quite like it. It seems that original patch was not > > > > > quite right and we are adding more workarounds. > > > > > > > > Agree. And I am starting to hate more and more the synaptics PS/2 and all > > > > the PS/2 drivers to be honest :) - trying to fit a heavy load data like > > > > multitouch in a simple and lightweight protocol like PS/2 is insane... > > > > > > > > We are internally trying to figure out if we can finally take advantage > > > > of the SMBus/RMI4 protocol, but we tried for one year without much > > > > success. > > > > > > > > > > > > > > Synaptics can only track 2 contacts, correct? Why 2 slots to track them > > > > > is not enough? > > > > > > > > IIRC, the problem was that upon a third finger down, with only 2 slots, > > > > the fingers were silently inverted in most cases. The thing is that the > > > > firmware forwards 2 fingers, but not necessarily the two first. So you > > > > generally get fingers 1+3 so the slot 2 needs to be removed. And that > > > > means the kernel tracking has to track 3 fingers upon transitions. > > > > > > > > This may be completely bullshit and we might not need to use 3 slots at > > > > all. I'll need to do further experiments to validate which one is best > > > > then. > > > > > > > > I am perfectly fine holding this one up for a little bit more testings > > > > and then we can decide which one needs to be done (revert or an other > > > > band-aid). > > > > > > > > > > So I carefully recorded each situation (initial with 2 slots, 2 slots > > > and then with the pinning in this patch*), and I am now convinced that > > > the pinning is the best sequence that we forward to the user space (best > > > among the 3). > > > > > > With 2 slots declared, there are 2 problems: > > > - the first finger jumps to the position of the 3rd when it lands > > > - the transition between 2 to 3 fingers goes to a state where the kernel > > > removes the second finger (while jumping the first to the position of > > > the 3rd finger), send a sync and then reallocate the first finger > > > position as the second slot in use > > > > > > -> that means that user space sees a small transition where the slots > > > count is 1 while the BTN_TOOL advertise triple tap :/ > > > > > > With 3 slots, we have the problem reported in the rhbz bug #1212230: > > > - during the transition, the fingers are stable, but we have at most 2 > > > active slots in one frame, which confuses libinput/xorg-synaptics. > > > > > > With the pinning, the user space is no more confused because BTN_TOOL is > > > always greater or equal than the active slots. > > > > > > So I think for now we have 3 possibilities: > > > 1. Just carry this patch, and hope that we will be able to switch the > > > synaptics device in the non-PS/2 mode > > > 2. Revert to 2 patches and fix the kernel tracking to accept 3 fingers > > > and return the 2 best matches > > > 3. Revert the use of the kernel tracking at all and re-introduce the > > > spaghetti code that was here before and hope that all cases where > > > properly handled. > > > > > > IMO that the solution 2. is the best, but I can not do it because I > > > don't understand what the code does. I can guess things but I can not > > > accurately change it because it is not readable IMO. > > > > > > (yes, there is also the solution 4: "screw up and let the user space deal > > > with it", but I'd rather not do that given the history of the multitouch > > > protocol) > > > > Dmitry, I feel like this discussion fell a little bit between the cracks > > and that we all forgot about it. > > > > I still believe that the patch is needed (even if it is not the best > > solution), so I am sending a gently ping on this one :) > > Sorry I lost track of this, but I still believe that introducing the 3rd > slot is not the right solution as is evidenced by the need of more > workarounds. If the hardware is only capable of tracking the 2 contacts > then we should be using 2 slots. It seems that userspace (and maybe the > kernel as well?) is not quite prepared to handle change of contact's > identity in a slot (i.e. assigning new tracking id to a slot without > transitioning through -1), but that is what we need to fix then. > > I think we should revert 63c4fda3c0bb. > It seems you are right: https://bugzilla.redhat.com/show_bug.cgi?id=1236540 shows that the fix is not good enough. I am not sure the kernel tracking is able to correctly track and implement the identity change within the same slot, but we should be able to remove these limitations by switching to RMI4 in the near future. Anyway, ACK for the revert. Back from my PTOs, Chandler sent us a mail saying that he managed to get the PS/2 pass-through for the trackstick working which means that we have in our trees a fully working solution for the Lenovo 40 and 50 series, and potentially all other RMI4 over SMBus ones (except ForcePads). Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c index 630af73..c69b308 100644 --- a/drivers/input/mouse/synaptics.c +++ b/drivers/input/mouse/synaptics.c @@ -935,6 +935,14 @@ static void synaptics_report_mt_data(struct psmouse *psmouse, input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z); } + /* keep (slot count <= num_fingers) by pinning all slots */ + if (num_fingers >= 3) { + for (i = 0; i < 3; i++) { + input_mt_slot(dev, i); + input_mt_report_slot_state(dev, MT_TOOL_FINGER, true); + } + } + input_mt_drop_unused(dev); /* Don't use active slot count to generate BTN_TOOL events. */
Synaptics PS/2 touchpad can send only 2 touches in a report. They can detect 4 or 5 and this information is valuable. In commit 63c4fda3c0bb ("Input: synaptics - allocate 3 slots to keep stability in image sensors"), we allocate 3 slots, but we still continue to report the 2 available fingers. That means that the client sees 2 used slots while there is a total of 3 fingers advertised by BTN_TOOL_TRIPLETAP. For old kernels this is not a problem because max_slots was 2 and libinput/ xorg-synaptics knew how to deal with that. Now that max_slot is 3, the clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP information, and goes wild. We can pin the 3 slots until we get a total number of fingers below 2. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1212230 Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/input/mouse/synaptics.c | 8 ++++++++ 1 file changed, 8 insertions(+)