diff mbox

[REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

Message ID 20170619232026.GG1855@TopQuark.net (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Donohue June 19, 2017, 11:20 p.m. UTC
On Mon, Jun 19, 2017 at 01:02:18PM -0700, Laura Abbott wrote:
> On 06/19/2017 11:43 AM, Paul Donohue wrote:
> > I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> > Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> > 
> > Maybe we should ask the user to try a few more tests?
> > 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> > psmouse_err(psmouse,
> >     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
> >     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
> >     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> > 2) Use the old electrode count code but the new pitch code:
> > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > 		num_x_electrode =
> > 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> > 		num_y_electrode =
> > 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> > 
> > 		priv->x_max =
> > 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > 		priv->y_max =
> > 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > 
> > 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > 
> > 	} else {
> > 3) Use the new electrode count code but the old pitch code:
> > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > 		num_x_electrode =
> > 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> > 		num_y_electrode =
> > 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> > 
> > 		priv->x_max =
> > 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > 		priv->y_max =
> > 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > 
> > 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> > 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> > 
> > 	} else {
> > 
> 
> Can you produce patches for these test cases?

I've reduced it to two test cases.  Patches attached.

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 262d1057c1da..f57dca5cecd5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2476,8 +2476,8 @@ static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 		priv->y_max =
 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
 
-		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
-		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
+		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
+		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
 
 	} else {
 		num_x_electrode =
@@ -2500,6 +2500,11 @@ static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 	priv->x_res = priv->x_max * 10 / x_phys; /* units / mm */
 	priv->y_res = priv->y_max * 10 / y_phys; /* units / mm */
 
+	psmouse_err(psmouse,
+		    "test2 pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
+		    x_pitch, y_pitch, num_x_electrode, num_y_electrode,
+		    x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
+
 	return 0;
 }

Comments

Takashi Iwai July 11, 2017, 3:50 p.m. UTC | #1
On Tue, 20 Jun 2017 01:20:26 +0200,
Paul Donohue wrote:
> 
> On Mon, Jun 19, 2017 at 01:02:18PM -0700, Laura Abbott wrote:
> > On 06/19/2017 11:43 AM, Paul Donohue wrote:
> > > I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> > > Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> > > 
> > > Maybe we should ask the user to try a few more tests?
> > > 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> > > psmouse_err(psmouse,
> > >     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
> > >     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
> > >     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> > > 2) Use the old electrode count code but the new pitch code:
> > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > 		num_x_electrode =
> > > 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> > > 		num_y_electrode =
> > > 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> > > 
> > > 		priv->x_max =
> > > 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > 		priv->y_max =
> > > 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > 
> > > 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > 
> > > 	} else {
> > > 3) Use the new electrode count code but the old pitch code:
> > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > 		num_x_electrode =
> > > 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> > > 		num_y_electrode =
> > > 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> > > 
> > > 		priv->x_max =
> > > 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > 		priv->y_max =
> > > 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > 
> > > 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> > > 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> > > 
> > > 	} else {
> > > 
> > 
> > Can you produce patches for these test cases?
> 
> I've reduced it to two test cases.  Patches attached.

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
--
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
Paul Donohue July 11, 2017, 7:58 p.m. UTC | #2
On Tue, Jul 11, 2017 at 05:50:07PM +0200, Takashi Iwai wrote:
> On Tue, 20 Jun 2017 01:20:26 +0200,
> Paul Donohue wrote:
> > 
> > On Mon, Jun 19, 2017 at 01:02:18PM -0700, Laura Abbott wrote:
> > > On 06/19/2017 11:43 AM, Paul Donohue wrote:
> > > > I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> > > > Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> > > > 
> > > > Maybe we should ask the user to try a few more tests?
> > > > 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> > > > psmouse_err(psmouse,
> > > >     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
> > > >     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
> > > >     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> > > > 2) Use the old electrode count code but the new pitch code:
> > > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > > 		num_x_electrode =
> > > > 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> > > > 		num_y_electrode =
> > > > 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> > > > 
> > > > 		priv->x_max =
> > > > 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > > 		priv->y_max =
> > > > 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > > 
> > > > 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > > 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > > 
> > > > 	} else {
> > > > 3) Use the new electrode count code but the old pitch code:
> > > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > > 		num_x_electrode =
> > > > 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> > > > 		num_y_electrode =
> > > > 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> > > > 
> > > > 		priv->x_max =
> > > > 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > > 		priv->y_max =
> > > > 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > > 
> > > > 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> > > > 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> > > > 
> > > > 	} else {
> > > > 
> > > 
> > > Can you produce patches for these test cases?
> > 
> > I've reduced it to two test cases.  Patches attached.
> 
> 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?

Thanks!
-Paul
--
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
Takashi Iwai July 12, 2017, 7:16 a.m. UTC | #3
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


Takashi
--
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
Paul Donohue July 12, 2017, 4:28 p.m. UTC | #4
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?

Thanks,
-Paul
--
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 mbox

Patch

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 262d1057c1da..341a6c3bf907 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2467,14 +2467,14 @@  static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 
 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
 		num_x_electrode =
-			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
+			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
 		num_y_electrode =
-			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
+			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
 
 		priv->x_max =
-			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
+			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
 		priv->y_max =
-			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
+			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
 
 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
@@ -2500,6 +2500,11 @@  static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 	priv->x_res = priv->x_max * 10 / x_phys; /* units / mm */
 	priv->y_res = priv->y_max * 10 / y_phys; /* units / mm */
 
+	psmouse_err(psmouse,
+		    "test1 pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
+		    x_pitch, y_pitch, num_x_electrode, num_y_electrode,
+		    x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
+
 	return 0;
 }