[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 15:10:30 CET 2021
On Tue, Nov 23, 2021 at 02:04:02PM +0000, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-11-23 13:53:08)
> > On 23/11/2021 12:14, Laurent Pinchart wrote:
> > > 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.
> >
> > I hesitated on this one, and there is already the rkisp1_cif_isp_version
> > enum and it sounded redundant. Or, I can change:
> > struct {
> > - uint32_t revision;
> > + rkisp1_cif_isp_version revision;
> > } hw;
> > };
Good point, we can use that.
> > And make it use the enum. But init is called with an unsigned int.
> > Should we change it already, move the revision into IPASettings and make
> > it a rkisp1_cif_isp_version type too ?
>
> Isn't IPASettings 'libcamera generic' currently though?
>
> If so - we'll need to be able to extend this for pipeline specific
> settings...
It is generic, but a hardware revision field seems common enough to be
included in a common structure, even if not used by all platforms.
If we move the hw revision to IPASettings, then I'd make it a uint32_t
there to match the hwrevision field from the MC API, and cast it to an
enum in the RkISP1 IPA.
> > >> + }
> > >> +
> > >> 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