[libcamera-devel] [PATCH v3 11/11] ipa: rkisp1: agc: Configure the number of cells used by HW

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Tue, Nov 23, 2021 at 04:04:23PM +0100, Jean-Michel Hautbois wrote:
> The ISP can use 25 or 81 cells depending on its revision. Remove the
> cached value in IPARkISP1 and use IPASessionConfiguration to store it
> and pass it to AGC.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> v3: - Use the rkisp1 enum for the HW revision
> 
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 12 +++++++++++-
>  src/ipa/rkisp1/algorithms/agc.h   |  2 ++
>  src/ipa/rkisp1/ipa_context.cpp    |  8 ++++++++
>  src/ipa/rkisp1/ipa_context.h      |  6 ++++++
>  src/ipa/rkisp1/rkisp1.cpp         | 10 +++++++---
>  5 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 0433813a..16966b16 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -61,7 +61,7 @@ Agc::Agc()
>   * \param[in] context The shared IPA context
>   * \param[in] configInfo The IPA configuration data
>   *
> - * \return 0
> + * \return 0 on success or a negative error code otherwise

You can drop this.

>   */
>  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  {
> @@ -79,6 +79,16 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.frameContext.agc.gain = minAnalogueGain_;
>  	context.frameContext.agc.exposure = 10ms / lineDuration_;
>  
> +	/*
> +	 * According to the RkISP1 documentation:
> +	 * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,

I would have gone for < V12 here and in the condition below, given that
the macro is named RKISP1_CIF_ISP_AE_MEAN_MAX_V12.

> +	 * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.
> +	 */
> +	if (context.configuration.hw.revision <= RKISP1_V11)
> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> +	else
> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> +
>  	return 0;
>  }
>  
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index ac95dea5..3ab3f347 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -45,6 +45,8 @@ private:
>  	double minAnalogueGain_;
>  	double maxAnalogueGain_;
>  
> +	uint32_t numCells_;
> +
>  	utils::Duration filteredExposure_;
>  	utils::Duration currentExposure_;
>  };
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 16fc248f..faa002d1 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 {
>   * \brief Maximum analogue gain supported with the configured sensor
>   */
>  
> +/**
> + * \var IPASessionConfiguration::hw
> + * \brief RkISP1-specific hardware information
> + *
> + * \var IPASessionConfiguration::hw.revision
> + * \brief Hardware revision of the ISP (RKISP1_V*)

I know I've proposed adding (RKISP1_V*), but that was before the enum.
You can drop it now if you prefer.

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

> + */
> +
>  /**
>   * \var IPAFrameContext::agc
>   * \brief Context for the Automatic Gain Control algorithm
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 8e756fb1..e526a0d3 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -8,6 +8,8 @@
>  #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__
>  #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__
>  
> +#include <linux/rkisp1-config.h>
> +
>  #include <libcamera/base/utils.h>
>  
>  namespace libcamera {
> @@ -21,6 +23,10 @@ struct IPASessionConfiguration {
>  		double minAnalogueGain;
>  		double maxAnalogueGain;
>  	} agc;
> +
> +	struct {
> +		rkisp1_cif_isp_version revision;
> +	} hw;
>  };
>  
>  struct IPAFrameContext {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 59e868ff..99ccd596 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -75,7 +75,7 @@ private:
>  	uint32_t maxGain_;
>  
>  	/* revision-specific data */
> -	unsigned int hwAeMeanMax_;
> +	rkisp1_cif_isp_version hwRevision_;
>  	unsigned int hwHistBinNMax_;
>  	unsigned int hwGammaOutMaxSamples_;
>  	unsigned int hwHistogramWeightGridsSize_;
> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>  	/* \todo Add support for other revisions */
>  	switch (hwRevision) {
>  	case RKISP1_V10:
> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
>  		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
>  		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
>  		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
>  		break;
>  	case RKISP1_V12:
> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
>  		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
>  		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
>  		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>  
>  	LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision;
>  
> +	/* Cache the value to set it in configure. */
> +	hwRevision_ = static_cast<rkisp1_cif_isp_version>(hwRevision);
> +
>  	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
>  	if (!camHelper_) {
>  		LOG(IPARkISP1, Error)
> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  	/* Clean context at configuration */
>  	context_ = {};
>  
> +	/* Set the hardware revision for the algorithms. */
> +	context_.configuration.hw.revision = hwRevision_;
> +
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
>  	 * to know the limits for shutter speed and analogue gain.

-- 
Regards,

Laurent Pinchart