Re: [PATCH] media: uvcvideo: Add boottime clock support

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Hi Tomasz,

On 06/08/2019 05:15, Tomasz Figa wrote:
> On Wed, Mar 13, 2019 at 11:38 AM Tomasz Figa <tfiga@chromium.org> wrote:
>>
>> On Wed, Mar 13, 2019 at 10:25 AM Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>>
>>> Hi Tomasz,
>>>
>>> On Fri, Nov 23, 2018 at 11:46:43PM +0900, Tomasz Figa wrote:
>>>> On Fri, Nov 2, 2018 at 12:03 AM Lars-Peter Clausen wrote:
>>>>> On 11/01/2018 03:30 PM, Tomasz Figa wrote:
>>>>>> On Thu, Nov 1, 2018 at 11:03 PM Laurent Pinchart wrote:
>>>>>>> On Thursday, 18 October 2018 20:28:06 EET Alexandru M Stan wrote:
>>>>>>>> On Wed, Oct 17, 2018 at 9:31 PM, Tomasz Figa wrote:
>>>>>>>>> On Thu, Oct 18, 2018 at 5:50 AM Laurent Pinchart wrote:
>>>>>>>>>> On Wednesday, 17 October 2018 11:28:52 EEST Tomasz Figa wrote:
>>>>>>>>>>> On Wed, Oct 17, 2018 at 5:02 PM Laurent Pinchart wrote:
>>>>>>>>>>>> On Wednesday, 17 October 2018 10:52:42 EEST Heng-Ruey Hsu wrote:
>>>>>>>>>>>>> Android requires camera timestamps to be reported with
>>>>>>>>>>>>> CLOCK_BOOTTIME to sync timestamp with other sensor sources.
>>>>>>>>>>>>
>>>>>>>>>>>> What's the rationale behind this, why can't CLOCK_MONOTONIC work ? If
>>>>>>>>>>>> the monotonic clock has shortcomings that make its use impossible for
>>>>>>>>>>>> proper synchronization, then we should consider switching to
>>>>>>>>>>>> CLOCK_BOOTTIME globally in V4L2, not in selected drivers only.
>>>>>>>>>>>
>>>>>>>>>>> CLOCK_BOOTTIME includes the time spent in suspend, while
>>>>>>>>>>> CLOCK_MONOTONIC doesn't. I can imagine the former being much more
>>>>>>>>>>> useful for anything that cares about the actual, long term, time
>>>>>>>>>>> tracking. Especially important since suspend is a very common event on
>>>>>>>>>>> Android and doesn't stop the time flow there, i.e. applications might
>>>>>>>>>>> wake up the device to perform various tasks at necessary times.
>>>>>>>>>>
>>>>>>>>>> Sure, but this patch mentions timestamp synchronization with other
>>>>>>>>>> sensors, and from that point of view, I'd like to know what is wrong with
>>>>>>>>>> the monotonic clock if all devices use it.
>>>>>>>>>
>>>>>>>>> AFAIK the sensors mentioned there are not camera sensors, but rather
>>>>>>>>> things we normally put under IIO, e.g. accelerometers, gyroscopes and
>>>>>>>>> so on. I'm not sure how IIO deals with timestamps, but Android seems
>>>>>>>>> to operate in the CLOCK_BOTTIME domain. Let me add some IIO folks.
>>>>>>>>>
>>>>>>>>> Gwendal, Alexandru, do you think you could shed some light on how we
>>>>>>>>> handle IIO sensors timestamps across the kernel, Chrome OS and
>>>>>>>>> Android?
>>>>>>>>
>>>>>>>> On our devices of interest have a specialized "sensor" that comes via
>>>>>>>> IIO (from the EC, cros-ec-ring driver) that can be used to more
>>>>>>>> accurately timestamp each frame (since it's recorded with very low
>>>>>>>> jitter by a realtime-ish OS). In some high level userspace thing
>>>>>>>> (specifically the Android Camera HAL) we try to pick the best
>>>>>>>> timestamp from the IIO, whatever's closest to what the V4L stuff gives
>>>>>>>> us.
>>>>>>>>
>>>>>>>> I guess the Android convention is for sensor timestamps to be in
>>>>>>>> CLOCK_BOOTTIME (maybe because it likes sleeping so much). There's
>>>>>>>> probably no advantage to using one over the other, but the important
>>>>>>>> thing is that they have to be the same, otherwise the closest match
>>>>>>>> logic would fail.
>>>>>>>
>>>>>>> That's my understanding too, I don't think CLOCK_BOOTTIME really brings much
>>>>>>> benefit in this case,
>>>>>>
>>>>>> I think it does have a significant benefit. CLOCK_MONOTONIC stops when
>>>>>> the device is sleeping, but the sensors can still capture various
>>>>>> actions. We would lose the time keeping of those actions if we use
>>>>>> CLOCK_MONOTONIC.

