[libcamera-devel,v3] ipa: rkisp1: Add support for manual gain and exposure
diff mbox series

Message ID 20221018054913.2325021-1-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel,v3] ipa: rkisp1: Add support for manual gain and exposure
Related show

Commit Message

Paul Elder Oct. 18, 2022, 5:49 a.m. UTC 
Add support for manual gain and exposure in the rkisp1 IPA.

Signed-off-by: Paul Elder <paul.elder@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(-)

Comments

Jacopo Mondi Oct. 19, 2022, 8:59 a.m. UTC | #1 
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

> Signed-off-by: Paul Elder <paul.elder@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;
> +
> +		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;
> +	}
> +}
> +

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);
>
> +	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
>
Kieran Bingham Oct. 19, 2022, 9:15 a.m. UTC | #2 
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@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 ?


> >
> > +     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
> >
Jacopo Mondi Oct. 19, 2022, 9:37 a.m. UTC | #3 
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@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.

> > >
> > > +     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
> > >
Laurent Pinchart Oct. 19, 2022, 9:42 a.m. UTC | #4 
On Wed, Oct 19, 2022 at 11:37:13AM +0200, Jacopo Mondi via libcamera-devel wrote:
> 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.
> >
> > > > Signed-off-by: Paul Elder <paul.elder@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.

I have a patch to drop it, I'll post it shortly (as part of a series).

> > > >
> > > > +     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;
> > > >
Kieran Bingham Oct. 19, 2022, 9:42 a.m. UTC | #5 
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@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
> > > >
Laurent Pinchart Oct. 20, 2022, 1:04 a.m. UTC | #6 
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@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;
> > > > >
Jacopo Mondi Oct. 20, 2022, 8 a.m. UTC | #7 
Hi Laurent

On Thu, Oct 20, 2022 at 04:04:09AM +0300, Laurent Pinchart wrote:
> 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@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

Ah indeed, we check in configure() but in init() we assume the
controls are there :/

> 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.

This is more of a general question about how much we can assume about
the PH/IPA module coupling... A possible concern is that someone can
use one of the two and replace the other with some custom component,
and all the assumptions that the PH has validated the sensor on behalf
of the IPA crumbles down.. BUT if anyone is taking
the direction of replacing pieces of a "mainline" platform for
whatever reason, I guess it's fair to say it's not an issue
mainline development should be concerned with.

For platforms whose support is in the master branch I guess we can
assume on the IPA that all controls have been correctly validated by
the PH, and the PH knows what controls the IPA needs.

My only concern here is that if we move the IPA requirements in the
CameraSensor validation (here's another assumption that PH will use
CameraSensor, but the same reasoning as before goes) we will rise the
bar a little more for sensor drivers. It's not different than today,
as the same validation that is today performed in the IPA will be
moved to CameraSensor, but I wonder if this will be problematic as
CameraSensor can be used in platforms without an IPA and that might
not have the same requirements as the ones we have today.

To make an example, we already require today in CameraSensor to have a
controllable exposure time, but for some platforms (Simple, in example)
it might not be strictly required and, as far as I can tell, the
exposure is not even controlled there. Adding one more requirement
could limit or make it more complicated to use YUV sensors with the
Simple pipeline handler is a sort of plug&play fashion.

>
> 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
Laurent Pinchart Oct. 20, 2022, 9:02 a.m. UTC | #8 
Hi Jacopo,

On Thu, Oct 20, 2022 at 10:00:05AM +0200, Jacopo Mondi wrote:
> On Thu, Oct 20, 2022 at 04:04:09AM +0300, Laurent Pinchart wrote:
> > 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@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
> 
> Ah indeed, we check in configure() but in init() we assume the
> controls are there :/
> 
> > 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.
> 
> This is more of a general question about how much we can assume about
> the PH/IPA module coupling... A possible concern is that someone can
> use one of the two and replace the other with some custom component,
> and all the assumptions that the PH has validated the sensor on behalf
> of the IPA crumbles down.. BUT if anyone is taking
> the direction of replacing pieces of a "mainline" platform for
> whatever reason, I guess it's fair to say it's not an issue
> mainline development should be concerned with.

IPA modules and pipeline handlers must be designed together, as shown by
the fact that they use a platform-specific interface (per-platform
.mojom files). It's perfectly valid in my opinion to make assumptions
part of that design. They should ideally be documented, and become part
of the ABI.

The particular assumption we're going to make here may not be something
we want in an ABI though. I can imagine people wanting to use a YUV
sensor with the RkISP1, bypassing the ISP completely. In that case, even
though exposure time and analog gain will likely be exposed by the
sensor, other controls that would be considered mandatory for a raw
sensor may not. We can always change this later as long as we don't have
external API modules.

> For platforms whose support is in the master branch I guess we can
> assume on the IPA that all controls have been correctly validated by
> the PH, and the PH knows what controls the IPA needs.

I agree. A bit of defensive programming against our own mistakes may not
be a bad idea, the same way we have assertions here and there, we could
also validate controls in init(), but we don't need to check everywhere
all the time as if the inputs to the IPA module were completely
untrusted.

> My only concern here is that if we move the IPA requirements in the
> CameraSensor validation (here's another assumption that PH will use
> CameraSensor, but the same reasoning as before goes) we will rise the
> bar a little more for sensor drivers. It's not different than today,
> as the same validation that is today performed in the IPA will be
> moved to CameraSensor, but I wonder if this will be problematic as
> CameraSensor can be used in platforms without an IPA and that might
> not have the same requirements as the ones we have today.

I wouldn't phrase the IPA protocol contract as "the pipeline handler
uses CameraSensor", but as "the pipeline handler guarantees that this
particular set of controls is available". How the pipeline handler
implements that, whether by delegating the checks to CameraSensor, or
implementing them manually, isn't something that the IPA module should
be concerned with.

> To make an example, we already require today in CameraSensor to have a
> controllable exposure time, but for some platforms (Simple, in example)
> it might not be strictly required and, as far as I can tell, the
> exposure is not even controlled there. Adding one more requirement
> could limit or make it more complicated to use YUV sensors with the
> Simple pipeline handler is a sort of plug&play fashion.

You're right, I certainly foresee the need to support different types of
sensors, with different feature sets. It will likely not affect exposure
time and analog gain, but will affect other features in the future.
There are multiple ways to implement the requirement on the pipeline
handler side. I'm not too concerned at this point about what option will
be picked in the long run. Maybe we'll still have those checks in the
CameraSensor class but condition them on the sensor type for instance.
Maybe the pipeline handler will explicitly tell the CameraSensor what
controls (or classes of controls) it requires. Maybe we'll do something
else, but in any case, how this is checked on the pipeline handler side
isn't something the IPA module needs to know.

> > 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;
> > > > > > >
Laurent Pinchart Oct. 21, 2022, 10:35 p.m. UTC | #9 
Hi Paul,

Another comment.

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.
> 
> Signed-off-by: Paul Elder <paul.elder@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;
> +
> +		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;
> +	}

This needs to take into account the boundaries for the exposure time and
gain.

> +
> +	frameContext.agc.autoEnabled = agc.autoEnabled;
> +
> +	if (!frameContext.agc.autoEnabled) {
> +		frameContext.agc.exposure = agc.manual.exposure;
> +		frameContext.agc.gain = agc.manual.gain;
> +	}
> +}
> +
>  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>())));
> +	}
> +
>  	*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);
>  
> +	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)
>  {
>  	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;
>

Patch
diff mbox series 

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;
+
+		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;
+	}
+}
+
 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>())));
+	}
+
 	*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);
 
+	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)
 {
 	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;