Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI

[Hans Verkuil]

On 11/26/19 4:17 PM, Arnd Bergmann wrote:
> On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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
>