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

12741 diff mbox series

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@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(-)

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@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_;