Message-ID: <s5h379r9yo1.wl-tiwai@suse.de>
Date:   Thu, 20 Jul 2017 12:43:10 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Masaki Ota <masaki.ota@...alps.com>
Cc:     Paul Donohue <linux-kernel@...lSD.com>,
        Laura Abbott <labbott@...hat.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Pali Rohar <pali.rohar@...il.com>,
        Nick Fletcher <nick.m.fletcher@...il.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "scott.s.lowe@...il.com" <scott.s.lowe@...il.com>
Subject: Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

On Thu, 20 Jul 2017 12:02:10 +0200,
Masaki Ota wrote:
> 
> Hi, 
> 
> Some user reported that this issue happened from kernel 4.10.7.
> Because the below patch was applied on it.
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0186e6a4e501d39f5f90dd7e5887bc668aef06c4
> 
> So, I thought the cause of this issue is this patch.
> Original code meaning is Kernel 4.10.6, it seems not to have this issue.

The bug has been present from the beginning, but it didn't surface
until another commit to change x_max and co because MT feature is
mostly used only for scrolling and such simple behavior.  For
scrolling, even the bogus MT_X value could work casually with the
certain x_max value used in the original commit.
Actually, by fiddling with x_max (either less than 2200 or greater
than 3000), the two-finger scroll "works", although it shouldn't.


Takashi

> 
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@...e.de] 
> Sent: Thursday, July 20, 2017 6:35 PM
> To: 太田 真喜 Masaki Ota <masaki.ota@...alps.com>
> Cc: Paul Donohue <linux-kernel@...lSD.com>; Laura Abbott <labbott@...hat.com>; Dmitry Torokhov <dmitry.torokhov@...il.com>; Pali Rohar <pali.rohar@...il.com>; Nick Fletcher <nick.m.fletcher@...il.com>; linux-input@...r.kernel.org; linux-kernel@...r.kernel.org; scott.s.lowe@...il.com
> Subject: Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")
> 
> 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 <tiwai@...e.de>
> 
> > 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@...e.de]
> > Sent: Wednesday, July 19, 2017 5:57 PM
> > To: Paul Donohue <linux-kernel@...lSD.com>
> > Cc: Laura Abbott <labbott@...hat.com>; 太田 真喜 Masaki Ota 
> > <masaki.ota@...alps.com>; Dmitry Torokhov <dmitry.torokhov@...il.com>; 
> > Pali Rohar <pali.rohar@...il.com>; Nick Fletcher 
> > <nick.m.fletcher@...il.com>; linux-input@...r.kernel.org; 
> > linux-kernel@...r.kernel.org; scott.s.lowe@...il.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) | \
> > 

Powered by blists - more mailing lists