Re: [Linuxwacom-devel] [PATCH input-wacom] backport: restore touch functionality to SW_MUTE_DEVICE devices

2018-04-25 Thread Jason Gerecke 
Tested the current input-wacom master (9d7af79697) *without* this
patch on my CentOS 6.9 VM using three devices (all with touch
switches): 1st-gen Intuos, 2nd-gen Intuos Pro, and Cintiq Pro 32. In
all cases, everything seemed to work as expected:

  * Single finger touch worked correctly
  * Touch switch enabled / disabled touch input
  * Touch switch state is correctly reported by xsetwacom
  * Multi-finger touch works correctly for the two Pro devices (it is
disabled for the 1st-gen Intuos by the HEAD commit due to its style of
multitouch events [DOUBLE,TRIPLE,QUADTAP] confusing the X driver).

There must be something different between our test setups...

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two, /
But you can’t take seven from three,/
So you look at the sixty-fours



On Tue, Apr 24, 2018 at 9:42 PM, Jason Gerecke  wrote:
> On Tue, Apr 24, 2018 at 12:19 AM, Ping Cheng  wrote:
>> On Mon, Apr 23, 2018 at 4:37 PM Peter Hutterer 
>> wrote:
>>>
>>> On Mon, Apr 23, 2018 at 12:44:51PM -0700, Aaron Armstrong Skomra wrote:
>>> > Setting the bit for SW_MUTE_DEVICE on kernels which do not have
>>> > SW_MUTE_DEVICE declared in input.h (prior to 3.13), causes these
>>> > devices' touch interfaces not to work.
>>>
>>> Why is that? is that switch always enabled or something? because if it's
>>> always on 0, no-one in userspace should care. This seems like a separate
>>> issue somewhere.
>>
>>
>> The switch can be enabled/disabled by some devices. So, if it is set, its
>> value could be 1.
>>
>
> Agreed with Peter... This sounds like a symptom of something else. In
> my experience the kernel doesn't care about "unknown" SW/ABS/KEY
> events, so any problem would have to be in our kernel or X driver. The
> fact that "only" removing the declaration SW_MUTE_DEVICE is enough to
> fix the issue is a head-scratcher.
>
>>> > Fixes: 7d13c26dd972 ("Backport support for the touch on/off switch").
>>> > Signed-off-by: Aaron Armstrong Skomra
>>> > ---
>>> > 1. Does INTUOSHT have a touch switch?
>>
>>
>> Yes, INTUOSHT has a hard touch switch.
>>
>>> Maybe the check for
>>> > BTN_TOOL_DOUBLETAP excludes INTUOSHT right after we catch its case.
>>
>>
>> Have to look into the details...
>>
>>> >
>>> > 2. Does TABLETPC somehow need this commit to disable touch? I don't
>>> > think
>>> > it could be using this non-existant constant, but I don't know which
>>> > devices TABLETPC applies to etc.
>>
>>
>> No TPC devices have touch switch.
>>
>>> On the Intuos Pro 2 and Intuos (CTH-680)
>>> > this switch is purely the reporting of the switch (the hardware disables
>>> > the touch) so removing our attempt at reporting the switch is of minor
>>> > consequence.
>>>
>>> with my "i don't follow input-wacom that closely" hat on:
>>> TABLETPC are the devices built into laptops. Google Bill Gates presenting
>>> a
>>> tablet pc, that's how far they go back. They have styli that only activate
>>> on press, with no proximity, etc. But they don't have the MUTE switch,
>>> that
>>> one was added for the 24QHD (or so?) to represent the physical hw switch.
>>
>>
>> Touch switch was first added to INTUOSHT.
>>
>>
>>> > 3. Why was SW_MUTE_DEVICE orignally backported? My guess was that it was
>>> > to remove differences between each of our kernel directories. But it
>>> > could
>>
>>
>> I feel it is just to sync with upstream code. Is that right Jason?
>>
>
> Git blame says it's been in the 2.6.38 tree since December 2013
> (commit 955bc05d68). I added it to the 2.6.30 tree in December 2017
> about a month after doing my Cintiq Pro backports. I *probably* tested
> that code the touch switch (and touch) worked for the Cintiq Pro, but
> I can absolutely guarantee that touch has been tested on 2.6.38 at
> multiple points in the past four years.
>
>>> > be for some other reason that I don't know of, in that case I will be
>>> > missing something here.
>>> >
>>> > 4. My guess is that SW_MUTE_DEVICE is supported in RHEL 7 (this bug was
>>> > found on RHEL 6) and that this code was tested against it, which is why
>>> > we
>>> > didn't catch this. ie RHEL probably added SW_MUTE_DEVICE to their
>>> > kernel.
>>>
>>> yep, looks like it was backported to the RHEL7.x kernel at some point but
>>> I
>>
>>
>> SW_MUTE_DEVICE is in RHEL 7 doesn’t mean other kernels older than 3.13
>> backported it. We have to support those vanilla kernels. Looks like we may
>> have to test more kernels and probably treat RHEL7 differently.
>> .
>
> I doubt RHEL having SW_MUTE_DEVICE "support" matters at all.
> SW_MUTE_DEVICE is only declared in a header and defined in our driver.
> Nothing else in the kernel should care if a random (known or unknown)
> SW integer is declared.
>
>>>
>>> don't have the exact version on-hand. We tend to backport the professional
>>> tablets so it was likely part of the 24QHD or

