diff mbox series

[2/2] Input: atmel_mxt_ts: atmel_mxt_ts: Fix event loss

Message ID 1624456597-9486-2-git-send-email-loic.poulain@linaro.org (mailing list archive)
State New, archived
Headers show
Series [1/2] Input: atmel_mxt_ts: Add wake-up support | expand

Commit Message

Loic Poulain June 23, 2021, 1:56 p.m. UTC
If both touch events and release are part of the same report,
userspace will not consider it as a touch-down & touch-up but as
a non-action. That can happen on resume when 'buffered' events are
dequeued in a row.

Make sure that release always causes previous events to be synced
before being reported.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Dmitry Torokhov June 24, 2021, 12:48 a.m. UTC | #1
On Wed, Jun 23, 2021 at 03:56:37PM +0200, Loic Poulain wrote:
> If both touch events and release are part of the same report,
> userspace will not consider it as a touch-down & touch-up but as
> a non-action. That can happen on resume when 'buffered' events are
> dequeued in a row.
> 
> Make sure that release always causes previous events to be synced
> before being reported.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 807f449..e05ec30 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
>  		input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
>  		input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
>  	} else {
> +		/*
> +		 * Always sync input before reporting release, to be sure
> +		 * previous event(s) are taking into account by user side.
> +		 */
> +		if (data->update_input)
> +			mxt_input_sync(data);

That means we sync for every contact release, whereas I think ideal
would be to only sync when we observe touch-down and touch-up in the
same slot.

Let's also add Peter to the conversation...

> +
>  		dev_dbg(dev, "[%u] release\n", id);
>  
>  		/* close out slot */
> -- 
> 2.7.4
> 

Thanks.
Peter Hutterer June 24, 2021, 5:57 a.m. UTC | #2
On Wed, Jun 23, 2021 at 05:48:49PM -0700, Dmitry Torokhov wrote:
> On Wed, Jun 23, 2021 at 03:56:37PM +0200, Loic Poulain wrote:
> > If both touch events and release are part of the same report,
> > userspace will not consider it as a touch-down & touch-up but as
> > a non-action. That can happen on resume when 'buffered' events are
> > dequeued in a row.
> > 
> > Make sure that release always causes previous events to be synced
> > before being reported.
> > 
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 807f449..e05ec30 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
> >  		input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
> >  		input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
> >  	} else {
> > +		/*
> > +		 * Always sync input before reporting release, to be sure
> > +		 * previous event(s) are taking into account by user side.
> > +		 */
> > +		if (data->update_input)
> > +			mxt_input_sync(data);
> 
> That means we sync for every contact release, whereas I think ideal
> would be to only sync when we observe touch-down and touch-up in the
> same slot.
> 
> Let's also add Peter to the conversation...

Thanks for the CC.

FTR, this is expected userspace behaviour, the device state is only looked
at during SYN_REPORT. Where you send event E=1 and E=0 in the same frame,
the state at SYN_REPORT time is 0, the 1 never happened.

The only device we (as in: libinput) make an exception for here are
keyboards because too many drivers get it wrong and it's too hard to fix all
of them. But especially for touch devices (and tablets!) we don't really
have any choice but to look at the state of the device at the end of the
frame.

So, yes, this patch is needed but I agree with Dmitry that you should only
send this for the special case that requires it.

Cheers,
   Peter
Loic Poulain June 24, 2021, 8:05 a.m. UTC | #3
Hi Peter, Dmitry,

On Thu, 24 Jun 2021 at 07:58, Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> On Wed, Jun 23, 2021 at 05:48:49PM -0700, Dmitry Torokhov wrote:
> > On Wed, Jun 23, 2021 at 03:56:37PM +0200, Loic Poulain wrote:
> > > If both touch events and release are part of the same report,
> > > userspace will not consider it as a touch-down & touch-up but as
> > > a non-action. That can happen on resume when 'buffered' events are
> > > dequeued in a row.
> > >
> > > Make sure that release always causes previous events to be synced
> > > before being reported.
> > >
> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > ---
> > >  drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > index 807f449..e05ec30 100644
> > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > @@ -990,6 +990,13 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
> > >             input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
> > >             input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
> > >     } else {
> > > +           /*
> > > +            * Always sync input before reporting release, to be sure
> > > +            * previous event(s) are taking into account by user side.
> > > +            */
> > > +           if (data->update_input)
> > > +                   mxt_input_sync(data);
> >
> > That means we sync for every contact release, whereas I think ideal
> > would be to only sync when we observe touch-down and touch-up in the
> > same slot.
> >
> > Let's also add Peter to the conversation...
>
> Thanks for the CC.
>
> FTR, this is expected userspace behaviour, the device state is only looked
> at during SYN_REPORT. Where you send event E=1 and E=0 in the same frame,
> the state at SYN_REPORT time is 0, the 1 never happened.
>
> The only device we (as in: libinput) make an exception for here are
> keyboards because too many drivers get it wrong and it's too hard to fix all
> of them. But especially for touch devices (and tablets!) we don't really
> have any choice but to look at the state of the device at the end of the
> frame.
>
> So, yes, this patch is needed but I agree with Dmitry that you should only
> send this for the special case that requires it.

I'm not really familiar with the input framework, so thanks for the
clarification. In that patch _sync() is 'forced' if there is any
previous event in the report, but from what you say, I should only
sync if one of the previous events is a touch-down, so a transition
E=1? right?

Regards,
Loic
diff mbox series

Patch

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 807f449..e05ec30 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -990,6 +990,13 @@  static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
 		input_report_abs(input_dev, ABS_MT_DISTANCE, distance);
 		input_report_abs(input_dev, ABS_MT_ORIENTATION, orientation);
 	} else {
+		/*
+		 * Always sync input before reporting release, to be sure
+		 * previous event(s) are taking into account by user side.
+		 */
+		if (data->update_input)
+			mxt_input_sync(data);
+
 		dev_dbg(dev, "[%u] release\n", id);
 
 		/* close out slot */