[libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better heruistics for calculating Unicam timeout

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via libcamera-devel wrote:
> 
> The existing mechanism of setting a timeout value simply uses the
> maximum possible frame length advertised by the sensor mode. This can be
> problematic when, for example, the IMX477 sensor can use a frame length
> of over 600 seconds. However, for typical usage the frame length will
> never go over several 100s of milliseconds, making the timeout very
> impractical.
> 
> Store a list of the last 10 frame length values requested by the AGC. On
> startup, and at the end of every frame, take the maximum frame length
> value from this list and return that to the pipeline handler through the
> setCameraTimeoutValue() signal. This allows the timeout value to better
> track the actual sensor usage.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index f6826bf27fe1..7b5aed644a67 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -8,6 +8,7 @@
>  #include <algorithm>
>  #include <array>
>  #include <cstring>
> +#include <deque>
>  #include <fcntl.h>
>  #include <math.h>
>  #include <stdint.h>
> @@ -64,6 +65,9 @@ using utils::Duration;
>  /* Number of metadata objects available in the context list. */
>  constexpr unsigned int numMetadataContexts = 16;
>  
> +/* Number of frame length times to hold in the queue. */
> +constexpr unsigned int FrameLengthsQueueSize = 10;
> +
>  /* Configure the sensor with these values initially. */
>  constexpr double defaultAnalogueGain = 1.0;
>  constexpr Duration defaultExposureTime = 20.0ms;
> @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface
>  public:
>  	IPARPi()
>  		: controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),
> -		  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)
> +		  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true),
> +		  lastTimeout_(0s)
>  	{
>  	}
>  
> @@ -155,6 +160,7 @@ private:
>  	void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
>  	RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
>  	void processStats(unsigned int bufferId, unsigned int ipaContext);
> +	void setCameraTimeoutValue();
>  	void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
>  	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
>  	void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
> @@ -220,6 +226,10 @@ private:
>  
>  	/* Maximum gain code for the sensor. */
>  	uint32_t maxSensorGainCode_;
> +
> +	/* Track the frame length times over FrameLengthsQueueSize frames. */
> +	std::deque<Duration> frameLengths_;

Another note to self: create a generic ring buffer class.

> +	Duration lastTimeout_;
>  };
>  
>  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  
>  	controller_.switchMode(mode_, &metadata);
>  
> +	/* Reset the frame lengths queue */

s/queue/queue./

> +	frameLengths_.clear();
> +	frameLengths_.resize(FrameLengthsQueueSize, 0s);

Should you also reset lastTimeout_ to 0s to ensure a consistent
behaviour at start time ?

> +
>  	/* SwitchMode may supply updated exposure/gain values to use. */
>  	AgcStatus agcStatus;
>  	agcStatus.shutterTime = 0.0s;
> @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  		ControlList ctrls(sensorCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>  		startConfig->controls = std::move(ctrls);
> +		setCameraTimeoutValue();
>  	}
>  
>  	/*
> @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  	}
>  
>  	startConfig->dropFrameCount = dropFrameCount_;
> -	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> -	setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());
>  
>  	firstStart_ = false;
>  	lastRunTimestamp_ = 0;
> @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)

Adding more context:

	if (agcStatus.shutterTime && agcStatus.analogueGain) {
		ControlList ctrls(sensorCtrls_);

>  		applyAGC(&agcStatus, ctrls);
>  
>  		setDelayedControls.emit(ctrls, ipaContext);
> +		setCameraTimeoutValue();

If the above condition is false, setCameraTimeoutValue() won't be
called. When can that happen, and could it cause any issue ?

> +	}
> +}
> +
> +void IPARPi::setCameraTimeoutValue()
> +{
> +	/*
> +	 * Take the maximum value of the exposure queue as the camera timeout
> +	 * value to pass back to the pipeline handler. Only signal if it has changed

Line wrap.

> +	 * from the last set value.
> +	 */
> +	auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());
> +
> +	if (*max != lastTimeout_) {
> +		setCameraTimeout.emit(max->get<std::milli>());
> +		lastTimeout_ = *max;
>  	}
>  }
>  
> @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  	 */
>  	if (mode_.minLineLength != mode_.maxLineLength)
>  		ctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));
> +
> +	/*
> +	 * Store the frame length times in a circular queue, up-to FrameLengthsQueueSize
> +	 * elements. This will be used to advertise a camera timeout value to the
> +	 * pipeline handler.

Line wraps here too.

> +	 */
> +	frameLengths_.pop_front();
> +	frameLengths_.push_back(helper_->exposure(vblank + mode_.height,
> +						  helper_->hblankToLineLength(hblank)));
>  }
>  
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)

-- 
Regards,

Laurent Pinchart