<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 6 Apr 2022 at 12:25, 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 Wed, Apr 06, 2022 at 08:58:58AM +0100, Naushir Patuck wrote:<br>
> From: Naushir Patuck via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>><br>
<br>
Here too.<br></blockquote><div><br></div><div>I don't really know what went wrong with this. I use my normal git-pw workflow to pull</div><div>the tagged patches from patchworks onto my tree, and normally it does the right thing.</div><div><br></div><div>Oh well.... I'll fix it up.</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>
> 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 an 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>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.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>
> <br>
> interface IPARPiInterface {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 1bf4e270b062..89767a9db471 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 8fd79be67988..98c463e39548 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::chrono_literals;<br>
> +<br>
> namespace libcamera {<br>
> <br>
> LOG_DEFINE_CATEGORY(RPI)<br>
> @@ -208,6 +210,7 @@ public:<br>
> void setIspControls(const ControlList &controls);<br>
> void setDelayedControls(const ControlList &controls);<br>
> void setSensorControls(ControlList &controls);<br>
> + void unicamTimeout();<br>
> <br>
> /* bufferComplete signal handlers. */<br>
> void unicamBufferDequeue(FrameBuffer *buffer);<br>
> @@ -1050,6 +1053,10 @@ 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 timeout(2 * startConfig.maxSensorFrameLengthMs * 1ms);<br>
<br>
This is a utils::duration, but setDequeueTimeout takes a<br>
utils::Duration (what have we done... ?). It will compile and I think do<br>
the right thing thanks to implicit conversion, but it would be better to<br>
use utils::Duration.<br></blockquote><div><br></div><div>Ouch good catch!</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>
I could fix this locally too, but I'd feel better if you could test it,<br>
would you mind sending a v5 that would also take the comments on 1/3<br>
into account ?<br></blockquote><div><br></div><div>No problem. I'll fix this and test before pushing another version.</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>
> + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);<br>
> +<br>
> return 0;<br>
> }<br>
> <br>
> @@ -1192,6 +1199,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);<br>
> <br>
> /* Wire up all the buffer connections. */<br>
> + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);<br>
> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);<br>
> data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);<br>
> data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);<br>
> @@ -1773,6 +1781,13 @@ void RPiCameraData::setSensorControls(ControlList &controls)<br>
> sensor_->setControls(&controls);<br>
> }<br>
> <br>
> +void RPiCameraData::unicamTimeout()<br>
> +{<br>
> + LOG(RPI, Error) << "Unicam has timed out!";<br>
> + LOG(RPI, Error) << "Please check that your camera sensor connector is attached securely.";<br>
> + LOG(RPI, Error) << "Alternatively, try another cable and/or sensor.";<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>