[v2,6/7] ipa: rpi: Update digital gain handling in IPA base and derived classes
diff mbox series

Message ID 20250721074853.1463358-7-naush@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi AEC/AGC update
Related show

Commit Message

Naushir Patuck July 21, 2025, 7:47 a.m. UTC 
From: David Plowman <david.plowman@raspberrypi.com>

Here we update the digital gain handling to use the value computed by
process() in the AgcStatus, not the version that was previously in the
AgcPrepareStatus.

Because we apply this digital gain directly with no further
modification, we have to update it to reflect any exposure/gain
quantisation that happens (in IpaBase::applyAGC).

We must also run the new platformPrepareAgc() even when we're skipping
platformPrepareIsp(), which has been split out of the previous
platformPrepareIsp() implementation.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 50 +++++++++++++++++++--------
 src/ipa/rpi/common/ipa_base.h   |  6 +++-
 src/ipa/rpi/pisp/pisp.cpp       | 61 +++++++++++++++++++++------------
 src/ipa/rpi/vc4/vc4.cpp         | 28 +++++++++------
 4 files changed, 98 insertions(+), 47 deletions(-)

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index 98690b80d5d3..1408c5e32a69 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -298,20 +298,23 @@  void IpaBase::start(const ControlList &controls, StartResult *result)
 	frameLengths_.clear();
 	frameLengths_.resize(FrameLengthsQueueSize, 0s);
 
-	/* SwitchMode may supply updated exposure/gain values to use. */
-	AgcStatus agcStatus;
-	agcStatus.exposureTime = 0.0s;
-	agcStatus.analogueGain = 0.0;
+	/*
+	 * SwitchMode may supply updated exposure/gain values to use.
+	 * agcStatus_ will store these values for us to use until delayed_status values
+	 * start to appear.
+	 */
+	agcStatus_.exposureTime = 0.0s;
+	agcStatus_.analogueGain = 0.0;
 
-	metadata.get("agc.status", agcStatus);
-	if (agcStatus.exposureTime && agcStatus.analogueGain) {
+	metadata.get("agc.status", agcStatus_);
+	if (agcStatus_.exposureTime && agcStatus_.analogueGain) {
 		ControlList ctrls(sensorCtrls_);
-		applyAGC(&agcStatus, ctrls);
+		applyAGC(&agcStatus_, ctrls);
 		result->controls = std::move(ctrls);
 		setCameraTimeoutValue();
 	}
 	/* Make a note of this as it tells us the HDR status of the first few frames. */
-	hdrStatus_ = agcStatus.hdr;
+	hdrStatus_ = agcStatus_.hdr;
 
 	/*
 	 * AF: If no lens position was specified, drive lens to a default position.
@@ -486,7 +489,9 @@  void IpaBase::prepareIsp(const PrepareParams &params)
 		controller_.prepare(&rpiMetadata);
 		/* Actually prepare the ISP parameters for the frame. */
 		platformPrepareIsp(params, rpiMetadata);
-	}
+		platformPrepareAgc(rpiMetadata);
+	} else
+		platformPrepareAgc(rpiMetadata);
 
 	frameCount_++;
 
@@ -525,6 +530,7 @@  void IpaBase::processStats(const ProcessParams &params)
 	if (rpiMetadata.get("agc.status", agcStatus) == 0) {
 		ControlList ctrls(sensorCtrls_);
 		applyAGC(&agcStatus, ctrls);
+		rpiMetadata.set("agc.status", agcStatus);
 		setDelayedControls.emit(ctrls, ipaContext);
 		setCameraTimeoutValue();
 	}
@@ -1422,9 +1428,6 @@  void IpaBase::reportMetadata(unsigned int ipaContext)
 	}
 
 	AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
-	if (agcPrepareStatus)
-		libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
-
 	RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
 		controller_.getAlgorithm("agc"));
 	if (agc) {
@@ -1437,6 +1440,13 @@  void IpaBase::reportMetadata(unsigned int ipaContext)
 						       : controls::AeStateSearching);
 	}
 
+	const AgcStatus *agcStatus = rpiMetadata.getLocked<AgcStatus>("agc.delayed_status");
+	if (agcStatus)
+		libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);
+	else
+		libcameraMetadata_.set(controls::DigitalGain, agcStatus_.digitalGain);
+	/* The HDR metadata reporting will use this agcStatus too. */
+
 	LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
 	if (luxStatus)
 		libcameraMetadata_.set(controls::Lux, luxStatus->lux);
