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

[Jacopo Mondi]

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 ?

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