| Message ID | 20221024000356.29521-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent On Mon, Oct 24, 2022 at 03:03:47AM +0300, Laurent Pinchart via libcamera-devel wrote: > From: Paul Elder <paul.elder@ideasonboard.com> > > Add support for manual gain and exposure in the rkisp1 IPA. > > Signed-off-by: Paul Elder <paul.elder@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>()))); > + } > + At configure() time const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); if (itExp == ctrls_.end()) { LOG(IPARkISP1, Error) << "Can't find exposure control"; return -EINVAL; } const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN); if (itGain == ctrls_.end()) { LOG(IPARkISP1, Error) << "Can't find gain control"; return -EINVAL; } Could well fail earlier at init() time ? > *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 >
Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:47) > From: Paul Elder <paul.elder@ideasonboard.com> > > Add support for manual gain and exposure in the rkisp1 IPA. > > Signed-off-by: Paul Elder <paul.elder@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. 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? 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 >
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@ideasonboard.com> > > > > Add support for manual gain and exposure in the rkisp1 IPA. > > > > Signed-off-by: Paul Elder <paul.elder@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;
Hi Jacopo, On Wed, Oct 26, 2022 at 04:26:25PM +0200, Jacopo Mondi wrote: > On Mon, Oct 24, 2022 at 03:03:47AM +0300, Laurent Pinchart via libcamera-devel wrote: > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > Add support for manual gain and exposure in the rkisp1 IPA. > > > > Signed-off-by: Paul Elder <paul.elder@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>()))); > > + } > > + > > At configure() time > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > if (itExp == ctrls_.end()) { > LOG(IPARkISP1, Error) << "Can't find exposure control"; > return -EINVAL; > } > > const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN); > if (itGain == ctrls_.end()) { > LOG(IPARkISP1, Error) << "Can't find gain control"; > return -EINVAL; > } > > Could well fail earlier at init() time ? I think we should, yes. > > *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;
Hi Paul, Thank you for the patch. On Mon, Oct 24, 2022 at 03:03:47AM +0300, Laurent Pinchart via libcamera-devel wrote: > From: Paul Elder <paul.elder@ideasonboard.com> > > Add support for manual gain and exposure in the rkisp1 IPA. > > Signed-off-by: Paul Elder <paul.elder@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; s/:// and below too. > + } > + > + 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()) { No need for curly braces. > + ctrlMap.emplace(&controls::ExposureTime, > + ControlInfo(exposureTimeInfo->second.min(), > + exposureTimeInfo->second.max())); To really benefit from emplace, you should write ctrlMap.emplace(std::piecewise_construct, std::forward_as_tuple(&controls::ExposureTime), std::forward_as_tuple(exposureTimeInfo->second.min(), exposureTimeInfo->second.max())); to avoid the construction of a temporary ControlInfo. The units are not right for this control, you need to convert it from V4L2 units (lines) to libcamera units (time). It may be easier to handle this on top of "[PATCH 4/4] ipa: rkisp1: add FrameDurationLimits control" which I hope we'll merge quickly. This applies to the AnalogueGain control below. I can handle these issues in v4. > + } > + > + 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>()))); > + } > + > *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;
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>()))); + } + *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;