[PATCH] HID: wacom: ask for a in-prox report when it was missed

[PATCH] HID: wacom: ask for a in-prox report when it was missed

[Benjamin Tissoires]

If noone listens to the input device when a tool comes in proximity,
the tablet does not send the in-prox event when a client becomes available.
That means that no events will be sent until the tool is taken out of
proximity.

In this situation, ask for the report WACOM_REPORT_INTUOSREAD which will
read the corresponding feature and generate an in-prox event.

We don't schedule this read while we are in an IO interrupt because we
know that usbhid will do it asynchronously. If this is triggered by uhid
then this is obviously a client side bug :)

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_wac.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 9ec4545..ff9a7ab 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -430,6 +430,19 @@ exit:
 	return retval;
 }
 
+static void wacom_intuos_schedule_prox_event(struct wacom_wac *wacom_wac)
+{
+	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+	struct hid_report *r;
+	struct hid_report_enum *re;
+
+	re = &(wacom->hdev->report_enum[HID_FEATURE_REPORT]);
+	r = re->report_id_hash[WACOM_REPORT_INTUOSREAD];
+	if (r) {
+		hid_hw_request(wacom->hdev, r, HID_REQ_GET_REPORT);
+	}
+}
+
 static int wacom_intuos_inout(struct wacom_wac *wacom)
 {
 	struct wacom_features *features = &wacom->features;
@@ -609,8 +622,11 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
 	}
 
 	/* don't report other events if we don't know the ID */
-	if (!wacom->id[idx])
+	if (!wacom->id[idx]) {
+		/* but reschedule a read of the current tool */
+		wacom_intuos_schedule_prox_event(wacom);
 		return 1;
+	}
 
 	return 0;
 }
-- 
2.1.0

Re: [PATCH] HID: wacom: ask for a in-prox report when it was missed

[Jason Gerecke]

On 3/3/2015 9:20 AM, Benjamin Tissoires wrote:
> If noone listens to the input device when a tool comes in proximity,
> the tablet does not send the in-prox event when a client becomes available.
> That means that no events will be sent until the tool is taken out of
> proximity.
>
> In this situation, ask for the report WACOM_REPORT_INTUOSREAD which will
> read the corresponding feature and generate an in-prox event.
>
> We don't schedule this read while we are in an IO interrupt because we
> know that usbhid will do it asynchronously. If this is triggered by uhid
> then this is obviously a client side bug :)
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Ping and I were both a little confused by this patch. Our first 
impression was that this wouldn't accomplish anything since the driver 
would see events from the tablet regardless of if a client were 
connected or not. Testing proved us wrong though, with the events 
clearly going to that great /dev/null in the sky. Is this behavior 
different from the USB and HID subsystems? IIRC, the prox code didn't 
use to behave like this...

Under the assumption that USB events are indeed being dropped until a 
client is connected, then this patch looks fairly reasonable. It 
doesn't, however, seem to work reliably across tablets. An Intuos Pro 
reacted as I'd expect (with the patch in place, already-in-prox tools 
actually send events once e.g. evemu-record is run) but didn't seem to 
do anything for an Intuos5 or Intuos3 (nothing comes through 
evemu-record until I removed the tool from prox and then put it back in).

-- 
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....

> ---
>   drivers/hid/wacom_wac.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 9ec4545..ff9a7ab 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -430,6 +430,19 @@ exit:
>   	return retval;
>   }
>   
> +static void wacom_intuos_schedule_prox_event(struct wacom_wac *wacom_wac)
> +{
> +	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
> +	struct hid_report *r;
> +	struct hid_report_enum *re;
> +
> +	re = &(wacom->hdev->report_enum[HID_FEATURE_REPORT]);
> +	r = re->report_id_hash[WACOM_REPORT_INTUOSREAD];
> +	if (r) {
> +		hid_hw_request(wacom->hdev, r, HID_REQ_GET_REPORT);
> +	}
> +}
> +
>   static int wacom_intuos_inout(struct wacom_wac *wacom)
>   {
>   	struct wacom_features *features = &wacom->features;
> @@ -609,8 +622,11 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
>   	}
>   
>   	/* don't report other events if we don't know the ID */
> -	if (!wacom->id[idx])
> +	if (!wacom->id[idx]) {
> +		/* but reschedule a read of the current tool */
> +		wacom_intuos_schedule_prox_event(wacom);
>   		return 1;
> +	}
>   
>   	return 0;
>   }
--
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

Re: [PATCH] HID: wacom: ask for a in-prox report when it was missed

[Benjamin Tissoires]

On Mar 03 2015 or thereabouts, Jason Gerecke wrote:
> On 3/3/2015 9:20 AM, Benjamin Tissoires wrote:
> >If noone listens to the input device when a tool comes in proximity,
> >the tablet does not send the in-prox event when a client becomes available.
> >That means that no events will be sent until the tool is taken out of
> >proximity.
> >
> >In this situation, ask for the report WACOM_REPORT_INTUOSREAD which will
> >read the corresponding feature and generate an in-prox event.
> >
> >We don't schedule this read while we are in an IO interrupt because we
> >know that usbhid will do it asynchronously. If this is triggered by uhid
> >then this is obviously a client side bug :)
> >
> >Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Ping and I were both a little confused by this patch. Our first impression
> was that this wouldn't accomplish anything since the driver would see events
> from the tablet regardless of if a client were connected or not. Testing
> proved us wrong though, with the events clearly going to that great
> /dev/null in the sky. Is this behavior different from the USB and HID
> subsystems? IIRC, the prox code didn't use to behave like this...

I made further tests this morning, and I found out that one of my
patches adds a more consistent (though a defect) behavior:
b905811a49bcd ("HID: usbhid: prevent unwanted events to be sent when
re-opening")

Without this patch applied, and with an Intuos Pro, I can reproduce the
bug, but sometimes it doesn't trigger. If I let the tool down on the
sensor long enough when no client is listening, then the in-prox event
is not sent to the kernel. But if the time between the landing of the
tool and the open of the device node is small enough, the in-prox event
is seen.

So I installed a v3.16 kernel and can reproduce the same behavior
observed previously.

My analysis is:
- the HID subsystem uses PM in the same way wacom_sys.c used to do in
  v3.16
- v3.16 showed the bugs too, but we never noticed it
- with b905811a49bcd, the bug is more obvious and reliable given that we
  drain the first few milliseconds when we open the device (so we miss
  the in-prox event if it was sent)
- I can not reproduce the second bug mentioned in
  http://lists.freedesktop.org/archives/wayland-devel/2015-March/020361.html
  so it must be device dependent

I think we still need to fetch the current tool in case we receive
events while no in-prox event has been seen. This gives a more reliable
behavior and may also help in some other corner cases.

I think we also still need to keep b905811a49bcd because it should
prevent the second problem Peter observed. The out-of-prox event should
disappear with b905811a49bcd, but we need this patch to actually retrieve
the current tool given that we might have dropped the in-prox event.
 
> Under the assumption that USB events are indeed being dropped until a client
> is connected, then this patch looks fairly reasonable. It doesn't, however,
> seem to work reliably across tablets. An Intuos Pro reacted as I'd expect
> (with the patch in place, already-in-prox tools actually send events once
> e.g. evemu-record is run) but didn't seem to do anything for an Intuos5 or
> Intuos3 (nothing comes through evemu-record until I removed the tool from
> prox and then put it back in).

I'll test on such devices tomorrow. The spec says that the report ID 5
was there since Intuos 2 at least, so I guess there must be a way to
retrieve the info correctly on old devices too.

Cheers,
Benjamin