Hi Hans: Appreciate your comments on this patch. > From: Hans Verkuil [mailto:hverkuil-cisco@xxxxxxxxx] > Sent: Tuesday, January 07, 2020 10:11 PM > To: jungo.lin@xxxxxxxxxxxx; tfiga@xxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx; matthias.bgg@xxxxxxxxx; mchehab@xxxxxxxxxx > Cc: linux-media@xxxxxxxxxxxxxxx; linux-mediatek@xxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; srv_heupstream@xxxxxxxxxxxx; ddavenport@xxxxxxxxxxxx; robh@xxxxxxxxxx; Sean.Cheng@xxxxxxxxxxxx; sj.huang@xxxxxxxxxxxx; Frederic.Chen@xxxxxxxxxxxx; Jerry-ch.Chen@xxxxxxxxxxxx; Frankie.Chiu@xxxxxxxxxxxx; ryan.yu@xxxxxxxxxxxx; Rynn.Wu@xxxxxxxxxxxx; yuzhao@xxxxxxxxxxxx; zwisler@xxxxxxxxxxxx; shik@xxxxxxxxxxxx; suleiman@xxxxxxxxxxxx > 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@xxxxxxxxxxxx> > > --- > > 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 > >
