Re: [PATCH] input: evdev: Add a read() callback

Re: [PATCH] input: evdev: Add a read() callback

[Mark Brown]

On Mon, Feb 21, 2011 at 08:18:50AM -0600, Bill Gatliff wrote:

> But implementing a rate-specifying sysfs attribute isn't enough,
> however.  Under the Linux kernel's current input device buffering
> scheme, drivers and applications can create and consume input events
> every 100ms, for example, but there is always the possibility that an
> input event will sit in the queue for 99ms waiting for an application
> to retrieve it.  For devices and applications where this potential
> delay causes problems, drivers must implement an external means for
> applications to trigger the timely production of an input event.

I don't see how any of the above shows an issue with rate limiting in
the application image.  As was discussed in some detail the last time
you posted this the issue that's causing the applications to experience
delayed notifications of events is that rather than blocking waiting for
an event to be delivered to them they're sleeping unconditionally.  If
the application were to use a rate control in conjunction with blocking
(which is the expected programming model) then a rate control will do
exactly what's expected.

> The enclosed patch address all of the above problems by implementing
> an advisory callback from the evdev read() and poll() methods to the
> associated input device driver.  The driver may then choose to
> populate the input event buffer at that time, rather than on a
> schedule implemented by a polling loop, sysfs trigger, or other means.

> Use of this callback by a driver naturally synchronizes the generation
> of input events to requests from userspace, because the driver now
> "knows" when userspace is attempting to retrieve an input event and
> can therefore produce one just-in-time.  It also allows the driver to
> easily match the rate of input event generation, by simply sampling the
> hardware only during this callback.

This doesn't address the issue raised in the previous discussion with
the poor interaction with applications that do behave sensibly and block
for events from the device - either we can't use the ability to trigger
readings from the hardware or we end up doing extra readings because
every read triggers an additional sample.

> If an input device driver chooses to use only the read() callback as
> its signal to produce an input event, then the driver need not
> implement a polling kernel thread or other means of pacing its event
> generation rate.  The driver also has no need to provide a sysfs
> attribute to allow userspace to request a polling rate or to trigger a
> measurement: userspace must only read() or poll() the interface at the
> desired rate.  This can greatly simplify input device driver
> implementation, while conveniently leaving the incoming event rate and

This is all stuff that can be put into a library (parts of it are
already) so there's no real cost to drivers from implementing anything.

> Finally, input device drivers might choose to implement a holdoff
> timer that gets reset in the read() callback; expiration of this timer
> would mean that userspace is no longer actively reading from the
> device, even though the interface itself might still be open.  In such
> cases the driver might wish to invoke a power-management method to
> idle the hardware until the next  callback occurs.

This will again interact poorly with event driven applications - if
notifications to userspace don't happen (eg, because there has been no
change) then there will be no cause for the application to read.

I can see a use for a read on demand callback in the implementation of
polling (eg, if rate is set to zero or for the core polling library to
use) but the userspace API thats being proposed here seems like it's
going to cause problems for applications that try to do the right thing.

Re: [PATCH] input: evdev: Add a read() callback

[Bill Gatliff]

Mark:


Thanks for the great feedback!

In a nutshell, I agree with your summary stating that my proposal "is
going to cause problems for applications that try to do the right
thing".  Were my suggestion to be adopted immediately and across the
board, things would turn hairy indeed!

I'm actually focused on a very narrow use case, however:
accelerometers, compasses, polled switches, and similar hardware.  I'm
wanting to propose a common method for dealing with these, as an
alternative to the raft of incompatible approaches that are currently
taking hold.  And in addition to avoiding the inconsistency of these
implementations across devices, I also want to eliminate as much
redundancy in the implementations as possible.

A good case in point comes up when someone moves an Android
implementation from one platform to another.  No two accelerometer
drivers seem to agree on how to specify the desired event generation
rate, so the migration effort is increased.  And the work is divided
between both kernel and userspace: the former in its mandate to
provide a rate-request interface, and the latter to utilize that
interface.  I really don't see the point in this.

Granted, on all of MY platforms I can go ahead and implement this
read-callback idea, and so far it has proven interesting and helpful.
I'm just thinking that my success could appeal to a wider audience.
Instead of all of us creating and repeating the same work over and
over again, let's agree on SOMETHING.  Mine is of course a
somewhat-different way of solving the problem, but not without merit.

As I mention in my commit log/essay, devices like USB keyboards and
other event-driven hardware have no use for my read callback idea, and
can safely ignore it.  And I don't see a day on the horizon where
anyone will plug an accelerometer into X and expect X to deal with it
as properly as it does today for keyboards and mice.  I don't think
the "do the right thing" applications you are referring to will be
affected by the read callback at all, since the callback doesn't apply
to ordinary, "do the right thing"-type situations.

