| Message ID | 20210419131433.22920-3-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 19/04/2021 14:14, 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: 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 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. Shouldn't that be handled by the sensor driver itself then? What defines the temporal location/point of the timestamp that is returned currently? I would have thought getting the time at the frameStart() signal would be susceptible to more latency due to the signal handling and calling up to userspace? This must surely be something better handled by the kernel drivers right? > + */ > + 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()) >
On 19/04/2021 14:35, Kieran Bingham wrote: > Hi Jacopo, > > On 19/04/2021 14:14, 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: 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 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. > > Shouldn't that be handled by the sensor driver itself then? > > What defines the temporal location/point of the timestamp that is > returned currently? > > I would have thought getting the time at the frameStart() signal would > be susceptible to more latency due to the signal handling and calling up > to userspace? > > This must surely be something better handled by the kernel drivers right? > > >> + */ >> + request->metadata().set(controls::SensorTimestamp, >> + buffer->metadata().timestamp); But for this.... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> if the todo is correctly applicable then of course it can stay ;) >> + >> /* If the buffer is cancelled force a complete of the whole request. */ >> if (buffer->metadata().status == FrameMetadata::FrameCancelled) { >> for (auto it : request->buffers()) >> >
Hi Kieran, On Mon, Apr 19, 2021 at 02:35:27PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 19/04/2021 14:14, 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: 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 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. > > Shouldn't that be handled by the sensor driver itself then? > I'm not aware of a mechanism to transport that information from the subdevice up, nor from what I've see the typical sensor driver has a mean to connect to that event and timestamp it. We could rely on the receiver's SOF interrupt, from which usually the V4L2_EVENT_FRAME_SYNC is signaled to userspace > What defines the temporal location/point of the timestamp that is > returned currently? > It depends on the platform. When the buffer whose timestamp is used to populate the SensorTimestamp property is the application facing buffer (uvc, simple, rkisp1) it is usually the time whene the EOF interrupt is triggered. Platforms with an off-line design where the raw frame is accessible to libcamera use the raw frametimestamp, which is for the CIO2 in example, the EOF interrupt time of the raw frame. > I would have thought getting the time at the frameStart() signal would > be susceptible to more latency due to the signal handling and calling up > to userspace? Probably yes, I cannot really quantify that at the moment.. > > This must surely be something better handled by the kernel drivers right? > Buffers are timestamped by the kernel. Thing is, even when we get the raw frame timestamp it usually refers to the end of the DMA transfer, not the start of the exposure. > > > + */ > > + 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()) > > > > -- > Regards > -- > Kieran
Hello, On Mon, Apr 19, 2021 at 03:53:42PM +0200, Jacopo Mondi wrote: > On Mon, Apr 19, 2021 at 02:35:27PM +0100, Kieran Bingham wrote: > > On 19/04/2021 14:14, 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: 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 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. > > > > Shouldn't that be handled by the sensor driver itself then? > > I'm not aware of a mechanism to transport that information from the > subdevice up, nor from what I've see the typical sensor driver has a > mean to connect to that event and timestamp it. > > We could rely on the receiver's SOF interrupt, from which usually the > V4L2_EVENT_FRAME_SYNC is signaled to userspace It's currently messy in V4L2. Drivers report timestamps in different ways, and most of them don't match the specification. The specification itself has some useful features (a driver can for instance opt to report the SOF timestamp if it has access to that information, but whether the driver does so or not can't be queried). I think we'll have to solve this in V4L2 first. > > What defines the temporal location/point of the timestamp that is > > returned currently? > > It depends on the platform. When the buffer whose timestamp is used to > populate the SensorTimestamp property is the application facing buffer > (uvc, simple, rkisp1) it is usually the time whene the EOF interrupt > is triggered. Platforms with an off-line design where the raw frame is > accessible to libcamera use the raw frametimestamp, which is for the > CIO2 in example, the EOF interrupt time of the raw frame. > > > I would have thought getting the time at the frameStart() signal would > > be susceptible to more latency due to the signal handling and calling up > > to userspace? > > Probably yes, I cannot really quantify that at the moment.. > > > This must surely be something better handled by the kernel drivers right? > > Buffers are timestamped by the kernel. Thing is, even when we get the > raw frame timestamp it usually refers to the end of the DMA transfer, > not the start of the exposure. When it comes to what drivers should do, I'd say they should provide enough information for userspace to obtain the most accurate timestamp, but not more. We will need to implement clock recovery logic based on statistics for some platforms, and that doesn't belong to kernelspace. > > > + */ > > > + 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())
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. + */ + 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())