[RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[John Stultz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 4302 bytes --]

Ok, hopefully getting close. Updated fixing the bool/int issue
Daniel pointed out. Let me know if you have any other thoughts!

thanks
-john

As noted by Arve and others since wall time can jump backwards, it
is difficult to use for input because one cannot determine if one
event occured before another or for how long a key was pressed.

However, the timestamp field is part of the kernel ABI, and cannot
be changed without possibly breaking existing users.

This patch adds a new IOCTL that allows a clockid to be set in
the evdev_client struct that will specify which time base to
use for event timestamps (ie: CLOCK_MONOTONIC instead
of CLOCK_REALTIME).

For now we only support CLOCK_MONOTONIC and CLOCK_REALTIME, but
in the future we could support other clockids if appropriate.

The default remains CLOCK_REALTIME, so we don't change the ABI.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Daniel Kurtz <djkurtz@google.com>
CC: linux-input@vger.kernel.org
CC: Arve Hjønnevåg <arve@android.com>
CC: Brian Swetland <swetland@google.com>
CC: Colin Cross <ccross@android.com>
CC: Dima Zavin <dima@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/input/evdev.c |   33 +++++++++++++++++++++++++++++----
 include/linux/input.h |    2 ++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 4cf2534..edabd9e 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -46,6 +46,7 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	int timestamp_clkid;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
@@ -54,8 +55,19 @@ static struct evdev *evdev_table[EVDEV_MINORS];
 static DEFINE_MUTEX(evdev_table_mutex);
 
 static void evdev_pass_event(struct evdev_client *client,
-			     struct input_event *event)
+			     struct input_event *event,
+			     ktime_t mono, ktime_t real)
 {
+	struct timespec ts;
+
+	if (client->timestamp_clkid == CLOCK_MONOTONIC)
+		ts = ktime_to_timespec(mono);
+	else
+		ts = ktime_to_timespec(real);
+	event->time.tv_sec = ts.tv_sec;
+	event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+
+
 	/* Interrupts are disabled, just acquire the lock. */
 	spin_lock(&client->buffer_lock);
 
@@ -94,8 +106,11 @@ static void evdev_event(struct input_handle *handle,
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;
 	struct input_event event;
+	ktime_t time_mono, time_real;
+
+	time_mono = ktime_get();
+	time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
 
-	do_gettimeofday(&event.time);
 	event.type = type;
 	event.code = code;
 	event.value = value;
@@ -103,11 +118,12 @@ static void evdev_event(struct input_handle *handle,
 	rcu_read_lock();
 
 	client = rcu_dereference(evdev->grab);
+
 	if (client)
-		evdev_pass_event(client, &event);
+		evdev_pass_event(client, &event, time_mono, time_real);
 	else
 		list_for_each_entry_rcu(client, &evdev->client_list, node)
-			evdev_pass_event(client, &event);
+			evdev_pass_event(client, &event, time_mono, time_real);
 
 	rcu_read_unlock();
 
@@ -683,6 +699,15 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCCLOCKID:
+		if (copy_from_user(&i, p, sizeof(unsigned int)))
+			return -EFAULT;
+		if ((i == CLOCK_MONOTONIC) || (i == CLOCK_REALTIME)) {
+			client->timestamp_clkid = i;
+			return 0;
+		}
+		return -EINVAL;
+
 	case EVIOCGKEYCODE:
 		return evdev_handle_get_keycode(dev, p);
 
diff --git a/include/linux/input.h b/include/linux/input.h
index 3862e32..9618e14 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -129,6 +129,8 @@ struct input_keymap_entry {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 
+#define EVIOCCLOCKID		_IOW('E', 0xA0, int)			/* Set clockid to be used for timestamps */
+
 /*
  * Device properties and quirks
  */
-- 
1.7.3.2.146.gca209

--
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: [RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[Daniel Kurtz]

On Sat, Jan 7, 2012 at 11:40 AM, John Stultz <john.stultz@linaro.org> wrote:
> Ok, hopefully getting close. Updated fixing the bool/int issue
> Daniel pointed out. Let me know if you have any other thoughts!
>
> thanks
> -john
>
> As noted by Arve and others since wall time can jump backwards, it
> is difficult to use for input because one cannot determine if one
> event occured before another or for how long a key was pressed.
>
> However, the timestamp field is part of the kernel ABI, and cannot
> be changed without possibly breaking existing users.
>
> This patch adds a new IOCTL that allows a clockid to be set in
> the evdev_client struct that will specify which time base to
> use for event timestamps (ie: CLOCK_MONOTONIC instead
> of CLOCK_REALTIME).
>
> For now we only support CLOCK_MONOTONIC and CLOCK_REALTIME, but
> in the future we could support other clockids if appropriate.
>
> The default remains CLOCK_REALTIME, so we don't change the ABI.
>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> CC: Daniel Kurtz <djkurtz@google.com>
> CC: linux-input@vger.kernel.org
> CC: Arve Hjønnevåg <arve@android.com>
> CC: Brian Swetland <swetland@google.com>
> CC: Colin Cross <ccross@android.com>
> CC: Dima Zavin <dima@android.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/input/evdev.c |   33 +++++++++++++++++++++++++++++----
>  include/linux/input.h |    2 ++
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 4cf2534..edabd9e 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -46,6 +46,7 @@ struct evdev_client {
>        struct fasync_struct *fasync;
>        struct evdev *evdev;
>        struct list_head node;
> +       int timestamp_clkid;
>        unsigned int bufsize;
>        struct input_event buffer[];
>  };
> @@ -54,8 +55,19 @@ static struct evdev *evdev_table[EVDEV_MINORS];
>  static DEFINE_MUTEX(evdev_table_mutex);
>
>  static void evdev_pass_event(struct evdev_client *client,
> -                            struct input_event *event)
> +                            struct input_event *event,
> +                            ktime_t mono, ktime_t real)
>  {
> +       struct timespec ts;
> +
> +       if (client->timestamp_clkid == CLOCK_MONOTONIC)
> +               ts = ktime_to_timespec(mono);
> +       else
> +               ts = ktime_to_timespec(real);
> +       event->time.tv_sec = ts.tv_sec;
> +       event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;

Maybe just:

 event->time = (client->timestamp_clkid == CLOCK_MONOTONIC) ?
                      ktime_to_timeval(mono) : ktime_to_timeval(real);

Either way, lgtm...

Reviewed-by: Daniel Kurtz <djkurtz@google.com>

> +
> +
>        /* Interrupts are disabled, just acquire the lock. */
>        spin_lock(&client->buffer_lock);
>
> @@ -94,8 +106,11 @@ static void evdev_event(struct input_handle *handle,
>        struct evdev *evdev = handle->private;
>        struct evdev_client *client;
>        struct input_event event;
> +       ktime_t time_mono, time_real;
> +
> +       time_mono = ktime_get();
> +       time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
>
> -       do_gettimeofday(&event.time);
>        event.type = type;
>        event.code = code;
>        event.value = value;
> @@ -103,11 +118,12 @@ static void evdev_event(struct input_handle *handle,
>        rcu_read_lock();
>
>        client = rcu_dereference(evdev->grab);
> +
>        if (client)
> -               evdev_pass_event(client, &event);
> +               evdev_pass_event(client, &event, time_mono, time_real);
>        else
>                list_for_each_entry_rcu(client, &evdev->client_list, node)
> -                       evdev_pass_event(client, &event);
> +                       evdev_pass_event(client, &event, time_mono, time_real);
>
>        rcu_read_unlock();
>
> @@ -683,6 +699,15 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>                else
>                        return evdev_ungrab(evdev, client);
>
> +       case EVIOCCLOCKID:
> +               if (copy_from_user(&i, p, sizeof(unsigned int)))
> +                       return -EFAULT;
> +               if ((i == CLOCK_MONOTONIC) || (i == CLOCK_REALTIME)) {
> +                       client->timestamp_clkid = i;
> +                       return 0;
> +               }
> +               return -EINVAL;
> +
>        case EVIOCGKEYCODE:
>                return evdev_handle_get_keycode(dev, p);
>
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 3862e32..9618e14 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -129,6 +129,8 @@ struct input_keymap_entry {
>
>  #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
>
> +#define EVIOCCLOCKID           _IOW('E', 0xA0, int)                    /* Set clockid to be used for timestamps */
> +
>  /*
>  * Device properties and quirks
>  */
> --
> 1.7.3.2.146.gca209
>
> --
> 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
--
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: [RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[Dmitry Torokhov]

On Sat, Jan 07, 2012 at 02:42:32PM +0800, Daniel Kurtz wrote:
> On Sat, Jan 7, 2012 at 11:40 AM, John Stultz <john.stultz@linaro.org> wrote:
> >  static void evdev_pass_event(struct evdev_client *client,
> > -                            struct input_event *event)
> > +                            struct input_event *event,
> > +                            ktime_t mono, ktime_t real)
> >  {
> > +       struct timespec ts;
> > +
> > +       if (client->timestamp_clkid == CLOCK_MONOTONIC)
> > +               ts = ktime_to_timespec(mono);
> > +       else
> > +               ts = ktime_to_timespec(real);
> > +       event->time.tv_sec = ts.tv_sec;
> > +       event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> 
> Maybe just:
> 
>  event->time = (client->timestamp_clkid == CLOCK_MONOTONIC) ?
>                       ktime_to_timeval(mono) : ktime_to_timeval(real);
> 
> Either way, lgtm...
> 
> Reviewed-by: Daniel Kurtz <djkurtz@google.com>
> 

I'm OK with the patch but do we actually have a buy-in from
Android/Chrome folks for this? If they will actually use it I'll apply
it, otherwise I'll sit on it.

Thanks.

-- 
Dmitry
--
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: [RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[Dmitry Torokhov]

On Mon, Jan 9, 2012 at 10:39 AM, Daniel Kurtz <djkurtz@google.com> wrote:
>
> On Jan 10, 2012 2:04 AM, "Dmitry Torokhov" <dmitry.torokhov@gmail.com>
> wrote:
>>
>> On Sat, Jan 07, 2012 at 02:42:32PM +0800, Daniel Kurtz wrote:
>> > On Sat, Jan 7, 2012 at 11:40 AM, John Stultz <john.stultz@linaro.org>
>> > wrote:
>> > >  static void evdev_pass_event(struct evdev_client *client,
>> > > -                            struct input_event *event)
>> > > +                            struct input_event *event,
>> > > +                            ktime_t mono, ktime_t real)
>> > >  {
>> > > +       struct timespec ts;
>> > > +
>> > > +       if (client->timestamp_clkid == CLOCK_MONOTONIC)
>> > > +               ts = ktime_to_timespec(mono);
>> > > +       else
>> > > +               ts = ktime_to_timespec(real);
>> > > +       event->time.tv_sec = ts.tv_sec;
>> > > +       event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>> >
>> > Maybe just:
>> >
>> >  event->time = (client->timestamp_clkid == CLOCK_MONOTONIC) ?
>> >                       ktime_to_timeval(mono) : ktime_to_timeval(real);
>> >
>> > Either way, lgtm...
>> >
>> > Reviewed-by: Daniel Kurtz <djkurtz@google.com>
>> >
>>
>> I'm OK with the patch but do we actually have a buy-in from
>> Android/Chrome folks for this? If they will actually use it I'll apply
>> it, otherwise I'll sit on it.
>
> Chrome/Chromium OS: yes.  That's me.  We would definitely like to start
> using this. But we haven't done so yet. If you want to sit on it, we could
> get back to you when we've tested it out.
>
> But it might not be for a little while, as I am suddenly very busy :)
>

Actually, I expect your productivity to skyrocket - suddenly you'll
have a few sleepless nights you'll have nothing else to do but code
when you not trying to calm her down ;)

-- 
Dmitry
--
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: [RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[Arve Hjønnevåg]

On Mon, Jan 9, 2012 at 10:04 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sat, Jan 07, 2012 at 02:42:32PM +0800, Daniel Kurtz wrote:
>> On Sat, Jan 7, 2012 at 11:40 AM, John Stultz <john.stultz@linaro.org> wrote:
>> >  static void evdev_pass_event(struct evdev_client *client,
>> > -                            struct input_event *event)
>> > +                            struct input_event *event,
>> > +                            ktime_t mono, ktime_t real)
>> >  {
>> > +       struct timespec ts;
>> > +
>> > +       if (client->timestamp_clkid == CLOCK_MONOTONIC)
>> > +               ts = ktime_to_timespec(mono);
>> > +       else
>> > +               ts = ktime_to_timespec(real);
>> > +       event->time.tv_sec = ts.tv_sec;
>> > +       event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>>
>> Maybe just:
>>
>>  event->time = (client->timestamp_clkid == CLOCK_MONOTONIC) ?
>>                       ktime_to_timeval(mono) : ktime_to_timeval(real);
>>
>> Either way, lgtm...
>>
>> Reviewed-by: Daniel Kurtz <djkurtz@google.com>
>>
>
> I'm OK with the patch but do we actually have a buy-in from
> Android/Chrome folks for this? If they will actually use it I'll apply
> it, otherwise I'll sit on it.
>

Yes, this seems reasonable to me.

-- 
Arve Hjønnevåg
--
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

[RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[John Stultz]

Here's another revision, incorperating Dmitry's suggestion.

As noted by Arve and others, since wall time can jump backwards, it
is difficult to use for input because one cannot determine if one
event occured before another or for how long a key was pressed.

However, the timestamp field is part of the kernel ABI, and cannot
be changed without possibly breaking existing users.

This patch adds a new IOCTL that allows a clockid to be set in
the evdev_client struct that will specify which time base to
use for event timestamps (ie: CLOCK_MONOTONIC instead
of CLOCK_REALTIME).

For now we only support CLOCK_MONOTONIC and CLOCK_REALTIME, but
in the future we could support other clockids if appropriate.

The default remains CLOCK_REALTIME, so we don't change the ABI.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Daniel Kurtz <djkurtz@google.com>
CC: linux-input@vger.kernel.org
CC: Arve Hjønnevåg <arve@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/input/evdev.c |   33 +++++++++++++++++++++++++++++----
 include/linux/input.h |    2 ++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 4cf2534..4b71484 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -46,6 +46,7 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	bool timestamp_clkid;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
@@ -54,8 +55,19 @@ static struct evdev *evdev_table[EVDEV_MINORS];
 static DEFINE_MUTEX(evdev_table_mutex);
 
 static void evdev_pass_event(struct evdev_client *client,
-			     struct input_event *event)
+			     struct input_event *event,
+			     ktime_t mono, ktime_t real)
 {
+	struct timespec ts;
+
+	if (client->timestamp_clkid == CLOCK_MONOTONIC)
+		ts = ktime_to_timespec(mono);
+	else
+		ts = ktime_to_timespec(real);
+	event->time.tv_sec = ts.tv_sec;
+	event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+
+
 	/* Interrupts are disabled, just acquire the lock. */
 	spin_lock(&client->buffer_lock);
 
@@ -94,8 +106,11 @@ static void evdev_event(struct input_handle *handle,
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;
 	struct input_event event;
+	ktime_t time_mono, time_real;
+
+	time_mono = ktime_get();
+	time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
 
-	do_gettimeofday(&event.time);
 	event.type = type;
 	event.code = code;
 	event.value = value;
@@ -103,11 +118,12 @@ static void evdev_event(struct input_handle *handle,
 	rcu_read_lock();
 
 	client = rcu_dereference(evdev->grab);
+
 	if (client)
-		evdev_pass_event(client, &event);
+		evdev_pass_event(client, &event, time_mono, time_real);
 	else
 		list_for_each_entry_rcu(client, &evdev->client_list, node)
-			evdev_pass_event(client, &event);
+			evdev_pass_event(client, &event, time_mono, time_real);
 
 	rcu_read_unlock();
 
@@ -683,6 +699,15 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCCLOCKID:
+		if (copy_from_user(&i, p, sizeof(unsigned int)))
+			return -EFAULT;
+		if ((i == CLOCK_MONOTONIC) || (i == CLOCK_REALTIME)) {
+			client->timestamp_clkid = i;
+			return 0;
+		}
+		return -EINVAL;
+
 	case EVIOCGKEYCODE:
 		return evdev_handle_get_keycode(dev, p);
 
diff --git a/include/linux/input.h b/include/linux/input.h
index 3862e32..9618e14 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -129,6 +129,8 @@ struct input_keymap_entry {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 
+#define EVIOCCLOCKID		_IOW('E', 0xA0, int)			/* Set clockid to be used for timestamps */
+
 /*
  * Device properties and quirks
  */
-- 
1.7.3.2.146.gca209

--
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: [RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[Daniel Kurtz]

On Fri, Jan 6, 2012 at 10:50 AM, John Stultz <john.stultz@linaro.org> wrote:
> Here's another revision, incorperating Dmitry's suggestion.
>
> As noted by Arve and others, since wall time can jump backwards, it
> is difficult to use for input because one cannot determine if one
> event occured before another or for how long a key was pressed.
>
> However, the timestamp field is part of the kernel ABI, and cannot
> be changed without possibly breaking existing users.
>
> This patch adds a new IOCTL that allows a clockid to be set in
> the evdev_client struct that will specify which time base to
> use for event timestamps (ie: CLOCK_MONOTONIC instead
> of CLOCK_REALTIME).
>
> For now we only support CLOCK_MONOTONIC and CLOCK_REALTIME, but
> in the future we could support other clockids if appropriate.

What about CLOCK_MONOTONIC_RAW?

Last time we discussed, I thought this clock was the most useful for
use with input devices.  But, you wrote this:
> So rawmonotonic isn't frequency corrected via NTP, while the monotonic
> clock is. So if you're calculating intervals, you will get more accurate
> times (where a second is a second) w/ ktime_get_ts().

Does this frequency correction involve timestamp jumps?
If so, of what magnitude?
In my experience, input event timestamp intervals are usually on the
order of a few milliseconds (5-25 ms).  If CLOCK_MONOTONIC experiences
frequent adjustments near this order of magnitude, I still think
CLOCK_MONOTONIC_RAW might be a better choice for event timestamps.

>
> The default remains CLOCK_REALTIME, so we don't change the ABI.
>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> CC: Daniel Kurtz <djkurtz@google.com>
> CC: linux-input@vger.kernel.org
> CC: Arve Hjønnevåg <arve@android.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/input/evdev.c |   33 +++++++++++++++++++++++++++++----
>  include/linux/input.h |    2 ++
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 4cf2534..4b71484 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -46,6 +46,7 @@ struct evdev_client {
>        struct fasync_struct *fasync;
>        struct evdev *evdev;
>        struct list_head node;
> +       bool timestamp_clkid;

This isn't bool anymore.

>        unsigned int bufsize;
>        struct input_event buffer[];
>  };
> @@ -54,8 +55,19 @@ static struct evdev *evdev_table[EVDEV_MINORS];
>  static DEFINE_MUTEX(evdev_table_mutex);
>
>  static void evdev_pass_event(struct evdev_client *client,
> -                            struct input_event *event)
> +                            struct input_event *event,
> +                            ktime_t mono, ktime_t real)
>  {
> +       struct timespec ts;
> +
> +       if (client->timestamp_clkid == CLOCK_MONOTONIC)
> +               ts = ktime_to_timespec(mono);
> +       else
> +               ts = ktime_to_timespec(real);
> +       event->time.tv_sec = ts.tv_sec;
> +       event->time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> +
> +
>        /* Interrupts are disabled, just acquire the lock. */
>        spin_lock(&client->buffer_lock);
>
> @@ -94,8 +106,11 @@ static void evdev_event(struct input_handle *handle,
>        struct evdev *evdev = handle->private;
>        struct evdev_client *client;
>        struct input_event event;
> +       ktime_t time_mono, time_real;
> +
> +       time_mono = ktime_get();
> +       time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
>
> -       do_gettimeofday(&event.time);
>        event.type = type;
>        event.code = code;
>        event.value = value;
> @@ -103,11 +118,12 @@ static void evdev_event(struct input_handle *handle,
>        rcu_read_lock();
>
>        client = rcu_dereference(evdev->grab);
> +
>        if (client)
> -               evdev_pass_event(client, &event);
> +               evdev_pass_event(client, &event, time_mono, time_real);
>        else
>                list_for_each_entry_rcu(client, &evdev->client_list, node)
> -                       evdev_pass_event(client, &event);
> +                       evdev_pass_event(client, &event, time_mono, time_real);
>
>        rcu_read_unlock();
>
> @@ -683,6 +699,15 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>                else
>                        return evdev_ungrab(evdev, client);
>
> +       case EVIOCCLOCKID:
> +               if (copy_from_user(&i, p, sizeof(unsigned int)))
> +                       return -EFAULT;
> +               if ((i == CLOCK_MONOTONIC) || (i == CLOCK_REALTIME)) {
> +                       client->timestamp_clkid = i;
> +                       return 0;
> +               }
> +               return -EINVAL;
> +
>        case EVIOCGKEYCODE:
>                return evdev_handle_get_keycode(dev, p);
>
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 3862e32..9618e14 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -129,6 +129,8 @@ struct input_keymap_entry {
>
>  #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
>
> +#define EVIOCCLOCKID           _IOW('E', 0xA0, int)                    /* Set clockid to be used for timestamps */
> +
>  /*
>  * Device properties and quirks
>  */
> --
> 1.7.3.2.146.gca209
>

Thanks,
-Daniel
--
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: [RFC][PATCH] Input: Add infrastrucutre for selecting clockid for event time stamps.

[John Stultz]

On Fri, 2012-01-06 at 17:20 +0800, Daniel Kurtz wrote:
> On Fri, Jan 6, 2012 at 10:50 AM, John Stultz <john.stultz@linaro.org> wrote:
> > Here's another revision, incorperating Dmitry's suggestion.
> >
> > As noted by Arve and others, since wall time can jump backwards, it
> > is difficult to use for input because one cannot determine if one
> > event occured before another or for how long a key was pressed.
> >
> > However, the timestamp field is part of the kernel ABI, and cannot
> > be changed without possibly breaking existing users.
> >
> > This patch adds a new IOCTL that allows a clockid to be set in
> > the evdev_client struct that will specify which time base to
> > use for event timestamps (ie: CLOCK_MONOTONIC instead
> > of CLOCK_REALTIME).
> >
> > For now we only support CLOCK_MONOTONIC and CLOCK_REALTIME, but
> > in the future we could support other clockids if appropriate.
> 
> What about CLOCK_MONOTONIC_RAW?

It could be used here, although I'm still not clear on the benefit of
using monotonic raw over just monotonic.

> Last time we discussed, I thought this clock was the most useful for
> use with input devices.  But, you wrote this:
> > So rawmonotonic isn't frequency corrected via NTP, while the monotonic
> > clock is. So if you're calculating intervals, you will get more accurate
> > times (where a second is a second) w/ ktime_get_ts().
> 
> Does this frequency correction involve timestamp jumps?

No. clock monotonic isn't jumped. But its rate may be slightly adjusted
(max of +/- 500ppm), so that it accurately matches the passing of time. 

> If so, of what magnitude?

At worse, assuming some sort of terribly mis-configured or malicious ntp
daemon, if you had two one second intervals that you had measured, they
could actually differ by up to a millisecond. 

> In my experience, input event timestamp intervals are usually on the
> order of a few milliseconds (5-25 ms).  If CLOCK_MONOTONIC experiences
> frequent adjustments near this order of magnitude, I still think
> CLOCK_MONOTONIC_RAW might be a better choice for event timestamps.

So for 5-25 ms intervals, you're looking at a worse case difference of
5-25 us. And again, this isn't likely, as the ntp freq adjustment would
have to go from -500ppm to +500ppm mid-interval. 

So I really suspect there isn't much practical difference between
CLOCK_MONOTONIC and CLOCK_MONOTONIC_RAW for input events. But I'd be
interested to hear if anyone has actually run into such complications.

CLOCK_MONOTONIC_RAW can be useful (especially for providing a stable
baseline when doing time adjustment calculations), but the downside is
that measurements (especially longer measurements) with
CLOCK_MONOTONIC_RAW may not be accurate.

And as it always is with time, everything is relative: even
CLOCK_MONOTONIC_RAW can vary over time, as the crystal driving the
clocksource may slightly fluctuate with temperature.

But again, nothing is keeping CLOCK_MONOTONIC_RAW from being supported
via the proposed ioctl.

> > +       bool timestamp_clkid;
> 
> This isn't bool anymore.

Good catch!

thanks!
-john