[libcamera-devel] [PATCH v2 08/10] ipa: ipu3: Remove unneeded function calls in AGC

[Kieran Bingham]

On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> We never use converged_ so remove its declaration.
> The controls may not need to be updated at each call, but it should be
> decided on the context side and not by a specific call.

Good, I see how this helps move towards making it more modular.
I guess the short term effect is that the controls will be set for every
frame, but if the values don't change, then that's likely a no-op in the
kernel anyway so I dont' think that's an issue.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     |  6 ++----
>  src/ipa/ipu3/ipu3_agc.cpp | 10 ++--------
>  src/ipa/ipu3/ipu3_agc.h   |  5 -----
>  3 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2a8225a0..27765aa6 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -309,8 +309,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  	for (auto const &algo : algorithms_)
>  		algo->prepare(context_, params_);
>  
> -	if (agcAlgo_->updateControls())
> -		awbAlgo_->prepare(context_, params_);
> +	awbAlgo_->prepare(context_, params_);
>  
>  	*params = params_;
>  
> @@ -338,8 +337,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  
>  	awbAlgo_->process(context_, stats);
>  
> -	if (agcAlgo_->updateControls())
> -		setControls(frame);
> +	setControls(frame);
>  
>  	/* \todo Use VBlank value calculated from each frame exposure. */
>  	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index 0cc35b9f..0d0584a8 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -51,9 +51,8 @@ static constexpr double kEvGainTarget = 0.5;
>  static constexpr uint8_t kCellSize = 8;
>  
>  IPU3Agc::IPU3Agc()
> -	: frameCount_(0), lastFrame_(0), converged_(false),
> -	  updateControls_(false), iqMean_(0.0),
> -	  lineDuration_(0s), maxExposureTime_(0s),
> +	: frameCount_(0), lastFrame_(0),
> +	  iqMean_(0.0), lineDuration_(0s), maxExposureTime_(0s),
>  	  prevExposure_(0s), prevExposureNoDg_(0s),
>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>  {
> @@ -146,8 +145,6 @@ void IPU3Agc::filterExposure()
>  
>  void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  {
> -	updateControls_ = false;
> -
>  	/* Algorithm initialization should wait for first valid frames */
>  	/* \todo - have a number of frames given by DelayedControls ?
>  	 * - implement a function for IIR */
> @@ -157,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  	/* Are we correctly exposed ? */
>  	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
>  		LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> -		converged_ = true;
>  	} else {
>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> @@ -180,12 +176,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
>  			newExposure = currentExposure_ / exposure;
>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> -			updateControls_ = true;
>  		} else if (currentShutter >= maxExposureTime_) {
>  			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
>  			newExposure = currentExposure_ / gain;
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
> -			updateControls_ = true;
>  		}
>  		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
>  	}
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index e096648b..0e922664 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -31,8 +31,6 @@ public:
>  
>  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> -	bool converged() { return converged_; }
> -	bool updateControls() { return updateControls_; }
>  
>  private:
>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
> @@ -44,9 +42,6 @@ private:
>  	uint64_t frameCount_;
>  	uint64_t lastFrame_;
>  
> -	bool converged_;
> -	bool updateControls_;
> -
>  	double iqMean_;
>  
>  	Duration lineDuration_;
>