[libcamera-devel] [PATCH v3 11/11] ipa: rkisp1: agc: Configure the number of cells used by HW
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 23 16:27:32 CET 2021
Hi Jean-Michel,
On Tue, Nov 23, 2021 at 04:20:41PM +0100, Jean-Michel Hautbois wrote:
> On 23/11/2021 16:13, Laurent Pinchart wrote:
> > 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.
>
> As we don't have its type here, it can help, I suppose, so I will keep
> it like this if you don't mind :-).
Up to you, but we do have the type, this documents the hw.revision
field. Doxygen will show the type in the HTML output.
> > 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
More information about the libcamera-devel
mailing list