[libcamera-devel,v4,05/16] libcamera: ipu3: Report sensor timestamp
diff mbox series

Message ID 20210430160026.190724-6-jacopo@jmondi.org
State Superseded
Headers show
Series
  • Support SensorTimestamp metadata
Related show

Commit Message

Jacopo Mondi April 30, 2021, 4 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Niklas Söderlund May 1, 2021, 6:26 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2021-04-30 18:00:15 +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@ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@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 73306cea6b37..88b7bd1e52c3 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 still believe getting the timestamp from the buffer as done in this 
patch is much better then creating a timestamp in user-space at the 
frameStart signal.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +	 */
> +	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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi May 1, 2021, 7:51 a.m. UTC | #2 
Hi Niklas,

On Sat, May 01, 2021 at 08:26:49AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2021-04-30 18:00:15 +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@ideasonboard.com>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@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 73306cea6b37..88b7bd1e52c3 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 still believe getting the timestamp from the buffer as done in this
> patch is much better then creating a timestamp in user-space at the
> frameStart signal.

Thing is the buffer is timestamped in kernel space at the end of the
DMA transfer in the CIO2 driver not at exposure begin time.

For sake of comparing frame durations I agree this is acceptable, but
contradicts the Control definition and this should be noted down imho.

>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> > +	 */
> > +	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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund May 1, 2021, 8:05 a.m. UTC | #3 
Hi Jacopo,

On 2021-05-01 09:51:03 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Sat, May 01, 2021 at 08:26:49AM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2021-04-30 18:00:15 +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@ideasonboard.com>
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@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 73306cea6b37..88b7bd1e52c3 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 still believe getting the timestamp from the buffer as done in this
> > patch is much better then creating a timestamp in user-space at the
> > frameStart signal.
> 
> Thing is the buffer is timestamped in kernel space at the end of the
> DMA transfer in the CIO2 driver not at exposure begin time.

My worry is that the jitter in user-space will render the timestamp 
unreliable and I don't see it as unreasonable that a timestamp created 
in user-space as a result of a SOE V4L2 event will be later then a 
timestamp from the kernels end of DMA.

If the V4L2 SOE event where to be augmented with a timestamp I agree 
that would be better then a timestamp from end of DMA.

> 
> For sake of comparing frame durations I agree this is acceptable, but
> contradicts the Control definition and this should be noted down imho.

Creating a timestamp in user-space for the SOE event also contradicts 
the Control definition. IMHO end of DMA and user-space ts are equally 
wrong with regard to the Control, but end of DMA is at least more 
deterministic. In a pinch 'end of DMA' - 'exposure time' would be a 
better estimate.

> 
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > > +	 */
> > > +	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@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
Jacopo Mondi May 1, 2021, 8:29 a.m. UTC | #4 
Hi Niklas,

On Sat, May 01, 2021 at 10:05:56AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2021-05-01 09:51:03 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Sat, May 01, 2021 at 08:26:49AM +0200, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2021-04-30 18:00:15 +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@ideasonboard.com>
> > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo@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 73306cea6b37..88b7bd1e52c3 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 still believe getting the timestamp from the buffer as done in this
> > > patch is much better then creating a timestamp in user-space at the
> > > frameStart signal.
> >
> > Thing is the buffer is timestamped in kernel space at the end of the
> > DMA transfer in the CIO2 driver not at exposure begin time.
>
> My worry is that the jitter in user-space will render the timestamp
> unreliable and I don't see it as unreasonable that a timestamp created
> in user-space as a result of a SOE V4L2 event will be later then a
> timestamp from the kernels end of DMA.

A full frame later ? 16ms @ 60FPS of jitter ? Are we planning to run
libcamera on an 8086 :) ?

