ipa: rkisp1: agc: Store per-frame information in frame context

Rework the algorithm's usage of the active state to store the value of
controls for the last queued request in the queueRequest() function, and
store a copy of the values in the corresponding frame context.

The frame context is used in the prepare() function to populate the ISP
parameters with values corresponding to the right frame.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

5 files changed, 62 insertions, 27 deletions

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 10958a7e..04062a36 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
 /**
  * \brief Estimate the new exposure and gain values
  * \param[inout] context The shared IPA Context
+ * \param[in] frameContext The FrameContext for this frame
  * \param[in] yGain The gain calculated on the current brightness level
  * \param[in] iqMeanGain The gain calculated based on the relative luminance target
  */
-void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)
+void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
+			  double yGain, double iqMeanGain)
 {
 	IPASessionConfiguration &configuration = context.configuration;
 	IPAActiveState &activeState = context.activeState;
 
 	/* Get the effective exposure and gain applied on the sensor. */
-	uint32_t exposure = activeState.sensor.exposure;
-	double analogueGain = activeState.sensor.gain;
+	uint32_t exposure = frameContext.sensor.exposure;
+	double analogueGain = frameContext.sensor.gain;
 
 	/* Use the highest of the two gain estimates. */
 	double evGain = std::max(yGain, iqMeanGain);
@@ -286,9 +288,16 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
  * new exposure and gain for the scene.
  */
 void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
-		  [[maybe_unused]] IPAFrameContext &frameContext,
-		  const rkisp1_stat_buffer *stats)
+		  IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats)
 {
+	/*
+	 * \todo Verify that the exposure and gain applied by the sensor for
+	 * this frame match what has been requested. This isn't a hard
+	 * requirement for stability of the AGC (the guarantee we need in
+	 * automatic mode is a perfect match between the frame and the values
+	 * we receive), but is important in manual mode.
+	 */
+
 	const rkisp1_cif_isp_stat *params = &stats->params;
 	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
 
@@ -320,7 +329,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 			break;
 	}
 
-	computeExposure(context, yGain, iqMeanGain);
+	computeExposure(context, frameContext, yGain, iqMeanGain);
 	frameCount_++;
 }
 
@@ -328,9 +337,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
  * \copydoc libcamera::ipa::Algorithm::prepare
  */
 void Agc::prepare(IPAContext &context, const uint32_t frame,
-		  [[maybe_unused]] IPAFrameContext &frameContext,
-		  rkisp1_params_cfg *params)
+		  IPAFrameContext &frameContext, rkisp1_params_cfg *params)
 {
+	frameContext.agc.exposure = context.activeState.agc.exposure;
+	frameContext.agc.gain = context.activeState.agc.gain;
+
 	if (frame > 0)
 		return;
 
diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
index f115ba2e..9ad5c32f 100644
--- a/src/ipa/rkisp1/algorithms/agc.h
+++ b/src/ipa/rkisp1/algorithms/agc.h
@@ -34,7 +34,8 @@ public:
 		     const rkisp1_stat_buffer *stats) override;
 
 private:
-	void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);
+	void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,
+			     double yGain, double iqMeanGain);
 	utils::Duration filterExposure(utils::Duration exposureValue);
 	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
 	double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
index 78a785f5..c7d5b1b6 100644
--- a/src/ipa/rkisp1/ipa_context.cpp
+++ b/src/ipa/rkisp1/ipa_context.cpp
@@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {
  * \var IPAActiveState::agc
  * \brief State for the Automatic Gain Control algorithm
  *
- * The exposure and gain determined are expected to be applied to the sensor
- * at the earliest opportunity.
+ * The exposure and gain are the latest values computed by the AGC algorithm.
  *
  * \var IPAActiveState::agc.exposure
  * \brief Exposure time expressed as a number of lines
  *
  * \var IPAActiveState::agc.gain
  * \brief Analogue gain multiplier
- *
- * The gain should be adapted to the sensor specific gain code before applying.
  */
 
 /**
@@ -182,21 +179,37 @@ namespace libcamera::ipa::rkisp1 {
  */
 
 /**
- * \var IPAActiveState::sensor
- * \brief Effective sensor values
+ * \struct IPAFrameContext
+ * \brief Per-frame context for algorithms
+ *
+ * \todo Populate the frame context for all algorithms
+ */
+
+/**
+ * \var IPAFrameContext::agc
+ * \brief Automatic Gain Control parameters for this frame
  *
- * \var IPAActiveState::sensor.exposure
+ * The exposure and gain are provided by the AGC algorithm, and are to be
+ * applied to the sensor in order to take effect for this frame.
+ *
+ * \var IPAFrameContext::agc.exposure
  * \brief Exposure time expressed as a number of lines
  *
- * \var IPAActiveState::sensor.gain
+ * \var IPAFrameContext::agc.gain
  * \brief Analogue gain multiplier
+ *
+ * The gain should be adapted to the sensor specific gain code before applying.
  */
 
 /**
- * \struct IPAFrameContext
- * \brief Per-frame context for algorithms
+ * \var IPAFrameContext::sensor
+ * \brief Sensor configuration that used been used for this frame
  *
- * \todo Populate the frame context for all algorithms
+ * \var IPAFrameContext::sensor.exposure
+ * \brief Exposure time expressed as a number of lines
+ *
+ * \var IPAFrameContext::sensor.gain
+ * \brief Analogue gain multiplier
  */
 
 /**
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 1390f803..3fbd6b18 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -82,14 +82,18 @@ struct IPAActiveState {
 		uint8_t sharpness;
 		bool updateParams;
 	} filter;
+};
 
+struct IPAFrameContext : public FrameContext {
 	struct {
 		uint32_t exposure;
 		double gain;
-	} sensor;
-};
+	} agc;
 
-struct IPAFrameContext : public FrameContext {
+	struct {
+		uint32_t exposure;
+		double gain;
+	} sensor;
 };
 
 struct IPAContext {
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 11fd8606..32feb168 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -333,9 +333,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
 		reinterpret_cast<rkisp1_stat_buffer *>(
 			mappedBuffers_.at(bufferId).planes()[0].data());
 
-	context_.activeState.sensor.exposure =
+	frameContext.sensor.exposure =
 		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-	context_.activeState.sensor.gain =
+	frameContext.sensor.gain =
 		camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
 
 	unsigned int aeState = 0;
@@ -350,8 +350,14 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
 
 void IPARkISP1::setControls(unsigned int frame)
 {
-	uint32_t exposure = context_.activeState.agc.exposure;
-	uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
+	/*
+	 * \todo The frame number is most likely wrong here, we need to take
+	 * internal sensor delays and other timing parameters into account.
+	 */
+
+	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
+	uint32_t exposure = frameContext.agc.exposure;
+	uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);
 
 	ControlList ctrls(ctrls_);
 	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));