More responses inline below:


On Mon, Feb 21, 2011 at 6:23 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> I don't see how any of the above shows an issue with rate limiting in
> the application image.  As was discussed in some detail the last time
> you posted this the issue that's causing the applications to experience
> delayed notifications of events is that rather than blocking waiting for
> an event to be delivered to them they're sleeping unconditionally.

That is definitely one source of problems.

But if the accelerometer measurement has to be synchronized with some
other event, things get a little trickier.  At the moment my concerns
for this are theoretical, but I can see some situations coming soon
where the theory could be usefully put to practice.


> If the application were to use a rate control in conjunction with blocking
> (which is the expected programming model) then a rate control will do
> exactly what's expected.

No argument with that.  My main complaint is that everyone is doing
rate control APIs differently.  I'm proposing a way to standardize
them by eliminating them altogether.  A side-effect of my idea is that
applications get complete control over event generation.  Many of them
won't care about that, but some will.

> This doesn't address the issue raised in the previous discussion with
> the poor interaction with applications that do behave sensibly and block
> for events from the device - either we can't use the ability to trigger
> readings from the hardware or we end up doing extra readings because
> every read triggers an additional sample.

I recognize your concern.  But I don't think that the applications
which could be confused by the behavior I'm proposing would be the
same applications that would take advantage of it.  Or, perhaps, such
applications might welcome eliminating the code needed to support the
different rate-limiting interfaces of a half-dozen or more devices
that they claim compatibility with.

Besides, it isn't like my proposal would be forced upon any of the
existing mainline drivers.  Most of them, like keyboards and mice,
couldn't use it anyway.

> This is all stuff that can be put into a library (parts of it are
> already) so there's no real cost to drivers from implementing anything.

... except the redundancy of everyone doing it slightly differently.
Why not settle on a common approach, so the library you mention isn't
necessary at all?

> This will again interact poorly with event driven applications - if
> notifications to userspace don't happen (eg, because there has been no
> change) then there will be no cause for the application to read.

Agreed.  But those situations don't describe users of accelerometers
and compasses.  At least not in the code/situations I have
encountered.

> I can see a use for a read on demand callback in the implementation of
> polling (eg, if rate is set to zero or for the core polling library to
> use) but the userspace API thats being proposed here seems like it's
> going to cause problems for applications that try to do the right thing.

Such applications probably aren't a factor here.


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] input: evdev: Add a read() callback

[Mark Brown]

On Mon, Feb 21, 2011 at 10:07:12PM -0600, Bill Gatliff wrote:

> A good case in point comes up when someone moves an Android
> implementation from one platform to another.  No two accelerometer
> drivers seem to agree on how to specify the desired event generation
> rate, so the migration effort is increased.  And the work is divided
> between both kernel and userspace: the former in its mandate to
> provide a rate-request interface, and the latter to utilize that
> interface.  I really don't see the point in this.

But surely the most obvious solution here is to standardise a rate
control interface?  Anything else means we need a completely different
implementation for hardware which does have the ability to schedule its
own conversions.

> As I mention in my commit log/essay, devices like USB keyboards and
> other event-driven hardware have no use for my read callback idea, and
> can safely ignore it.  And I don't see a day on the horizon where
> anyone will plug an accelerometer into X and expect X to deal with it
> as properly as it does today for keyboards and mice.  I don't think
> the "do the right thing" applications you are referring to will be
> affected by the read callback at all, since the callback doesn't apply
> to ordinary, "do the right thing"-type situations.

The problem you're trying to solve is also an issue for really common
and standard things like touchscreens and polled switches/keys (the
latter of which you mentioned in your mail) which are used by standard
applications.

> But if the accelerometer measurement has to be synchronized with some
> other event, things get a little trickier.  At the moment my concerns
> for this are theoretical, but I can see some situations coming soon
> where the theory could be usefully put to practice.

Have you looked at the IIO subsystem for things like this?  There has
been talk of putting accelerometers in there and it certainly fits in
with the sync requirements you're mentioning.

> > If the application were to use a rate control in conjunction with blocking
> > (which is the expected programming model) then a rate control will do
> > exactly what's expected.

> No argument with that.  My main complaint is that everyone is doing
> rate control APIs differently.  I'm proposing a way to standardize
> them by eliminating them altogether.  A side-effect of my idea is that
> applications get complete control over event generation.  Many of them
> won't care about that, but some will.

