| Message ID | 20220321124828.1845058-1-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Quoting Naushir Patuck via libcamera-devel (2022-03-21 12:48:28) > Add a timer to the pipeline handler that get set on start(), and subsequently > reset on each frame dequeue from the sensor. If the timeout expires, log an > error message indicating so. This is useful for debugging sensor failures were > the device just stops streaming frames to Unicam, or the pipeline handler has > failed to queue buffers to the Unicam device driver. > > The timeout is calculated as 2x the maximum frame length possible for a given > mode. > This is an interesting idea, and I think will prove useful for identifying stalls in streams. Would it make sense to make this a feature of the V4L2VideoDevice? Then it would (automatically?) cover ISP hangs as well as sensor hangs... Perhaps the hard part is in making sure the correct timeout is determined or set? > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/ipa/raspberrypi.mojom | 1 + > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++ > .../pipeline/raspberrypi/raspberrypi.cpp | 22 +++++++++++++++++++ > 3 files changed, 30 insertions(+) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index acd3cafe6c91..33d2a97ca916 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -41,6 +41,7 @@ struct IPAConfig { > struct StartConfig { > libcamera.ControlList controls; > int32 dropFrameCount; > + uint32 sensorTimeoutMs; > }; > > interface IPARPiInterface { > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index fd8fecb07f81..983d6e998b4c 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -281,6 +281,13 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > startConfig->dropFrameCount = dropFrameCount_; > > + /* > + * Set the pipeline handler's sensor timeout to 2x the maximum possible > + * frame duration for this mode. > + */ > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > + startConfig->sensorTimeoutMs = 2 * maxSensorFrameDuration.get<std::milli>(); > + > firstStart_ = false; > lastRunTimestamp_ = 0; > } > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index c2230199fed7..86d952b52aed 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -15,6 +15,7 @@ > #include <utility> > > #include <libcamera/base/shared_fd.h> > +#include <libcamera/base/timer.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -202,6 +203,7 @@ public: > void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > void setSensorControls(ControlList &controls); > + void sensorTimeout(); > > /* bufferComplete signal handlers. */ > void unicamBufferDequeue(FrameBuffer *buffer); > @@ -279,6 +281,10 @@ public: > */ > std::optional<int32_t> notifyGainsUnity_; > > + /* Timer to ensure the sensor is producing frames for the pipeline handler. */ > + Timer sensorTimeout_; > + std::chrono::milliseconds sensorTimeoutDuration_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -1032,6 +1038,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > } > } > > + LOG(RPI, Debug) << "Setting sensor timeout to " << startConfig.sensorTimeoutMs << " ms"; > + data->sensorTimeoutDuration_ = std::chrono::milliseconds(startConfig.sensorTimeoutMs); > + data->sensorTimeout_.start(data->sensorTimeoutDuration_); > + data->sensorTimeout_.timeout.connect(data, &RPiCameraData::sensorTimeout); > + > return 0; > } > > @@ -1040,6 +1051,8 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > RPiCameraData *data = cameraData(camera); > > data->state_ = RPiCameraData::State::Stopped; > + data->sensorTimeout_.timeout.disconnect(); > + data->sensorTimeout_.stop(); > > /* Disable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > @@ -1757,6 +1770,12 @@ void RPiCameraData::setSensorControls(ControlList &controls) > sensor_->setControls(&controls); > } > > +void RPiCameraData::sensorTimeout() > +{ > + LOG(RPI, Error) << "Sensor has timed out after " > + << sensorTimeoutDuration_.count() << " ms!"; > +} > + > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr; > @@ -1792,6 +1811,9 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > */ > ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp); > bayerQueue_.push({ buffer, std::move(ctrl) }); > + > + /* Restart the sensor timer. */ > + sensorTimeout_.start(sensorTimeoutDuration_); > } else { > embeddedQueue_.push(buffer); > } > -- > 2.25.1 >
Hi Kieran, On Mon, 21 Mar 2022 at 13:13, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > Quoting Naushir Patuck via libcamera-devel (2022-03-21 12:48:28) > > Add a timer to the pipeline handler that get set on start(), and > subsequently > > reset on each frame dequeue from the sensor. If the timeout expires, log > an > > error message indicating so. This is useful for debugging sensor > failures were > > the device just stops streaming frames to Unicam, or the pipeline > handler has > > failed to queue buffers to the Unicam device driver. > > > > The timeout is calculated as 2x the maximum frame length possible for a > given > > mode. > > > > This is an interesting idea, and I think will prove useful for > identifying stalls in streams. > > Would it make sense to make this a feature of the V4L2VideoDevice? Then > it would (automatically?) cover ISP hangs as well as sensor hangs... > It would be nice to add this to a level above, and allow all pipeline handlers to gain this functionality! I can look to do that if we want to go this way. > > Perhaps the hard part is in making sure the correct timeout is > determined or set? > Well, I've already done it for the sensor in this patch :-) Adding it for the ISP is a bit simpler, as it is (normally) not mode dependent. Regards, Naush > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++ > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 +++++++++++++++++++ > > 3 files changed, 30 insertions(+) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > index acd3cafe6c91..33d2a97ca916 100644 > > --- a/include/libcamera/ipa/raspberrypi.mojom > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > @@ -41,6 +41,7 @@ struct IPAConfig { > > struct StartConfig { > > libcamera.ControlList controls; > > int32 dropFrameCount; > > + uint32 sensorTimeoutMs; > > }; > > > > interface IPARPiInterface { > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index fd8fecb07f81..983d6e998b4c 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -281,6 +281,13 @@ void IPARPi::start(const ControlList &controls, > ipa::RPi::StartConfig *startConf > > > > startConfig->dropFrameCount = dropFrameCount_; > > > > + /* > > + * Set the pipeline handler's sensor timeout to 2x the maximum > possible > > + * frame duration for this mode. > > + */ > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * > mode_.line_length; > > + startConfig->sensorTimeoutMs = 2 * > maxSensorFrameDuration.get<std::milli>(); > > + > > firstStart_ = false; > > lastRunTimestamp_ = 0; > > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index c2230199fed7..86d952b52aed 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -15,6 +15,7 @@ > > #include <utility> > > > > #include <libcamera/base/shared_fd.h> > > +#include <libcamera/base/timer.h> > > #include <libcamera/base/utils.h> > > > > #include <libcamera/camera.h> > > @@ -202,6 +203,7 @@ public: > > void setIspControls(const ControlList &controls); > > void setDelayedControls(const ControlList &controls); > > void setSensorControls(ControlList &controls); > > + void sensorTimeout(); > > > > /* bufferComplete signal handlers. */ > > void unicamBufferDequeue(FrameBuffer *buffer); > > @@ -279,6 +281,10 @@ public: > > */ > > std::optional<int32_t> notifyGainsUnity_; > > > > + /* Timer to ensure the sensor is producing frames for the > pipeline handler. */ > > + Timer sensorTimeout_; > > + std::chrono::milliseconds sensorTimeoutDuration_; > > + > > private: > > void checkRequestCompleted(); > > void fillRequestMetadata(const ControlList &bufferControls, > > @@ -1032,6 +1038,11 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > } > > } > > > > + LOG(RPI, Debug) << "Setting sensor timeout to " << > startConfig.sensorTimeoutMs << " ms"; > > + data->sensorTimeoutDuration_ = > std::chrono::milliseconds(startConfig.sensorTimeoutMs); > > + data->sensorTimeout_.start(data->sensorTimeoutDuration_); > > + data->sensorTimeout_.timeout.connect(data, > &RPiCameraData::sensorTimeout); > > + > > return 0; > > } > > > > @@ -1040,6 +1051,8 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > RPiCameraData *data = cameraData(camera); > > > > data->state_ = RPiCameraData::State::Stopped; > > + data->sensorTimeout_.timeout.disconnect(); > > + data->sensorTimeout_.stop(); > > > > /* Disable SOF event generation. */ > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > @@ -1757,6 +1770,12 @@ void RPiCameraData::setSensorControls(ControlList > &controls) > > sensor_->setControls(&controls); > > } > > > > +void RPiCameraData::sensorTimeout() > > +{ > > + LOG(RPI, Error) << "Sensor has timed out after " > > + << sensorTimeoutDuration_.count() << " ms!"; > > +} > > + > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > { > > RPi::Stream *stream = nullptr; > > @@ -1792,6 +1811,9 @@ void > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > */ > > ctrl.set(controls::SensorTimestamp, > buffer->metadata().timestamp); > > bayerQueue_.push({ buffer, std::move(ctrl) }); > > + > > + /* Restart the sensor timer. */ > > + sensorTimeout_.start(sensorTimeoutDuration_); > > } else { > > embeddedQueue_.push(buffer); > > } > > -- > > 2.25.1 > > >
Hi Naush, On Mon, Mar 21, 2022 at 01:22:43PM +0000, Naushir Patuck via libcamera-devel wrote: > On Mon, 21 Mar 2022 at 13:13, Kieran Bingham wrote: > > Quoting Naushir Patuck via libcamera-devel (2022-03-21 12:48:28) > > > Add a timer to the pipeline handler that get set on start(), and subsequently > > > reset on each frame dequeue from the sensor. If the timeout expires, log an > > > error message indicating so. This is useful for debugging sensor failures were > > > the device just stops streaming frames to Unicam, or the pipeline handler has > > > failed to queue buffers to the Unicam device driver. > > > > > > The timeout is calculated as 2x the maximum frame length possible for a given > > > mode. > > > > This is an interesting idea, and I think will prove useful for > > identifying stalls in streams. > > > > Would it make sense to make this a feature of the V4L2VideoDevice? Then > > it would (automatically?) cover ISP hangs as well as sensor hangs... > > It would be nice to add this to a level above, and allow all pipeline handlers > to gain this functionality! I can look to do that if we want to go this way. That would be nice indeed. Let's be careful not to generate false positives though. I suppose the mechanism would need to be enabled by the V4L2VideoDevice user by setting a timeout, so the risk shouldn't be too high. > > Perhaps the hard part is in making sure the correct timeout is > > determined or set? > > Well, I've already done it for the sensor in this patch :-) Adding it for the > ISP is a bit simpler, as it is (normally) not mode dependent. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++ > > > .../pipeline/raspberrypi/raspberrypi.cpp | 22 +++++++++++++++++++ > > > 3 files changed, 30 insertions(+) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > > index acd3cafe6c91..33d2a97ca916 100644 > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > @@ -41,6 +41,7 @@ struct IPAConfig { > > > struct StartConfig { > > > libcamera.ControlList controls; > > > int32 dropFrameCount; > > > + uint32 sensorTimeoutMs; > > > }; > > > > > > interface IPARPiInterface { > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index fd8fecb07f81..983d6e998b4c 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -281,6 +281,13 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > > > > > startConfig->dropFrameCount = dropFrameCount_; > > > > > > + /* > > > + * Set the pipeline handler's sensor timeout to 2x the maximum possible > > > + * frame duration for this mode. > > > + */ > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > > > + startConfig->sensorTimeoutMs = 2 * maxSensorFrameDuration.get<std::milli>(); I don't like having the IPA instruct the pipeline handler. It may be considered as nitpicking, but I'd rather expose data from the API, (such as the frame duration for instance), and let the pipeline handler determine the timeout. Doesn't the pipeline handler already have access to the frame duration, as it's configurable by applications through libcamera controls ? > > > + > > > firstStart_ = false; > > > lastRunTimestamp_ = 0; > > > } > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index c2230199fed7..86d952b52aed 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -15,6 +15,7 @@ > > > #include <utility> > > > > > > #include <libcamera/base/shared_fd.h> > > > +#include <libcamera/base/timer.h> > > > #include <libcamera/base/utils.h> > > > > > > #include <libcamera/camera.h> > > > @@ -202,6 +203,7 @@ public: > > > void setIspControls(const ControlList &controls); > > > void setDelayedControls(const ControlList &controls); > > > void setSensorControls(ControlList &controls); > > > + void sensorTimeout(); > > > > > > /* bufferComplete signal handlers. */ > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > @@ -279,6 +281,10 @@ public: > > > */ > > > std::optional<int32_t> notifyGainsUnity_; > > > > > > + /* Timer to ensure the sensor is producing frames for the pipeline handler. */ > > > + Timer sensorTimeout_; > > > + std::chrono::milliseconds sensorTimeoutDuration_; > > > + > > > private: > > > void checkRequestCompleted(); > > > void fillRequestMetadata(const ControlList &bufferControls, > > > @@ -1032,6 +1038,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > } > > > } > > > > > > + LOG(RPI, Debug) << "Setting sensor timeout to " << startConfig.sensorTimeoutMs << " ms"; > > > + data->sensorTimeoutDuration_ = std::chrono::milliseconds(startConfig.sensorTimeoutMs); > > > + data->sensorTimeout_.start(data->sensorTimeoutDuration_); > > > + data->sensorTimeout_.timeout.connect(data, &RPiCameraData::sensorTimeout); > > > + > > > return 0; > > > } > > > > > > @@ -1040,6 +1051,8 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > > RPiCameraData *data = cameraData(camera); > > > > > > data->state_ = RPiCameraData::State::Stopped; > > > + data->sensorTimeout_.timeout.disconnect(); You don't have to connect/disconnect the signal when starting and stopping the camera, you can connect it when creating the RPiCameraData instance and keep it connected. > > > + data->sensorTimeout_.stop(); > > > > > > /* Disable SOF event generation. */ > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > @@ -1757,6 +1770,12 @@ void RPiCameraData::setSensorControls(ControlList &controls) > > > sensor_->setControls(&controls); > > > } > > > > > > +void RPiCameraData::sensorTimeout() > > > +{ > > > + LOG(RPI, Error) << "Sensor has timed out after " > > > + << sensorTimeoutDuration_.count() << " ms!"; > > > +} > > > + > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > { > > > RPi::Stream *stream = nullptr; > > > @@ -1792,6 +1811,9 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > */ > > > ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp); > > > bayerQueue_.push({ buffer, std::move(ctrl) }); > > > + > > > + /* Restart the sensor timer. */ > > > + sensorTimeout_.start(sensorTimeoutDuration_); > > > } else { > > > embeddedQueue_.push(buffer); > > > }
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index acd3cafe6c91..33d2a97ca916 100644 --- a/include/libcamera/ipa/raspberrypi.mojom +++ b/include/libcamera/ipa/raspberrypi.mojom @@ -41,6 +41,7 @@ struct IPAConfig { struct StartConfig { libcamera.ControlList controls; int32 dropFrameCount; + uint32 sensorTimeoutMs; }; interface IPARPiInterface { diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index fd8fecb07f81..983d6e998b4c 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -281,6 +281,13 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf startConfig->dropFrameCount = dropFrameCount_; + /* + * Set the pipeline handler's sensor timeout to 2x the maximum possible + * frame duration for this mode. + */ + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; + startConfig->sensorTimeoutMs = 2 * maxSensorFrameDuration.get<std::milli>(); + firstStart_ = false; lastRunTimestamp_ = 0; } diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index c2230199fed7..86d952b52aed 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -15,6 +15,7 @@ #include <utility> #include <libcamera/base/shared_fd.h> +#include <libcamera/base/timer.h> #include <libcamera/base/utils.h> #include <libcamera/camera.h> @@ -202,6 +203,7 @@ public: void setIspControls(const ControlList &controls); void setDelayedControls(const ControlList &controls); void setSensorControls(ControlList &controls); + void sensorTimeout(); /* bufferComplete signal handlers. */ void unicamBufferDequeue(FrameBuffer *buffer); @@ -279,6 +281,10 @@ public: */ std::optional<int32_t> notifyGainsUnity_; + /* Timer to ensure the sensor is producing frames for the pipeline handler. */ + Timer sensorTimeout_; + std::chrono::milliseconds sensorTimeoutDuration_; + private: void checkRequestCompleted(); void fillRequestMetadata(const ControlList &bufferControls, @@ -1032,6 +1038,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) } } + LOG(RPI, Debug) << "Setting sensor timeout to " << startConfig.sensorTimeoutMs << " ms"; + data->sensorTimeoutDuration_ = std::chrono::milliseconds(startConfig.sensorTimeoutMs); + data->sensorTimeout_.start(data->sensorTimeoutDuration_); + data->sensorTimeout_.timeout.connect(data, &RPiCameraData::sensorTimeout); + return 0; } @@ -1040,6 +1051,8 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) RPiCameraData *data = cameraData(camera); data->state_ = RPiCameraData::State::Stopped; + data->sensorTimeout_.timeout.disconnect(); + data->sensorTimeout_.stop(); /* Disable SOF event generation. */ data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); @@ -1757,6 +1770,12 @@ void RPiCameraData::setSensorControls(ControlList &controls) sensor_->setControls(&controls); } +void RPiCameraData::sensorTimeout() +{ + LOG(RPI, Error) << "Sensor has timed out after " + << sensorTimeoutDuration_.count() << " ms!"; +} + void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) { RPi::Stream *stream = nullptr; @@ -1792,6 +1811,9 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) */ ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp); bayerQueue_.push({ buffer, std::move(ctrl) }); + + /* Restart the sensor timer. */ + sensorTimeout_.start(sensorTimeoutDuration_); } else { embeddedQueue_.push(buffer); }
Add a timer to the pipeline handler that get set on start(), and subsequently reset on each frame dequeue from the sensor. If the timeout expires, log an error message indicating so. This is useful for debugging sensor failures were the device just stops streaming frames to Unicam, or the pipeline handler has failed to queue buffers to the Unicam device driver. The timeout is calculated as 2x the maximum frame length possible for a given mode. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/ipa/raspberrypi.mojom | 1 + src/ipa/raspberrypi/raspberrypi.cpp | 7 ++++++ .../pipeline/raspberrypi/raspberrypi.cpp | 22 +++++++++++++++++++ 3 files changed, 30 insertions(+)