Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964941AbdGTJf2 (ORCPT ); Thu, 20 Jul 2017 05:35:28 -0400 Received: from mx2.suse.de ([195.135.220.15]:46527 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933620AbdGTJfX (ORCPT ); Thu, 20 Jul 2017 05:35:23 -0400 Date: Thu, 20 Jul 2017 11:35:21 +0200 Message-ID: From: Takashi Iwai To: Masaki Ota Cc: Paul Donohue , Laura Abbott , Dmitry Torokhov , Pali Rohar , Nick Fletcher , "linux-input@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "scott.s.lowe@gmail.com" Subject: Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)") In-Reply-To: References: <75b84b97-5f36-a3be-bb4e-e43cd72d9921@redhat.com> <20170619184315.GF1855@TopQuark.net> <0995261b-8d19-64af-9b66-e1eab579a3d4@redhat.com> <20170619232026.GG1855@TopQuark.net> <20170711195821.GG1855@TopQuark.net> <20170712162841.GI1855@TopQuark.net> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9017 Lines: 230 On Thu, 20 Jul 2017 11:20:20 +0200, Masaki Ota wrote: > > Hi, Takashi-san, > > That's great! > > Actually, SS4 PLUS device is a little different from SS4 device. > I added SS4 PLUS code as below. > Could you try it? Yes, it works as expected. Feel free to take: Tested-by: Takashi Iwai > And I still wonder why this issue does not happen on original code though X value is so large. Which original code are you referring to? The bug manifests itself only for decoding multi-touch events. thanks, Takashi > > --- a/drivers/input/mouse/alps.c > +++ b/drivers/input/mouse/alps.c > @@ -1210,14 +1210,24 @@ static int alps_decode_ss4_v2(struct alps_fields *f, > > case SS4_PACKET_ID_TWO: > if (priv->flags & ALPS_BUTTONPAD) { > - f->mt[0].x = SS4_BTL_MF_X_V2(p, 0); > + if (IS_SS4PLUS_DEV(priv->dev_id)) { > + f->mt[0].x = SS4_PLUS_BTL_MF_X_V2(p, 0); > + f->mt[1].x = SS4_PLUS_BTL_MF_X_V2(p, 1); > + } else { > + f->mt[0].x = SS4_BTL_MF_X_V2(p, 0); > + f->mt[1].x = SS4_BTL_MF_X_V2(p, 1); > + } > f->mt[0].y = SS4_BTL_MF_Y_V2(p, 0); > - f->mt[1].x = SS4_BTL_MF_X_V2(p, 1); > f->mt[1].y = SS4_BTL_MF_Y_V2(p, 1); > } else { > - f->mt[0].x = SS4_STD_MF_X_V2(p, 0); > + if (IS_SS4PLUS_DEV(priv->dev_id)) { > + f->mt[0].x = SS4_PLUS_STD_MF_X_V2(p, 0); > + f->mt[1].x = SS4_PLUS_STD_MF_X_V2(p, 1); > + } else { > + f->mt[0].x = SS4_STD_MF_X_V2(p, 0); > + f->mt[1].x = SS4_STD_MF_X_V2(p, 1); > + } > f->mt[0].y = SS4_STD_MF_Y_V2(p, 0); > - f->mt[1].x = SS4_STD_MF_X_V2(p, 1); > f->mt[1].y = SS4_STD_MF_Y_V2(p, 1); > } > f->pressure = SS4_MF_Z_V2(p, 0) ? 0x30 : 0; > @@ -1234,16 +1244,27 @@ static int alps_decode_ss4_v2(struct alps_fields *f, > > case SS4_PACKET_ID_MULTI: > if (priv->flags & ALPS_BUTTONPAD) { > - f->mt[2].x = SS4_BTL_MF_X_V2(p, 0); > + if (IS_SS4PLUS_DEV(priv->dev_id)) { > + f->mt[0].x = SS4_PLUS_BTL_MF_X_V2(p, 0); > + f->mt[1].x = SS4_PLUS_BTL_MF_X_V2(p, 1); > + } else { > + f->mt[2].x = SS4_BTL_MF_X_V2(p, 0); > + f->mt[3].x = SS4_BTL_MF_X_V2(p, 1); > + } > + > f->mt[2].y = SS4_BTL_MF_Y_V2(p, 0); > - f->mt[3].x = SS4_BTL_MF_X_V2(p, 1); > f->mt[3].y = SS4_BTL_MF_Y_V2(p, 1); > no_data_x = SS4_MFPACKET_NO_AX_BL; > no_data_y = SS4_MFPACKET_NO_AY_BL; > } else { > - f->mt[2].x = SS4_STD_MF_X_V2(p, 0); > + if (IS_SS4PLUS_DEV(priv->dev_id)) { > + f->mt[0].x = SS4_PLUS_STD_MF_X_V2(p, 0); > + f->mt[1].x = SS4_PLUS_STD_MF_X_V2(p, 1); > + } else { > + f->mt[0].x = SS4_STD_MF_X_V2(p, 0); > + f->mt[1].x = SS4_STD_MF_X_V2(p, 1); > + } > f->mt[2].y = SS4_STD_MF_Y_V2(p, 0); > - f->mt[3].x = SS4_STD_MF_X_V2(p, 1); > f->mt[3].y = SS4_STD_MF_Y_V2(p, 1); > no_data_x = SS4_MFPACKET_NO_AX; > no_data_y = SS4_MFPACKET_NO_AY; > @@ -2536,8 +2557,8 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse, > > memset(otp, 0, sizeof(otp)); > > - if (alps_get_otp_values_ss4_v2(psmouse, 0, &otp[0][0]) || > - alps_get_otp_values_ss4_v2(psmouse, 1, &otp[1][0])) > + if (alps_get_otp_values_ss4_v2(psmouse, 1, &otp[1][0]) || > + alps_get_otp_values_ss4_v2(psmouse, 0, &otp[0][0])) > return -1; > > alps_update_device_area_ss4_v2(otp, priv); > diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h > index 4334f2805d93..75542199edda 100644 > --- a/drivers/input/mouse/alps.h > +++ b/drivers/input/mouse/alps.h > @@ -99,6 +99,10 @@ enum SS4_PACKET_ID { > ((_b[1 + _i * 3] << 5) & 0x1F00) \ > ) > > +#define SS4_PLUS_STD_MF_X_V2(_b, _i) (((_b[0 + (_i) * 3] << 4) & 0x0070) | \ > + ((_b[1 + (_i) * 3] << 4) & 0x0F80) \ > + ) > + > #define SS4_STD_MF_Y_V2(_b, _i) (((_b[1 + (_i) * 3] << 3) & 0x0010) | \ > ((_b[2 + (_i) * 3] << 5) & 0x01E0) | \ > ((_b[2 + (_i) * 3] << 4) & 0x0E00) \ > @@ -108,6 +112,10 @@ enum SS4_PACKET_ID { > ((_b[0 + (_i) * 3] >> 3) & 0x0010) \ > ) > > +#define SS4_PLUS_BTL_MF_X_V2(_b, _i) (SS4_PLUS_STD_MF_X_V2(_b, _i) | \ > + ((_b[0 + (_i) * 3] >> 4) & 0x0008) \ > + ) > + > #define SS4_BTL_MF_Y_V2(_b, _i) (SS4_STD_MF_Y_V2(_b, _i) | \ > ((_b[0 + (_i) * 3] >> 3) & 0x0008) \ > ) > -- > > Best Regards, > Masaki Ota > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Wednesday, July 19, 2017 5:57 PM > To: Paul Donohue > Cc: Laura Abbott ; 太田 真喜 Masaki Ota ; Dmitry Torokhov ; Pali Rohar ; Nick Fletcher ; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; scott.s.lowe@gmail.com > Subject: Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)") > > On Wed, 12 Jul 2017 18:28:41 +0200, > Paul Donohue wrote: > > > > On Wed, Jul 12, 2017 at 09:16:43AM +0200, Takashi Iwai wrote: > > > On Tue, 11 Jul 2017 21:58:21 +0200, > > > Paul Donohue wrote: > > > > > > > > On Tue, Jul 11, 2017 at 05:50:07PM +0200, Takashi Iwai wrote: > > > > > Hi, just joining to the party in the middle, as I'm also facing > > > > > the same problem on Dell E7270 laptop. Has this issue already > > > > > been addressed? > > > > > > > > > > If not, the following was my result: > > > > > > > > > > - the first patch slowed the pointer movement a lot, it's even slower > > > > > than the old kernel (e.g. 4.4.x). > > > > > The two finger scroll works fine on all touchpad area now. > > > > > > > > > > - the second patch made the pointer movement even faster than now (as > > > > > I feel, not quite sure). The two finger scroll doesn't work at the > > > > > right side of the touchpad. > > > > > > > > > > > > > > > The kernel output from the first patch is below: > > > > > psmouse serio1: alps: test1 pitch 37x37 num-electrodes 8x7 > > > > > physical size 25x22mm res 69x69 max 1792x1536 > > > > > > > > > > Let me know if you have any further test. > > > > > > > > > > > > > > > thanks, > > > > > > > > > > Takashi > > > > > > > > Do you have the kernel output from the second patch? > > > > > > Here it is: > > > > > > psmouse serio1: alps: test2 pitch 50x54 num-electrodes 20x11 > > > physical size 95x54mm res 25x23 max 2432x1280 > > > > Thanks! > > > > Wow, this is weird ... Values that cause scrolling issues for you work fine for me. > > > > It looks like the priv->x_max value that is causing your problem, but perhaps you can confirm this with another test: > > Revert the patches above, then just before the 'return' at the end of alps_update_device_area_ss4_v2() add the following line: > > priv->x_max = 1792; > > I suspect this line will make two-finger scrolling work again. > > > > It might also help if you could experiment with other x_max values. 2432 is the correct value that causes problems for you.. Does a small value like 500 or a large value like 5000 change the behavior? > > > > Another test you can try is to add a print statement in the 'case SS4_PACKET_ID_TWO' section of alps_decode_ss4_v2() and print out the f->mt[0].x and f->mt[1].x values. With x_max set to 2432, what range of x values does this print when scrolling works, and what range of values are printed when scrolling doesn't work? > > Sorry for the late reply. > > I checked your suggestion, and noticed that actually x_max is a red herring. Then I started looking at the output of evtest (should have done from the beginning!), and the problem became obvious: the reported X values for MT with finger > 1 are doubled, e.g. it reports > 4800 instead of 2400. The MT_Y coordinate looks OK, so it's only about MT_X. And finger=1 reports correctly. > > After reading the code, I ended up the patch like below. This seems working through a quick test. Could anyone check the patch to see whether it works / breaks anything? > > The evtest showed that the MT_X for finger 3 and 4 are also bogus, and the patch should address it, too. > > My only concern so far is whether correcting this globally is right, or it's specific to some certain models. This needs the confirmation from ALPS people... > > > thanks, > > Takashi > > > --- a/drivers/input/mouse/alps.h > +++ b/drivers/input/mouse/alps.h > @@ -96,8 +96,8 @@ enum SS4_PACKET_ID { > > #define SS4_BTN_V2(_b) ((_b[0] >> 5) & SS4_MASK_NORMAL_BUTTONS) > > -#define SS4_STD_MF_X_V2(_b, _i) (((_b[0 + (_i) * 3] << 5) & 0x00E0) | \ > - ((_b[1 + _i * 3] << 5) & 0x1F00) \ > +#define SS4_STD_MF_X_V2(_b, _i) (((_b[0 + (_i) * 3] << 4) & 0x0070) | \ > + ((_b[1 + (_i) * 3] << 4) & 0x0F80) \ > ) > > #define SS4_STD_MF_Y_V2(_b, _i) (((_b[1 + (_i) * 3] << 3) & 0x0010) | \ > @@ -106,7 +106,7 @@ enum SS4_PACKET_ID { > ) > > #define SS4_BTL_MF_X_V2(_b, _i) (SS4_STD_MF_X_V2(_b, _i) | \ > - ((_b[0 + (_i) * 3] >> 3) & 0x0010) \ > + ((_b[0 + (_i) * 3] >> 2) & 0x0008) \ > ) > > #define SS4_BTL_MF_Y_V2(_b, _i) (SS4_STD_MF_Y_V2(_b, _i) | \ >