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

From: Jungo Lin <jungo.lin@mediatek.com>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: <tfiga@chromium.org>, <laurent.pinchart@ideasonboard.com>,
	<matthias.bgg@gmail.com>, <mchehab@kernel.org>,
	<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
Date: Fri, 10 Jan 2020 18:08:15 +0800	[thread overview]
Message-ID: <1578650895.3348.18.camel@mtksdccf07> (raw)
In-Reply-To: <081d7d77-2bf7-0fc2-60ea-8ba8faa5b6af@xs4all.nl>

Hi Hans:

Appreciate your comments on this patch.

On Tue, 2020-01-07 at 15:10 +0100, Hans Verkuil wrote:
> 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_UNKNOWN		0x00000000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> > +#define V4L2_BUF_FLAG_TIMESTAMP_BOOTIME		0x00008000
> 
> 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.

> > +
> >  /* Timestamp sources. */
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
> > 
> 

Sincerely

Jungo