[libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for manual gain and exposure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 30 17:44:16 CET 2022
Hi Kieran,
On Fri, Oct 28, 2022 at 11:02:58PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:47)
> > From: Paul Elder <paul.elder at ideasonboard.com>
> >
> > Add support for manual gain and exposure in the rkisp1 IPA.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> > include/libcamera/ipa/rkisp1.mojom | 2 +-
> > src/ipa/rkisp1/algorithms/agc.cpp | 62 +++++++++++++++++++++---
> > src/ipa/rkisp1/algorithms/agc.h | 4 ++
> > src/ipa/rkisp1/ipa_context.h | 13 ++++-
> > src/ipa/rkisp1/rkisp1.cpp | 17 +++++++
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > 6 files changed, 89 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index eaf3955e4096..b526450d9949 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 169afe372803..377a3e8a0efd 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -74,9 +74,14 @@ Agc::Agc()
> > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > {
> > /* Configure the default exposure and gain. */
> > - context.activeState.agc.gain = std::max(context.configuration.sensor.minAnalogueGain,
> > - kMinAnalogueGain);
> > - context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> > + context.activeState.agc.automatic.gain =
> > + std::max(context.configuration.sensor.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:
> > @@ -108,14 +113,57 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > return 0;
> > }
> >
> > +/**
> > + * \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;
> > +
> > + LOG(RkISP1Agc, Debug)
> > + << (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> > + }
> > +
> > + const auto &exposure = controls.get(controls::ExposureTime);
> > + if (exposure && !agc.autoEnabled) {
> > + agc.manual.exposure = *exposure;
> > +
> > + 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;
> > + }
> > +}
> > +
> > /**
> > * \copydoc libcamera::ipa::Algorithm::prepare
> > */
> > 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;
> > @@ -263,8 +311,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;
> > }
> >
> > /**
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index da4d2d4e8359..a228d0c37768 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -26,6 +26,10 @@ public:
> > ~Agc() = default;
> >
> > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > + void queueRequest(IPAContext &context,
> > + const uint32_t frame,
> > + IPAFrameContext &frameContext,
> > + const ControlList &controls) override;
> > void prepare(IPAContext &context, const uint32_t frame,
> > IPAFrameContext &frameContext,
> > rkisp1_params_cfg *params) override;
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 3e47ac663c58..b9b2065328d6 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -54,8 +54,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 {
> > @@ -96,6 +104,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 538e42cb6f7f..5c041ded0db4 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -50,6 +50,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 */
> > @@ -184,6 +186,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>())));
> > + }
>
> It's likely a yak - but this is another area I've been thinking about
> lately.
It's definitely a yak we need to shave. Any volunteer ? :-)
> We're deciding what controls are supported by the algorithms at the top
> level, and I think this is probably something we should push down to
> each algorithm to report back. (and do any validation they require).
>
> Yet Another Call into each algorithm for init() though?
We could do that in Algorithm::init(), but we will likely need to update
the range of some of the controls at configure() time as well. Maybe
that can be handled in a second step ?
> But then we could push the algorith to validate it has the information
> it needs from the sensor (and fail early otherwise) and then the ctrlMap
> would only get populated with controls that are supported by the
> algorithms.
>
> I.e. - if (for some reason) AGC is disabled, no AGC controls would get
> registered.
>
> > +
> > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> >
> > return 0;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 455ee2a0a711..1418dc9a47fb 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -348,7 +348,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > }
> >
> > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > - &controlInfo_);
> > + sensor_->controls(), &controlInfo_);
> > if (ret < 0) {
> > LOG(RkISP1, Error) << "IPA initialization failure";
> > return ret;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list