[libcamera-devel] [PATCH v3] ipa: rkisp1: Add support for manual gain and exposure
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 19 11:42:44 CEST 2022
Quoting Jacopo Mondi (2022-10-19 10:37:13)
> Hi Kieran
>
> On Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:
> > Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)
> > > Hi Paul
> > >
> > > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:
> > > > Add support for manual gain and exposure in the rkisp1 IPA.
> > > >
> > >
> > > I wish we could base this on the new AEGC controls
> >
> > Likewise.
> >
> > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > >
> > > > ---
> > > > Changes in v3:
> > > > - Report the exposure time and analogue gain in metadata
> > > >
> > > > Changes in v2:
> > > > - set the min and max values of ExposureTime and AnalogueGain properly
> > > > - to that end, plumb the sensorControls through the pipeline handler's
> > > > loadIPA() and the IPA's init()
> > > > ---
> > > > include/libcamera/ipa/rkisp1.mojom | 2 +-
> > > > src/ipa/rkisp1/algorithms/agc.cpp | 59 +++++++++++++++++++++---
> > > > src/ipa/rkisp1/algorithms/agc.h | 4 ++
> > > > src/ipa/rkisp1/ipa_context.h | 13 +++++-
> > > > src/ipa/rkisp1/rkisp1.cpp | 21 +++++++++
> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--
> > > > 6 files changed, 95 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > > index eaf3955e..b526450d 100644
> > > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > > @@ -10,7 +10,7 @@ import "include/libcamera/ipa/core.mojom";
> > > >
> > > > interface IPARkISP1Interface {
> > > > init(libcamera.IPASettings settings,
> > > > - uint32 hwRevision)
> > > > + uint32 hwRevision, libcamera.ControlInfoMap sensorControls)
> > > > => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > start() => (int32 ret);
> > > > stop();
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index 04062a36..09ec6ac4 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -14,6 +14,7 @@
> > > > #include <libcamera/base/log.h>
> > > > #include <libcamera/base/utils.h>
> > > >
> > > > +#include <libcamera/control_ids.h>
> > > > #include <libcamera/ipa/core_ipa_interface.h>
> > > >
> > > > #include "libipa/histogram.h"
> > > > @@ -73,8 +74,11 @@ Agc::Agc()
> > > > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > > {
> > > > /* Configure the default exposure and gain. */
> > > > - context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> > > > - context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> > > > + context.activeState.agc.automatic.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> > > > + context.activeState.agc.automatic.exposure = 10ms / context.configuration.sensor.lineDuration;
> > > > + context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> > > > + context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > > > + context.activeState.agc.autoEnabled = true;
> > > >
> > > > /*
> > > > * According to the RkISP1 documentation:
> > > > @@ -221,8 +225,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > > > << stepGain;
> > > >
> > > > /* Update the estimated exposure and gain. */
> > > > - activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> > > > - activeState.agc.gain = stepGain;
> > > > + activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> > > > + activeState.agc.automatic.gain = stepGain;
> > > > }
> > > >
> > > > /**
> > > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > void Agc::prepare(IPAContext &context, const uint32_t frame,
> > > > IPAFrameContext &frameContext, rkisp1_params_cfg *params)
> > > > {
> > > > - frameContext.agc.exposure = context.activeState.agc.exposure;
> > > > - frameContext.agc.gain = context.activeState.agc.gain;
> > > > + if (frameContext.agc.autoEnabled) {
> > > > + frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> > > > + frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > > > + }
> > > >
> > > > if (frame > 0)
> > > > return;
> > > > @@ -373,6 +379,47 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_HST;
> > > > }
> > > >
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> > > > + */
> > > > +void Agc::queueRequest(IPAContext &context,
> > > > + [[maybe_unused]] const uint32_t frame,
> > > > + IPAFrameContext &frameContext,
> > > > + const ControlList &controls)
> > > > +{
> > > > + auto &agc = context.activeState.agc;
> > > > +
> > > > + const auto &agcEnable = controls.get(controls::AeEnable);
> > > > + if (agcEnable && *agcEnable != agc.autoEnabled) {
> > > > + agc.autoEnabled = *agcEnable;
> >
> > I've wondered if a helpful pattern on these might be:
> >
> > bool agcEnable = controls.get(controls::AeEnable)
> > .value_or(agc.autoEnabled);
> > if (agcEnable != agc.autoEnabled) {
> > agc.autoEnabled = agcEnable;
> > LOG(...)
> > }
> >
> > But it's not specifically better than what you have.
> >
> >
> > > > +
> > > > + LOG(RkISP1Agc, Debug)
> > > > + << (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> > > > + }
> > > > +
> > > > + const auto &exposure = controls.get(controls::ExposureTime);
> > > > + if (exposure && !agc.autoEnabled) {
> > > > + agc.manual.exposure = *exposure;
> > > > +
> >
> > But this looks good, and can't use the .value_or() anyway.
> >
> > > > + LOG(RkISP1Agc, Debug)
> > > > + << "Set exposure to: " << agc.manual.exposure;
> > > > + }
> > > > +
> > > > + const auto &gain = controls.get(controls::AnalogueGain);
> > > > + if (gain && !agc.autoEnabled) {
> > > > + agc.manual.gain = *gain;
> > > > +
> > > > + LOG(RkISP1Agc, Debug) << "Set gain to: " << agc.manual.gain;
> > > > + }
> > > > +
> > > > + frameContext.agc.autoEnabled = agc.autoEnabled;
> > > > +
> > > > + if (!frameContext.agc.autoEnabled) {
> > > > + frameContext.agc.exposure = agc.manual.exposure;
> > > > + frameContext.agc.gain = agc.manual.gain;
> > > > + }
> > > > +}
> > > > +
> > >
> > > This seems correct to me!
> > >
> > > > REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > >
> > > > } /* namespace ipa::rkisp1::algorithms */
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > > > index 9ad5c32f..ebceeef4 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.h
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > > > @@ -32,6 +32,10 @@ public:
> > > > void process(IPAContext &context, const uint32_t frame,
> > > > IPAFrameContext &frameContext,
> > > > const rkisp1_stat_buffer *stats) override;
> > > > + void queueRequest(IPAContext &context,
> > > > + const uint32_t frame,
> > > > + IPAFrameContext &frameContext,
> > > > + const ControlList &controls) override;
> > > >
> > > > private:
> > > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index c85d8abe..035d67e2 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -50,8 +50,16 @@ struct IPASessionConfiguration {
> > > >
> > > > struct IPAActiveState {
> > > > struct {
> > > > - uint32_t exposure;
> > > > - double gain;
> > > > + struct {
> > > > + uint32_t exposure;
> > > > + double gain;
> > > > + } manual;
> > > > + struct {
> > > > + uint32_t exposure;
> > > > + double gain;
> > > > + } automatic;
> > > > +
> > > > + bool autoEnabled;
> > > > } agc;
> > > >
> > > > struct {
> > > > @@ -92,6 +100,7 @@ struct IPAFrameContext : public FrameContext {
> > > > struct {
> > > > uint32_t exposure;
> > > > double gain;
> > > > + bool autoEnabled;
> > > > } agc;
> > > >
> > > > struct {
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 3f5c1a58..33692e5d 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -49,6 +49,7 @@ public:
> > > > IPARkISP1();
> > > >
> > > > int init(const IPASettings &settings, unsigned int hwRevision,
> > > > + const ControlInfoMap &sensorControls,
> > > > ControlInfoMap *ipaControls) override;
> > > > int start() override;
> > > > void stop() override;
> > > > @@ -116,6 +117,7 @@ std::string IPARkISP1::logPrefix() const
> > > > }
> > > >
> > > > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > > + const ControlInfoMap &sensorControls,
> > > > ControlInfoMap *ipaControls)
> > > > {
> > > > /* \todo Add support for other revisions */
> > > > @@ -183,6 +185,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > >
> > > > /* Return the controls handled by the IPA. */
> > > > ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > > > +
> > > > + auto exposureTimeInfo = sensorControls.find(V4L2_CID_EXPOSURE);
> > > > + if (exposureTimeInfo != sensorControls.end()) {
> > > > + ctrlMap.emplace(&controls::ExposureTime,
> > > > + ControlInfo(exposureTimeInfo->second.min(),
> > > > + exposureTimeInfo->second.max()));
> > > > + }
> > > > +
> > > > + auto analogueGainInfo = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);
> > > > + if (analogueGainInfo != sensorControls.end()) {
> > > > + ctrlMap.emplace(&controls::AnalogueGain,
> > > > + ControlInfo(static_cast<float>(analogueGainInfo->second.min().get<int32_t>()),
> > > > + static_cast<float>(analogueGainInfo->second.max().get<int32_t>())));
> > > > + }
> > > > +
> > >
> > > We have a list of mandatory controls in camera_sensor.cpp, among which
> > > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.
> > >
> > > I wonder if we shouldn't make it mandatory too.
> > >
> > > Apart from that, I wonder if the IPA can assume the pipeline handler
> > > has already validated the sensor driver by using CameraSensor or it
> > > should re-check here.
> > >
> > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > > >
> > > > return 0;
> > > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)
> > > >
> > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)
> > > > {
> > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > ControlList ctrls(controls::controls);
> > > >
> > > > if (aeState)
> > > > ctrls.set(controls::AeLocked, aeState == 2);
> >
> > Not your patch - but I don't like seeing aeState==2 ... What is 2 ?
> >
> >
>
> This has been bothering me as well, and I thought I had a patch in my
> tree somewhere to drop the whole prepareMetadata() function as (before
> this patch) it was just a stub.
>
> I think as part of this patch the aeState argument can now be dropped.
>
> Also consider it is always called as:
>
> unsigned int aeState = 0;
>
> for (auto const &algo : algorithms())
> algo->process(context_, frame, frameContext, stats);
>
> setControls(frame);
>
> prepareMetadata(frame, aeState);
>
> So the argument is always effectively zero. It feels like a leftover
> from some initial IPA skeleton.
Ok - lets get rid of it. Who's going to post the patch. Let me know if
you want me to do it.
--
Kieran
>
> > > >
> > > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);
> > > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);
> > > > +
> > > > metadataReady.emit(frame, ctrls);
> > > > }
> > > >
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 455ee2a0..4f8e467a 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -91,7 +91,7 @@ public:
> > > > }
> > > >
> > > > PipelineHandlerRkISP1 *pipe();
> > > > - int loadIPA(unsigned int hwRevision);
> > > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);
> > > >
> > > > Stream mainPathStream_;
> > > > Stream selfPathStream_;
> > > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()
> > > > return static_cast<PipelineHandlerRkISP1 *>(Camera::Private::pipe());
> > > > }
> > > >
> > > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,
> > > > + const ControlInfoMap &sensorControls)
> > >
> > > Do you need to pass the controls list in or can you retrieve it from
> > > the RkISP1CameraData::sensor_ class member ?
> > >
> > > > {
> > > > ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe(), 1, 1);
> > > > if (!ipa_)
> > > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > > > }
> > > >
> > > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > > > - &controlInfo_);
> > > > + sensorControls, &controlInfo_);
> > > > if (ret < 0) {
> > > > LOG(RkISP1, Error) << "IPA initialization failure";
> > > > return ret;
> > > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > > > isp_->frameStart.connect(data->delayedCtrls_.get(),
> > > > &DelayedControls::applyControls);
> > > >
> > > > - ret = data->loadIPA(media_->hwRevision());
> > > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());
> > > > if (ret)
> > > > return ret;
> > > >
> > > > --
> > > > 2.30.2
> > > >
More information about the libcamera-devel
mailing list