| Message ID | 20240405144729.2992219-3-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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); > }
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); > > } >
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 >
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); }
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(+)