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

[Daniel Kurtz <djkurtz@xxxxxxxxxx>]

On Fri, Jan 6, 2012 at 10:50 AM, John Stultz <john.stultz@xxxxxxxxxx> 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@xxxxxxxxx>
> CC: Daniel Kurtz <djkurtz@xxxxxxxxxx>
> CC: linux-input@xxxxxxxxxxxxxxx
> CC: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html