[libcamera-devel] [PATCH v2 11/11] ipa: rkisp1: agc: Configure the number of cells used by HW
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Nov 23 14:53:08 CET 2021
Hi Laurent,
On 23/11/2021 12:14, Laurent Pinchart wrote:
> 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.
>
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;
};
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 ?
>> + }
>> +
>> 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.
>
More information about the libcamera-devel
mailing list