The problem with your proposal as it stands is that if they do use the
new interface they interact badly with existing applications.  This
isn't really a decision the driver should be taking, it needs to be
userspace policy if it's going to be a decision at all.

> I recognize your concern.  But I don't think that the applications
> which could be confused by the behavior I'm proposing would be the
> same applications that would take advantage of it.  Or, perhaps, such
> applications might welcome eliminating the code needed to support the
> different rate-limiting interfaces of a half-dozen or more devices
> that they claim compatibility with.

Again, you appear to be assuming that there are many rate limiting
interfaces.  Surely the solution to an excess of rate limit interfaces
is to standardise the rate limiting interfaces rather than invent a new
interface, especially one that interoperates poorly with blocking apps?

> > This is all stuff that can be put into a library (parts of it are
> > already) so there's no real cost to drivers from implementing anything.

> ... except the redundancy of everyone doing it slightly differently.
> Why not settle on a common approach, so the library you mention isn't
> necessary at all?

Note that I'm talking about in-kernel code here, not a linked library in
userspace.  One way or another *something* is going to have to provide
the scheduling support.

Re: [PATCH] input: evdev: Add a read() callback

[Jonathan Cameron]

On 02/22/11 04:33, Mark Brown wrote:
> On Mon, Feb 21, 2011 at 10:07:12PM -0600, Bill Gatliff wrote:
> 
>> A good case in point comes up when someone moves an Android
>> implementation from one platform to another.  No two accelerometer
>> drivers seem to agree on how to specify the desired event generation
>> rate, so the migration effort is increased.  And the work is divided
>> between both kernel and userspace: the former in its mandate to
>> provide a rate-request interface, and the latter to utilize that
>> interface.  I really don't see the point in this.
> 
> But surely the most obvious solution here is to standardise a rate
> control interface?  Anything else means we need a completely different
> implementation for hardware which does have the ability to schedule its
> own conversions.
I agree that standardizing would be a good idea.  Note that far from all
accelerometers act as 'polled' devices.  Most I've encountered have their
own sampling clocks and in many cases these can be set pretty low, saving
power and sometimes applying filters to give lower noise on the output.

For these I don't think the approach given in this thread makes sense.
It's more promising for those devices that are using an in driver
polling thread that actually initializes the sampling (more common
in slow devices like magnetometers).  Still, whilst I initially liked
the idea, the whole point about it interoperating badly with apps that
are event driven is clearly a big issue...
> 
>> As I mention in my commit log/essay, devices like USB keyboards and
>> other event-driven hardware have no use for my read callback idea, and
>> can safely ignore it.  And I don't see a day on the horizon where
>> anyone will plug an accelerometer into X and expect X to deal with it
>> as properly as it does today for keyboards and mice.  I don't think
>> the "do the right thing" applications you are referring to will be
>> affected by the read callback at all, since the callback doesn't apply
>> to ordinary, "do the right thing"-type situations.
> 
> The problem you're trying to solve is also an issue for really common
> and standard things like touchscreens and polled switches/keys (the
> latter of which you mentioned in your mail) which are used by standard
> applications.
> 
>> But if the accelerometer measurement has to be synchronized with some
>> other event, things get a little trickier.  At the moment my concerns
>> for this are theoretical, but I can see some situations coming soon
>> where the theory could be usefully put to practice.
> 
> Have you looked at the IIO subsystem for things like this?  There has
> been talk of putting accelerometers in there and it certainly fits in
> with the sync requirements you're mentioning.
Hmm. We still need to do more work on this to get it to the level I'd like.
To give background info:

A common reason for wanting this is Inertial Measurement Units (combination
of typically accelerometers, gyros and magnetometers) made up of a number
of discrete parts.  Some of these will run on their own internal clocks, so
not much we can do about them, but others as for the devices you are considering
here will sample based on a trigger pin, command over bus, or the bus read
itself.

IIO handles this via 'triggers'.  These are provided either by hardware
(data ready signals from chips, or gpio pins), timers or userspace triggering.
Any trigger can have as many separate poll functions attached as you like,
allowing the 'trigger event' to cause data to be captured from lots of different
sensors.  There are two queues, one for hammering capture pins as fast as possible
and one for doing slower bus based reads.  Aim is to get all the captures as close
as possible together subject to constraints of the actual board.

Things we don't handle well:
* delays.  Some devices have deterministic capture delays. Ideally we would take
these into account though it isn't entirely clear what the desired behaviour actually
is.
* Triggering based on the data ready signal of a part that isn't attached to the
same trigger.  This is fiddly and right now you can end up with the trigger not being
cleared and hence only firing once.
* Triggering form more esoteric events. E.g. Grab values from one device when another
passes some threshold.  However these will only be added if anyone actually has a use
case and even then will require support in the particular parts driver.

