[libcamera-devel] [PATCH v1 6/7] ipa: ipu3: Call exposure and gain controls from AGC

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Jun 28, 2021 at 10:22:54PM +0200, Jean-Michel Hautbois wrote:
> IPAIPU3 does not need to directly know the exposure and gain limits.
> Move the control min and max values to IPU3Agc as for the moment it is the one which
> needs to use the values.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     | 14 +++-----------
>  src/ipa/ipu3/ipu3_agc.cpp | 28 +++++++++++++++++++++++-----
>  src/ipa/ipu3/ipu3_agc.h   | 11 +++++++++--
>  3 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 9a2def64..40b626ed 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -67,11 +67,7 @@ private:
>  	/* Camera sensor controls. */
>  	uint32_t defVBlank_;
>  	uint32_t exposure_;
> -	uint32_t minExposure_;
> -	uint32_t maxExposure_;
>  	uint32_t gain_;
> -	uint32_t minGain_;
> -	uint32_t maxGain_;
>  
>  	/* Interface to the AWB algorithm */
>  	std::unique_ptr<IPU3Awb> awbAlgo_;
> @@ -187,13 +183,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  		return -EINVAL;
>  	}
>  
> -	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
> -	maxExposure_ = itExp->second.max().get<int32_t>();
> -	exposure_ = minExposure_;
> +	exposure_ = itExp->second.def().get<int32_t>();

This changes the exposure_ value, it used to be set to the minimum, it's
now the default. Is that intentiona ? If so, it should be explained in
the commit message.
>  
> -	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
> -	maxGain_ = itGain->second.max().get<int32_t>();
> -	gain_ = minGain_;
> +	gain_ = itGain->second.def().get<int32_t>();

Same here.

>  
>  	defVBlank_ = itVBlank->second.def().get<int32_t>();
>  
> @@ -205,7 +197,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
> -	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> +	agcAlgo_->initialise(bdsGrid_, configInfo);
>  
>  	return 0;
>  }
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index 6253ab94..042d67fa 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -10,10 +10,12 @@
>  #include <algorithm>
>  #include <cmath>
>  #include <numeric>
> +#include <stdint.h>
>  
> -#include <libcamera/base/log.h>
> +#include <linux/v4l2-controls.h>
>  
> -#include <libcamera/ipa/core_ipa_interface.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/utils.h>
>  
>  #include "libipa/histogram.h"
>  
> @@ -59,12 +61,28 @@ IPU3Agc::IPU3Agc()
>  {
>  }
>  
> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)
> +void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPAConfigInfo &configInfo)
>  {
>  	aeGrid_ = bdsGrid;
> +	ctrls_ = configInfo.entityControls.at(0);
>  
> -	lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
> -	maxExposureTime_ = kMaxExposure * lineDuration_;
> +	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> +	if (itExp == ctrls_.end()) {
> +		LOG(IPU3Agc, Debug) << "Can't find exposure control";
> +		return;
> +	}
> +	minExposure_ = itExp->second.min().get<int32_t>();
> +	maxExposure_ = itExp->second.max().get<int32_t>();
> +	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s / configInfo.sensorInfo.pixelRate;
> +	maxExposureTime_ = maxExposure_ * lineDuration_;
> +
> +	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> +	if (itGain == ctrls_.end()) {
> +		LOG(IPU3Agc, Debug) << "Can't find gain control";
> +		return;
> +	}
> +	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
> +	maxGain_ = itGain->second.max().get<int32_t>();
>  }
>  
>  void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index 774c8385..ce43c534 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -15,7 +15,7 @@
>  #include <linux/intel-ipu3.h>
>  
>  #include <libcamera/base/utils.h>
> -
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
>  #include <libcamera/geometry.h>
>  
>  #include "libipa/algorithm.h"
> @@ -35,8 +35,8 @@ public:
>  	IPU3Agc();
>  	~IPU3Agc() = default;
>  
> -	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
>  	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
> +	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPAConfigInfo &configInfo);
>  	bool converged() { return converged_; }
>  	bool updateControls() { return updateControls_; }
>  	/* \todo Use a metadata exchange between IPAs */
> @@ -48,6 +48,13 @@ private:
>  	void lockExposureGain(uint32_t &exposure, double &gain);
>  
>  	struct ipu3_uapi_grid_config aeGrid_;
> +	ControlInfoMap ctrls_;

I'm not too fond of storing another copy of this, especially given that
it's not used.

> +
> +	uint32_t minExposure_;
> +	uint32_t maxExposure_;
> +
> +	uint32_t minGain_;
> +	uint32_t maxGain_;
>  
>  	uint64_t frameCount_;
>  	uint64_t lastFrame_;

-- 
Regards,

Laurent Pinchart