| Message ID | 1624456597-9486-2-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] Input: atmel_mxt_ts: Add wake-up support | expand |
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. -- Dmitry
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
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 --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 */
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(+) -- 2.7.4