On Mon, Jan 4, 2016 at 2:22 PM, Benjamin Tissoires <benjamin.tissoi...@gmail.com> wrote: > On Mon, Jan 4, 2016 at 8:50 AM, Aniroop Mathur <a.mat...@samsung.com> wrote: >> On Jan 4, 2016 5:08 AM, "Peter Hutterer" <peter.hutte...@who-t.net> 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.mat...@samsung.com> >>> > > --- >>> > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html