On Mon, Jan 4, 2016 at 2:22 PM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxx> wrote:
> On Mon, Jan 4, 2016 at 8:50 AM, Aniroop Mathur <a.mathur@xxxxxxxxxxx> wrote:
>> On Jan 4, 2016 5:08 AM, "Peter Hutterer" <peter.hutterer@xxxxxxxxx> wrote:
>>>
>>> On Sat, Jan 02, 2016 at 08:39:21PM -0800, Dmitry Torokhov wrote:
>>> > On Thu, Dec 31, 2015 at 03:36:47AM +0530, Aniroop Mathur wrote:
>>> > > This patch introduces concept to drop partial events in evdev handler
>>> > > itself after emptying the buffer which are dropped by all evdev
>>> > > clients in userspace after SYN_DROPPED occurs.
>>> > > This in turn saves space of partial events in evdev handler buffer
>>> > > and reduces evdev client reading requests.
>>> >
>>> > Let's add a few people who write consumer code to see if this is
>>> > something that they consider useful.
>>>
>>> yeah, it's useful though we already have the code in libevdev to work around
>>> this. Still, it reduces the number of events discarde by the client, so it'll be a net
>>> plus. but, afaict, there's a bug in this implementation.
>>> The doc states: "Client should ignore all events up to and including next
>>> SYN_REPORT event". If you drop partial events, you need to have an empty
>>> SYN_REPORT after the SYN_DROPPED before you start with full events again.
>>> This patch skips that, so after the SYN_DROPPED you have a valid full event
>>> that will be ignored by any client currently capable of handling
>>> SYN_DROPPED.
>>> Example: let's assume a device sending ABS_X/ABS_Y fast enough to cause a
>>> SYN_DROPPED, you may have this queue:
>>> ABS_X
>>> ABS_Y
>>> SYN_REPORT
>>> ...
>>> SYN_DROPPED
>>> ABS_Y <---- partial event
>>> SYN_REPORT <---- client discards up to here, sync state
>>> ABS_X
>>> ABS_Y
>>> SYN_REPORT <---- first full event after sync
>>>
>>> With this patch this sequence becomes:
>>> ABS_X
>>> ABS_Y
>>> SYN_REPORT
>>> ...
>>> SYN_DROPPED
>>> [kernel discards ABS_Y + SYN_REPORT as partial event]
>>> ABS_X
>>> ABS_Y
>>> SYN_REPORT <--- client discards up to here, sync state
>>> <--- there is no event after sync
>>>
>>> That's a change in kernel behaviour and will make all current clients
>>> potentially buggy, you'll really need the empty SYN_REPORT here.
>>>
>>
>> Thank you for your input, Mr. Peter.
>> Actually, there is a need to update the documentation as well after this patch
>> so that clients no more ignore the events after SYN_DROPPED occurs and
>> should read the events normally. I skipped updating the documentation in
>> this patch as I thought of getting a consent first.
>> * SYN_DROPPED:
>> - Used to indicate buffer overrun in the evdev client's event queue.
>> Client should ignore all events up to and including next SYN_REPORT
>> event and query the device (using EVIOCG* ioctls) to obtain its
>> current state
>> + From kernel version <4.4.x> onwards, clients do no need to ignore
>> + events anymore and should read normally as there will be no
>> + partial events after SYN_DROPPED occurs.
>
> Hi Aniroop,
>
> this just won't do. As Peter said, there are current implementation of
> SYN_DROPPED around, which ignore the events until the next SYN_REPORT.
> If you change the protocol by updating the doc, you will just break
> existing userspace which has not included a check on the kernel
> version (and honestly, checking the kernel version from the userspace
> point of view is just a nightmare when distributions start backporting
> changes here and there).
>
> The kernel rule is "do not break userspace", so we can not accept this.
> Peter suggested you just add an empty SYN_REPORT after SYN_DROPPED
> which would solve the whole problem: clients already handling
> SYN_DROPPED will receive the next valid event, and those who don't (or
> which will be updated) will not have to do anything more.
>
> The only cons I can think of is that multitouch protocol A will be a
> pain to handle with this empty SYN_REPORT if you do not handle the
> SYN_DROPPED as per the doc.
> But on the other hand, if you have a MT protocol A device, you are
> screwed anyway because you need mtdev and so let's use libevdev at
> this point.
>
>>
>> As far as I've worked on client codes, this client code change is easy and
>
> Nope, checking the kernel version is not "easy" as this is not reliable.
>
>> even if some clients miss to update the code then it seems not much of
>> a problem because 8 packets are already dropped so an additional packet
>> would not cause any trouble in case of buffer overrun.
>
> Nope again. In case of a SYN_DROPPED, the client syncs its internal
> state by using ioctls. So if you drop one valid event, you are not in
> sync again and the SYN_DROPPED is moot.
>
Thanks for the explanation Mr. Benjamin.
I understood your concern and I have sent the v2 for this patch.
Regards,
Aniroop Mathur
> Cheers,
> Benjamin
>
>>
>> Regards,
>> Aniroop Mathur
>>
>>> > >
>>> > > Signed-off-by: Aniroop Mathur <a.mathur@xxxxxxxxxxx>
>>> > > ---
>>> > > drivers/input/evdev.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>>> > > 1 file changed, 45 insertions(+), 4 deletions(-)
>>> > >
>>> > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>>> > > index e9ae3d5..e7b612e 100644
>>> > > --- a/drivers/input/evdev.c
>>> > > +++ b/drivers/input/evdev.c
>>> > > @@ -58,6 +58,7 @@ struct evdev_client {
>>> > > struct list_head node;
>>> > > unsigned int clk_type;
>>> > > bool revoked;
>>> > > + bool drop_pevent; /* specifies whether partial events need to be dropped */
>>> > > unsigned long *evmasks[EV_CNT]; > > unsigned int bufsize;
>>> > > struct input_event buffer[];
>>> > > @@ -192,6 +193,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>>> > > {
>>> > > unsigned long flags;
>>> > > unsigned int clk_type;
>>> > > + struct input_event *ev;
>>> > >
>>> > > switch (clkid) {
>>> > >
>>> > > @@ -218,6 +220,17 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>>> > > spin_lock_irqsave(&client->buffer_lock, flags);
>>> > >
>>> > > if (client->head != client->tail) {
>>> > > +
>>> > > + /*
>>> > > + * Set drop_pevent to true if last event packet is
>>> > > + * not stored completely in buffer.
>>> > > + */
>>> > > + client->head--;
>>> > > + client->head &= client->bufsize - 1;
>>> > > + ev = &client->buffer[client->head];
>>> > > + if (!(ev->type == EV_SYN && ev->code == SYN_REPORT))
>>> > > + client->drop_pevent = true;
>>> > > +
>>> > > client->packet_head = client->head = client->tail;
>>> > > __evdev_queue_syn_dropped(client);
>>> > > }
>>> > > @@ -228,31 +241,51 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>>> > > return 0;
>>> > > }
>>> > >
>>> > > -static void __pass_event(struct evdev_client *client,
>>> > > +static bool __pass_event(struct evdev_client *client,
>>> > > const struct input_event *event)
>>> > > {
>>> > > + struct input_event *prev_ev;
>>> > > +
>>> > > client->buffer[client->head++] = *event;
>>> > > client->head &= client->bufsize - 1;
>>> > >
>>> > > if (unlikely(client->head == client->tail)) {
>>> > > /*
>>> > > - * This effectively "drops" all unconsumed events, leaving
>>> > > - * EV_SYN/SYN_DROPPED plus the newest event in the queue.
>>> > > + * This effectively "drops" all unconsumed events, storing
>>> > > + * EV_SYN/SYN_DROPPED and the newest event in the queue but
>>> > > + * only if it is not part of partial packet.
>>> > > + * Set drop_pevent to true if last event packet is not stored
>>> > > + * completely in buffer and set to false if SYN_REPORT occurs.
>>> > > */
>>> > > +
>>> > > client->tail = (client->head - 2) & (client->bufsize - 1);
>>> > >
>>> > > + prev_ev = &client->buffer[client->tail];
>>> > > + if (!(prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT)) {
>>>
>>> IMO a (prev_ev->type != EV_SYN || prev_ev->code != SYN_REPORT) would be
>>> easier to read than this (!(a && b)).
>>>
>>> Cheers,
>>> Peter
>>>
>>> > > + client->drop_pevent = true;
>>> > > + client->head--;
>>> > > + client->head &= client->bufsize - 1;
>>> > > + }
>>> > > +
>>> > > client->buffer[client->tail].time = event->time;
>>> > > client->buffer[client->tail].type = EV_SYN;
>>> > > client->buffer[client->tail].code = SYN_DROPPED;
>>> > > client->buffer[client->tail].value = 0;
>>> > >
>>> > > client->packet_head = client->tail;
>>> > > +
>>> > > + if (event->type == EV_SYN && event->code == SYN_REPORT) {
>>> > > + client->drop_pevent = false;
>>> > > + return true;
>>> > > + }
>>> > > }
>>> > >
>>> > > if (event->type == EV_SYN && event->code == SYN_REPORT) {
>>> > > client->packet_head = client->head;
>>> > > kill_fasync(&client->fasync, SIGIO, POLL_IN);
>>> > > }
>>> > > +
>>> > > + return false;
>>> > > }
>>> > >
>>> > > static void evdev_pass_values(struct evdev_client *client,
>>> > > @@ -284,10 +317,18 @@ static void evdev_pass_values(struct evdev_client *client,
>>> > > wakeup = true;
>>> > > }
>>> > >
>>> > > + /* drop partial events until SYN_REPORT occurs */
>>> > > + if (client->drop_pevent) {
>>> > > + if (v->type == EV_SYN && v->code == SYN_REPORT)
>>> > > + client->drop_pevent = false;
>>> > > + continue;
>>> > > + }
>>> > > +
>>> > > event.type = v->type;
>>> > > event.code = v->code;
>>> > > event.value = v->value;
>>> > > - __pass_event(client, &event);
>>> > > + if (__pass_event(client, &event))
>>> > > + wakeup = false;
>>> > > }
>>> > >
>>> > > spin_unlock(&client->buffer_lock);
>>> > > --
>>> > > 2.6.2
>>> > >
>>> >
>>> > --
>>> > Dmitry
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