@@ -1526,7 +1536,6 @@  void IpaBase::reportMetadata(unsigned int ipaContext)
 	 * delayed_status to be available, we use the HDR status that came out of the
 	 * switchMode call.
 	 */
-	const AgcStatus *agcStatus = rpiMetadata.getLocked<AgcStatus>("agc.delayed_status");
 	const HdrStatus &hdrStatus = agcStatus ? agcStatus->hdr : hdrStatus_;
 	if (!hdrStatus.mode.empty() && hdrStatus.mode != "Off") {
 		int32_t hdrMode = controls::HdrModeOff;
@@ -1584,7 +1593,7 @@  void IpaBase::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDu
 		agc->setMaxExposureTime(maxExposureTime);
 }
 
-void IpaBase::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
+void IpaBase::applyAGC(struct AgcStatus *agcStatus, ControlList &ctrls)
 {
 	const int32_t minGainCode = helper_->gainCode(mode_.minAnalogueGain);
 	const int32_t maxGainCode = helper_->gainCode(mode_.maxAnalogueGain);
@@ -1613,6 +1622,19 @@  void IpaBase::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
 	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
 
+	/*
+	 * We must update the digital gain to make up for any quantisation that happens, and
+	 * communicate that back into the metadata so that it will appear as the "delayed" status.
+	 * (Note that "exposure" is already the "actual" exposure.)
+	 */
+	double actualGain = helper_->gain(gainCode);
+	double ratio = agcStatus->analogueGain / actualGain;
+	ratio *= agcStatus->exposureTime / exposure;
+	double newDigitalGain = agcStatus->digitalGain * ratio;
+	agcStatus->digitalGain = newDigitalGain;
+	agcStatus->analogueGain = actualGain;
+	agcStatus->exposureTime = exposure;
+
 	/*
 	 * At present, there is no way of knowing if a control is read-only.
 	 * As a workaround, assume that if the minimum and maximum values of
diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
index e818104ba633..e2f6e330b2ab 100644
--- a/src/ipa/rpi/common/ipa_base.h
+++ b/src/ipa/rpi/common/ipa_base.h
@@ -73,6 +73,9 @@  protected:
 	/* Remember the HDR status after a mode switch. */
 	HdrStatus hdrStatus_;
 
+	/* Remember the AGC status after a mode switch. */
+	AgcStatus agcStatus_;
+
 	/* Whether the stitch block (if available) needs to swap buffers. */
 	bool stitchSwapBuffers_;
 
@@ -86,6 +89,7 @@  private:
 
 	virtual void platformPrepareIsp(const PrepareParams &params,
 					RPiController::Metadata &rpiMetadata) = 0;
+	virtual void platformPrepareAgc(RPiController::Metadata &rpiMetadata) = 0;
 	virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0;
 
 	void setMode(const IPACameraSensorInfo &sensorInfo);
@@ -97,7 +101,7 @@  private:
 	void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
 	void reportMetadata(unsigned int ipaContext);
 	void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);
-	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
+	void applyAGC(struct AgcStatus *agcStatus, ControlList &ctrls);
 
 	std::map<unsigned int, MappedFrameBuffer> buffers_;
 
diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp
index e1a804f533bb..ab70d8f42636 100644
--- a/src/ipa/rpi/pisp/pisp.cpp
+++ b/src/ipa/rpi/pisp/pisp.cpp
@@ -218,13 +218,14 @@  private:
 
 	void platformPrepareIsp(const PrepareParams &params,
 				RPiController::Metadata &rpiMetadata) override;
+	void platformPrepareAgc(RPiController::Metadata &rpiMetadata) override;
 	RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) override;
 
 	void handleControls(const ControlList &controls) override;
 
-	void applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcStatus,
+	void applyWBG(const AwbStatus *awbStatus, double digitalGain,
 		      pisp_be_global_config &global);
-	void applyDgOnly(const AgcPrepareStatus *agcPrepareStatus, pisp_be_global_config &global);
+	void applyDgOnly(double digitalGain, pisp_be_global_config &global);
 	void applyCAC(const CacStatus *cacStatus, pisp_be_global_config &global);
 	void applyContrast(const ContrastStatus *contrastStatus,
 			   pisp_be_global_config &global);