Still, it's a work in progress...
> 
>>> If the application were to use a rate control in conjunction with blocking
>>> (which is the expected programming model) then a rate control will do
>>> exactly what's expected.
> 
>> No argument with that.  My main complaint is that everyone is doing
>> rate control APIs differently.  I'm proposing a way to standardize
>> them by eliminating them altogether.  A side-effect of my idea is that
>> applications get complete control over event generation.  Many of them
>> won't care about that, but some will.
> 
> The problem with your proposal as it stands is that if they do use the
> new interface they interact badly with existing applications.  This
> isn't really a decision the driver should be taking, it needs to be
> userspace policy if it's going to be a decision at all.
> 
>> I recognize your concern.  But I don't think that the applications
>> which could be confused by the behavior I'm proposing would be the
>> same applications that would take advantage of it.  Or, perhaps, such
>> applications might welcome eliminating the code needed to support the
>> different rate-limiting interfaces of a half-dozen or more devices
>> that they claim compatibility with.
> 
> Again, you appear to be assuming that there are many rate limiting
> interfaces.  Surely the solution to an excess of rate limit interfaces
> is to standardise the rate limiting interfaces rather than invent a new
> interface, especially one that interoperates poorly with blocking apps?
> 
>>> This is all stuff that can be put into a library (parts of it are
>>> already) so there's no real cost to drivers from implementing anything.
> 
>> ... except the redundancy of everyone doing it slightly differently.
>> Why not settle on a common approach, so the library you mention isn't
>> necessary at all?
> 
> Note that I'm talking about in-kernel code here, not a linked library in
> userspace.  One way or another *something* is going to have to provide
> the scheduling support.

Re: [PATCH] input: evdev: Add a read() callback

[Bill Gatliff]

Jonathan:

On Tue, Feb 22, 2011 at 7:13 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> Hmm. We still need to do more work on this to get it to the level I'd like.

I've been looking at the iio code some.  Honestly, I don't grok it at
all yet.  :)


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] input: evdev: Add a read() callback

[Bill Gatliff]

Mark:

On Mon, Feb 21, 2011 at 10:33 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> But surely the most obvious solution here is to standardise a rate
> control interface?

Yes, and no.  A standardized rate control interface would deal with
the rate control problem, but leave the synchronization problem
unsolved.

> The problem you're trying to solve is also an issue for really common
> and standard things like touchscreens and polled switches/keys (the
> latter of which you mentioned in your mail) which are used by standard
> applications.

The existing "polled switch" implementation sets up a throttled
polling loop in kernel code.  The "polled switch" that I was thinking
of when I wrote the essay for the commit was "a switch that is polled
each time read() gets called".  I should have been clearer--- and
probably picked a different name.  :)

> Have you looked at the IIO subsystem for things like this?  There has
> been talk of putting accelerometers in there and it certainly fits in
> with the sync requirements you're mentioning.

Now that I have found the code, I'm looking at it.  Can't say yet
whether it's the solution I'm seeking, or not.

> The problem with your proposal as it stands is that if they do use the
> new interface they interact badly with existing applications.  This
> isn't really a decision the driver should be taking, it needs to be
> userspace policy if it's going to be a decision at all.

Yea, I'm agreeing with you more than I was at the beginning of this thread.


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] input: evdev: Add a read() callback

[Mark Brown]

On Fri, Feb 25, 2011 at 08:46:24PM -0600, Bill Gatliff wrote:
> On Mon, Feb 21, 2011 at 10:33 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:

> > But surely the most obvious solution here is to standardise a rate
> > control interface?

> Yes, and no.  A standardized rate control interface would deal with
> the rate control problem, but leave the synchronization problem
> unsolved.

The main source of the problem with delayed readings was that
applications weren't blocking waiting for events.  If applications block
waiting for events then the data will be delivered to them promptly.
For cases where multiple readings need to be synced IIO looks like the
way forward.

> > The problem you're trying to solve is also an issue for really common
> > and standard things like touchscreens and polled switches/keys (the
> > latter of which you mentioned in your mail) which are used by standard
> > applications.

> The existing "polled switch" implementation sets up a throttled
> polling loop in kernel code.  The "polled switch" that I was thinking
> of when I wrote the essay for the commit was "a switch that is polled
> each time read() gets called".  I should have been clearer--- and
> probably picked a different name.  :)

It's the same thing, though - I'd really expect it to be handled by the
same code in kernel.

[PATCH] input: evdev: Add a read() callback

[Bill Gatliff]

