<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 23 Mar 2022 at 10: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>
On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote:<br>
> On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote:<br>
> > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via<br>
> > 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>
> <br>
> Yes please!! :-)<br>
> <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>
> > > /* 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>
> ><br>
> <br>
> Yes, you are right.<br>
> <br>
> I actually meant to use Error here - Fatal was a bit leftover from my testing<br>
> to see the timeout hit as expected.<br>
<br>
Maybe the error message in the V4L2VideoDevice class is enough then ?<br>
<br>
> > How will this work with sensors that use an external trigger ?<br>
> <br>
> Short answer is it won't and cannot right now. Given this is a purely debug feature,<br>
> the only effect it will have is to dump a log message to the console which<br>
> I think is fine.<br>
<br>
Won't the message be repeated at regular intervals though ? That may not<br>
be very nice.<br></blockquote><div><br></div><div>Yes they will. However, for now, I feel the advantages of having the log message for</div><div>users with malfunctioning sensors outweigh the (very rare) cases where users will be</div><div>driving the sensor in bulb mode. Also given none of our sensors have this support</div><div>(well, imx477 does, but the kernel driver does not have the swith to do it yet), we may</div><div>not encounter it at all :-)</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>
> In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we<br>
> are in "bulb" mode and we must disable timeouts. Or perhaps the application can pass<br>
> a config flag?<br>
<br>
Or perhaps libcamera should handle the internal/external sync<br>
configuration itself, I wouldn't be surprised if the IPA module could<br>
benefit from more knowledge of how the sensor is triggered. We'll of<br>
course need a trigger mode control in that case.<br></blockquote><div><br></div><div>I think this makes sense. This probably wants to have a dedicated v4l2 control behind it</div><div>to control from userland.</div><div> </div><div>Naush</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>
> > > +}<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>