[2/5] ipa: rkisp1: agc: Add digital gain
diff mbox series

Message ID 20240405144729.2992219-3-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Improve AGC (plumbing)
Related show

Commit Message

Paul Elder April 5, 2024, 2:47 p.m. UTC
Add support to the rkisp1 AGC to set digital gain.

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

Comments

Umang Jain April 12, 2024, 7:59 a.m. UTC | #1
Hi Paul,

On 05/04/24 8:17 pm, Paul Elder wrote:
> Add support to the rkisp1 AGC to set digital gain.
>
> Signed-off-by: Paul Elder <paul.elder@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>

Is this required? Not sure by reading this particular patch

Looks good to me otherwise. With this addressed,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>   
>   #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);
>   
>   	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);
>   }
Kieran Bingham April 12, 2024, 8:50 a.m. UTC | #2
Quoting Umang Jain (2024-04-12 08:59:01)
> Hi Paul,
> 
> On 05/04/24 8:17 pm, Paul Elder wrote:
> > Add support to the rkisp1 AGC to set digital gain.
> >
> > Signed-off-by: Paul Elder <paul.elder@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>
> 
> Is this required? Not sure by reading this particular patch
> 
> Looks good to me otherwise. With this addressed,
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> >   
> >   #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);
> >   
> >       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));

Not all sensors will have a digital gain, and digital gain models can be
different to analogue gain models so they would require a separate
helper, and can not use gainCode().

I assume we can handle some digital gain on the RKISP1 right? So this
should be being applied through there - not the sensor.

--
Kieran


> >   
> >       setSensorControls.emit(frame, ctrls);
> >   }
>
Stefan Klug April 15, 2024, 1:38 p.m. UTC | #3
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@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
>

Patch
diff mbox series

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);
 
 	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);
 }