| Message ID | 1358449462.28710.139.camel@fourier |
|---|---|
| State | New |
| Headers | show |
I agree the risk of regression is low and any fallout will be quickly detectable, i.e., your pointing device will quit working. rtg
On Thu, Jan 17, 2013 at 11:04:22AM -0800, Kamal Mostafa wrote: > The Cypress PS/2 Trackpad driver (which we're already shipping in > Precise and Quantal, but not Raring) has been committed to the Input > subsystem maintainer's master branch[0] pending merge into mainline > Linux 3.9. > > Please consider this pull for Raring > http://kernel.ubuntu.com/git?p=kamal/ubuntu-raring.git;a=shortlog;h=refs/heads/cypress-for-raring > ... which includes the Cypress driver cherry-picked from the Input > subsystem maintainer's repo, plus my "simulated multitouch" Cypress > driver enhancement (denied by upstream, but required for Unity > multitouch functionality; is included in our P and Q version). I'm not really so keen on this simulated multitouch patch. First, I think you overstate the case for the simulated multitouch functionality. Without that patch the driver still has semi-mt support, which should be adequate to support most two-finger gestures: drag, pinch, spread, and two-finger clicks at minimum. It looks like there's enough information coming out of the driver to support 3, 4, and 5 finger clicks as well. My understanding is that our userspace doesn't handle those events the same as with full mt devices, but I don't see that as being a driver problem. As for gestures involving 3+ fingers, the code just generates fake touch points offset horizontally from the first one. That means every gesture looks like a drag. In fact, with knowing the number of contacts and the bounding box I'd think that userspace should even be able to support a 3+ finger drag gesture. Probably better than the simulated multitouch even, because if the bounding box isn't staying approximately the same size the gesture likely isn't a drag. What's wrong with limiting it to semi-mt, in line with the actual capabilities of the hardware? One more comment, regarding the implementation. The patch lacks protection for report_data->contact_cnt > CTYP_MAX_MT_SLOTS whereas the rest of the driver seems to protect against it. I'm assuming that because you've increased that max number of slots from 2 to 5 there's no longer a concern of having more contacts than slots? Because if that happens you're going to overrun report_data->contacts. Seth
On Thu, 2013-01-17 at 14:38 -0600, Seth Forshee wrote: > On Thu, Jan 17, 2013 at 11:04:22AM -0800, Kamal Mostafa wrote: > > The Cypress PS/2 Trackpad driver [...] > > plus my "simulated multitouch" Cypress > > driver enhancement (denied by upstream, but required for Unity > > multitouch functionality; is included in our P and Q version). > I'm not really so keen on this simulated multitouch patch. > Hi Seth- Thanks for reviewing this, and for all your help with this driver. I'm certainly open to discussion about the validity of the Cypress simulated multitouch patch... > First, I think you overstate the case for the simulated multitouch > functionality. The main case for it is that the shipping Precise and Quantal SAUCE versions of this driver do include the simulated multitouch enhancement. The original driver as supplied by Cypress included a variant of it. So for the Cypress trackpad, Unity multi-touch gestures (>2 fingers) work now in P and Q, but if we omit my simul-mt patch then they won't work anymore in Raring: a regression. > Without that patch the driver still has semi-mt support, > which should be adequate to support most two-finger gestures: drag, > pinch, spread, and two-finger clicks at minimum. It looks like there's > enough information coming out of the driver to support 3, 4, and 5 > finger clicks as well. Correct. The Cypress driver (with or without the simul-mt patch) does already emit the newer fingers-count API events along with the semi-mt bounding box via a call to input_mt_report_finger_count(), but ... > My understanding is that our userspace doesn't > handle those events the same as with full mt devices, but I don't see > that as being a driver problem. Correct. Our userspace (Unity via x-x-input-synaptics) does not support that newer fingers-count API at all for semi-mt devices. If my reading of x-x-i-s is correct, even the latest upstream version of x-x-i-s does not. I spent several frustrating hours trying to make x-x-i-s to support it, but couldn't figure out how without a major overhaul. Upstream input subsystem maintainers agree with you that this is "not a driver problem", but userspace doesn't seem to want to take responsibility for it either. From the perspective of a user of the machine, I do not care whether the code is in the driver or in userspace... If the hardware is capable of >2 finger support and we have working code to support it, then I would like it to work on my Ubuntu system. > > As for gestures involving 3+ fingers, the code [Kamal's simul-mt patch] > just generates fake touch > points offset horizontally from the first one. That means every gesture > looks like a drag. A drag or a tap. As coded, it works very nicely for Unity's 3-finger drag and 4-finger tap gestures. > > In fact, with knowing the number of contacts and the bounding box I'd > think that userspace should even be able to support a 3+ finger drag > gesture. Probably better than the simulated multitouch even, because if > the bounding box isn't staying approximately the same size the gesture > likely isn't a drag. Agreed. That's exactly what userspace *should* be doing for semi-mt devices. ;-) > > What's wrong with limiting it to semi-mt, in line with the actual > capabilities of the hardware? My feeling is that the addition of the simul-mt patch is useful and harmless, so what's wrong with including it? > One more comment, regarding the implementation. The patch lacks > protection for report_data->contact_cnt > CTYP_MAX_MT_SLOTS whereas the > rest of the driver seems to protect against it. I'm assuming that > because you've increased that max number of slots from 2 to 5 there's no > longer a concern of having more contacts than slots? Because if that > happens you're going to overrun report_data->contacts. Your assumption is correct. contact_cnt cannot ever become > 5, and the checks for contact_cnt > CTYP_MAX_MT_SLOTS in the rest of the driver are actually necessary *only* to support the semi-mt case. Those checks aren't for overrun protection, they are what implements the regular semi-mt "maximum-of-two-coordinate-pairs" behavior. -Kamal
On Thu, Jan 17, 2013 at 02:03:16PM -0800, Kamal Mostafa wrote: > On Thu, 2013-01-17 at 14:38 -0600, Seth Forshee wrote: > > On Thu, Jan 17, 2013 at 11:04:22AM -0800, Kamal Mostafa wrote: > > > The Cypress PS/2 Trackpad driver [...] > > > plus my "simulated multitouch" Cypress > > > driver enhancement (denied by upstream, but required for Unity > > > multitouch functionality; is included in our P and Q version). > > > > I'm not really so keen on this simulated multitouch patch. > > > > Hi Seth- > > Thanks for reviewing this, and for all your help with this driver. I'm > certainly open to discussion about the validity of the Cypress simulated > multitouch patch... I don't think there's much to discuss as far as validity goes. The driver just makes stuff up. From a technical perspective I don't see how that could be considered valid. > > First, I think you overstate the case for the simulated multitouch > > functionality. > > The main case for it is that the shipping Precise and Quantal SAUCE > versions of this driver do include the simulated multitouch enhancement. > The original driver as supplied by Cypress included a variant of it. > > So for the Cypress trackpad, Unity multi-touch gestures (>2 fingers) > work now in P and Q, but if we omit my simul-mt patch then they won't > work anymore in Raring: a regression. The fact that this would be a regression is the *only* legitimate argument I see in favor of keeping the patch. But it's a strong argument. Too bad we didn't notice this and get rid of it before putting it into P and Q. > > As for gestures involving 3+ fingers, the code [Kamal's simul-mt patch] > > just generates fake touch > > points offset horizontally from the first one. That means every gesture > > looks like a drag. > > A drag or a tap. As coded, it works very nicely for Unity's 3-finger > drag and 4-finger tap gestures. Okay, sure. I wasn't really thinking of taps as being gesutres ;-) > > What's wrong with limiting it to semi-mt, in line with the actual > > capabilities of the hardware? > > My feeling is that the addition of the simul-mt patch is useful and > harmless, so what's wrong with including it? One minor problem would be if any desktop environment running on top of our kernel supports gestures with >2 fingers other than tap and drag. Users may be confused to find that every gesture gets treated like a drag. > > One more comment, regarding the implementation. The patch lacks > > protection for report_data->contact_cnt > CTYP_MAX_MT_SLOTS whereas the > > rest of the driver seems to protect against it. I'm assuming that > > because you've increased that max number of slots from 2 to 5 there's no > > longer a concern of having more contacts than slots? Because if that > > happens you're going to overrun report_data->contacts. > > Your assumption is correct. contact_cnt cannot ever become > 5, and the > checks for contact_cnt > CTYP_MAX_MT_SLOTS in the rest of the driver are > actually necessary *only* to support the semi-mt case. Those checks > aren't for overrun protection, they are what implements the regular > semi-mt "maximum-of-two-coordinate-pairs" behavior. Good, I just wanted to be sure. Thanks for confirming. Seth
The Cypress PS/2 Trackpad driver (which we're already shipping in Precise and Quantal, but not Raring) has been committed to the Input subsystem maintainer's master branch[0] pending merge into mainline Linux 3.9. Please consider this pull for Raring http://kernel.ubuntu.com/git?p=kamal/ubuntu-raring.git;a=shortlog;h=refs/heads/cypress-for-raring ... which includes the Cypress driver cherry-picked from the Input subsystem maintainer's repo, plus my "simulated multitouch" Cypress driver enhancement (denied by upstream, but required for Unity multitouch functionality; is included in our P and Q version). After the driver commits land in mainline 3.9, we will replace the cherry-picks. I believe the possibility of regression is low; the driver has been well tested, and (aside from its detection routine) will only affect systems with Cypress PS/2 trackpads. Thanks, -Kamal [0] http://git.kernel.org/?p=linux/kernel/git/dtor/input.git;a=commitdiff;h=80524f083e2c3e70057f5bb476db92baa14cb22b http://git.kernel.org/?p=linux/kernel/git/dtor/input.git;a=commitdiff;h=0799a924bc93ba46a23e8e7e6b1431ab585fd2ea -- >8 ----------------------------- The following changes since commit 18cc3a6a1debcf3819c47b5226ff44f9f89b5105: UBUNTU: Ubuntu-3.8.0-0.4 (2013-01-15 13:26:26 -0700) are available in the git repository at: git://kernel.ubuntu.com/kamal/ubuntu-raring.git cypress-for-raring for you to fetch changes up to 0c6dabecd306d66e56674db2e67a084db5abeb50: UBUNTU: [Config] Add CONFIG_PS2_CYPRESS (2013-01-17 10:13:40 -0800) ---------------------------------------------------------------- Dudley Du (1): UBUNTU: SAUCE: Input: add support for Cypress PS/2 Trackpads Kamal Mostafa (3): UBUNTU: SAUCE: Input: increase struct ps2dev cmdbuf[] to 8 bytes UBUNTU: SAUCE: Input: Cypress PS/2 Trackpad simulated multitouch UBUNTU: [Config] Add CONFIG_PS2_CYPRESS debian.master/config/config.common.ubuntu | 1 + drivers/input/mouse/Kconfig | 10 + drivers/input/mouse/Makefile | 1 + drivers/input/mouse/cypress_ps2.c | 743 +++++++++++++++++++++++++++++ drivers/input/mouse/cypress_ps2.h | 202 ++++++++ drivers/input/mouse/psmouse-base.c | 32 ++ drivers/input/mouse/psmouse.h | 1 + include/linux/libps2.h | 2 +- 8 files changed, 991 insertions(+), 1 deletion(-) create mode 100644 drivers/input/mouse/cypress_ps2.c create mode 100644 drivers/input/mouse/cypress_ps2.h