On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil hverkuil@xs4all.nl wrote:
On 11/11/19 9:38 PM, Arnd Bergmann wrote:
As a preparation for adding 64-bit time_t support in the uapi, change the drivers to no longer care about the format of the timestamp field in struct v4l2_buffer.
The v4l2_timeval_to_ns() function is no longer needed in the kernel after this, but there may be userspace code relying on it because it is part of the uapi header.
There is indeed userspace code that relies on this.
Ok, good to know. I rephrased the changelog text as
The v4l2_timeval_to_ns() function is no longer needed in the kernel after this, but there is userspace code relying on it to be part of the uapi header.
+static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf) +{
return buf->timestamp.tv_sec * NSEC_PER_SEC +(u32)buf->timestamp.tv_usec * NSEC_PER_USEC;Why the (u32) cast?
Simple question, long answer:
on 32-bit architectures, the tv_usec member may be 32-bit wide plus padding in user space when interpreted as a regular 'struct timeval', but the kernel implementation now sees it as a 64-bit member, with half of it being possibly uninitialized user space data.
The 32-bit cast avoids that uninitialized data and ensures user space passing garbage in the upper half gets ignored, as it has to be on 32-bit user space.
On 64-bit native user space, the tv_usec field is always 64 bit wide, so this is a change in behavior for denormalized timeval data with tv_usec > U32_MAX, but the current behavior does not appear worth preserving either.
The correct way would probably be to return an error for tv_usec >USEC_PER_SEC, but as the code never did that, this would risk a regression for user space that relies on passing invalid timestamps without getting an error.
+static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
u64 timestamp)+{
struct timespec64 ts = ns_to_timespec64(timestamp);buf->timestamp.tv_sec = ts.tv_sec;buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;+}
This does not belong in the public header. This is kernel specific,
Note: this is not the uapi header but the in-kernel one.
so media/v4l2-common.h would be a good place.
Ok, sounds good. I wasn't sure where to put it, and ended up with include/linux/videodev2.h as the best replacement for include/uapi/linux/videodev2.h, changed it to include/media/v4l2-common.h now.
Arnd