[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 15:51:04 CET 2021
On 23/11/2021 15:10, Laurent Pinchart wrote:
> 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.
>
All right, then let's split the difference. I will use the existing enum
in IPAContext and we'll add a revision field later in IPASettings.
>>>>> + }
>>>>> +
>>>>> 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