[libcamera-devel] [PATCH v2 1/3] pipeline: ipa: raspberrypi: Change Unicam timeout handling

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Mar 02, 2023 at 01:54:27PM +0000, Naushir Patuck via libcamera-devel wrote:
> 
> Add an explicit helper function setCameraTimeout() in the pipeline
> handler to set the Unicam timeout value. This function is signalled from
> the IPA to set up an appropriate timeout. This replaces the
> maxSensorFrameLengthMs value parameter returned back from
> IPARPi::start().
> 
> Adjust the timeout to be 5x the maximum frame duration reported by the
> IPA.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 24 ++++++++++++-------
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 8e78f167f179..80e0126618c8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -49,7 +49,6 @@ struct IPAConfigResult {
>  struct StartConfig {
>  	libcamera.ControlList controls;
>  	int32 dropFrameCount;
> -	uint32 maxSensorFrameLengthMs;
>  };
>  
>  interface IPARPiInterface {
> @@ -132,4 +131,5 @@ interface IPARPiEventInterface {
>  	setIspControls(libcamera.ControlList controls);
>  	setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
>  	setLensControls(libcamera.ControlList controls);
> +	setCameraTimeout(uint32 maxFrameLengthMs);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9b08ae4ca622..f6826bf27fe1 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -341,7 +341,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  
>  	startConfig->dropFrameCount = dropFrameCount_;
>  	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> -	startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
> +	setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());

I'm not a great fan of adding more function calls between the pipeline
handler and the IPA module, as they're costly when the IPA module runs
in a separate process. While that's not expected to be a common use case
on Raspberry Pi platforms, it would be best to still avoid the overhead
if possible.

Please also note that this is one of the few cases where the camera
manager event loop is run reentrantly. When the pipeline handler calls
the start() function of the IPA module, it will wait synchronously for
the call to complete, but re-enters the event loop to wait for the IPA
to signal completion of the start() call by processing replies. The
setCameraTimeout signal is emitted by the IPA module before the start()
function returns, so from a pipeline handler point of view it will be
processed while the start() call is in progress. I don't think there's
any issue in this case as the connected slot shouldn't interfere with
the pipeline handler start() function, but it could easily lead to
unexpected behaviour later if you don't pay attention.

>  
>  	firstStart_ = false;
>  	lastRunTimestamp_ = 0;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 841209548350..3d04842a2440 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -212,6 +212,7 @@ public:
>  	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls, uint32_t delayContext);
>  	void setLensControls(const ControlList &controls);
> +	void setCameraTimeout(uint32_t maxExposureTimeMs);
>  	void setSensorControls(ControlList &controls);
>  	void unicamTimeout();
>  
> @@ -1166,14 +1167,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  		}
>  	}
>  
> -	/*
> -	 * Set the dequeue timeout to the larger of 2x the maximum possible
> -	 * frame duration or 1 second.
> -	 */
> -	utils::Duration timeout =
> -		std::max<utils::Duration>(1s, 2 * startConfig.maxSensorFrameLengthMs * 1ms);
> -	data->unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> -
>  	return 0;
>  }
>  
> @@ -1645,6 +1638,7 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
>  	ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>  	ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>  	ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);
> +	ipa_->setCameraTimeout.connect(this, &RPiCameraData::setCameraTimeout);
>  
>  	/*
>  	 * The configuration (tuning file) is made from the sensor name unless
> @@ -1957,6 +1951,20 @@ void RPiCameraData::setLensControls(const ControlList &controls)
>  	}
>  }
>  
> +void RPiCameraData::setCameraTimeout(uint32_t maxFrameLengthMs)

Note to self: we should really add support for duration types to the IPA
parameters.

In case you'd prefer not minimizing the number of function calls (as
discussed above),

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +{
> +	/*
> +	 * Set the dequeue timeout to the larger of 5x the maximum reported
> +	 * frame length advertised by the IPA over a number of frames. Allow
> +	 * a minimum timeout value of 1s.
> +	 */
> +	utils::Duration timeout =
> +		std::max<utils::Duration>(1s, 5 * maxFrameLengthMs * 1ms);
> +
> +	LOG(RPI, Debug) << "Setting Unicam timeout to " << timeout;
> +	unicam_[Unicam::Image].dev()->setDequeueTimeout(timeout);
> +}
> +
>  void RPiCameraData::setSensorControls(ControlList &controls)
>  {
>  	/*

-- 
Regards,

Laurent Pinchart