Re: [PATCH 7/7] [RFC] [media] introduce v4l2_timespec type for timestamps

[Hans Verkuil <hverkuil@xs4all.nl>]

On 09/15/2015 05:49 PM, Arnd Bergmann wrote:
> The v4l2 event queue uses a 'struct timespec' to pass monotonic
> timestamps. This is not a problem by itself, but it breaks when user
> space redefines timespec to use 'long long' on 32-bit systems.
> 
> To avoid that problem, we define our own replacement for timespec here,
> using 'long' tv_sec and tv_nsec members. This means no change to the
> existing API, but lets us keep using it with a y2038 compatible libc.
> 
> As a side-effect, this changes the API for x32 programs to be the same
> as native 32-bit, using a 32-bit tv_sec/tv_nsec value, but both old and
> new kernels already support both formats for on the binary level for
> compat and x32.
> 
> Alternatively, we could leave the header file to define the interface
> based on 'struct timespec', and implement both ioctls for native
> processes. I picked the approach in this case because it matches what
> we do for v4l2_timeval in the respective patch, but both would work
> equally well here.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/v4l2-core/v4l2-event.c | 20 +++++++++++++-------
>  include/uapi/linux/videodev2.h       |  8 +++++++-
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index 8d3171c6bee8..2a26ad591f59 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -108,7 +108,7 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(
>  }
>  
>  static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev,
> -		const struct timespec *ts)
> +		const struct v4l2_timespec *ts)
>  {
>  	struct v4l2_subscribed_event *sev;
>  	struct v4l2_kevent *kev;
> @@ -170,17 +170,20 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
>  {
>  	struct v4l2_fh *fh;
>  	unsigned long flags;
> -	struct timespec timestamp;
> +	struct timespec64 timestamp;
> +	struct v4l2_timespec vts;
>  
>  	if (vdev == NULL)
>  		return;
>  
> -	ktime_get_ts(&timestamp);
> +	ktime_get_ts64(&timestamp);
> +	vts.tv_sec = timestamp.tv_sec;
> +	vts.tv_nsec = timestamp.tv_nsec;

I prefer to take this opportunity to create a v4l2_get_timespec helper
function, just like v4l2_get_timeval.

>  
>  	spin_lock_irqsave(&vdev->fh_lock, flags);
>  
>  	list_for_each_entry(fh, &vdev->fh_list, list)
> -		__v4l2_event_queue_fh(fh, ev, &timestamp);
> +		__v4l2_event_queue_fh(fh, ev, &vts);
>  
>  	spin_unlock_irqrestore(&vdev->fh_lock, flags);
>  }
> @@ -189,12 +192,15 @@ EXPORT_SYMBOL_GPL(v4l2_event_queue);
>  void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev)
>  {
>  	unsigned long flags;
> -	struct timespec timestamp;
> +	struct timespec64 timestamp;
> +	struct v4l2_timespec vts;
>  
> -	ktime_get_ts(&timestamp);
> +	ktime_get_ts64(&timestamp);
> +	vts.tv_sec = timestamp.tv_sec;
> +	vts.tv_nsec = timestamp.tv_nsec;
>  
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> -	__v4l2_event_queue_fh(fh, ev, &timestamp);
> +	__v4l2_event_queue_fh(fh, ev, &vts);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_queue_fh);
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index b02cf054fbb8..bc659be87260 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -809,6 +809,12 @@ struct v4l2_timeval {
>  	long tv_usec;
>  };
>  
> +/* used for monotonic times, therefore y2038 safe */
> +struct v4l2_timespec {
> +	long tv_sec;
> +	long tv_nsec;
> +};
> +
>  /**
>   * struct v4l2_buffer - video buffer info
>   * @index:	id number of the buffer
> @@ -2088,7 +2094,7 @@ struct v4l2_event {
>  	} u;
>  	__u32				pending;
>  	__u32				sequence;
> -	struct timespec			timestamp;
> +	struct v4l2_timespec		timestamp;
>  	__u32				id;
>  	__u32				reserved[8];
>  };
> 

I think I am OK with this. This timestamp is used much more rarely and I do
not expect this ABI change to cause any problems in userspace.

Regards,

	Hans