On 11/26/19 12:34 PM, Arnd Bergmann wrote:
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.
But that's only valid for little endian 32 bit systems, right? Is this only an issue for x86 platforms?
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.
This long answer needs to be added to a comment to that function. Because otherwise someone will come along later and remove that seemingly unnecessary cast.
It's OK if it is a long comment, it's a non-trivial reason.
+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.
Ah, I missed that.
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.
Never use include/linux/videodev2.h. It's just a wrapper around the uapi header and should not contain any 'real' code.
It's also why I missed that you modified that header since we never touch it.
Regards,
Hans
Arnd