[libcamera-devel] [PATCH v3] ipa: rkisp1: Add support for manual gain and exposure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 20 03:04:09 CEST 2022
Hi Paul,
Thank you for the patch.
On Wed, Oct 19, 2022 at 10:42:44AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi (2022-10-19 10:37:13)
> > 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)
> > > > 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.
I'll get to this. Really sorry for the delay.
> > > > > 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()
Could I ask you to rebase on top of "[PATCH v2 0/3] ipa: Fill metadata
in individual algorithms" ? It shouldn't hurt too much, the only larger
conflict is in IPARkISP1::prepareMetadata(), and you can just drop your
changes to that function as it's now gone :-) There should be no need to
move that code anywhere else, it should be handled correctly in agc.cpp
already.
> > > > > ---
> > > > > 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;
Some line wrapping could be nice.
> > > > > + 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.
I was going to reply to the previous comment to tell that I was tempted,
and then that I realized we couldn't use the same pattern everywhere.
Seems I'm just following your train of thoughts :-)
> > > > > + LOG(RkISP1Agc, Debug)
> > > > > + << "Set exposure to: " << agc.manual.exposure;
s/://
> > > > > + }
> > > > > +
> > > > > + const auto &gain = controls.get(controls::AnalogueGain);
> > > > > + if (gain && !agc.autoEnabled) {
> > > > > + agc.manual.gain = *gain;
> > > > > +
> > > > > + LOG(RkISP1Agc, Debug) << "Set gain to: " << agc.manual.gain;
Ditto.
> > > > > + }
> > > > > +
> > > > > + 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()));
Let's add the default value.
And this isn't right, ExposureTime is expressed in µs, while the V4L2
controls is expressed as a number of lines. Look at the IPU3 IPA module
to see how to fix it.
> > > > > + }
> > > > > +
> > > > > + 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>())));
Here too, default value, and units.
> > > > > + }
> > > > > +
> > > >
> > > > 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.
We also perform the same validation in IPARkISP1::configure() and return
an error if it fails. At the very least, I would align the two, there's
no point in handling the lack of those controls gracefully in init() if
we fail configure().
If we want to be on the safe side, I would move the check from
configure() to init(), as getting a valid ControlInfoMap in init() but
not in configure() seems like a very pathological case to me. We could
go one step further and consider that the pipeline handler has already
performed the required validation.
We also have IPAIPU3::validateSensorControls(), which is called in
configure(), and we access the sensor controls in IPAIPU3::init(),
without any check. Sounds like there's room for some cleanups to align
the two IPA modules. Let's reach a consensus on this, and it can then be
handled on top, as aligning the two requires passed the sensor control
info map to init(), which is introduced in this patch for the RkISP1 IPA
module.
In the future, I could possibly imagine needing to support sensors that
have no controllable analog gain, although that sounds a bit
far-fetched. If that happens we'll have to update the AGC algorithm
anyway, so I don't think we need to care about this for now.
> > > > > *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.
v2 is on the list, just missing a few reviews :-)
> > > > >
> > > > > + 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 ?
Agreed, let's use sensor_.
> > > > > {
> > > > > 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;
> > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list