<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 Mon, 6 Mar 2023 at 17:11, 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 Thu, Mar 02, 2023 at 01:54:27PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> <br>
> Add an explicit helper function setCameraTimeout() in the pipeline<br>
> handler to set the Unicam timeout value. This function is signalled from<br>
> the IPA to set up an appropriate timeout. This replaces the<br>
> maxSensorFrameLengthMs value parameter returned back from<br>
> IPARPi::start().<br>
> <br>
> Adjust the timeout to be 5x the maximum frame duration reported by the<br>
> IPA.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 2 +-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 24 ++++++++++++-------<br>
> 3 files changed, 18 insertions(+), 10 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index 8e78f167f179..80e0126618c8 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -49,7 +49,6 @@ struct IPAConfigResult {<br>
> struct StartConfig {<br>
> libcamera.ControlList controls;<br>
> int32 dropFrameCount;<br>
> - uint32 maxSensorFrameLengthMs;<br>
> };<br>
> <br>
> interface IPARPiInterface {<br>
> @@ -132,4 +131,5 @@ interface IPARPiEventInterface {<br>
> setIspControls(libcamera.ControlList controls);<br>
> setDelayedControls(libcamera.ControlList controls, uint32 delayContext);<br>
> setLensControls(libcamera.ControlList controls);<br>
> + setCameraTimeout(uint32 maxFrameLengthMs);<br>
> };<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 9b08ae4ca622..f6826bf27fe1 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -341,7 +341,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)<br>
> <br>
> startConfig->dropFrameCount = dropFrameCount_;<br>
> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;<br>
> - startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();<br>
> + setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());<br>
<br>
I'm not a great fan of adding more function calls between the pipeline<br>
handler and the IPA module, as they're costly when the IPA module runs<br>
in a separate process. While that's not expected to be a common use case<br>
on Raspberry Pi platforms, it would be best to still avoid the overhead<br>
if possible.<br></blockquote><div><br></div><div>I understand the concern. One alternative would be to pass it as a control<br>value (FrameDuration?) through the setDelayedControls signal that happens<br>roughly at the same time? But then I'm overloading the meaning of FrameDuration<br>control, which is not very tidy. I'll leave things as-is for now. There is a<br>bit of an IPA-API rework happening in the background where I will look to reduce<br>the inter-process calls.<br></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>
Please also note that this is one of the few cases where the camera<br>
manager event loop is run reentrantly. When the pipeline handler calls<br>
the start() function of the IPA module, it will wait synchronously for<br>
the call to complete, but re-enters the event loop to wait for the IPA<br>
to signal completion of the start() call by processing replies. The<br>
setCameraTimeout signal is emitted by the IPA module before the start()<br>
function returns, so from a pipeline handler point of view it will be<br>
processed while the start() call is in progress. I don't think there's<br>
any issue in this case as the connected slot shouldn't interfere with<br>
the pipeline handler start() function, but it could easily lead to<br>
unexpected behaviour later if you don't pay attention.<br></blockquote><div><br></div><div>It does seem to work ;-) I will look to remove this setCameraTimeout() signal as<br>part of the rework to avoid any future nasties.<br></div><div><br></div><div>Regards,</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>
> firstStart_ = false;<br>
> lastRunTimestamp_ = 0;<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 841209548350..3d04842a2440 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -212,6 +212,7 @@ public:<br>
> void setIspControls(const ControlList &controls);<br>
> void setDelayedControls(const ControlList &controls, uint32_t delayContext);<br>
> void setLensControls(const ControlList &controls);<br>
> + void setCameraTimeout(uint32_t maxExposureTimeMs);<br>
> void setSensorControls(ControlList &controls);<br>
> void unicamTimeout();<br>
> <br>
> @@ -1166,14 +1167,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
> }<br>
> }<br>
> <br>
> - /*<br>
> - * Set the dequeue timeout to the larger of 2x the maximum possible<br>
> - * frame duration or 1 second.<br>
> - */<br>
> - utils::Duration timeout =<br>
> - std::max<utils::Duration>(1s, 2 * startConfig.maxSensorFrameLengthMs * 1ms);<br>
> - data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);<br>
> -<br>
> return 0;<br>
> }<br>
> <br>
> @@ -1645,6 +1638,7 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)<br>
> ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);<br>
> ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);<br>
> ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);<br>
> + ipa_->setCameraTimeout.connect(this, &RPiCameraData::setCameraTimeout);<br>
> <br>
> /*<br>
> * The configuration (tuning file) is made from the sensor name unless<br>
> @@ -1957,6 +1951,20 @@ void RPiCameraData::setLensControls(const ControlList &controls)<br>
> }<br>
> }<br>
> <br>
> +void RPiCameraData::setCameraTimeout(uint32_t maxFrameLengthMs)<br>
<br>
Note to self: we should really add support for duration types to the IPA<br>
parameters.<br>
<br>
In case you'd prefer not minimizing the number of function calls (as<br>
discussed above),<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> +{<br>
> + /*<br>
> + * Set the dequeue timeout to the larger of 5x the maximum reported<br>
> + * frame length advertised by the IPA over a number of frames. Allow<br>
> + * a minimum timeout value of 1s.<br>
> + */<br>
> + utils::Duration timeout =<br>
> + std::max<utils::Duration>(1s, 5 * maxFrameLengthMs * 1ms);<br>
> +<br>
> + LOG(RPI, Debug) << "Setting Unicam timeout to " << timeout;<br>
> + unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);<br>
> +}<br>
> +<br>
> void RPiCameraData::setSensorControls(ControlList &controls)<br>
> {<br>
> /*<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>