@@ -341,7 +342,6 @@  void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
 				PISP_BE_RGB_ENABLE_SHARPEN + PISP_BE_RGB_ENABLE_SAT_CONTROL);
 
 	NoiseStatus *noiseStatus = rpiMetadata.getLocked<NoiseStatus>("noise.status");
-	AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
 
 	{
 		/* All Frontend config goes first, we do not want to hold the FE lock for long! */
@@ -355,14 +355,6 @@  void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
 		if (blackLevelStatus)
 			applyBlackLevel(blackLevelStatus, global);
 
-		AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status");
-		if (awbStatus && agcPrepareStatus) {
-			/* Applies digital gain as well. */
-			applyWBG(awbStatus, agcPrepareStatus, global);
-		} else if (agcPrepareStatus) {
-			/* Mono sensor fallback for digital gain. */
-			applyDgOnly(agcPrepareStatus, global);
-		}
 	}
 
 	CacStatus *cacStatus = rpiMetadata.getLocked<CacStatus>("cac.status");
@@ -443,6 +435,34 @@  void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
 	}
 }
 
+void IpaPiSP::platformPrepareAgc(RPiController::Metadata &rpiMetadata)
+{
+	std::scoped_lock<RPiController::Metadata> l(rpiMetadata);
+
+	AgcStatus *delayedAgcStatus = rpiMetadata.getLocked<AgcStatus>("agc.delayed_status");
+	/* If no delayed status, use the gain from the last mode switch. */
+	double digitalGain = delayedAgcStatus ? delayedAgcStatus->digitalGain : agcStatus_.digitalGain;
+	AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status");
+
+	pisp_be_global_config global;
+	be_->GetGlobal(global);
+
+	{
+		/* All Frontend config goes first, we do not want to hold the FE lock for long! */
+		std::scoped_lock<FrontEnd> lf(*fe_);
+
+		if (awbStatus) {
+			/* Applies digital gain as well. */
+			applyWBG(awbStatus, digitalGain, global);
+		} else {
+			/* Mono sensor fallback for digital gain. */
+			applyDgOnly(digitalGain, global);
+		}
+	}
+
+	be_->SetGlobal(global);
+}
+
 RPiController::StatisticsPtr IpaPiSP::platformProcessStats(Span<uint8_t> mem)
 {
 	using namespace RPiController;
@@ -515,12 +535,11 @@  void IpaPiSP::handleControls(const ControlList &controls)
 	}
 }
 
-void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPrepareStatus,
+void IpaPiSP::applyWBG(const AwbStatus *awbStatus, double digitalGain,
 		       pisp_be_global_config &global)
 {
 	pisp_wbg_config wbg;
 	pisp_fe_rgby_config rgby = {};
-	double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0;
 	double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 });
 	/* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */
 	double extraGain = 1.0 / std::max({ minColourGain, 0.1 });
@@ -536,9 +555,9 @@  void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr
 	double gainG = awbStatus->gainG * extraGain;
 	double gainB = awbStatus->gainB * extraGain;
 
