| Message ID | 20220322131635.2869526-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via libcamera-devel wrote: > 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(+) > > 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; We really need duration types in mojo. > }; > > interface IPARPiInterface { > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index fd8fecb07f81..675f9ba1b350 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 c2230199fed7..50b39f1adf93 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -45,6 +45,8 @@ > #include "dma_heaps.h" > #include "rpi_stream.h" > > +using namespace std::literals::chrono_literals; > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(RPI) > @@ -202,6 +204,7 @@ public: > void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > void setSensorControls(ControlList &controls); > + void unicamTimeout(V4L2VideoDevice *dev); > > /* bufferComplete signal handlers. */ > void unicamBufferDequeue(FrameBuffer *buffer); > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > } > } > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms; > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration); > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout); > + LOG(RPI, Debug) << "Setting sensor timeout to " << duration; > + > return 0; > } > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > RPiCameraData *data = cameraData(camera); > > data->state_ = RPiCameraData::State::Stopped; > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect(); No need to connect and disconnect the signal when starting and stopping, you can connect it at init time and keep it connected forever. > > /* Disable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls) > sensor_->setControls(&controls); > } > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev) > +{ > + LOG(RPI, Fatal) << "Unicam has timed out!"; That's harsh, and is likely to trigger if we ever miss a frame due to high CPU load. If the goal is to detect a complete stall, I'd increase the timeout to at least 10 frames. How will this work with sensors that use an external trigger ? > +} > + > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr;
Hi Laurent, Thank you for your feedback. On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via > libcamera-devel wrote: > > 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(+) > > > > 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; > > We really need duration types in mojo. > Yes please!! :-) > > > }; > > > > interface IPARPiInterface { > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index fd8fecb07f81..675f9ba1b350 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 c2230199fed7..50b39f1adf93 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -45,6 +45,8 @@ > > #include "dma_heaps.h" > > #include "rpi_stream.h" > > > > +using namespace std::literals::chrono_literals; > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(RPI) > > @@ -202,6 +204,7 @@ public: > > void setIspControls(const ControlList &controls); > > void setDelayedControls(const ControlList &controls); > > void setSensorControls(ControlList &controls); > > + void unicamTimeout(V4L2VideoDevice *dev); > > > > /* bufferComplete signal handlers. */ > > void unicamBufferDequeue(FrameBuffer *buffer); > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > } > > } > > > > + /* Set the dequeue timeout to 2x the maximum possible frame > duration. */ > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 > * 1ms; > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration); > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, > &RPiCameraData::unicamTimeout); > > + LOG(RPI, Debug) << "Setting sensor timeout to " << duration; > > + > > return 0; > > } > > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > RPiCameraData *data = cameraData(camera); > > > > data->state_ = RPiCameraData::State::Stopped; > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect(); > > No need to connect and disconnect the signal when starting and stopping, > you can connect it at init time and keep it connected forever. > > > > > /* Disable SOF event generation. */ > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList > &controls) > > sensor_->setControls(&controls); > > } > > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev) > > +{ > > + LOG(RPI, Fatal) << "Unicam has timed out!"; > > That's harsh, and is likely to trigger if we ever miss a frame due to > high CPU load. If the goal is to detect a complete stall, I'd increase > the timeout to at least 10 frames. > Yes, you are right. I actually meant to use Error here - Fatal was a bit leftover from my testing to see the timeout hit as expected. > > How will this work with sensors that use an external trigger ? > Short answer is it won't and cannot right now. Given this is a purely debug feature, the only effect it will have is to dump a log message to the console which I think is fine. In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass a config flag? Regards, Naush > > > +} > > + > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > { > > RPi::Stream *stream = nullptr; > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote: > On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote: > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via > > libcamera-devel wrote: > > > 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(+) > > > > > > 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; > > > > We really need duration types in mojo. > > Yes please!! :-) > > > > }; > > > > > > interface IPARPiInterface { > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index fd8fecb07f81..675f9ba1b350 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 c2230199fed7..50b39f1adf93 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -45,6 +45,8 @@ > > > #include "dma_heaps.h" > > > #include "rpi_stream.h" > > > > > > +using namespace std::literals::chrono_literals; > > > + > > > namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(RPI) > > > @@ -202,6 +204,7 @@ public: > > > void setIspControls(const ControlList &controls); > > > void setDelayedControls(const ControlList &controls); > > > void setSensorControls(ControlList &controls); > > > + void unicamTimeout(V4L2VideoDevice *dev); > > > > > > /* bufferComplete signal handlers. */ > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > } > > > } > > > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms; > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration); > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout); > > > + LOG(RPI, Debug) << "Setting sensor timeout to " << duration; > > > + > > > return 0; > > > } > > > > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > > RPiCameraData *data = cameraData(camera); > > > > > > data->state_ = RPiCameraData::State::Stopped; > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect(); > > > > No need to connect and disconnect the signal when starting and stopping, > > you can connect it at init time and keep it connected forever. > > > > > /* Disable SOF event generation. */ > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls) > > > sensor_->setControls(&controls); > > > } > > > > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev) > > > +{ > > > + LOG(RPI, Fatal) << "Unicam has timed out!"; > > > > That's harsh, and is likely to trigger if we ever miss a frame due to > > high CPU load. If the goal is to detect a complete stall, I'd increase > > the timeout to at least 10 frames. > > > > Yes, you are right. > > I actually meant to use Error here - Fatal was a bit leftover from my testing > to see the timeout hit as expected. Maybe the error message in the V4L2VideoDevice class is enough then ? > > How will this work with sensors that use an external trigger ? > > Short answer is it won't and cannot right now. Given this is a purely debug feature, > the only effect it will have is to dump a log message to the console which > I think is fine. Won't the message be repeated at regular intervals though ? That may not be very nice. > In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we > are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass > a config flag? Or perhaps libcamera should handle the internal/external sync configuration itself, I wouldn't be surprised if the IPA module could benefit from more knowledge of how the sensor is triggered. We'll of course need a trigger mode control in that case. > > > +} > > > + > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > { > > > RPi::Stream *stream = nullptr;
Hi Laurent, On Wed, 23 Mar 2022 at 10:51, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote: > > On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote: > > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via > > > libcamera-devel wrote: > > > > 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(+) > > > > > > > > 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; > > > > > > We really need duration types in mojo. > > > > Yes please!! :-) > > > > > > }; > > > > > > > > interface IPARPiInterface { > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index fd8fecb07f81..675f9ba1b350 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 c2230199fed7..50b39f1adf93 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -45,6 +45,8 @@ > > > > #include "dma_heaps.h" > > > > #include "rpi_stream.h" > > > > > > > > +using namespace std::literals::chrono_literals; > > > > + > > > > namespace libcamera { > > > > > > > > LOG_DEFINE_CATEGORY(RPI) > > > > @@ -202,6 +204,7 @@ public: > > > > void setIspControls(const ControlList &controls); > > > > void setDelayedControls(const ControlList &controls); > > > > void setSensorControls(ControlList &controls); > > > > + void unicamTimeout(V4L2VideoDevice *dev); > > > > > > > > /* bufferComplete signal handlers. */ > > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > > > } > > > > } > > > > > > > > + /* Set the dequeue timeout to 2x the maximum possible frame > duration. */ > > > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs > * 2 * 1ms; > > > > + > data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration); > > > > + > data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, > &RPiCameraData::unicamTimeout); > > > > + LOG(RPI, Debug) << "Setting sensor timeout to " << duration; > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera > *camera) > > > > RPiCameraData *data = cameraData(camera); > > > > > > > > data->state_ = RPiCameraData::State::Stopped; > > > > + > data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect(); > > > > > > No need to connect and disconnect the signal when starting and > stopping, > > > you can connect it at init time and keep it connected forever. > > > > > > > /* Disable SOF event generation. */ > > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > > @@ -1757,6 +1767,11 @@ void > RPiCameraData::setSensorControls(ControlList &controls) > > > > sensor_->setControls(&controls); > > > > } > > > > > > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice > *dev) > > > > +{ > > > > + LOG(RPI, Fatal) << "Unicam has timed out!"; > > > > > > That's harsh, and is likely to trigger if we ever miss a frame due to > > > high CPU load. If the goal is to detect a complete stall, I'd increase > > > the timeout to at least 10 frames. > > > > > > > Yes, you are right. > > > > I actually meant to use Error here - Fatal was a bit leftover from my > testing > > to see the timeout hit as expected. > > Maybe the error message in the V4L2VideoDevice class is enough then ? > > > > How will this work with sensors that use an external trigger ? > > > > Short answer is it won't and cannot right now. Given this is a purely > debug feature, > > the only effect it will have is to dump a log message to the console > which > > I think is fine. > > Won't the message be repeated at regular intervals though ? That may not > be very nice. > Yes they will. However, for now, I feel the advantages of having the log message for users with malfunctioning sensors outweigh the (very rare) cases where users will be driving the sensor in bulb mode. Also given none of our sensors have this support (well, imx477 does, but the kernel driver does not have the swith to do it yet), we may not encounter it at all :-) > > > In the longer term, perhaps we need to be informed (through the v4l2 > driver?) that we > > are in "bulb" mode and we must disable timeouts. Or perhaps the > application can pass > > a config flag? > > Or perhaps libcamera should handle the internal/external sync > configuration itself, I wouldn't be surprised if the IPA module could > benefit from more knowledge of how the sensor is triggered. We'll of > course need a trigger mode control in that case. > I think this makes sense. This probably wants to have a dedicated v4l2 control behind it to control from userland. Naush > > > > +} > > > > + > > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > { > > > > RPi::Stream *stream = nullptr; > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Mar 23, 2022 at 03:10:00PM +0000, Naushir Patuck wrote: > On Wed, 23 Mar 2022 at 10:51, Laurent Pinchart wrote: > > On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote: > > > On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote: > > > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via > > > > libcamera-devel wrote: > > > > > 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(+) > > > > > > > > > > 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; > > > > > > > > We really need duration types in mojo. > > > > > > Yes please!! :-) > > > > > > > > }; > > > > > > > > > > interface IPARPiInterface { > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > index fd8fecb07f81..675f9ba1b350 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 c2230199fed7..50b39f1adf93 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -45,6 +45,8 @@ > > > > > #include "dma_heaps.h" > > > > > #include "rpi_stream.h" > > > > > > > > > > +using namespace std::literals::chrono_literals; > > > > > + > > > > > namespace libcamera { > > > > > > > > > > LOG_DEFINE_CATEGORY(RPI) > > > > > @@ -202,6 +204,7 @@ public: > > > > > void setIspControls(const ControlList &controls); > > > > > void setDelayedControls(const ControlList &controls); > > > > > void setSensorControls(ControlList &controls); > > > > > + void unicamTimeout(V4L2VideoDevice *dev); > > > > > > > > > > /* bufferComplete signal handlers. */ > > > > > void unicamBufferDequeue(FrameBuffer *buffer); > > > > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > > > } > > > > > } > > > > > > > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ > > > > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms; > > > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration); > > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout); > > > > > + LOG(RPI, Debug) << "Setting sensor timeout to " << duration; > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) > > > > > RPiCameraData *data = cameraData(camera); > > > > > > > > > > data->state_ = RPiCameraData::State::Stopped; > > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect(); > > > > > > > > No need to connect and disconnect the signal when starting and stopping, > > > > you can connect it at init time and keep it connected forever. > > > > > > > > > /* Disable SOF event generation. */ > > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls) > > > > > sensor_->setControls(&controls); > > > > > } > > > > > > > > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev) > > > > > +{ > > > > > + LOG(RPI, Fatal) << "Unicam has timed out!"; > > > > > > > > That's harsh, and is likely to trigger if we ever miss a frame due to > > > > high CPU load. If the goal is to detect a complete stall, I'd increase > > > > the timeout to at least 10 frames. > > > > > > Yes, you are right. > > > > > > I actually meant to use Error here - Fatal was a bit leftover from my testing > > > to see the timeout hit as expected. > > > > Maybe the error message in the V4L2VideoDevice class is enough then ? > > > > > > How will this work with sensors that use an external trigger ? > > > > > > Short answer is it won't and cannot right now. Given this is a purely debug feature, > > > the only effect it will have is to dump a log message to the console which > > > I think is fine. > > > > Won't the message be repeated at regular intervals though ? That may not > > be very nice. > > Yes they will. However, for now, I feel the advantages of having the log message for > users with malfunctioning sensors outweigh the (very rare) cases where users will be > driving the sensor in bulb mode. Also given none of our sensors have this support > (well, imx477 does, but the kernel driver does not have the swith to do it yet), we may > not encounter it at all :-) We can address it when/if needed, that's fine with me, I'll forward bug reports your way ;-) > > > In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we > > > are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass > > > a config flag? > > > > Or perhaps libcamera should handle the internal/external sync > > configuration itself, I wouldn't be surprised if the IPA module could > > benefit from more knowledge of how the sensor is triggered. We'll of > > course need a trigger mode control in that case. > > I think this makes sense. This probably wants to have a dedicated v4l2 control behind it > to control from userland. > > > > > > +} > > > > > + > > > > > 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 fd8fecb07f81..675f9ba1b350 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 c2230199fed7..50b39f1adf93 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -45,6 +45,8 @@ #include "dma_heaps.h" #include "rpi_stream.h" +using namespace std::literals::chrono_literals; + namespace libcamera { LOG_DEFINE_CATEGORY(RPI) @@ -202,6 +204,7 @@ public: void setIspControls(const ControlList &controls); void setDelayedControls(const ControlList &controls); void setSensorControls(ControlList &controls); + void unicamTimeout(V4L2VideoDevice *dev); /* bufferComplete signal handlers. */ void unicamBufferDequeue(FrameBuffer *buffer); @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) } } + /* Set the dequeue timeout to 2x the maximum possible frame duration. */ + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms; + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration); + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout); + LOG(RPI, Debug) << "Setting sensor timeout to " << duration; + return 0; } @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera) RPiCameraData *data = cameraData(camera); data->state_ = RPiCameraData::State::Stopped; + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect(); /* Disable SOF event generation. */ data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls) sensor_->setControls(&controls); } +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev) +{ + LOG(RPI, Fatal) << "Unicam has timed out!"; +} + 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(+)