[v4,6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
Commit Message
The v4l2_buffer structure contains a 'struct timeval' member that is
defined by the user space C library, creating an ABI incompatibility
when that gets updated to a 64-bit time_t.
As in v4l2_event, handle this with a special case in video_put_user()
and video_get_user() to replace the memcpy there.
Since the structure also contains a pointer, there are now two
native versions (on 32-bit systems) as well as two compat versions
(on 64-bit systems), which unfortunately complicates the compat
handler quite a bit.
Duplicating the existing handlers for the new types is a safe
conversion for now, but unfortunately this may turn into a
maintenance burden later. A larger-scale rework of the
compat code might be a better alternative, but is out of scope
of the y2038 work.
Sparc64 needs a special case because of their special suseconds_t
definition.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 57 ++++++++++++++++++++++++++--
include/uapi/linux/videodev2.h | 45 ++++++++++++++++++++++
2 files changed, 98 insertions(+), 4 deletions(-)
Comments
On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> The v4l2_buffer structure contains a 'struct timeval' member that is
> defined by the user space C library, creating an ABI incompatibility
> when that gets updated to a 64-bit time_t.
>
> As in v4l2_event, handle this with a special case in video_put_user()
> and video_get_user() to replace the memcpy there.
>
> Since the structure also contains a pointer, there are now two
> native versions (on 32-bit systems) as well as two compat versions
> (on 64-bit systems), which unfortunately complicates the compat
> handler quite a bit.
>
> Duplicating the existing handlers for the new types is a safe
> conversion for now, but unfortunately this may turn into a
> maintenance burden later. A larger-scale rework of the
> compat code might be a better alternative, but is out of scope
> of the y2038 work.
>
> Sparc64 needs a special case because of their special suseconds_t
> definition.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 57 ++++++++++++++++++++++++++--
> include/uapi/linux/videodev2.h | 45 ++++++++++++++++++++++
> 2 files changed, 98 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 1de939d11628..4ae1bcaec3fa 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -474,10 +474,10 @@ static void v4l_print_buffer(const void *arg, bool write_only)
> const struct v4l2_plane *plane;
> int i;
>
> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
> - p->timestamp.tv_sec / 3600,
> - (int)(p->timestamp.tv_sec / 60) % 60,
> - (int)(p->timestamp.tv_sec % 60),
> + pr_cont("%02d:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
> + (int)p->timestamp.tv_sec / 3600,
> + ((int)p->timestamp.tv_sec / 60) % 60,
> + ((int)p->timestamp.tv_sec % 60),
> (long)p->timestamp.tv_usec,
> p->index,
> prt_names(p->type, v4l2_type_names), p->request_fd,
> @@ -3014,6 +3014,14 @@ static unsigned int video_translate_cmd(unsigned int cmd)
> #ifdef CONFIG_COMPAT_32BIT_TIME
> case VIDIOC_DQEVENT_TIME32:
> return VIDIOC_DQEVENT;
> + case VIDIOC_QUERYBUF_TIME32:
> + return VIDIOC_QUERYBUF;
> + case VIDIOC_QBUF_TIME32:
> + return VIDIOC_QBUF;
> + case VIDIOC_DQBUF_TIME32:
> + return VIDIOC_DQBUF;
> + case VIDIOC_PREPARE_BUF_TIME32:
> + return VIDIOC_PREPARE_BUF;
> #endif
> }
>
> @@ -3032,6 +3040,30 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd,
> }
>
> switch (cmd) {
> +#ifdef COMPAT_32BIT_TIME
> + case VIDIOC_QUERYBUF_TIME32:
> + case VIDIOC_QBUF_TIME32:
> + case VIDIOC_DQBUF_TIME32:
> + case VIDIOC_PREPARE_BUF_TIME32: {
> + struct v4l2_buffer_time32 vb32;
> + struct v4l2_buffer *vb = parg;
> +
> + if (copy_from_user(&vb32, arg, sizeof(vb32)))
> + return -EFAULT;
> +
> + memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
> + vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
> + vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
> + memcpy(&vb->timecode, &vb32.timecode,
> + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
I have similar concerns as with dqevent about whether this memcpy is the right approach.
Unless you can prove with a utility like pahole that this memcpy is safe.
> +
> + if (cmd == VIDIOC_QUERYBUF_TIME32)
> + memset(&vb->length, 0, sizeof(*vb) -
> + offsetof(struct v4l2_buffer, length));
> +
> + break;
> + }
> +#endif
> default:
> /*
> * In some cases, only a few fields are used as input,
> @@ -3080,6 +3112,23 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
> return -EFAULT;
> break;
> }
> + case VIDIOC_QUERYBUF_TIME32:
> + case VIDIOC_QBUF_TIME32:
> + case VIDIOC_DQBUF_TIME32:
> + case VIDIOC_PREPARE_BUF_TIME32: {
> + struct v4l2_buffer_time32 vb32;
> + struct v4l2_buffer *vb = parg;
> +
> + memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
> + vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
> + vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
> + memcpy(&vb32.timecode, &vb->timecode,
> + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
Ditto.
> +
> + if (copy_to_user(arg, &vb32, sizeof(vb32)))
> + return -EFAULT;
> + break;
> + }
> #endif
> default:
> /* Copy results into user buffer */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1d2553d4ed5b..f05c54d63f96 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -990,7 +990,47 @@ struct v4l2_buffer {
> __u32 bytesused;
> __u32 flags;
> __u32 field;
> +#ifdef __KERNEL__
> + /* match glibc timeval64 format */
> + struct {
> + long long tv_sec;
> +# if defined(__sparc__) && defined(__arch64__)
> + int tv_usec;
> + int __pad;
> +# else
> + long long tv_usec;
> +# endif
> + } timestamp;
Ewww!
Are there more places where this is needed? If so, then I very much prefer
that a __kernel_timeval struct is defined somewhere, with appropriate
comments.
> +#else
> struct timeval timestamp;
> +#endif
> + struct v4l2_timecode timecode;
> + __u32 sequence;
> +
> + /* memory location */
> + __u32 memory;
> + union {
> + __u32 offset;
> + unsigned long userptr;
> + struct v4l2_plane *planes;
> + __s32 fd;
> + } m;
> + __u32 length;
> + __u32 reserved2;
> + union {
> + __s32 request_fd;
> + __u32 reserved;
> + };
> +};
> +
> +#ifdef __KERNEL__
> +struct v4l2_buffer_time32 {
> + __u32 index;
> + __u32 type;
> + __u32 bytesused;
> + __u32 flags;
> + __u32 field;
> + struct old_timeval32 timestamp;
> struct v4l2_timecode timecode;
> __u32 sequence;
>
> @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
> __u32 reserved;
> };
> };
> +#endif
Can this be moved to v4l2-ioctls.h?
>
> #ifndef __KERNEL__
> /**
> @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
> #define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format)
> #define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers)
> #define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer)
> +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32)
And all these should be moved there as well.
> #define VIDIOC_G_FBUF _IOR('V', 10, struct v4l2_framebuffer)
> #define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer)
> #define VIDIOC_OVERLAY _IOW('V', 14, int)
> #define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer)
> +#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32)
> #define VIDIOC_EXPBUF _IOWR('V', 16, struct v4l2_exportbuffer)
> #define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer)
> +#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32)
> #define VIDIOC_STREAMON _IOW('V', 18, int)
> #define VIDIOC_STREAMOFF _IOW('V', 19, int)
> #define VIDIOC_G_PARM _IOWR('V', 21, struct v4l2_streamparm)
> @@ -2520,6 +2564,7 @@ struct v4l2_create_buffers {
> #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
> #define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers)
> #define VIDIOC_PREPARE_BUF _IOWR('V', 93, struct v4l2_buffer)
> +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
> #define VIDIOC_G_SELECTION _IOWR('V', 94, struct v4l2_selection)
> #define VIDIOC_S_SELECTION _IOWR('V', 95, struct v4l2_selection)
> #define VIDIOC_DECODER_CMD _IOWR('V', 96, struct v4l2_decoder_cmd)
>
Regards,
Hans
On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> > switch (cmd) {
> > +#ifdef COMPAT_32BIT_TIME
> > + case VIDIOC_QUERYBUF_TIME32:
> > + case VIDIOC_QBUF_TIME32:
> > + case VIDIOC_DQBUF_TIME32:
> > + case VIDIOC_PREPARE_BUF_TIME32: {
> > + struct v4l2_buffer_time32 vb32;
> > + struct v4l2_buffer *vb = parg;
> > +
> > + if (copy_from_user(&vb32, arg, sizeof(vb32)))
> > + return -EFAULT;
> > +
> > + memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
> > + vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
> > + vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
> > + memcpy(&vb->timecode, &vb32.timecode,
> > + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>
> I have similar concerns as with dqevent about whether this memcpy is the right approach.
> Unless you can prove with a utility like pahole that this memcpy is safe.
This is the video_get_user() function, so the input data comes from user
space and gets copied into the kernel, which has to check each field for
validity already, so I think this is safe regardless of the padding (which
exists before the 64-bit timestamp on 32-bit architectures). The fields
match because the definition of all members other than the timeval is
the same.
On the other hand, I agree it's not obvious from the code why this
is correct. I've changed my copy to this version below now, do you like
that better?
struct v4l2_buffer_time32 vb32;
struct v4l2_buffer *vb = parg;
if (copy_from_user(&vb32, arg, sizeof(vb32)))
return -EFAULT;
*vb = (struct v4l2_buffer) {
.index = vb32.index,
.type = vb32.type,
.bytesused = vb32.bytesused,
.flags = vb32.flags,
.field = vb32.field,
.timestamp.tv_sec = vb32.timestamp.tv_sec,
.timestamp.tv_usec = vb32.timestamp.tv_usec,
.timecode = vb32.timecode,
.memory = vb32.memory,
.m.userptr = vb32.usercopy,
.length = vb32.length,
.request_fd = vb32.request_fd,
};
if (cmd == VIDIOC_QUERYBUF_TIME32)
memset(&vb->length, 0, sizeof(*vb) -
offsetof(struct v4l2_buffer, length));
This way, all padding is zeroed out, and it's obvious to human
readers that each field gets set in the correct location.
> > + memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
> > + vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
> > + vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
> > + memcpy(&vb32.timecode, &vb->timecode,
> > + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>
> Ditto.
This is my new version:
struct v4l2_buffer *vb = parg;
struct v4l2_buffer_time32 vb32 = {
.index = vb->index,
.type = vb->type,
.bytesused = vb->bytesused,
.flags = vb->flags,
.field = vb->field,
.timestamp.tv_sec = vb->timestamp.tv_sec,
.timestamp.tv_usec = vb->timestamp.tv_usec,
.timecode = vb->timecode,
.memory = vb->memory,
.m.userptr = vb->usercopy,
.length = vb->length,
.request_fd = vb->request_fd,
};
if (copy_to_user(arg, &vb32, sizeof(vb32)))
return -EFAULT;
> > __u32 field;
> > +#ifdef __KERNEL__
> > + /* match glibc timeval64 format */
> > + struct {
> > + long long tv_sec;
> > +# if defined(__sparc__) && defined(__arch64__)
> > + int tv_usec;
> > + int __pad;
> > +# else
> > + long long tv_usec;
> > +# endif
> > + } timestamp;
>
> Ewww!
>
> Are there more places where this is needed? If so, then I very much prefer
> that a __kernel_timeval struct is defined somewhere, with appropriate
> comments.
I was trying hard to avoid adding a modern version of timeval, because
all new code should be encouraged to use __kernel_timespec instead.
There are not many users of timeval in the uapi, and this is the last one
after the others all got invididual treatment.
Usually what I would do is to have a kernel-internal type based
on timespec or u64, and then define three uapi types:
old native (based on __kernel_old_timeval), old compat (using
old_timeval32) and the new type with 64-bit time_t.
The problem with v4l2_buffer is that it includes another
compat-incompatible field (m.userptr) and that it's passed
between kernel functions, so then I'd probably need five variants
of it in total, and it would slow down the common case (64-bit
native) because it would require an extra copy.
I can try a few more things here, but I don't expect to find anything
much better than this.
> > +#ifdef __KERNEL__
> > +struct v4l2_buffer_time32 {
> > + __u32 index;
> > + __u32 type;
> > + __u32 bytesused;
> > + __u32 flags;
> > + __u32 field;
> > + struct old_timeval32 timestamp;
> > struct v4l2_timecode timecode;
> > __u32 sequence;
> >
> > @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
> > __u32 reserved;
> > };
> > };
> > +#endif
>
> Can this be moved to v4l2-ioctls.h?
done.
> > #ifndef __KERNEL__
> > /**
> > @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
> > #define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format)
> > #define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers)
> > #define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer)
> > +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32)
>
> And all these should be moved there as well.
done.
Arnd
On 11/26/19 2:50 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
>
>>> switch (cmd) {
>>> +#ifdef COMPAT_32BIT_TIME
>>> + case VIDIOC_QUERYBUF_TIME32:
>>> + case VIDIOC_QBUF_TIME32:
>>> + case VIDIOC_DQBUF_TIME32:
>>> + case VIDIOC_PREPARE_BUF_TIME32: {
>>> + struct v4l2_buffer_time32 vb32;
>>> + struct v4l2_buffer *vb = parg;
>>> +
>>> + if (copy_from_user(&vb32, arg, sizeof(vb32)))
>>> + return -EFAULT;
>>> +
>>> + memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
>>> + vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
>>> + vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
>>> + memcpy(&vb->timecode, &vb32.timecode,
>>> + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>>
>> I have similar concerns as with dqevent about whether this memcpy is the right approach.
>> Unless you can prove with a utility like pahole that this memcpy is safe.
>
> This is the video_get_user() function, so the input data comes from user
> space and gets copied into the kernel, which has to check each field for
> validity already, so I think this is safe regardless of the padding (which
> exists before the 64-bit timestamp on 32-bit architectures). The fields
> match because the definition of all members other than the timeval is
> the same.
>
> On the other hand, I agree it's not obvious from the code why this
> is correct. I've changed my copy to this version below now, do you like
> that better?
>
> struct v4l2_buffer_time32 vb32;
> struct v4l2_buffer *vb = parg;
>
> if (copy_from_user(&vb32, arg, sizeof(vb32)))
> return -EFAULT;
>
> *vb = (struct v4l2_buffer) {
> .index = vb32.index,
> .type = vb32.type,
> .bytesused = vb32.bytesused,
> .flags = vb32.flags,
> .field = vb32.field,
> .timestamp.tv_sec = vb32.timestamp.tv_sec,
> .timestamp.tv_usec = vb32.timestamp.tv_usec,
> .timecode = vb32.timecode,
> .memory = vb32.memory,
> .m.userptr = vb32.usercopy,
> .length = vb32.length,
> .request_fd = vb32.request_fd,
> };
>
> if (cmd == VIDIOC_QUERYBUF_TIME32)
> memset(&vb->length, 0, sizeof(*vb) -
> offsetof(struct v4l2_buffer, length));
>
> This way, all padding is zeroed out, and it's obvious to human
> readers that each field gets set in the correct location.
>
>>> + memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
>>> + vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
>>> + vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
>>> + memcpy(&vb32.timecode, &vb->timecode,
>>> + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>>
>> Ditto.
>
> This is my new version:
>
> struct v4l2_buffer *vb = parg;
> struct v4l2_buffer_time32 vb32 = {
> .index = vb->index,
> .type = vb->type,
> .bytesused = vb->bytesused,
> .flags = vb->flags,
> .field = vb->field,
> .timestamp.tv_sec = vb->timestamp.tv_sec,
> .timestamp.tv_usec = vb->timestamp.tv_usec,
> .timecode = vb->timecode,
> .memory = vb->memory,
> .m.userptr = vb->usercopy,
> .length = vb->length,
> .request_fd = vb->request_fd,
> };
That looks clean.
>
> if (copy_to_user(arg, &vb32, sizeof(vb32)))
> return -EFAULT;
>
>>> __u32 field;
>>> +#ifdef __KERNEL__
>>> + /* match glibc timeval64 format */
>>> + struct {
>>> + long long tv_sec;
>>> +# if defined(__sparc__) && defined(__arch64__)
>>> + int tv_usec;
>>> + int __pad;
>>> +# else
>>> + long long tv_usec;
>>> +# endif
>>> + } timestamp;
>>
>> Ewww!
>>
>> Are there more places where this is needed? If so, then I very much prefer
>> that a __kernel_timeval struct is defined somewhere, with appropriate
>> comments.
>
> I was trying hard to avoid adding a modern version of timeval, because
> all new code should be encouraged to use __kernel_timespec instead.
>
> There are not many users of timeval in the uapi, and this is the last one
> after the others all got invididual treatment.
>
> Usually what I would do is to have a kernel-internal type based
> on timespec or u64, and then define three uapi types:
> old native (based on __kernel_old_timeval), old compat (using
> old_timeval32) and the new type with 64-bit time_t.
>
> The problem with v4l2_buffer is that it includes another
> compat-incompatible field (m.userptr) and that it's passed
> between kernel functions, so then I'd probably need five variants
> of it in total, and it would slow down the common case (64-bit
> native) because it would require an extra copy.
>
> I can try a few more things here, but I don't expect to find anything
> much better than this.
How about something like this in videodev2.h:
Split off the ugly kernel timeval definition in a separate struct:
#ifdef __KERNEL__
/* match glibc timeval64 format */
struct __kernel_v4l2_timeval {
long long tv_sec;
# if defined(__sparc__) && defined(__arch64__)
int tv_usec;
int __pad;
# else
long long tv_usec;
# endif
};
#endif
Then use that in the struct v4l2_buffer definition:
struct v4l2_buffer {
...
#ifdef __KERNEL__
struct __kernel_v4l2_timeval timestamp;
#else
struct timeval timestamp;
#endif
That keeps struct v4l2_buffer fairly clean. And it also makes it
possible to have a bit more extensive documentation for the
struct __kernel_v4l2_timeval without polluting the actual struct
v4l2_buffer definition.
The videodev2.h header is something users of the API look at a
lot and having this really ugly kernel timestamp in there is
not acceptably IMHO. But splitting it off should work.
>
>>> +#ifdef __KERNEL__
>>> +struct v4l2_buffer_time32 {
>>> + __u32 index;
>>> + __u32 type;
>>> + __u32 bytesused;
>>> + __u32 flags;
>>> + __u32 field;
>>> + struct old_timeval32 timestamp;
>>> struct v4l2_timecode timecode;
>>> __u32 sequence;
>>>
>>> @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
>>> __u32 reserved;
>>> };
>>> };
>>> +#endif
>>
>> Can this be moved to v4l2-ioctls.h?
>
> done.
>
>>> #ifndef __KERNEL__
>>> /**
>>> @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
>>> #define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format)
>>> #define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers)
>>> #define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer)
>>> +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32)
>>
>> And all these should be moved there as well.
>
> done.
>
> Arnd
>
Regards,
Hans
On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Then use that in the struct v4l2_buffer definition:
>
> struct v4l2_buffer {
> ...
> #ifdef __KERNEL__
> struct __kernel_v4l2_timeval timestamp;
> #else
> struct timeval timestamp;
> #endif
>
> That keeps struct v4l2_buffer fairly clean. And it also makes it
> possible to have a bit more extensive documentation for the
> struct __kernel_v4l2_timeval without polluting the actual struct
> v4l2_buffer definition.
Yes, good idea. I've added this version now:
#ifdef __KERNEL__
/*
* This corresponds to the user space version of timeval
* for 64-bit time_t. sparc64 is different from everyone
* else, using the microseconds in the wrong half of the
* second 64-bit word.
*/
struct __kernel_v4l2_timeval {
long long tv_sec;
#if defined(__sparc__) && defined(__arch64__)
int tv_usec;
int __pad;
#else
long long tv_usec;
#endif
};
#endif
I briefly considered using #else #define __kernel_v4l2_timeval timeval
to avoid the second #ifdef, but went back to your version again
for clarify.
> The videodev2.h header is something users of the API look at a
> lot and having this really ugly kernel timestamp in there is
> not acceptably IMHO. But splitting it off should work.
Do you also mean moving it into a separate header file, or
just outside of struct v4l2_buffer? Since it's hidden in #ifdef
__KERNEL__, it could be moved to media/ioctl.h or elsewhere.
Arnd
On 11/26/19 4:17 PM, Arnd Bergmann wrote:
> On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Then use that in the struct v4l2_buffer definition:
>>
>> struct v4l2_buffer {
>> ...
>> #ifdef __KERNEL__
>> struct __kernel_v4l2_timeval timestamp;
>> #else
>> struct timeval timestamp;
>> #endif
>>
>> That keeps struct v4l2_buffer fairly clean. And it also makes it
>> possible to have a bit more extensive documentation for the
>> struct __kernel_v4l2_timeval without polluting the actual struct
>> v4l2_buffer definition.
>
> Yes, good idea. I've added this version now:
>
> #ifdef __KERNEL__
> /*
> * This corresponds to the user space version of timeval
> * for 64-bit time_t. sparc64 is different from everyone
> * else, using the microseconds in the wrong half of the
> * second 64-bit word.
> */
> struct __kernel_v4l2_timeval {
> long long tv_sec;
> #if defined(__sparc__) && defined(__arch64__)
> int tv_usec;
> int __pad;
> #else
> long long tv_usec;
> #endif
> };
> #endif
>
> I briefly considered using #else #define __kernel_v4l2_timeval timeval
> to avoid the second #ifdef, but went back to your version again
> for clarify.
>
>> The videodev2.h header is something users of the API look at a
>> lot and having this really ugly kernel timestamp in there is
>> not acceptably IMHO. But splitting it off should work.
>
> Do you also mean moving it into a separate header file, or
> just outside of struct v4l2_buffer? Since it's hidden in #ifdef
> __KERNEL__, it could be moved to media/ioctl.h or elsewhere.
I've thought about that, but that risks having to change drivers
since they would now have to include another header to get the
right timeval definition. In the end I don't think it is worth the
effort.
I think it is best to define __kernel_v4l2_timeval just before
the struct v4l2_requestbuffers definition rather than before the
struct v4l2_buffer. That way it doesn't interfere with the
userspace structs for the buffer API.
Regards,
Hans
>
> Arnd
>
@@ -474,10 +474,10 @@ static void v4l_print_buffer(const void *arg, bool write_only)
const struct v4l2_plane *plane;
int i;
- pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
- p->timestamp.tv_sec / 3600,
- (int)(p->timestamp.tv_sec / 60) % 60,
- (int)(p->timestamp.tv_sec % 60),
+ pr_cont("%02d:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
+ (int)p->timestamp.tv_sec / 3600,
+ ((int)p->timestamp.tv_sec / 60) % 60,
+ ((int)p->timestamp.tv_sec % 60),
(long)p->timestamp.tv_usec,
p->index,
prt_names(p->type, v4l2_type_names), p->request_fd,
@@ -3014,6 +3014,14 @@ static unsigned int video_translate_cmd(unsigned int cmd)
#ifdef CONFIG_COMPAT_32BIT_TIME
case VIDIOC_DQEVENT_TIME32:
return VIDIOC_DQEVENT;
+ case VIDIOC_QUERYBUF_TIME32:
+ return VIDIOC_QUERYBUF;
+ case VIDIOC_QBUF_TIME32:
+ return VIDIOC_QBUF;
+ case VIDIOC_DQBUF_TIME32:
+ return VIDIOC_DQBUF;
+ case VIDIOC_PREPARE_BUF_TIME32:
+ return VIDIOC_PREPARE_BUF;
#endif
}
@@ -3032,6 +3040,30 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd,
}
switch (cmd) {
+#ifdef COMPAT_32BIT_TIME
+ case VIDIOC_QUERYBUF_TIME32:
+ case VIDIOC_QBUF_TIME32:
+ case VIDIOC_DQBUF_TIME32:
+ case VIDIOC_PREPARE_BUF_TIME32: {
+ struct v4l2_buffer_time32 vb32;
+ struct v4l2_buffer *vb = parg;
+
+ if (copy_from_user(&vb32, arg, sizeof(vb32)))
+ return -EFAULT;
+
+ memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
+ vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
+ vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
+ memcpy(&vb->timecode, &vb32.timecode,
+ sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
+
+ if (cmd == VIDIOC_QUERYBUF_TIME32)
+ memset(&vb->length, 0, sizeof(*vb) -
+ offsetof(struct v4l2_buffer, length));
+
+ break;
+ }
+#endif
default:
/*
* In some cases, only a few fields are used as input,
@@ -3080,6 +3112,23 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
return -EFAULT;
break;
}
+ case VIDIOC_QUERYBUF_TIME32:
+ case VIDIOC_QBUF_TIME32:
+ case VIDIOC_DQBUF_TIME32:
+ case VIDIOC_PREPARE_BUF_TIME32: {
+ struct v4l2_buffer_time32 vb32;
+ struct v4l2_buffer *vb = parg;
+
+ memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
+ vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
+ vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
+ memcpy(&vb32.timecode, &vb->timecode,
+ sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
+
+ if (copy_to_user(arg, &vb32, sizeof(vb32)))
+ return -EFAULT;
+ break;
+ }
#endif
default:
/* Copy results into user buffer */
@@ -990,7 +990,47 @@ struct v4l2_buffer {
__u32 bytesused;
__u32 flags;
__u32 field;
+#ifdef __KERNEL__
+ /* match glibc timeval64 format */
+ struct {
+ long long tv_sec;
+# if defined(__sparc__) && defined(__arch64__)
+ int tv_usec;
+ int __pad;
+# else
+ long long tv_usec;
+# endif
+ } timestamp;
+#else
struct timeval timestamp;
+#endif
+ struct v4l2_timecode timecode;
+ __u32 sequence;
+
+ /* memory location */
+ __u32 memory;
+ union {
+ __u32 offset;
+ unsigned long userptr;
+ struct v4l2_plane *planes;
+ __s32 fd;
+ } m;
+ __u32 length;
+ __u32 reserved2;
+ union {
+ __s32 request_fd;
+ __u32 reserved;
+ };
+};
+
+#ifdef __KERNEL__
+struct v4l2_buffer_time32 {
+ __u32 index;
+ __u32 type;
+ __u32 bytesused;
+ __u32 flags;
+ __u32 field;
+ struct old_timeval32 timestamp;
struct v4l2_timecode timecode;
__u32 sequence;
@@ -1009,6 +1049,7 @@ struct v4l2_buffer {
__u32 reserved;
};
};
+#endif
#ifndef __KERNEL__
/**
@@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
#define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format)
#define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers)
#define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer)
+#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32)
#define VIDIOC_G_FBUF _IOR('V', 10, struct v4l2_framebuffer)
#define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer)
#define VIDIOC_OVERLAY _IOW('V', 14, int)
#define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer)
+#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32)
#define VIDIOC_EXPBUF _IOWR('V', 16, struct v4l2_exportbuffer)
#define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer)
+#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32)
#define VIDIOC_STREAMON _IOW('V', 18, int)
#define VIDIOC_STREAMOFF _IOW('V', 19, int)
#define VIDIOC_G_PARM _IOWR('V', 21, struct v4l2_streamparm)
@@ -2520,6 +2564,7 @@ struct v4l2_create_buffers {
#define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers)
#define VIDIOC_PREPARE_BUF _IOWR('V', 93, struct v4l2_buffer)
+#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
#define VIDIOC_G_SELECTION _IOWR('V', 94, struct v4l2_selection)
#define VIDIOC_S_SELECTION _IOWR('V', 95, struct v4l2_selection)
#define VIDIOC_DECODER_CMD _IOWR('V', 96, struct v4l2_decoder_cmd)