Re: [Linuxwacom-devel] [PATCH input-wacom] backport: restore touch functionality to SW_MUTE_DEVICE devices

2018-04-24 Thread Ping Cheng 
On Mon, Apr 23, 2018 at 4:37 PM Peter Hutterer 
wrote:

> On Mon, Apr 23, 2018 at 12:44:51PM -0700, Aaron Armstrong Skomra wrote:
> > Setting the bit for SW_MUTE_DEVICE on kernels which do not have
> > SW_MUTE_DEVICE declared in input.h (prior to 3.13), causes these
> > devices' touch interfaces not to work.
>
> Why is that? is that switch always enabled or something? because if it's
> always on 0, no-one in userspace should care. This seems like a separate
> issue somewhere.


The switch can be enabled/disabled by some devices. So, if it is set, its
value could be 1.

> Fixes: 7d13c26dd972 ("Backport support for the touch on/off switch").
> > Signed-off-by: Aaron Armstrong Skomra
> > ---
> > 1. Does INTUOSHT have a touch switch?


Yes, INTUOSHT has a hard touch switch.

Maybe the check for
> > BTN_TOOL_DOUBLETAP excludes INTUOSHT right after we catch its case.


Have to look into the details...

>
> > 2. Does TABLETPC somehow need this commit to disable touch? I don't think
> > it could be using this non-existant constant, but I don't know which
> > devices TABLETPC applies to etc.


No TPC devices have touch switch.

On the Intuos Pro 2 and Intuos (CTH-680)
> > this switch is purely the reporting of the switch (the hardware disables
> > the touch) so removing our attempt at reporting the switch is of minor
> > consequence.
>
> with my "i don't follow input-wacom that closely" hat on:
> TABLETPC are the devices built into laptops. Google Bill Gates presenting a
> tablet pc, that's how far they go back. They have styli that only activate
> on press, with no proximity, etc. But they don't have the MUTE switch, that
> one was added for the 24QHD (or so?) to represent the physical hw switch.


Touch switch was first added to INTUOSHT.


> 3. Why was SW_MUTE_DEVICE orignally backported? My guess was that it was
> > to remove differences between each of our kernel directories. But it
> could


I feel it is just to sync with upstream code. Is that right Jason?

> be for some other reason that I don't know of, in that case I will be
> > missing something here.
> >
> > 4. My guess is that SW_MUTE_DEVICE is supported in RHEL 7 (this bug was
> > found on RHEL 6) and that this code was tested against it, which is why
> we
> > didn't catch this. ie RHEL probably added SW_MUTE_DEVICE to their kernel.
>
> yep, looks like it was backported to the RHEL7.x kernel at some point but I


SW_MUTE_DEVICE is in RHEL 7 doesn’t mean other kernels older than 3.13
backported it. We have to support those vanilla kernels. Looks like we may
have to test more kernels and probably treat RHEL7 differently.
.