That's an important distinction. If there are operations that can run
while the main host is in 'suspend' and still maintain "relative"
timestamps in any form - then time must continue during suspend.


>>>>>>> but it's important than all timestamps use the same
>>>>>>> clock. The question is thus which clock we should select. Mainline mostly uses
>>>>>>> CLOCK_MONOTONIC, and Android CLOCK_BOOTTIME. Would you like to submit patches
>>>>>>> to switch Android to CLOCK_MONOTONIC ? :-)
>>>>>> Is it Android using CLOCK_BOOTTIME or the sensors (IIO?). I have
>>>>>> almost zero familiarity with the IIO subsystem and was hoping someone
>>>>>> from there could comment on what time domain is used for those
>>>>>> sensors.
>>>>>
>>>>> IIO has the option to choose between BOOTTIME or MONOTONIC (and a few
>>>>> others) for the timestamp on a per device basis.
>>>>>
>>>>> There was a bit of a discussion about this a while back. See
>>>>> https://lkml.org/lkml/2018/7/10/432 and the following thread.
>>>>
>>>> Given that IIO supports BOOTTIME in upstream already and also the
>>>> important advantage of using it over MONOTONIC for systems which keep
>>>> capturing events during sleep, do you think we could move on with some
>>>> way to support it in uvcvideo or preferably V4L2 in general?
>>>
>>> I'm not opposed to that, but I don't think we should approach that from
>>> a UVC point of view. The issue should be addressed in V4L2, and then
>>> driver-specific support could be added, if needed.

Agreed, this is a V4L2 topic - not a UVC specific topic.


>> Yes, fully agreed. The purpose of sending this patch was just to start
>> the discussion on how to support this.
>>
>> Do you think something like a buffer flag called
>> V4L2_BUF_FLAG_TIMESTAMP_BOOTTIME that could be set by the userspace at
>> QBUF could work here? (That would change the timestamp flags
>> semantics, because it used to be just the information from the driver,
>> but shouldn't have any compatibility implications.) I suppose we would
>> also need some capability flag for querying purposes, possibly added
>> to the capability flags returned by REQBUFS/CREATE_BUFS?

What kind of 'compatibility' do we actually need to maintain here? IMO -
CLOCK_BOOTTIME makes much more sense globally for video, because it's
more representative of the temporal difference between frames captured
if a system goes into suspend.

If frames are captured:

A B         C D
   <suspend>

Then I believe it would be correct for the timestamp delta between B-C
to be large <representative of the suspend duration/real time>


> Any thoughts?

Aha, there might be some gotchas around non-live sources operating
across suspend-resume boundaries .. so perhaps there are certainly
use-cases where both _MONOTONIC and _BOOTTIME have their relevance ...


> Adding Hans and Kieran for more insight.

I think if we're talking about core-V4L2, Hans' opinion takes more
weight than my mumblings :-) - but overall - supporting _BOOTTIME in
some form sounds beneficial to me.


> Best regards,
> Tomasz
> 

-- 
Regards
--
Kieran