Some devices, like accelerometers, can manufacture as many input
events as userspace can consume.  The traditional method for limiting
the number of input events sent to userspace by such devices is to use
a kernel thread looping around an idle timer, to "throttle" the
creation and queuing of input events to a manageable rate.  Such
drivers often also provide a sysfs attribute (or similar means) that
allows userspace to indicate a desired event rate.

Some operating systems and/or applications e.g. Android, might prefer
to control input event creation rates themselves.  They frequently
attempt to do so by implementing a userspace polling thread that loops
around an idle timer; this idle timer throttles the rate of calls to
the input devices' read() method, which (at least in theory) limits
the rate of input events arriving at the application to a desired
level.

In practice, you want to use only one of the two aforementioned
approaches; if you try to do both, you'll get event buffer overflows,
weird behaviors, and delay stackups as the two timers apply their rate
limiting throttles against each other.

Userspace throttling of input events doesn't work very well with the
Linux kernel's current input event buffering scheme, because there is
no direct connection between the read() system call and the driver for
the input device being queried.  Put simply, current input device
drivers simply cannot tell when--- or even if--- userspace is polling
for an input event.

But implementing a rate-specifying sysfs attribute isn't enough,
however.  Under the Linux kernel's current input device buffering
scheme, drivers and applications can create and consume input events
every 100ms, for example, but there is always the possibility that an
input event will sit in the queue for 99ms waiting for an application
to retrieve it.  For devices and applications where this potential
delay causes problems, drivers must implement an external means for
applications to trigger the timely production of an input event.

The enclosed patch address all of the above problems by implementing
an advisory callback from the evdev read() and poll() methods to the
associated input device driver.  The driver may then choose to
populate the input event buffer at that time, rather than on a
schedule implemented by a polling loop, sysfs trigger, or other means.

Use of this callback by a driver naturally synchronizes the generation
of input events to requests from userspace, because the driver now
"knows" when userspace is attempting to retrieve an input event and
can therefore produce one just-in-time.  It also allows the driver to
easily match the rate of input event generation, by simply sampling the
hardware only during this callback.

If an input device driver chooses to use only the read() callback as
its signal to produce an input event, then the driver need not
implement a polling kernel thread or other means of pacing its event
generation rate.  The driver also has no need to provide a sysfs
attribute to allow userspace to request a polling rate or to trigger a
measurement: userspace must only read() or poll() the interface at the
desired rate.  This can greatly simplify input device driver
implementation, while conveniently leaving the incoming event rate and
synchronization issues completely up to the application. (See POSIX.1b
interval timers and scheduler options for APIs allowing precise timing
within user applications).

Finally, input device drivers might choose to implement a holdoff
timer that gets reset in the read() callback; expiration of this timer
would mean that userspace is no longer actively reading from the
device, even though the interface itself might still be open.  In such
cases the driver might wish to invoke a power-management method to
idle the hardware until the next  callback occurs.

Polled devices like accelerometers and compasses (and perhaps some
types of pushbuttons or switch inputs) will get the most benefit from
utilizing the read() callback.  Other devices, like USB keyboards and
mice, will have no use for it and may safely ignore it.

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 drivers/input/evdev.c |   10 ++++++++++
 include/linux/input.h |    1 +
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c8471a2..b49b601 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -370,12 +370,19 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
 {
 	struct evdev_client *client = file->private_data;
 	struct evdev *evdev = client->evdev;
+	struct input_dev *dev = evdev->handle.dev;
 	struct input_event event;
 	int retval;
 
 	if (count < input_event_size())
 		return -EINVAL;
 
+	if (client->head == client->tail && dev->read) {
+		retval = dev->read(dev);
+		if (retval)
+			return retval;
+	}
+
 	if (client->head == client->tail && evdev->exist &&
 	    (file->f_flags & O_NONBLOCK))
 		return -EAGAIN;
@@ -407,6 +414,9 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
 	struct evdev *evdev = client->evdev;
 	unsigned int mask;
 
+	if (client->head == client->tail && dev->read)
+		dev->read(dev);
+
 	poll_wait(file, &evdev->wait, wait);
 
 	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
diff --git a/include/linux/input.h b/include/linux/input.h
index e428382..eac7f77 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -1264,6 +1264,7 @@ struct input_dev {
 
 	int (*open)(struct input_dev *dev);
 	void (*close)(struct input_dev *dev);
+	int (*read)(struct input_dev *dev);
 	int (*flush)(struct input_dev *dev, struct file *file);
 	int (*event)(struct input_dev *dev, unsigned int type, unsigned int code, int value);
 
-- 
1.7.2.3