> don't have the exact version on-hand. We tend to backport the professional
> tablets so it was likely part of the 24QHD or whichever device needed that
> one.


Intuos Pro 2 is the first professional tablet that has a hardware touch
switch.

Cheers,
Ping

> It could also be that later input subsystems fail more gracefully when
> > they recieve a "__set_bit" that they are unfamiliar with, I'm not sure if
> > we want to find supported vanilla versions of these older kernels to test
> > against.  Also, I have no idea how to check RHEL 7 source or if that's
> > possible. If necessary we can either revise this commit to check for RHEL
> > 7 or come back and do that at a later date for this event which again is
> > of minor importance.
>
> The official way if you don't have a RHEL install
> is https://git.centos.org/project/rpms
> yes, that's a CentOS link, for more backstory see:
> https://lwn.net/Articles/603865/
>
> The kernel is here:
> https://git.centos.org/summary/?r=rpms/kernel.git
>
> And to 'unzip' the RPM file, I have a script that wraps this:
>   rpm2cpio $rpmfile | cpio -idmv --no-absolute-filenames
>
>
> Alternatively, there's also the Red Hat Developers program which gives you
> free access to RHEL.
> https://developers.redhat.com/
>
> Cheers,
>Peter
>
>
> >
> >  2.6.32/wacom_wac.c | 3 ---
> >  2.6.38/wacom_wac.c | 3 ---
> >  3.7/wacom_wac.c| 3 ---
> >  3 files changed, 9 deletions(-)
> >
> > diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
> > index 9f9899917a41..98589bea8f17 100644
> > --- a/2.6.32/wacom_wac.c
> > +++ b/2.6.32/wacom_wac.c
> > @@ -2276,7 +2276,6 @@ void wacom_setup_input_capabilities(struct
> input_dev *input_dev,
> >   __set_bit(BTN_TOOL_DOUBLETAP, input_dev->keybit);
> >
> >   input_dev->evbit[0] |= BIT_MASK(EV_SW);
> > - __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> >   wacom_wac->shared->has_mute_touch_switch = true;
> >   } else {
> >   __set_bit(BTN_STYLUS3, input_dev->keybit);
> > @@ -2351,7 +2350,6 @@ void wacom_setup_input_capabilities(struct
> input_dev *input_dev,
> >
> >   if ((input_dev->id.product >= 0x353 &&
> input_dev->id.product <= 0x356)) {
> >   input_dev->evbit[0] |=

Re: [Linuxwacom-devel] [PATCH input-wacom] backport: restore touch functionality to SW_MUTE_DEVICE devices

2018-04-23 Thread Peter Hutterer 
On Mon, Apr 23, 2018 at 12:44:51PM -0700, Aaron Armstrong Skomra wrote:
> Setting the bit for SW_MUTE_DEVICE on kernels which do not have
> SW_MUTE_DEVICE declared in input.h (prior to 3.13), causes these
> devices' touch interfaces not to work.

Why is that? is that switch always enabled or something? because if it's
always on 0, no-one in userspace should care. This seems like a separate
issue somewhere.

> Fixes: 7d13c26dd972 ("Backport support for the touch on/off switch").
> Signed-off-by: Aaron Armstrong Skomra
> ---
> 1. Does INTUOSHT have a touch switch? Maybe the check for
> BTN_TOOL_DOUBLETAP excludes INTUOSHT right after we catch its case.
> 
> 2. Does TABLETPC somehow need this commit to disable touch? I don't think
> it could be using this non-existant constant, but I don't know which
> devices TABLETPC applies to etc. On the Intuos Pro 2 and Intuos (CTH-680)
> this switch is purely the reporting of the switch (the hardware disables
> the touch) so removing our attempt at reporting the switch is of minor
> consequence.

with my "i don't follow input-wacom that closely" hat on:
TABLETPC are the devices built into laptops. Google Bill Gates presenting a
tablet pc, that's how far they go back. They have styli that only activate
on press, with no proximity, etc. But they don't have the MUTE switch, that
one was added for the 24QHD (or so?) to represent the physical hw switch.

> 3. Why was SW_MUTE_DEVICE orignally backported? My guess was that it was
> to remove differences between each of our kernel directories. But it could
> be for some other reason that I don't know of, in that case I will be
> missing something here. 
> 
> 4. My guess is that SW_MUTE_DEVICE is supported in RHEL 7 (this bug was
> found on RHEL 6) and that this code was tested against it, which is why we
> didn't catch this. ie RHEL probably added SW_MUTE_DEVICE to their kernel.

yep, looks like it was backported to the RHEL7.x kernel at some point but I
don't have the exact version on-hand. We tend to backport the professional
tablets so it was likely part of the 24QHD or whichever device needed that
one.

> It could also be that later input subsystems fail more gracefully when
> they recieve a "__set_bit" that they are unfamiliar with, I'm not sure if
> we want to find supported vanilla versions of these older kernels to test
> against.  Also, I have no idea how to check RHEL 7 source or if that's
> possible. If necessary we can either revise this commit to check for RHEL
> 7 or come back and do that at a later date for this event which again is
> of minor importance. 

The official way if you don't have a RHEL install
is https://git.centos.org/project/rpms
yes, that's a CentOS link, for more backstory see:
https://lwn.net/Articles/603865/

The kernel is here:
https://git.centos.org/summary/?r=rpms/kernel.git

And to 'unzip' the RPM file, I have a script that wraps this:
  rpm2cpio $rpmfile | cpio -idmv --no-absolute-filenames


Alternatively, there's also the Red Hat Developers program which gives you
free access to RHEL.
https://developers.redhat.com/

Cheers,
   Peter


> 
>  2.6.32/wacom_wac.c | 3 ---
>  2.6.38/wacom_wac.c | 3 ---
>  3.7/wacom_wac.c| 3 ---
>  3 files changed, 9 deletions(-)
> 
> diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
> index 9f9899917a41..98589bea8f17 100644
> --- a/2.6.32/wacom_wac.c
> +++ b/2.6.32/wacom_wac.c
> @@ -2276,7 +2276,6 @@ void wacom_setup_input_capabilities(struct input_dev 
> *input_dev,
>   __set_bit(BTN_TOOL_DOUBLETAP, input_dev->keybit);
>  
>   input_dev->evbit[0] |= BIT_MASK(EV_SW);
> - __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
>   wacom_wac->shared->has_mute_touch_switch = true;
>   } else {
>   __set_bit(BTN_STYLUS3, input_dev->keybit);
> @@ -2351,7 +2350,6 @@ void wacom_setup_input_capabilities(struct input_dev 
> *input_dev,
>  
>   if ((input_dev->id.product >= 0x353 && 
> input_dev->id.product <= 0x356)) {
>   input_dev->evbit[0] |= BIT_MASK(EV_SW);
> - __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
>   wacom_wac->shared->has_mute_touch_switch = true;
>   wacom_wac->shared->is_touch_on = true;
>   }
> @@ -2394,7 +2392,6 @@ void wacom_setup_input_capabilities(struct input_dev 
> *input_dev,
>   if (features->touch_max &&
>   features->device_type == BTN_TOOL_DOUBLETAP) {
>   input_dev->evbit[0] |= BIT_MASK(EV_SW);
> - __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
>   wacom_wac->shared->has_mute_touch_switch = true;
>   }
>   /* fall through */
> diff --git a/2.6.38/wacom_wac.c b/2.6.38/wacom_wac.c
> index e0b5af1fc850..858602daf993 100644
> --- a/2.6.38/wacom_wac.c
> +++

[Linuxwacom-devel] [PATCH input-wacom] backport: restore touch functionality to SW_MUTE_DEVICE devices

2018-04-23 Thread Aaron Armstrong Skomra 
Setting the bit for SW_MUTE_DEVICE on kernels which do not have
SW_MUTE_DEVICE declared in input.h (prior to 3.13), causes these
devices' touch interfaces not to work.

Fixes: 7d13c26dd972 ("Backport support for the touch on/off switch").
Signed-off-by: Aaron Armstrong Skomra
---
1. Does INTUOSHT have a touch switch? Maybe the check for BTN_TOOL_DOUBLETAP 
excludes INTUOSHT right after we catch its case.

2. Does TABLETPC somehow need this commit to disable touch? I don't think it 
could be using this non-existant constant, but I don't know which devices 
TABLETPC applies to etc. On the Intuos Pro 2 and Intuos (CTH-680) this switch 
is purely the reporting of the switch (the hardware disables the touch) so 
removing our attempt at reporting the switch is of minor consequence.

3. Why was SW_MUTE_DEVICE orignally backported? My guess was that it was to 
remove differences between each of our kernel directories. But it could be for 
some other reason that I don't know of, in that case I will be missing 
something here. 

4. My guess is that SW_MUTE_DEVICE is supported in RHEL 7 (this bug was found 
on RHEL 6) and that this code was tested against it, which is why we didn't 
catch this. ie RHEL probably added SW_MUTE_DEVICE to their kernel. It could 
also be that later input subsystems fail more gracefully when they recieve a 
"__set_bit" that they are unfamiliar with, I'm not sure if we want to find 
supported vanilla versions of these older kernels to test against.  Also, I 
have no idea how to check RHEL 7 source or if that's possible. If necessary we 
can either revise this commit to check for RHEL 7 or come back and do that at a 
later date for this event which again is of minor importance. 

 2.6.32/wacom_wac.c | 3 ---
 2.6.38/wacom_wac.c | 3 ---
 3.7/wacom_wac.c| 3 ---
 3 files changed, 9 deletions(-)

diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
index 9f9899917a41..98589bea8f17 100644
--- a/2.6.32/wacom_wac.c
+++ b/2.6.32/wacom_wac.c
@@ -2276,7 +2276,6 @@ void wacom_setup_input_capabilities(struct input_dev 
*input_dev,
__set_bit(BTN_TOOL_DOUBLETAP, input_dev->keybit);
 
input_dev->evbit[0] |= BIT_MASK(EV_SW);
-   __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
wacom_wac->shared->has_mute_touch_switch = true;
} else {
__set_bit(BTN_STYLUS3, input_dev->keybit);
@@ -2351,7 +2350,6 @@ void wacom_setup_input_capabilities(struct input_dev 
*input_dev,
 
if ((input_dev->id.product >= 0x353 && 
input_dev->id.product <= 0x356)) {
input_dev->evbit[0] |= BIT_MASK(EV_SW);
-   __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
wacom_wac->shared->has_mute_touch_switch = true;
wacom_wac->shared->is_touch_on = true;
}
@@ -2394,7 +2392,6 @@ void wacom_setup_input_capabilities(struct input_dev 
*input_dev,
if (features->touch_max &&
features->device_type == BTN_TOOL_DOUBLETAP) {
input_dev->evbit[0] |= BIT_MASK(EV_SW);
-   __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
wacom_wac->shared->has_mute_touch_switch = true;
}
/* fall through */
diff --git a/2.6.38/wacom_wac.c b/2.6.38/wacom_wac.c
index e0b5af1fc850..858602daf993 100644
--- a/2.6.38/wacom_wac.c
+++ b/2.6.38/wacom_wac.c
@@ -2539,7 +2539,6 @@ int wacom_setup_input_capabilities(struct input_dev 
*input_dev,
}
else {
input_dev->evbit[0] |= BIT_MASK(EV_SW);
-   __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
wacom_wac->shared->has_mute_touch_switch = true;
}
err = wacom_create_slots(wacom_wac);
@@ -2647,7 +2646,6 @@ int wacom_setup_input_capabilities(struct input_dev 
*input_dev,
if ((features->device_type == BTN_TOOL_FINGER) &&
(input_dev->id.product >= 0x353 && input_dev->id.product <= 
0x356)) {
input_dev->evbit[0] |= BIT_MASK(EV_SW);
-   __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
wacom_wac->shared->has_mute_touch_switch = true;
wacom_wac->shared->is_touch_on = true;
}
@@ -2693,7 +2691,6 @@ int wacom_setup_input_capabilities(struct input_dev 
*input_dev,
if (features->touch_max &&
features->device_type == BTN_TOOL_FINGER) {
input_dev->evbit[0] |= BIT_MASK(EV_SW);
-   __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
wacom_wac->shared->has_mute_touch_switch = true;
}
/* fall through */
diff --git a/3.7/wacom_wac.c