Re: [RFC PATCH v9 1/6] media: videobuf2: Move timestamp to vb2_buffer

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/05/2015 04:12 AM, Junghak Sung wrote:
>>> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c b/drivers/media/platform/vivid/vivid-kthread-cap.c
>>> index 83cc6d3..b0ad054 100644
>>> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
>>> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
>>> @@ -441,7 +441,7 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct vivid_buffer *buf)
>>>   	 * "Start of Exposure".
>>>   	 */
>>>   	if (dev->tstamp_src_is_soe)
>>> -		v4l2_get_timestamp(&buf->vb.timestamp);
>>> +		buf->vb.vb2_buf.timestamp = ktime_get_ns();
>>>   	if (dev->field_cap == V4L2_FIELD_ALTERNATE) {
>>>   		/*
>>>   		 * 60 Hz standards start with the bottom field, 50 Hz standards
>>> @@ -558,8 +558,9 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct vivid_buffer *buf)
>>>   	 * the timestamp now.
>>>   	 */
>>>   	if (!dev->tstamp_src_is_soe)
>>> -		v4l2_get_timestamp(&buf->vb.timestamp);
>>> -	buf->vb.timestamp.tv_sec += dev->time_wrap_offset;
>>> +		buf->vb.vb2_buf.timestamp = ktime_get_ns();
>>> +	buf->vb.vb2_buf.timestamp +=
>>> +			((u64) dev->time_wrap_offset * NSEC_PER_SEC);
>>
>> I'd do this differently: make time_wrap_offset of type u64 and assign it
>> accordingly with nanoseconds. That way you can just do:
>>
>> 	timestamp += dev->time_wrap_offset
>>
>> vivid-ctrls.c also needs to be modified (vivid_streaming_s_ctrl(), VIVID_CID_TIME_WRAP
>> case) to:
>>
>> 	dev->time_wrap_offset = (0x100000000ULL - 16) * NSEC_PER_SEC - ktime_get_ns();
>>
>> The v4l2_get_timestamp() call there can be dropped.
>>
> 
> I agree with your opinion. But it is too hard to read the code
> getting time_wrap_offset.
> How about this way?
> 
>   in vivid_streaming_s_ctrl() of vivid-ctrls.c
>          dev->time_wrap_offset = ktime_get_ns() + 16 * NSEC_PER_SEC;
>   and in vivid_fillbuff() of vivid-kthread-cap.c
> 	buf->vb.vb2_buf.timestamp -= dev->time_wrap_offset;

That doesn't do what I want it to do, which is to wrap around in the struct timeval
where seconds are 32 bits. The code above is actually wrong since I forgot that
tv_sec in struct timeval is signed, so 0x100000000ULL should be 0x80000000ULL.
Also, that code will fail after 2038, so that's no good either.

On a related note I send out a question to Arnd whether a timestamp should be u64
or s64. It's not clear to me which should be used as ktime_get_ns() returns a s64.

Once we return the full 64 bits to userspace, then we have a second wrap around when
the u64 (or s64) wraps. I'll add a second wrap-around control to vivid at that time.

I'll wait for Arnd to answer before fixing the time_wrap_offset calculation since I
need to know whether a timestamp is u64 or s64.

Regards,

	Hans