[PATCH v5 18/18] libcamera: Soft IPA: use CameraSensorHelper for analogue gain

[Milan Zamazal]

Hans de Goede <hdegoede at redhat.com> writes:

> From: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>
> Use CameraSensorHelper to convert the analogue gain code read from the
> camera sensor into real analogue gain value. In the future this makes
> it possible to use faster AE/AGC algorithm. For now the same AE/AGC
> algorithm is used, but even then the CameraSensorHelper lets us use the
> full range of analogue gain values.
>
> If there is no CameraSensorHelper for the camera sensor in use, a
> warning log message is printed, and the AE/AGC works exactly as before
> this change.
>
> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>

Reviewed-by: Milan Zamazal <mzamazal at redhat.com>

> ---
>  .../internal/software_isp/software_isp.h      |  3 +-
>  src/ipa/simple/soft_simple.cpp                | 77 ++++++++++++-------
>  src/libcamera/pipeline/simple/simple.cpp      |  2 +-
>  src/libcamera/software_isp/software_isp.cpp   |  8 +-
>  4 files changed, 57 insertions(+), 33 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 8d25e979..2a6db7ba 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -26,6 +26,7 @@
>  #include <libcamera/ipa/soft_ipa_interface.h>
>  #include <libcamera/ipa/soft_ipa_proxy.h>
>  
> +#include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/dma_heaps.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/shared_mem_object.h"
> @@ -43,7 +44,7 @@ LOG_DECLARE_CATEGORY(SoftwareIsp)
>  class SoftwareIsp
>  {
>  public:
> -	SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> +	SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor);
>  	~SoftwareIsp();
>  
>  	int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index ac027568..e4d64762 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -22,6 +22,8 @@
>  #include "libcamera/internal/software_isp/debayer_params.h"
>  #include "libcamera/internal/software_isp/swisp_stats.h"
>  
> +#include "libipa/camera_sensor_helper.h"
> +
>  #include "black_level.h"
>  
>  namespace libcamera {
> @@ -67,18 +69,27 @@ private:
>  	DebayerParams *params_;
>  	SwIspStats *stats_;
>  	BlackLevel blackLevel_;
> +	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
>  	int32_t exposure_min_, exposure_max_;
> -	int32_t again_min_, again_max_;
> -	int32_t again_, exposure_;
> +	int32_t exposure_;
> +	double again_min_, again_max_, againMinStep_;
> +	double again_;
>  	unsigned int ignore_updates_;
>  };
>  
> -int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> +int IPASoftSimple::init(const IPASettings &settings,
>  			const SharedFD &fdStats,
>  			const SharedFD &fdParams,
>  			const ControlInfoMap &sensorInfoMap)
>  {
> +	camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);
> +	if (camHelper_ == nullptr) {
> +		LOG(IPASoft, Warning)
> +			<< "Failed to create camera sensor helper for "
> +			<< settings.sensorModel;
> +	}
> +
>  	fdStats_ = fdStats;
>  	if (!fdStats_.isValid()) {
>  		LOG(IPASoft, Error) << "Invalid Statistics handle";
> @@ -132,25 +143,35 @@ int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>  		exposure_min_ = 1;
>  	}
>  
> -	again_min_ = gain_info.min().get<int32_t>();
> -	again_max_ = gain_info.max().get<int32_t>();
> -	/*
> -	 * The camera sensor gain (g) is usually not equal to the value written
> -	 * into the gain register (x). But the way how the AGC algorithm changes
> -	 * the gain value to make the total exposure closer to the optimum assumes
> -	 * that g(x) is not too far from linear function. If the minimal gain is 0,
> -	 * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
> -	 * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
> -	 * one edge, and very small near the other) we limit the range of the gain
> -	 * values used.
> -	 */
> -	if (!again_min_) {
> -		LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> -		again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
> +	int32_t again_min = gain_info.min().get<int32_t>();
> +	int32_t again_max = gain_info.max().get<int32_t>();
> +
> +	if (camHelper_) {
> +		again_min_ = camHelper_->gain(again_min);
> +		again_max_ = camHelper_->gain(again_max);
> +		againMinStep_ = (again_max_ - again_min_) / 100.0;
> +	} else {
> +		/*
> +		 * The camera sensor gain (g) is usually not equal to the value written
> +		 * into the gain register (x). But the way how the AGC algorithm changes
> +		 * the gain value to make the total exposure closer to the optimum assumes
> +		 * that g(x) is not too far from linear function. If the minimal gain is 0,
> +		 * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
> +		 * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
> +		 * one edge, and very small near the other) we limit the range of the gain
> +		 * values used.
> +		 */
> +		again_max_ = again_max;
> +		if (!again_min) {
> +			LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> +			again_min_ = std::min(100, again_min / 2 + again_max / 2);
> +		}
> +		againMinStep_ = 1.0;
>  	}
>  
>  	LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> -			   << ", gain " << again_min_ << "-" << again_max_;
> +			   << ", gain " << again_min_ << "-" << again_max_
> +			   << " (" << againMinStep_ << ")";
>  
>  	return 0;
>  }
> @@ -252,12 +273,14 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>  	ControlList ctrls(sensorControls);
>  
>  	exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +	int32_t again = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +	again_ = camHelper_ ? camHelper_->gain(again) : again;
>  
>  	updateExposure(exposureMSV);
>  
>  	ctrls.set(V4L2_CID_EXPOSURE, exposure_);
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN,
> +		  static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(again_) : again_));
>  
>  	ignore_updates_ = 2;
>  
> @@ -276,7 +299,7 @@ void IPASoftSimple::updateExposure(double exposureMSV)
>  	static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>  	static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>  
> -	int next;
> +	double next;
>  
>  	if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>  		next = exposure_ * kExpNumeratorUp / kExpDenominator;
> @@ -286,18 +309,18 @@ void IPASoftSimple::updateExposure(double exposureMSV)
>  			exposure_ = next;
>  		if (exposure_ >= exposure_max_) {
>  			next = again_ * kExpNumeratorUp / kExpDenominator;
> -			if (next - again_ < 1)
> -				again_ += 1;
> +			if (next - again_ < againMinStep_)
> +				again_ += againMinStep_;
>  			else
>  				again_ = next;
>  		}
>  	}
>  
>  	if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> -		if (exposure_ == exposure_max_ && again_ != again_min_) {
> +		if (exposure_ == exposure_max_ && again_ > again_min_) {
>  			next = again_ * kExpNumeratorDown / kExpDenominator;
> -			if (again_ - next < 1)
> -				again_ -= 1;
> +			if (again_ - next < againMinStep_)
> +				again_ -= againMinStep_;
>  			else
>  				again_ = next;
>  		} else {
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index e491ab62..32b56c57 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -526,7 +526,7 @@ int SimpleCameraData::init()
>  	 * Instantiate Soft ISP if this is enabled for the given driver and no converter is used.
>  	 */
>  	if (!converter_ && pipe->swIspEnabled()) {
> -		swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_->controls());
> +		swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get());
>  		if (!swIsp_->isValid()) {
>  			LOG(SimplePipeline, Warning)
>  				<< "Failed to create software ISP, disabling software debayering";
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 9b49be41..ea4d96e4 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -60,9 +60,9 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
>  /**
>   * \brief Constructs SoftwareIsp object
>   * \param[in] pipe The pipeline handler in use
> - * \param[in] sensorControls ControlInfoMap describing the controls supported by the sensor
> + * \param[in] sensor Pointer to the CameraSensor instance owned by the pipeline handler
>   */
> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls)
> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
>  	: debayer_(nullptr),
>  	  debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f, 0 },
>  	  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
> @@ -97,10 +97,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorCont
>  		return;
>  	}
>  
> -	int ret = ipa_->init(IPASettings{ "No cfg file", "No sensor model" },
> +	int ret = ipa_->init(IPASettings{ "No cfg file", sensor->model() },
>  			     debayer_->getStatsFD(),
>  			     sharedParams_.fd(),
> -			     sensorControls);
> +			     sensor->controls());
>  	if (ret) {
>  		LOG(SoftwareIsp, Error) << "IPA init failed";
>  		debayer_.reset();