[PATCH 2/5] ipa: rkisp1: agc: Add digital gain

[Stefan Klug]

Hi Paul,

thanks for the patch.

On Fri, Apr 05, 2024 at 11:47:26PM +0900, Paul Elder wrote:
> Add support to the rkisp1 AGC to set digital gain.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 5 +++++
>  src/ipa/rkisp1/ipa_context.h      | 3 +++
>  src/ipa/rkisp1/rkisp1.cpp         | 2 ++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index fd47ba4e..7220f00a 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -10,6 +10,7 @@
>  #include <algorithm>
>  #include <chrono>
>  #include <cmath>
> +#include <tuple>
>  
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
> @@ -152,8 +153,10 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;
>  	context.activeState.agc.automatic.exposure =
>  		10ms / context.configuration.sensor.lineDuration;
> +	context.activeState.agc.automatic.dgain = 1;
>  	context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
>  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> +	context.activeState.agc.manual.dgain = 1;
>  	context.activeState.agc.autoEnabled = !context.configuration.raw;
>  
>  	/*
> @@ -237,6 +240,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>  	if (frameContext.agc.autoEnabled) {
>  		frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
>  		frameContext.agc.gain = context.activeState.agc.automatic.gain;
> +		frameContext.agc.dgain = context.activeState.agc.automatic.dgain;
>  	}
>  
>  	/* \todo Remove this when we can set the below with controls */
> @@ -380,6 +384,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	/* Update the estimated exposure and gain. */
>  	activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
>  	activeState.agc.automatic.gain = aGain;
> +	activeState.agc.automatic.dgain = dGain;
>  
>  	fillMetadata(context, frameContext, metadata);
>  }
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 256b75eb..a70c7eb3 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -61,10 +61,12 @@ struct IPAActiveState {
>  		struct {
>  			uint32_t exposure;
>  			double gain;
> +			double dgain;
>  		} manual;
>  		struct {
>  			uint32_t exposure;
>  			double gain;
> +			double dgain;
>  		} automatic;
>  
>  		bool autoEnabled;
> @@ -110,6 +112,7 @@ struct IPAFrameContext : public FrameContext {
>  	struct {
>  		uint32_t exposure;
>  		double gain;
> +		double dgain;
>  		bool autoEnabled;
>  	} agc;
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index b0bbcd8c..d66dfdd7 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -446,10 +446,12 @@ void IPARkISP1::setControls(unsigned int frame)
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  	uint32_t exposure = frameContext.agc.exposure;
>  	uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);
> +	uint32_t dgain = camHelper_->gainCode(frameContext.agc.dgain);

I seem to be missing something here. The camHelper doesn't know if we
are requesting digital or analog gain (or combined). This only works if
the gainCode is the same for analog and digital which is not generically
the case. Do we need this patch at the moment? I fear that it might make
things more difficult at the moment. There is no big benefit in digital
gain and things will clear up a bit when the camera helpers where moved
to the correct location.

Best regards,
Stefan
  
>  	ControlList ctrls(sensorControls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> +	ctrls.set(V4L2_CID_DIGITAL_GAIN, static_cast<int32_t>(dgain));
>  
>  	setSensorControls.emit(frame, ctrls);
>  }
> -- 
> 2.39.2
>