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

[Loic Poulain <loic.poulain@linaro.org>]

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