>
> If the V4L2 SOE event where to be augmented with a timestamp I agree
> that would be better then a timestamp from end of DMA.
>
> >
> > For sake of comparing frame durations I agree this is acceptable, but
> > contradicts the Control definition and this should be noted down imho.
>
> Creating a timestamp in user-space for the SOE event also contradicts
> the Control definition. IMHO end of DMA and user-space ts are equally
> wrong with regard to the Control, but end of DMA is at least more
> deterministic. In a pinch 'end of DMA' - 'exposure time' would be a
> better estimate.

The exposure might well be smaller than the frame duration.

I agree that having a timestamp in the event would be best and that
timestamping in userspace is subject to variables out of our controls
(system load being the easiest one to mention).

Tbh I don't mind the \todo, it remember we should do better than this,
but possibly doing that in the library is not the best way forward I
agree.

>
> >
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >
> > > > +	 */
> > > > +	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@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund May 1, 2021, 9:02 a.m. UTC | #5 
Hi Jacopo,

On 2021-05-01 10:29:13 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Sat, May 01, 2021 at 10:05:56AM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > On 2021-05-01 09:51:03 +0200, Jacopo Mondi wrote:
> > > Hi Niklas,
> > >
> > > On Sat, May 01, 2021 at 08:26:49AM +0200, Niklas Söderlund wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thanks for your work.
> > > >
> > > > On 2021-04-30 18:00:15 +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@ideasonboard.com>
> > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Signed-off-by: Jacopo Mondi <jacopo@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 73306cea6b37..88b7bd1e52c3 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 still believe getting the timestamp from the buffer as done in this
> > > > patch is much better then creating a timestamp in user-space at the
> > > > frameStart signal.
> > >
> > > Thing is the buffer is timestamped in kernel space at the end of the
> > > DMA transfer in the CIO2 driver not at exposure begin time.
> >
> > My worry is that the jitter in user-space will render the timestamp
> > unreliable and I don't see it as unreasonable that a timestamp created
> > in user-space as a result of a SOE V4L2 event will be later then a
> > timestamp from the kernels end of DMA.
> 
> A full frame later ? 16ms @ 60FPS of jitter ?

Not a full frame later. My point was that the ts from user-space from a 
SOE might be larger then the ts in the CIO2 buffer due to jitter. All 
that is needed for that to happen is that the user-space jitter is 
larger then the difference from SOE to end of DMA right?

> Are we planning to run libcamera on an 8086 :) ?

I have a 8086 prototype board from way back when, it however needs the 
program to be inputted using a hexadecimal keypad and I'm not sure I 
will manage to fit musl in there...

> 
> >
> > If the V4L2 SOE event where to be augmented with a timestamp I agree
> > that would be better then a timestamp from end of DMA.
> >
> > >
> > > For sake of comparing frame durations I agree this is acceptable, but
> > > contradicts the Control definition and this should be noted down imho.
> >
> > Creating a timestamp in user-space for the SOE event also contradicts
> > the Control definition. IMHO end of DMA and user-space ts are equally
> > wrong with regard to the Control, but end of DMA is at least more
> > deterministic. In a pinch 'end of DMA' - 'exposure time' would be a
> > better estimate.
> 
> The exposure might well be smaller than the frame duration.

My bad, yes it's probably better to subtract the frame duration then the 
expose time,

> 
> I agree that having a timestamp in the event would be best and that
> timestamping in userspace is subject to variables out of our controls
> (system load being the easiest one to mention).
> 
> Tbh I don't mind the \todo, it remember we should do better than this,
> but possibly doing that in the library is not the best way forward I
> agree.

I agree that todo is highlighting that we could do better and I don't 
mind that. I think that hinting that the V4L2Device::frameStart is ready 
to be the next step is wrong as without a kernel change to add a ts I 
think it would make things worse. But we are arguing about future 
changes so if you feel good about the todo you already have my tag and 
we can instead argue when we try to address the todo item :-)

> 
> >
> > >
> > > >
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > >
> > > > > +	 */
> > > > > +	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@lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> >
> > --
> > Regards,
> > Niklas Söderlund

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 73306cea6b37..88b7bd1e52c3 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.
+	 */
+	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()) {