<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>