-	wbg.gain_r = clampField(dg * gainR, 14, 10);
-	wbg.gain_g = clampField(dg * gainG, 14, 10);
-	wbg.gain_b = clampField(dg * gainB, 14, 10);
+	wbg.gain_r = clampField(digitalGain * gainR, 14, 10);
+	wbg.gain_g = clampField(digitalGain * gainG, 14, 10);
+	wbg.gain_b = clampField(digitalGain * gainB, 14, 10);
 
 	/*
 	 * The YCbCr conversion block should contain the appropriate YCbCr
@@ -561,15 +580,15 @@  void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr
 	global.bayer_enables |= PISP_BE_BAYER_ENABLE_WBG;
 }
 
-void IpaPiSP::applyDgOnly(const AgcPrepareStatus *agcPrepareStatus, pisp_be_global_config &global)
+void IpaPiSP::applyDgOnly(double digitalGain, pisp_be_global_config &global)
 {
 	pisp_wbg_config wbg;
 
-	wbg.gain_r = clampField(agcPrepareStatus->digitalGain, 14, 10);
-	wbg.gain_g = clampField(agcPrepareStatus->digitalGain, 14, 10);
-	wbg.gain_b = clampField(agcPrepareStatus->digitalGain, 14, 10);
+	wbg.gain_r = clampField(digitalGain, 14, 10);
+	wbg.gain_g = clampField(digitalGain, 14, 10);
+	wbg.gain_b = clampField(digitalGain, 14, 10);
 
-	LOG(IPARPI, Debug) << "Applying DG (only) : " << agcPrepareStatus->digitalGain;
+	LOG(IPARPI, Debug) << "Applying DG (only) : " << digitalGain;
 
 	be_->SetWbg(wbg);
 	global.bayer_enables |= PISP_BE_BAYER_ENABLE_WBG;
diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp
index 8a7a37c870ed..b2fec9344804 100644
--- a/src/ipa/rpi/vc4/vc4.cpp
+++ b/src/ipa/rpi/vc4/vc4.cpp
@@ -57,14 +57,14 @@  private:
 	int32_t platformConfigure(const ConfigParams &params, ConfigResult *result) override;
 
 	void platformPrepareIsp(const PrepareParams &params, RPiController::Metadata &rpiMetadata) override;
+	void platformPrepareAgc([[maybe_unused]] RPiController::Metadata &rpiMetadata) override;
 	RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) override;
 
 	void handleControls(const ControlList &controls) override;
 	bool validateIspControls();
 
 	void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
-	void applyDG(const struct AgcPrepareStatus *dgStatus,
-		     const struct AwbStatus *awbStatus, ControlList &ctrls);
+	void applyDG(double digitalGain, const struct AwbStatus *awbStatus, ControlList &ctrls);
 	void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
 	void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);
 	void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);
@@ -78,6 +78,7 @@  private:
 
 	/* VC4 ISP controls. */
 	ControlInfoMap ispCtrls_;
+	ControlList ctrls_;
 
 	/* LS table allocation passed in from the pipeline handler. */
 	SharedFD lsTableHandle_;
@@ -107,6 +108,7 @@  int32_t IpaVc4::platformStart([[maybe_unused]] const ControlList &controls,
 int32_t IpaVc4::platformConfigure(const ConfigParams &params, [[maybe_unused]] ConfigResult *result)
 {
 	ispCtrls_ = params.ispControls;
+	ctrls_ = ControlList(ispCtrls_);
 	if (!validateIspControls()) {
 		LOG(IPARPI, Error) << "ISP control validation failed.";
 		return -1;
@@ -139,7 +141,7 @@  int32_t IpaVc4::platformConfigure(const ConfigParams &params, [[maybe_unused]] C
 void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
 				RPiController::Metadata &rpiMetadata)
 {
-	ControlList ctrls(ispCtrls_);
+	ControlList &ctrls = ctrls_;
 
 	/* Lock the metadata buffer to avoid constant locks/unlocks. */
 	std::unique_lock<RPiController::Metadata> lock(rpiMetadata);
@@ -152,9 +154,6 @@  void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
 	if (ccmStatus)
 		applyCCM(ccmStatus, ctrls);
 
-	AgcPrepareStatus *dgStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
-	applyDG(dgStatus, awbStatus, ctrls);
-
 	AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status");
 	if (lsStatus)
 		applyLS(lsStatus, ctrls);
@@ -190,9 +189,18 @@  void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams &params,
 		if (!lensctrls.empty())
 			setLensControls.emit(lensctrls);
 	}
+}
 
-	if (!ctrls.empty())
-		setIspControls.emit(ctrls);
+void IpaVc4::platformPrepareAgc(RPiController::Metadata &rpiMetadata)
+{
+	AgcStatus *delayedAgcStatus = rpiMetadata.getLocked<AgcStatus>("agc.delayed_status");
+	double digitalGain = delayedAgcStatus ? delayedAgcStatus->digitalGain : agcStatus_.digitalGain;
+	AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status");
+
+	applyDG(digitalGain, awbStatus, ctrls_);
+
+	setIspControls.emit(ctrls_);
+	ctrls_ = ControlList(ispCtrls_);
 }
 
 RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem)
@@ -329,11 +337,9 @@  void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 		  static_cast<int32_t>(awbStatus->gainB * 1000));
 }
 
-void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus,
+void IpaVc4::applyDG(double digitalGain,
 		     const struct AwbStatus *awbStatus, ControlList &ctrls)
 {
-	double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0;
-
 	if (awbStatus) {
 		/*
 		 * We must apply sufficient extra digital gain to stop any of the channel gains being