| Message ID | 20220329112929.465434-2-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28) > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and > connect the timeout signal to a slot in the pipeline handler. This slot will > log a fatal error message informing the user of a possible hardware stall. No longer a fatal error message. > > The timeout is calculated as 2x the maximum frame length possible for a given > mode, returned by the IPA. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/ipa/raspberrypi.mojom | 1 + > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++ > 3 files changed, 18 insertions(+) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs; > }; > > interface IPARPiInterface { > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 1bf4e270b062..89767a9db471 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > } > > startConfig->dropFrameCount = dropFrameCount_; > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > + startConfig->maxSensorFrameLengthMs = 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 8fd79be67988..93931aaf3b55 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -208,6 +208,7 @@ public: > void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > void setSensorControls(ControlList &controls); > + void unicamTimeout(); > > /* bufferComplete signal handlers. */ > void unicamBufferDequeue(FrameBuffer *buffer); > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > } > } > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > + std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2); > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec); > + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms."; > + > return 0; > } > > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > /* Wire up all the buffer connections. */ > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout); > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls) > sensor_->setControls(&controls); > } > > +void RPiCameraData::unicamTimeout() > +{ > + LOG(RPI, Error) > + << "Unicam has timed out!\n" > + "Please check that your camera sensor connector is attached securely.\n" The '\n' should probably be std::endl ... but I don't think that makes a big difference here? Both of those could be fixed while applying, there's nothing complex there. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + "Alternatively, try another cable and/or sensor."; > +} > + > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr; > -- > 2.25.1 >
On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via libcamera-devel wrote: > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28) > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and > > connect the timeout signal to a slot in the pipeline handler. This slot will > > log a fatal error message informing the user of a possible hardware stall. > > No longer a fatal error message. > > > The timeout is calculated as 2x the maximum frame length possible for a given > > mode, returned by the IPA. I mentioned in the review of v1 that we should probably increase the timeout (to 10 times the frame duration). That was based on the error message being fatal though, but maybe it's still good to avoid a short watchdog timeout ? > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++ > > 3 files changed, 18 insertions(+) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs; > > }; > > > > interface IPARPiInterface { > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 1bf4e270b062..89767a9db471 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > } > > > > startConfig->dropFrameCount = dropFrameCount_; > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > > + startConfig->maxSensorFrameLengthMs = 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 8fd79be67988..93931aaf3b55 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -208,6 +208,7 @@ public: > > void setIspControls(const ControlList &controls); > > void setDelayedControls(const ControlList &controls); > > void setSensorControls(ControlList &controls); > > + void unicamTimeout(); > > > > /* bufferComplete signal handlers. */ > > void unicamBufferDequeue(FrameBuffer *buffer); > > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > } > > } > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > > + std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2); s/msec/timeout/ > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec); > > + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms."; I may have dropped this message, I'm not sure it helps much. > > + > > return 0; > > } > > > > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > > > /* Wire up all the buffer connections. */ > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout); > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); > > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls) > > sensor_->setControls(&controls); > > } > > > > +void RPiCameraData::unicamTimeout() > > +{ > > + LOG(RPI, Error) > > + << "Unicam has timed out!\n" > > + "Please check that your camera sensor connector is attached securely.\n" > > The '\n' should probably be std::endl ... but I don't think that makes a > big difference here? It would still be good, for consistency. > Both of those could be fixed while applying, there's nothing complex > there. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + "Alternatively, try another cable and/or sensor."; > > +} > > + > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > { > > RPi::Stream *stream = nullptr;
On Tue, Apr 05, 2022 at 06:34:21PM +0300, Laurent Pinchart via libcamera-devel wrote: > On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via libcamera-devel wrote: > > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28) > > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and > > > connect the timeout signal to a slot in the pipeline handler. This slot will > > > log a fatal error message informing the user of a possible hardware stall. > > > > No longer a fatal error message. > > > > > The timeout is calculated as 2x the maximum frame length possible for a given > > > mode, returned by the IPA. > > I mentioned in the review of v1 that we should probably increase the > timeout (to 10 times the frame duration). That was based on the error > message being fatal though, but maybe it's still good to avoid a short > watchdog timeout ? > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++ > > > 3 files changed, 18 insertions(+) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > > index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs; > > > }; > > > > > > interface IPARPiInterface { > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 1bf4e270b062..89767a9db471 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > > } > > > > > > startConfig->dropFrameCount = dropFrameCount_; > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > > > + startConfig->maxSensorFrameLengthMs = 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 8fd79be67988..93931aaf3b55 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -208,6 +208,7 @@ public: > > > void setIspControls(const ControlList &controls); > > > void setDelayedControls(const ControlList &controls); > > > void setSensorControls(ControlList &controls); > > > + void unicamTimeout(); > > > > > > /* bufferComplete signal handlers. */ > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > } > > > } > > > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > > > + std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2); > > s/msec/timeout/ > > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec); > > > + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms."; > > I may have dropped this message, I'm not sure it helps much. > > > > + > > > return 0; > > > } > > > > > > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > > > > > /* Wire up all the buffer connections. */ > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout); > > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > > > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > > > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); > > > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls) > > > sensor_->setControls(&controls); > > > } > > > > > > +void RPiCameraData::unicamTimeout() > > > +{ > > > + LOG(RPI, Error) > > > + << "Unicam has timed out!\n" > > > + "Please check that your camera sensor connector is attached securely.\n" > > > > The '\n' should probably be std::endl ... but I don't think that makes a > > big difference here? > > It would still be good, for consistency. Also, a \n in the middle of the log may be a bit messy, as the next lines will not have the right prefixes. We could add some sort of utils::logEndl that would do the right thing (but that's out of scope for this series), or maybe shorten the message to one line here, or even print multiple messages ? > > Both of those could be fixed while applying, there's nothing complex > > there. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + "Alternatively, try another cable and/or sensor."; > > > +} > > > + > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > { > > > RPi::Stream *stream = nullptr; > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Thank you for your feedback. On Tue, 5 Apr 2022 at 16:34, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via > libcamera-devel wrote: > > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28) > > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, > and > > > connect the timeout signal to a slot in the pipeline handler. This > slot will > > > log a fatal error message informing the user of a possible hardware > stall. > > > > No longer a fatal error message. > > > > > The timeout is calculated as 2x the maximum frame length possible for > a given > > > mode, returned by the IPA. > > I mentioned in the review of v1 that we should probably increase the > timeout (to 10 times the frame duration). That was based on the error > message being fatal though, but maybe it's still good to avoid a short > watchdog timeout ? > I did wonder what to do here for the timeout limit. Having 10x frame duration is going to be very long for imx477 that has a 200s frame length. I think given this is only a log message I'm inclined to leave it short to catch the obvious failure case. If I get complaints of too many false positives we can bump this up a bit :-) I'll fix up all the suggested changes and post a new revision shortly. Regards, Naush > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++ > > > 3 files changed, 18 insertions(+) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > > index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs; > > > }; > > > > > > interface IPARPiInterface { > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > > index 1bf4e270b062..89767a9db471 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, > ipa::RPi::StartConfig *startConf > > > } > > > > > > startConfig->dropFrameCount = dropFrameCount_; > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length > * mode_.line_length; > > > + startConfig->maxSensorFrameLengthMs = > 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 8fd79be67988..93931aaf3b55 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -208,6 +208,7 @@ public: > > > void setIspControls(const ControlList &controls); > > > void setDelayedControls(const ControlList &controls); > > > void setSensorControls(ControlList &controls); > > > + void unicamTimeout(); > > > > > > /* bufferComplete signal handlers. */ > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > > } > > > } > > > > > > + /* Set the dequeue timeout to 2x the maximum possible frame > duration. */ > > > + std::chrono::milliseconds > msec(startConfig.maxSensorFrameLengthMs * 2); > > s/msec/timeout/ > > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec); > > > + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " > ms."; > > I may have dropped this message, I'm not sure it helps much. > > > > + > > > return 0; > > > } > > > > > > @@ -1192,6 +1198,7 @@ int > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > > > > > /* Wire up all the buffer connections. */ > > > + > data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), > &RPiCameraData::unicamTimeout); > > > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), > &RPiCameraData::frameStarted); > > > > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), > &RPiCameraData::unicamBufferDequeue); > > > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), > &RPiCameraData::ispInputDequeue); > > > @@ -1773,6 +1780,14 @@ void > RPiCameraData::setSensorControls(ControlList &controls) > > > sensor_->setControls(&controls); > > > } > > > > > > +void RPiCameraData::unicamTimeout() > > > +{ > > > + LOG(RPI, Error) > > > + << "Unicam has timed out!\n" > > > + "Please check that your camera sensor connector is > attached securely.\n" > > > > The '\n' should probably be std::endl ... but I don't think that makes a > > big difference here? > > It would still be good, for consistency. > > > Both of those could be fixed while applying, there's nothing complex > > there. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + "Alternatively, try another cable and/or sensor."; > > > +} > > > + > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > { > > > RPi::Stream *stream = nullptr; > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Apr 06, 2022 at 08:12:26AM +0100, Naushir Patuck wrote: > On Tue, 5 Apr 2022 at 16:34, Laurent Pinchart wrote: > > On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via > > libcamera-devel wrote: > > > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28) > > > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and > > > > connect the timeout signal to a slot in the pipeline handler. This slot will > > > > log a fatal error message informing the user of a possible hardware stall. > > > > > > No longer a fatal error message. > > > > > > > The timeout is calculated as 2x the maximum frame length possible for a given > > > > mode, returned by the IPA. > > > > I mentioned in the review of v1 that we should probably increase the > > timeout (to 10 times the frame duration). That was based on the error > > message being fatal though, but maybe it's still good to avoid a short > > watchdog timeout ? > > I did wonder what to do here for the timeout limit. Having 10x frame duration is going to > be very long for imx477 that has a 200s frame length. I think given this is only a log message > I'm inclined to leave it short to catch the obvious failure case. If I get complaints of too many > false positives we can bump this up a bit :-) Ah right it's the maximum frame duration, not the current frame duration. Well, in that case I wonder how useful it will be with the IMX477, with a 400s timeout :-) That's fine, there's no need to worry about it. On the other hand, for sensors that have a small maximum frame duration, would it make sense to set the timeout to, for instance, std::max(1s, max frame duration * 2) ? > I'll fix up all the suggested changes and post a new revision shortly. > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++ > > > > 3 files changed, 18 insertions(+) > > > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > > > index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs; > > > > }; > > > > > > > > interface IPARPiInterface { > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index 1bf4e270b062..89767a9db471 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf > > > > } > > > > > > > > startConfig->dropFrameCount = dropFrameCount_; > > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; > > > > + startConfig->maxSensorFrameLengthMs = 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 8fd79be67988..93931aaf3b55 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -208,6 +208,7 @@ public: > > > > void setIspControls(const ControlList &controls); > > > > void setDelayedControls(const ControlList &controls); > > > > void setSensorControls(ControlList &controls); > > > > + void unicamTimeout(); > > > > > > > > /* bufferComplete signal handlers. */ > > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > > } > > > > } > > > > > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > > > > + std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2); > > > > s/msec/timeout/ > > > > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec); > > > > + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms."; > > > > I may have dropped this message, I'm not sure it helps much. > > > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > > > data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > > > > > > > /* Wire up all the buffer connections. */ > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout); > > > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > > > > data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); > > > > data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); > > > > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls) > > > > sensor_->setControls(&controls); > > > > } > > > > > > > > +void RPiCameraData::unicamTimeout() > > > > +{ > > > > + LOG(RPI, Error) > > > > + << "Unicam has timed out!\n" > > > > + "Please check that your camera sensor connector is attached securely.\n" > > > > > > The '\n' should probably be std::endl ... but I don't think that makes a > > > big difference here? > > > > It would still be good, for consistency. > > > > > Both of those could be fixed while applying, there's nothing complex > > > there. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > + "Alternatively, try another cable and/or sensor."; > > > > +} > > > > + > > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > { > > > > RPi::Stream *stream = nullptr;
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index acd3cafe6c91..5a228b75cd2f 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 maxSensorFrameLengthMs; }; interface IPARPiInterface { diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 1bf4e270b062..89767a9db471 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf } startConfig->dropFrameCount = dropFrameCount_; + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length; + startConfig->maxSensorFrameLengthMs = 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 8fd79be67988..93931aaf3b55 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -208,6 +208,7 @@ public: void setIspControls(const ControlList &controls); void setDelayedControls(const ControlList &controls); void setSensorControls(ControlList &controls); + void unicamTimeout(); /* bufferComplete signal handlers. */ void unicamBufferDequeue(FrameBuffer *buffer); @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) } } + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ + std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2); + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec); + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms."; + return 0; } @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); /* Wire up all the buffer connections. */ + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout); data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue); @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls) sensor_->setControls(&controls); } +void RPiCameraData::unicamTimeout() +{ + LOG(RPI, Error) + << "Unicam has timed out!\n" + "Please check that your camera sensor connector is attached securely.\n" + "Alternatively, try another cable and/or sensor."; +} + void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) { RPi::Stream *stream = nullptr;
Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and connect the timeout signal to a slot in the pipeline handler. This slot will log a fatal error message informing the user of a possible hardware stall. The timeout is calculated as 2x the maximum frame length possible for a given mode, returned by the IPA. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/ipa/raspberrypi.mojom | 1 + src/ipa/raspberrypi/raspberrypi.cpp | 2 ++ .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++ 3 files changed, 18 insertions(+)