[PATCH] media: uvcvideo: Add boottime clock support

[PATCH] media: uvcvideo: Add boottime clock support

[Heng-Ruey Hsu]

Android requires camera timestamps to be reported with
CLOCK_BOOTTIME to sync timestamp with other sensor sources.

Signed-off-by: Heng-Ruey Hsu <henryhsu@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 4 ++++
 drivers/media/usb/uvc/uvc_video.c  | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index d46dc432456c..a9658f38c586 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2287,6 +2287,8 @@ static int uvc_clock_param_get(char *buffer, const struct kernel_param *kp)
 {
 	if (uvc_clock_param == CLOCK_MONOTONIC)
 		return sprintf(buffer, "CLOCK_MONOTONIC");
+	else if (uvc_clock_param == CLOCK_BOOTTIME)
+		return sprintf(buffer, "CLOCK_BOOTTIME");
 	else
 		return sprintf(buffer, "CLOCK_REALTIME");
 }
@@ -2298,6 +2300,8 @@ static int uvc_clock_param_set(const char *val, const struct kernel_param *kp)
 
 	if (strcasecmp(val, "monotonic") == 0)
 		uvc_clock_param = CLOCK_MONOTONIC;
+	else if (strcasecmp(val, "boottime") == 0)
+		uvc_clock_param = CLOCK_BOOTTIME;
 	else if (strcasecmp(val, "realtime") == 0)
 		uvc_clock_param = CLOCK_REALTIME;
 	else
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 86a99f461fd8..d4248d5cd9cd 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -425,6 +425,8 @@ static inline ktime_t uvc_video_get_time(void)
 {
 	if (uvc_clock_param == CLOCK_MONOTONIC)
 		return ktime_get();
+	else if (uvc_clock_param == CLOCK_BOOTTIME)
+		return ktime_get_boottime();
 	else
 		return ktime_get_real();
 }
-- 
2.19.1.331.ge82ca0e54c-goog

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

[Laurent Pinchart]

Hi Heng-Ruey,

Thank you for the patch.

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.

> Signed-off-by: Heng-Ruey Hsu <henryhsu@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 4 ++++
>  drivers/media/usb/uvc/uvc_video.c  | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index d46dc432456c..a9658f38c586
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2287,6 +2287,8 @@ static int uvc_clock_param_get(char *buffer, const
> struct kernel_param *kp) {
>  	if (uvc_clock_param == CLOCK_MONOTONIC)
>  		return sprintf(buffer, "CLOCK_MONOTONIC");
> +	else if (uvc_clock_param == CLOCK_BOOTTIME)
> +		return sprintf(buffer, "CLOCK_BOOTTIME");
>  	else
>  		return sprintf(buffer, "CLOCK_REALTIME");
>  }
> @@ -2298,6 +2300,8 @@ static int uvc_clock_param_set(const char *val, const
> struct kernel_param *kp)
> 
>  	if (strcasecmp(val, "monotonic") == 0)
>  		uvc_clock_param = CLOCK_MONOTONIC;
> +	else if (strcasecmp(val, "boottime") == 0)
> +		uvc_clock_param = CLOCK_BOOTTIME;
>  	else if (strcasecmp(val, "realtime") == 0)
>  		uvc_clock_param = CLOCK_REALTIME;
>  	else
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 86a99f461fd8..d4248d5cd9cd 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -425,6 +425,8 @@ static inline ktime_t uvc_video_get_time(void)
>  {
>  	if (uvc_clock_param == CLOCK_MONOTONIC)
>  		return ktime_get();
> +	else if (uvc_clock_param == CLOCK_BOOTTIME)
> +		return ktime_get_boottime();
>  	else
>  		return ktime_get_real();
>  }

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

Hi Laurent,

On Wed, Oct 17, 2018 at 5:02 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Heng-Ruey,
>
> Thank you for the patch.
>
> 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.

Generally, I'd see a V4L2_BUF_FLAG_TIMESTAMP_BOOTTIME flag being added
and user space being given choice to select the time stamp base it
needs, perhaps by setting the flag in v4l2_buffer struct at QBUF time?

Best regards,
Tomasz

