[libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report sensor timestamp

[Niklas Söderlund]

Hi Jacopo,

On 2021-04-22 08:25:03 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Apr 21, 2021 at 08:37:18PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > On 2021-04-21 18:03:08 +0200, Jacopo Mondi wrote:
> > > Report the sensor's timestamp in the Request metadata by using the
> > > CIO2 buffer timestamp as an initial approximation.
> > >
> > > The buffer's timestamp is recorded at DMA-transfer time, and it does not
> > > theoretically matches the 'start of exposure' definition, but when used
> > > to compare two consecutive frames it gives an acceptable estimation of
> > > the sensor frame period duration.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 51446fcf5bc1..28e849a43a3e 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1255,6 +1255,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> > >
> > >  	Request *request = info->request;
> > >
> > > +	/*
> > > +	 * Record the sensor's timestamp in the request metadata.
> > > +	 *
> > > +	 * \todo The sensor timestamp should be better estimated by connecting
> > > +	 * to the V4L2Device::frameStart signal.
> > > +	 */
> >
> > I'm OK with this patch as is, but just in case you missed it. The signal
> > is already wired to CIO2Device::frameStart() is and used to feed the
> > DelayedControls state machine. So all the building blocks for this todo
> > I think is already in place,
> >
> >     void PipelineHandlerIPU3::stamp(uint32_t sequence)
> >     {
> >         IPU3Frames::Info *info = frameInfos_.find(buffer);
> >
> >         ...
> >
> >         info->request->metadata().set(controls::SensorTimestamp, ...);
> >     }
> >
> >     int PipelineHandlerIPU3::registerCameras()
> >     {
> >         ....
> >
> >         data->cio2_.frameStart().connect(this, &PipelineHandlerIPU3::stamp);
> >
> >         ....
> >     }
> 
> I've seen that, but plumbing in the infrastructure for timestamping in
> the library was considered not necessary at this time
> 
> >
> > That being said I think we might get better values using the buffer
> > timestamp as done in this patch, if nothing else less jitter. Only draw
> > back I can think of is that we can't grantee the timestamp coming from
> > the kernel relates to BOOTTIME.
> >
> 
> I keep hitting walls all over the places....
> Timestamping is performed in the cio2 receiver driver with:
> 
>                 ns = ktime_get_ns()
> 
> ktime_get() documentation reads as:
> 
>         /*
>         * ktime_get() family: read the current time in a multitude of ways,
>         *
>         * The default time reference is CLOCK_MONOTONIC, starting at
>         * boot time but not counting the time spent in suspend.
>         * For other references, use the functions with "real", "clocktai",
>         * "boottime" and "raw" suffixes.
>         *
>         * To get the time in a different format, use the ones wit
>         * "ns", "ts64" and "seconds" suffix.
>         *
>         * See Documentation/core-api/timekeeping.rst for more details.
>         */
> 
> I recall I repeated multiple times the reference clock was BOOTTIME,
> not sure because I overlooked it or because at the time (last week...)
> I had no real understanding of the differences between MONOTONIC and
> BOOTTIME...
> 
> Should we change the definition of the control again to support
> multiple time sources, and add one property to report which reference
> is used ?

I'm a bit slow and I still don't understand the need for users to know 
what the reference clock is used :-) Is it not enough that it have a ts 
it can diff between two exposers and get a usable delta in a known time 
unit? And of course that it's not subjected to the "fun" of NTP clock 
adjusting mechanisms.

I don't want to distract for the real work in this patch and as I said 
I'm fine with the patch as is, only wanted to point out that the todo 
was ready to be acted on. And that IMHO I think we would gain more from 
the solution without the todo addressed, or of course change the SOE 
signal API to include a ts from the kernel set at the IRQ. My fear being 
that if we produce the ts in user-space it will be subjected to jitter 
that will be worse then using the buffer ts as done here.

> 
> > > +	request->metadata().set(controls::SensorTimestamp,
> > > +				buffer->metadata().timestamp);
> > > +
> > >  	/* If the buffer is cancelled force a complete of the whole request. */
> > >  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > >  		for (auto it : request->buffers())
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

-- 
Regards,
Niklas Söderlund