<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, 5 Apr 2022 at 16:34, 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">On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via libcamera-devel wrote:<br>
> Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)<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>
> No longer a fatal error message.<br>
> <br>
> > The timeout is calculated as 2x the maximum frame length possible for a given<br>
> > mode, returned by the IPA.<br>
<br>
I mentioned in the review of v1 that we should probably increase the<br>
timeout (to 10 times the frame duration). That was based on the error<br>
message being fatal though, but maybe it's still good to avoid a short<br>
watchdog timeout ?<br></blockquote><div><br></div><div>I did wonder what to do here for the timeout limit. Having 10x frame duration is going to</div><div>be very long for imx477 that has a 200s frame length. I think given this is only a log message</div><div>I'm inclined to leave it short to catch the obvious failure case. If I get complaints of too many</div><div>false positives we can bump this up a bit :-)</div><div><br></div><div>I'll fix up all the suggested changes and post a new revision shortly.</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>
> > 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>
> > <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..93931aaf3b55 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > @@ -208,6 +208,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 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
> > }<br>
> > }<br>
> > <br>
> > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */<br>
> > + std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2);<br>
<br>
s/msec/timeout/<br>
<br>
> > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);<br>
> > + LOG(RPI, Debug) << "Setting Unicam timeout to " << msec << " ms.";<br>
<br>
I may have dropped this message, I'm not sure it helps much.<br>
<br>
> > +<br>
> > return 0;<br>
> > }<br>
> > <br>
> > @@ -1192,6 +1198,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 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls)<br>
> > sensor_->setControls(&controls);<br>
> > }<br>
> > <br>
> > +void RPiCameraData::unicamTimeout()<br>
> > +{<br>
> > + LOG(RPI, Error)<br>
> > + << "Unicam has timed out!\n"<br>
> > + "Please check that your camera sensor connector is attached securely.\n"<br>
> <br>
> The '\n' should probably be std::endl ... but I don't think that makes a<br>
> big difference here?<br>
<br>
It would still be good, for consistency.<br>
<br>
> Both of those could be fixed while applying, there's nothing complex<br>
> there.<br>
> <br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> > + "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>