[libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure the number of cells used by HW
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 23 12:14:59 CET 2021
Hi Jean-Michel,
Thank you for the patch.
On Tue, Nov 23, 2021 at 10:14:51AM +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>
> ---
> src/ipa/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++-
> src/ipa/rkisp1/algorithms/agc.h | 2 ++
> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++
> src/ipa/rkisp1/ipa_context.h | 4 ++++
> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++---
> 5 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 0433813a..e5358ca3 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
> */
> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> {
> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> context.frameContext.agc.gain = minAnalogueGain_;
> context.frameContext.agc.exposure = 10ms / lineDuration_;
>
> + switch (context.configuration.hw.revision) {
> + case RKISP1_V10:
> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> + break;
Blank line. Below too.
> + case RKISP1_V12:
> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> + break;
> + default:
> + LOG(RkISP1Agc, Error)
> + << "Hardware revision " << context.configuration.hw.revision
> + << " is currently not supported";
> + return -ENODEV;
We already fail at init() time if the hardware revision is unknown, so
I'd skip this.
If you want to make it more full-proof, you could create an enum for the
hw.revision field, the compiler will then warn if some values are not
handled.
> + }
> +
> 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..cdb952be 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
s/ specific/-specific/
> + *
> + * \var IPASessionConfiguration::hw.revision
> + * \brief RkISP1 revision reported
* \brief Hardware revision of the ISP (RKISP1_V*)
> + */
> +
> /**
> * \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..316036c6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -21,6 +21,10 @@ struct IPASessionConfiguration {
> double minAnalogueGain;
> double maxAnalogueGain;
> } agc;
> +
> + struct {
> + uint32_t revision;
> + } hw;
> };
>
> struct IPAFrameContext {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 59e868ff..a472d111 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_;
> + unsigned int 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_ = 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
More information about the libcamera-devel
mailing list