> > Signed-off-by: Heng-Ruey Hsu <henryhsu@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 4 ++++
> >  drivers/media/usb/uvc/uvc_video.c  | 2 ++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index d46dc432456c..a9658f38c586
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2287,6 +2287,8 @@ static int uvc_clock_param_get(char *buffer, const
> > struct kernel_param *kp) {
> >       if (uvc_clock_param == CLOCK_MONOTONIC)
> >               return sprintf(buffer, "CLOCK_MONOTONIC");
> > +     else if (uvc_clock_param == CLOCK_BOOTTIME)
> > +             return sprintf(buffer, "CLOCK_BOOTTIME");
> >       else
> >               return sprintf(buffer, "CLOCK_REALTIME");
> >  }
> > @@ -2298,6 +2300,8 @@ static int uvc_clock_param_set(const char *val, const
> > struct kernel_param *kp)
> >
> >       if (strcasecmp(val, "monotonic") == 0)
> >               uvc_clock_param = CLOCK_MONOTONIC;
> > +     else if (strcasecmp(val, "boottime") == 0)
> > +             uvc_clock_param = CLOCK_BOOTTIME;
> >       else if (strcasecmp(val, "realtime") == 0)
> >               uvc_clock_param = CLOCK_REALTIME;
> >       else
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index 86a99f461fd8..d4248d5cd9cd 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -425,6 +425,8 @@ static inline ktime_t uvc_video_get_time(void)
> >  {
> >       if (uvc_clock_param == CLOCK_MONOTONIC)
> >               return ktime_get();
> > +     else if (uvc_clock_param == CLOCK_BOOTTIME)
> > +             return ktime_get_boottime();
> >       else
> >               return ktime_get_real();
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

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

[Laurent Pinchart]

Hi Tomasz,

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.

> Generally, I'd see a V4L2_BUF_FLAG_TIMESTAMP_BOOTTIME flag being added
> and user space being given choice to select the time stamp base it
> needs, perhaps by setting the flag in v4l2_buffer struct at QBUF time?

I would indeed prefer a mechanism specified at the V4L2 API level, with an 
implementation in the V4L2 core, over a module parameter. If the goal is to 
use the boottime clock for synchronization purpose, we need to make sure that 
all drivers will support it correctly. Patching drivers one by one doesn't 
really scale.

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

On Thu, Oct 18, 2018 at 5:50 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> 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?

>
> > Generally, I'd see a V4L2_BUF_FLAG_TIMESTAMP_BOOTTIME flag being added
> > and user space being given choice to select the time stamp base it
> > needs, perhaps by setting the flag in v4l2_buffer struct at QBUF time?
>
> I would indeed prefer a mechanism specified at the V4L2 API level, with an
> implementation in the V4L2 core, over a module parameter. If the goal is to
> use the boottime clock for synchronization purpose, we need to make sure that
> all drivers will support it correctly. Patching drivers one by one doesn't
> really scale.

Agreed.

Best regards,
Tomasz

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

[Alexandru M Stan]

On Wed, Oct 17, 2018 at 9:31 PM, Tomasz Figa <tfiga@chromium.org> wrote:
> On Thu, Oct 18, 2018 at 5:50 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Tomasz,
>>
>> 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.

Regards,
Alexandru Stan

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

[Laurent Pinchart]

Hi Alexandru,

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, 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 ? :-)

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

On Thu, Nov 1, 2018 at 11:03 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Alexandru,
>
> 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.

> 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.

Best regards,
Tomasz

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

[Lars-Peter Clausen]

On 11/01/2018 03:30 PM, Tomasz Figa wrote:
> On Thu, Nov 1, 2018 at 11:03 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Alexandru,
>>
>> 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.
> 
>> 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.

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

[Tomasz Figa]

Hi Laurent,

On Fri, Nov 2, 2018 at 12:03 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 11/01/2018 03:30 PM, Tomasz Figa wrote:
> > On Thu, Nov 1, 2018 at 11:03 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> Hi Alexandru,
> >>
> >> 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.
> >
> >> 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?

Best regards,
Tomasz

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

[Tomasz Figa]

On Fri, Nov 23, 2018 at 11:46 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Laurent,
>
> On Fri, Nov 2, 2018 at 12:03 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> >
> > On 11/01/2018 03:30 PM, Tomasz Figa wrote:
> > > On Thu, Nov 1, 2018 at 11:03 PM Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> wrote:
> > >>
> > >> Hi Alexandru,
> > >>
> > >> 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.
> > >
> > >> 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?

Gentle ping.

Best regards,
Tomasz

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

[Laurent Pinchart]

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.
> >>
> >>> 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.

-- 
Regards,

Laurent Pinchart

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

[Tomasz Figa]

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.
> > >>
> > >>> 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.

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?

Best regards,
Tomasz

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

[Tomasz Figa]

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.
> > > >>
> > > >>> 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.
>
> 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?

Any thoughts?

Adding Hans and Kieran for more insight.

Best regards,
Tomasz

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

[Kieran Bingham]

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

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

[Tomasz Figa]

On Tue, Aug 6, 2019 at 5:34 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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?

The existing applications would expect the timestamps to come from
CLOCK_MONOTONIC, so I believe that we can't make CLOCK_BOOTTIME the
default.

> 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>
>'

Indeed.

>
> > 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 ...
>

What would be an example of such a non-live source?

>
> > 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.
>

Your input is very valuable. Thanks a lot! :)

>
> > Best regards,
> > Tomasz
> >
>
> --
> Regards
> --
> Kieran