[libcamera-devel] [PATCH v2 02/13] ipa: ipu3: set frameContext before controls

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Wed, Oct 20, 2021 at 05:45:56PM +0200, Jean-Michel Hautbois wrote:
> The AGC frame context needs to be initialised correctly for the first
> iteration.
> 
> Set the gain and exposure appropriately to the current values known to
> the IPA.

This patch has changed a lot compared to v1, the commit message needs to
be updated to explain what you're now doing.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp |  8 ++++-
>  src/ipa/ipu3/ipa_context.h      |  9 +++++
>  src/ipa/ipu3/ipu3.cpp           | 64 +++++++++++++++++++++++++++++----
>  3 files changed, 73 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 970ec424..a001e349 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -60,7 +60,13 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  
>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>  		      / configInfo.sensorInfo.pixelRate;
> -	maxExposureTime_ = kMaxExposure * lineDuration_;
> +	maxExposureTime_ = context.configuration.agc.maxShutterSpeed;
> +
> +	/* Configure the default exposure and gain */

s/gain/gain./

> +	context.frameContext.agc.gain =
> +		context.configuration.agc.minAnalogueGain;
> +	context.frameContext.agc.exposure =
> +		context.configuration.agc.minShutterSpeed / lineDuration_;
>  
>  	return 0;
>  }
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 5bab684c..847c03fe 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -10,6 +10,8 @@
>  
>  #include <linux/intel-ipu3.h>
>  
> +#include <libcamera/base/utils.h>
> +
>  #include <libcamera/geometry.h>
>  
>  namespace libcamera {
> @@ -22,6 +24,13 @@ struct IPASessionConfiguration {
>  		Size bdsOutputSize;
>  		uint32_t stride;
>  	} grid;
> +
> +	struct {
> +		utils::Duration minShutterSpeed;
> +		utils::Duration maxShutterSpeed;
> +		double minAnalogueGain;
> +		double maxAnalogueGain;
> +	} agc;
>  };
>  
>  struct IPAFrameContext {
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 388f1902..36ca83f5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -97,6 +97,23 @@
>   * \brief Number of cells on one line including the ImgU padding
>   */
>  
> +/**
> + * \struct IPASessionConfiguration::agc
> + * \brief AGC parameters configuration of the IPA
> + *
> + * \var IPASessionConfiguration::agc::minShutterSpeed
> + * \brief Minimum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::maxShutterSpeed
> + * \brief Maximum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::minAnalogueGain
> + * \brief Minimum analogue gain supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::maxAnalogueGain
> + * \brief Maximum analogue gain supported with the configured sensor
> + */
> +
>  /**
>   * \struct IPAFrameContext::agc
>   * \brief Context for the Automatic Gain Control algorithm
> @@ -158,6 +175,8 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
>  
> +using namespace std::literals::chrono_literals;
> +
>  namespace ipa::ipu3 {
>  
>  class IPAIPU3 : public IPAIPU3Interface
> @@ -182,6 +201,8 @@ private:
>  	void updateControls(const IPACameraSensorInfo &sensorInfo,
>  			    const ControlInfoMap &sensorControls,
>  			    ControlInfoMap *ipaControls);
> +	void updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
> +					const ControlInfoMap &sensorControls);
>  	void processControls(unsigned int frame, const ControlList &controls);
>  	void fillParams(unsigned int frame, ipu3_uapi_params *params);
>  	void parseStatistics(unsigned int frame,
> @@ -216,6 +237,36 @@ private:
>  	struct IPAContext context_;
>  };
>  
> +/*
> + * Compute IPASessionConfiguration using the sensor information and the sensor
> + * v4l2 controls.
> + */
> +void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
> +					 const ControlInfoMap &sensorControls)
> +{
> +	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> +	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
> +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
> +
> +	utils::Duration lineDuration = sensorInfo.lineLength * 1.0s
> +				     / sensorInfo.pixelRate;
> +
> +	const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
> +	int32_t minGain = v4l2Gain.min().get<int32_t>();
> +	int32_t maxGain = v4l2Gain.max().get<int32_t>();
> +
> +	/*
> +	 * When the AGC computes the new exposure values for a frame, it needs
> +	 * to know the limits for shutter speed and analogue gain.
> +	 * As it depends on the sensor, update it with the controls.
> +	 *
> +	 * \todo take VBLANK into account for maximum shutter speed
> +	 */
> +	context_.configuration.agc.minShutterSpeed = minExposure * lineDuration;
> +	context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration;
> +	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
> +	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> +}
>  
>  /*
>   * Compute camera controls using the sensor information and the sensor
> @@ -436,15 +487,18 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>  	calculateBdsGrid(configInfo.bdsOutputSize);
>  
> +	/* Update the camera controls using the new sensor settings. */
> +	updateControls(sensorInfo_, ctrls_, ipaControls);
> +
> +	/* Update the IPASessionConfiguration using the sensor settings */

s/settings/settings./

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	updateSessionConfiguration(sensorInfo_, ctrls_);
> +
>  	for (auto const &algo : algorithms_) {
>  		int ret = algo->configure(context_, configInfo);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	/* Update the camera controls using the new sensor settings. */
> -	updateControls(sensorInfo_, ctrls_, ipaControls);
> -
>  	return 0;
>  }
>  
> @@ -543,10 +597,6 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  {
>  	ControlList ctrls(controls::controls);
>  
> -	/* \todo These fields should not be written by the IPAIPU3 layer */
> -	context_.frameContext.agc.gain = camHelper_->gain(gain_);
> -	context_.frameContext.agc.exposure = exposure_;
> -
>  	for (auto const &algo : algorithms_)
>  		algo->process(context_, stats);
>  

-- 
Regards,

Laurent Pinchart