<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and<br>
> connect the timeout signal to a slot in the pipeline handler. This slot will<br>
> log a fatal error message informing the user of a possible hardware stall.<br>
> <br>
> The timeout is calculated as 2x the maximum frame length possible for a given<br>
> mode, returned by the IPA.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 1 +<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++<br>
> 3 files changed, 18 insertions(+)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index acd3cafe6c91..5a228b75cd2f 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -41,6 +41,7 @@ struct IPAConfig {<br>
> struct StartConfig {<br>
> libcamera.ControlList controls;<br>
> int32 dropFrameCount;<br>
> + uint32 maxSensorFrameLengthMs;<br>
<br>
We really need duration types in mojo.<br></blockquote><div><br></div><div>Yes please!! :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> };<br>
> <br>
> interface IPARPiInterface {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index fd8fecb07f81..675f9ba1b350 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf<br>
> }<br>
> <br>
> startConfig->dropFrameCount = dropFrameCount_;<br>
> + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;<br>
> + startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();<br>
> <br>
> firstStart_ = false;<br>
> lastRunTimestamp_ = 0;<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index c2230199fed7..50b39f1adf93 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -45,6 +45,8 @@<br>
> #include "dma_heaps.h"<br>
> #include "rpi_stream.h"<br>
> <br>
> +using namespace std::literals::chrono_literals;<br>
> +<br>
> namespace libcamera {<br>
> <br>
> LOG_DEFINE_CATEGORY(RPI)<br>
> @@ -202,6 +204,7 @@ public:<br>
> void setIspControls(const ControlList &controls);<br>
> void setDelayedControls(const ControlList &controls);<br>
> void setSensorControls(ControlList &controls);<br>
> + void unicamTimeout(V4L2VideoDevice *dev);<br>
> <br>
> /* bufferComplete signal handlers. */<br>
> void unicamBufferDequeue(FrameBuffer *buffer);<br>
> @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
> }<br>
> }<br>
> <br>
> + /* Set the dequeue timeout to 2x the maximum possible frame duration. */<br>
> + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms;<br>
> + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);<br>
> + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout);<br>
> + LOG(RPI, Debug) << "Setting sensor timeout to " << duration;<br>
> +<br>
> return 0;<br>
> }<br>
> <br>
> @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)<br>
> RPiCameraData *data = cameraData(camera);<br>
> <br>
> data->state_ = RPiCameraData::State::Stopped;<br>
> + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();<br>
<br>
No need to connect and disconnect the signal when starting and stopping,<br>
you can connect it at init time and keep it connected forever.<br>
<br>
> <br>
> /* Disable SOF event generation. */<br>
> data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);<br>
> @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls)<br>
> sensor_->setControls(&controls);<br>
> }<br>
> <br>
> +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)<br>
> +{<br>
> + LOG(RPI, Fatal) << "Unicam has timed out!";<br>
<br>
That's harsh, and is likely to trigger if we ever miss a frame due to<br>
high CPU load. If the goal is to detect a complete stall, I'd increase<br>
the timeout to at least 10 frames.<br></blockquote><div><br></div><div><div>Yes, you are right.</div><div><br></div><div>I actually meant to use Error here - Fatal was a bit leftover from my testing</div><div>to see the timeout hit as expected.</div></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
How will this work with sensors that use an external trigger ?<br></blockquote><div><br></div><div>Short answer is it won't and cannot right now. Given this is a purely debug feature,</div><div>the only effect it will have is to dump a log message to the console which I think is fine.</div><div><br></div><div>In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we</div><div>are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass</div><div>a config flag?</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +}<br>
> +<br>
> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> {<br>
> RPi::Stream *stream = nullptr;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>