Re: Re: [v6, 3/5] media: videodev2.h: Add new boottime timestamp type

[Jungo Lin <jungo.lin@mediatek.com>]

Hi Hans:

Appreciate your comments on this patch.

> From: Hans Verkuil [mailto:hverkuil-cisco@xs4all.nl]
> Sent: Tuesday, January 07, 2020 10:11 PM
> To: jungo.lin@mediatek.com; tfiga@chromium.org; laurent.pinchart@ideasonboard.com; matthias.bgg@gmail.com; mchehab@kernel.org
> Cc: linux-media@vger.kernel.org; linux-mediatek@lists.infradead.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; srv_heupstream@mediatek.com; ddavenport@chromium.org; robh@kernel.org; Sean.Cheng@mediatek.com; sj.huang@mediatek.com; Frederic.Chen@mediatek.com; Jerry-ch.Chen@mediatek.com; Frankie.Chiu@mediatek.com; ryan.yu@mediatek.com; Rynn.Wu@mediatek.com; yuzhao@chromium.org; zwisler@chromium.org; shik@chromium.org; suleiman@chromium.org
> Subject: Re: [v6, 3/5] media: videodev2.h: Add new boottime timestamp type
> 
> On 12/19/19 6:49 AM, Jungo Lin wrote:
> > For Camera AR(Augmented Reality) application requires camera
> > timestamps to be reported with CLOCK_BOOTTIME to sync timestamp with
> > other sensor sources.
> >
> > The boottime timestamp is identical to monotonic timestamp, except it
> > also includes any time that the system is suspended.
> >
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> > Changes from v6:
> >  - No change.
> > ---
> >  Documentation/media/uapi/v4l/buffer.rst | 11 ++++++++++-
> >  include/uapi/linux/videodev2.h          |  2 ++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/media/uapi/v4l/buffer.rst
> > b/Documentation/media/uapi/v4l/buffer.rst
> > index 9149b57728e5..f45bfce7fddd 100644
> > --- a/Documentation/media/uapi/v4l/buffer.rst
> > +++ b/Documentation/media/uapi/v4l/buffer.rst
> > @@ -662,13 +662,22 @@ Buffer Flags
> >        - 0x00002000
> >        - The buffer timestamp has been taken from the ``CLOCK_MONOTONIC``
> >  clock. To access the same clock outside V4L2, use
> > -:c:func:`clock_gettime`.
> > +:c:func:`clock_gettime` using clock IDs ``CLOCK_MONOTONIC``.
> 
> IDs -> ID
> 

Ok, fix in next version.

> >      * .. _`V4L2-BUF-FLAG-TIMESTAMP-COPY`:
> >
> >        - ``V4L2_BUF_FLAG_TIMESTAMP_COPY``
> >        - 0x00004000
> >        - The CAPTURE buffer timestamp has been taken from the corresponding
> >  OUTPUT buffer. This flag applies only to mem2mem devices.
> > +    * .. _`V4L2_BUF_FLAG_TIMESTAMP_BOOTIME`:
> 
> You mistyped BOOTTIME as BOOTIME in a lot of places. Please check.
> 

Ok, fix this typo in next version.

> > +
> > +      - ``V4L2_BUF_FLAG_TIMESTAMP_BOOTIME``
> > +      - 0x00008000
> > +      - The buffer timestamp has been taken from the ``CLOCK_BOOTTIME``
> > +clock. To access the same clock outside V4L2, use
> > +:c:func:`clock_gettime` using clock IDs ``CLOCK_BOOTTIME``.
> 
> IDs -> ID
> 

Ditto.

> > +Identical to CLOCK_MONOTONIC, except it also includes any time that
> > +the system is suspended.
> >      * .. _`V4L2-BUF-FLAG-TSTAMP-SRC-MASK`:
> >
> >        - ``V4L2_BUF_FLAG_TSTAMP_SRC_MASK`` diff --git
> > a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 04481c717fee..74ef9472e702 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1060,6 +1060,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> >  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN0x00000000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC0x00002000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_COPY0x00004000
> > +#define V4L2_BUF_FLAG_TIMESTAMP_BOOTIME0x00008000
> 
> This should be 0x00006000.
> 
> (flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) is a value that determines the timestamp source, so these timestamp defines are values, not bitmasks.
> 
> However, I don't like your approach. Whether to use MONOTONIC or BOOTTIME is really a userspace decision, and locking a driver to one of these two options seems wrong to me.
> 
> Instead add new V4L2_BUF_FLAG_USE_BOOTTIME flag that userspace can set when queuing the buffer and that indicates that instead of the MONOTONIC timestamp, it should return the BOOTTIME timestamp. This requires a simple helper function that returns either ktime_get_ns or ktime_get_boottime_ns based on the vb2_v4l2_buffer flags field.
> 
> It's definitely more work (although it can be limited to drivers that use vb2), but much more useful.
> 
> Regards,
> 
> Hans
> 

Agree.
We will add new V4L2_BUF_FLAG_USE_BOOTTIME flag (0x00006000.) to replace
this V4L2_BUF_FLAG_TIMESTAMP_BOOTIME flag for better usage.

Sincerely

Jungo

> > +
> >  /* Timestamp sources. */
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK0x00070000
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF0x00000000
> >