| Message ID | 20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Quoting Niklas Söderlund (2025-08-27 23:20:32) > The digital gain is already calculated in the AGC algorithm but the > value is not consumed by the AWB algorithm, which is where the gain > stage is located. Add the needed plumbing to carry the value between the > two algorithms. > > This is needed to get good images from sensors such as the IMX219 where > large sensor frames needs a small VBLANK value. This results in images Do you really mean vblank here? Or is it just that you're hitting a limitation on the exposure time or frame duration limit ? (and thus end up with a small vblank...?) > that are so under exposed that even the sensors analog gain can't > compensate. The digital gain stage of the ISP helps with this. This is interesting - but I think would soon be superceeded by the capabilities we can use in the companding block or such with recent development from Stefan in his WDR series ... See [0] and in particular [1] .... [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 [1] https://patchwork.libcamera.org/patch/24120/ Using AWB to apply a gain here can be problematic because we can saturate a single channel and I think the quantisation of the gain here is quite coarse? But perhaps we'll end up with potentially multiple places we 'could' apply the digital gain ... Kieran > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > src/ipa/rkisp1/ipa_context.h | 3 +++ > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 35440b67e999..17aaa259244b 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > { > /* Configure the default exposure and gain. */ > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > + context.activeState.agc.automatic.ispGain = 1.0; > context.activeState.agc.automatic.exposure = > 10ms / context.configuration.sensor.lineDuration; > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > + context.activeState.agc.manual.ispGain = 1.0; > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > if (!frameContext.agc.autoExposureEnabled) > frameContext.agc.exposure = agc.manual.exposure; > - if (!frameContext.agc.autoGainEnabled) > + if (!frameContext.agc.autoGainEnabled) { > frameContext.agc.gain = agc.manual.gain; > + frameContext.agc.ispGain = agc.manual.ispGain; > + } > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > if (meteringMode) { > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > { > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > double activeAutoGain = context.activeState.agc.automatic.gain; > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > /* Populate exposure and gain in auto mode */ > if (frameContext.agc.autoExposureEnabled) > frameContext.agc.exposure = activeAutoExposure; > - if (frameContext.agc.autoGainEnabled) > + if (frameContext.agc.autoGainEnabled) { > frameContext.agc.gain = activeAutoGain; > + frameContext.agc.ispGain = activeAutoIspGain; > + } > > /* > * Populate manual exposure and gain from the active auto values when > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > /* Update the estimated exposure and gain. */ > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > activeState.agc.automatic.gain = aGain; > + activeState.agc.automatic.ispGain = dGain; > > /* > * Expand the target frame duration so that we do not run faster than > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 399fb51be414..569cac4a3466 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > */ > if (frameContext.awb.autoEnabled) { > const auto &awb = context.activeState.awb; > - frameContext.awb.gains = awb.automatic.gains; > + const auto &agc = context.activeState.agc; > + > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > frameContext.awb.temperatureK = awb.automatic.temperatureK; > } > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 7ccc7b501aff..7180b0b7dff2 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -73,10 +73,12 @@ struct IPAActiveState { > struct { > uint32_t exposure; > double gain; > + double ispGain; > } manual; > struct { > uint32_t exposure; > double gain; > + double ispGain; > } automatic; > > bool autoExposureEnabled; > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > struct { > uint32_t exposure; > double gain; > + double ispGain; > double exposureValue; > uint32_t vblank; > bool autoExposureEnabled; > -- > 2.51.0 >
Hi Kieran, On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote: > Hi Niklas, > > Quoting Niklas Söderlund (2025-08-27 23:20:32) > > The digital gain is already calculated in the AGC algorithm but the > > value is not consumed by the AWB algorithm, which is where the gain > > stage is located. Add the needed plumbing to carry the value between the > > two algorithms. > > > > This is needed to get good images from sensors such as the IMX219 where > > large sensor frames needs a small VBLANK value. This results in images > > Do you really mean vblank here? Or is it just that you're hitting a > limitation on the exposure time or frame duration limit ? (and thus end > up with a small vblank...?) Yes, they are all intertwined in my head ;-) The default FrameLimits renders a VBLANK setting, this VBLANK setting limits the range of the EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is very dark. From a .ko modules point of view the driving factor is the VBLANK setting. > > > that are so under exposed that even the sensors analog gain can't > > compensate. The digital gain stage of the ISP helps with this. > > This is interesting - but I think would soon be superceeded by the > capabilities we can use in the companding block or such with recent > development from Stefan in his WDR series ... I can't tell if it will, or not. > > See [0] and in particular [1] .... > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 > [1] https://patchwork.libcamera.org/patch/24120/ > > Using AWB to apply a gain here can be problematic because we can > saturate a single channel I can set the gain on each channel separately, but I only get one value from libipa. Maybe I'm misunderstanding you, or maybe this is why it will be superseded by Stefan's work? > and I think the quantisation of the gain here > is quite coarse? The digital gain I setting used by this patch comes from libipa at the same time as the exposure and analog gain settings. It's just that the rksip1 IPA have never used the digital gain setting returned. > > But perhaps we'll end up with potentially multiple places we 'could' > apply the digital gain ... I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and it applies DIGITAL_GAIN on the sensor. I have tried this and it works, but I gather using the sensor for DIGITAL_GAIN is not the best idea these days. And I could not find any other block in the ISP where this could be done. > > Kieran > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > > src/ipa/rkisp1/ipa_context.h | 3 +++ > > 3 files changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 35440b67e999..17aaa259244b 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > { > > /* Configure the default exposure and gain. */ > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > + context.activeState.agc.automatic.ispGain = 1.0; > > context.activeState.agc.automatic.exposure = > > 10ms / context.configuration.sensor.lineDuration; > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > > + context.activeState.agc.manual.ispGain = 1.0; > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > > > if (!frameContext.agc.autoExposureEnabled) > > frameContext.agc.exposure = agc.manual.exposure; > > - if (!frameContext.agc.autoGainEnabled) > > + if (!frameContext.agc.autoGainEnabled) { > > frameContext.agc.gain = agc.manual.gain; > > + frameContext.agc.ispGain = agc.manual.ispGain; > > + } > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > if (meteringMode) { > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > { > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > double activeAutoGain = context.activeState.agc.automatic.gain; > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > > > /* Populate exposure and gain in auto mode */ > > if (frameContext.agc.autoExposureEnabled) > > frameContext.agc.exposure = activeAutoExposure; > > - if (frameContext.agc.autoGainEnabled) > > + if (frameContext.agc.autoGainEnabled) { > > frameContext.agc.gain = activeAutoGain; > > + frameContext.agc.ispGain = activeAutoIspGain; > > + } > > > > /* > > * Populate manual exposure and gain from the active auto values when > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > /* Update the estimated exposure and gain. */ > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > activeState.agc.automatic.gain = aGain; > > + activeState.agc.automatic.ispGain = dGain; > > > > /* > > * Expand the target frame duration so that we do not run faster than > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 399fb51be414..569cac4a3466 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > > */ > > if (frameContext.awb.autoEnabled) { > > const auto &awb = context.activeState.awb; > > - frameContext.awb.gains = awb.automatic.gains; > > + const auto &agc = context.activeState.agc; > > + > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > > frameContext.awb.temperatureK = awb.automatic.temperatureK; > > } > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index 7ccc7b501aff..7180b0b7dff2 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -73,10 +73,12 @@ struct IPAActiveState { > > struct { > > uint32_t exposure; > > double gain; > > + double ispGain; > > } manual; > > struct { > > uint32_t exposure; > > double gain; > > + double ispGain; > > } automatic; > > > > bool autoExposureEnabled; > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > > struct { > > uint32_t exposure; > > double gain; > > + double ispGain; > > double exposureValue; > > uint32_t vblank; > > bool autoExposureEnabled; > > -- > > 2.51.0 > >
Quoting Niklas Söderlund (2025-08-27 23:58:56) > Hi Kieran, > > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote: > > Hi Niklas, > > > > Quoting Niklas Söderlund (2025-08-27 23:20:32) > > > The digital gain is already calculated in the AGC algorithm but the > > > value is not consumed by the AWB algorithm, which is where the gain > > > stage is located. Add the needed plumbing to carry the value between the > > > two algorithms. > > > > > > This is needed to get good images from sensors such as the IMX219 where > > > large sensor frames needs a small VBLANK value. This results in images > > > > Do you really mean vblank here? Or is it just that you're hitting a > > limitation on the exposure time or frame duration limit ? (and thus end > > up with a small vblank...?) > > Yes, they are all intertwined in my head ;-) The default FrameLimits > renders a VBLANK setting, this VBLANK setting limits the range of the > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is > very dark. From a .ko modules point of view the driving factor is the > VBLANK setting. > > > > > > that are so under exposed that even the sensors analog gain can't > > > compensate. The digital gain stage of the ISP helps with this. > > > > This is interesting - but I think would soon be superceeded by the > > capabilities we can use in the companding block or such with recent > > development from Stefan in his WDR series ... > > I can't tell if it will, or not. > > > > > See [0] and in particular [1] .... > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 > > [1] https://patchwork.libcamera.org/patch/24120/ > > > > Using AWB to apply a gain here can be problematic because we can > > saturate a single channel > > I can set the gain on each channel separately, but I only get one value > from libipa. Maybe I'm misunderstanding you, or maybe this is why it > will be superseded by Stefan's work? > > > and I think the quantisation of the gain here > > is quite coarse? > > The digital gain I setting used by this patch comes from libipa at the > same time as the exposure and analog gain settings. It's just that the > rksip1 IPA have never used the digital gain setting returned. I mean the available values that can be applied through the AWB gains... > > But perhaps we'll end up with potentially multiple places we 'could' > > apply the digital gain ... > > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and > it applies DIGITAL_GAIN on the sensor. I have tried this and it works, > but I gather using the sensor for DIGITAL_GAIN is not the best idea We haven't used digital gain on sensors yet - but perhaps that is indeed another source that could be utilised. > these days. And I could not find any other block in the ISP where this > could be done. The companding/compress block implemented by the WDR series has a 'fine-grained/floating point' range gain that can be applied. I've already been talking with Stefan to use this to similarly apply digital gain - so I'm just waiting for his series to land ;-) --- Kieran > > > > > Kieran > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > > > src/ipa/rkisp1/ipa_context.h | 3 +++ > > > 3 files changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 35440b67e999..17aaa259244b 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > { > > > /* Configure the default exposure and gain. */ > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > > + context.activeState.agc.automatic.ispGain = 1.0; > > > context.activeState.agc.automatic.exposure = > > > 10ms / context.configuration.sensor.lineDuration; > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > > > + context.activeState.agc.manual.ispGain = 1.0; > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > > > > > if (!frameContext.agc.autoExposureEnabled) > > > frameContext.agc.exposure = agc.manual.exposure; > > > - if (!frameContext.agc.autoGainEnabled) > > > + if (!frameContext.agc.autoGainEnabled) { > > > frameContext.agc.gain = agc.manual.gain; > > > + frameContext.agc.ispGain = agc.manual.ispGain; > > > + } > > > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > > if (meteringMode) { > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > { > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > > double activeAutoGain = context.activeState.agc.automatic.gain; > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > > > > > /* Populate exposure and gain in auto mode */ > > > if (frameContext.agc.autoExposureEnabled) > > > frameContext.agc.exposure = activeAutoExposure; > > > - if (frameContext.agc.autoGainEnabled) > > > + if (frameContext.agc.autoGainEnabled) { > > > frameContext.agc.gain = activeAutoGain; > > > + frameContext.agc.ispGain = activeAutoIspGain; > > > + } > > > > > > /* > > > * Populate manual exposure and gain from the active auto values when > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > /* Update the estimated exposure and gain. */ > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > > activeState.agc.automatic.gain = aGain; > > > + activeState.agc.automatic.ispGain = dGain; > > > > > > /* > > > * Expand the target frame duration so that we do not run faster than > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > index 399fb51be414..569cac4a3466 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > > > */ > > > if (frameContext.awb.autoEnabled) { > > > const auto &awb = context.activeState.awb; > > > - frameContext.awb.gains = awb.automatic.gains; > > > + const auto &agc = context.activeState.agc; > > > + > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > > > frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > } > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index 7ccc7b501aff..7180b0b7dff2 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -73,10 +73,12 @@ struct IPAActiveState { > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double ispGain; > > > } manual; > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double ispGain; > > > } automatic; > > > > > > bool autoExposureEnabled; > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double ispGain; > > > double exposureValue; > > > uint32_t vblank; > > > bool autoExposureEnabled; > > > -- > > > 2.51.0 > > > > > -- > Kind Regards, > Niklas Söderlund
Hi Niklas and Kieran, On Wed, 27 Aug 2025 at 23:59, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > > Hi Kieran, > > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote: > > Hi Niklas, > > > > Quoting Niklas Söderlund (2025-08-27 23:20:32) > > > The digital gain is already calculated in the AGC algorithm but the > > > value is not consumed by the AWB algorithm, which is where the gain > > > stage is located. Add the needed plumbing to carry the value between the > > > two algorithms. > > > > > > This is needed to get good images from sensors such as the IMX219 where > > > large sensor frames needs a small VBLANK value. This results in images > > > > Do you really mean vblank here? Or is it just that you're hitting a > > limitation on the exposure time or frame duration limit ? (and thus end > > up with a small vblank...?) > > Yes, they are all intertwined in my head ;-) The default FrameLimits > renders a VBLANK setting, this VBLANK setting limits the range of the > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is > very dark. From a .ko modules point of view the driving factor is the > VBLANK setting. > > > > > > that are so under exposed that even the sensors analog gain can't > > > compensate. The digital gain stage of the ISP helps with this. > > > > This is interesting - but I think would soon be superceeded by the > > capabilities we can use in the companding block or such with recent > > development from Stefan in his WDR series ... > > I can't tell if it will, or not. > > > > > See [0] and in particular [1] .... > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 > > [1] https://patchwork.libcamera.org/patch/24120/ > > > > Using AWB to apply a gain here can be problematic because we can > > saturate a single channel > > I can set the gain on each channel separately, but I only get one value > from libipa. Maybe I'm misunderstanding you, or maybe this is why it > will be superseded by Stefan's work? > > > and I think the quantisation of the gain here > > is quite coarse? > > The digital gain I setting used by this patch comes from libipa at the > same time as the exposure and analog gain settings. It's just that the > rksip1 IPA have never used the digital gain setting returned. > > > > > But perhaps we'll end up with potentially multiple places we 'could' > > apply the digital gain ... > > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and > it applies DIGITAL_GAIN on the sensor. I have tried this and it works, > but I gather using the sensor for DIGITAL_GAIN is not the best idea > these days. And I could not find any other block in the ISP where this > could be done. Just a quick correction, we don't apply digital gain on the sensor. Instead, we only apply digital gain (if needed) in the ISP pipeline. That said, we don't have any issues with setting up a bright image with a shutter speed/analogue gain combination on the IMX219. With digital gain, we only aim to correct the line length and/or gain code quantization effects, so you would typically only add a few percent gain. I suspect you might be encountering another problem in the IPA perhaps that is limiting your shutter speed, likely based on VBLANK as you mentioned earlier. Naush > > > > > Kieran > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > > > src/ipa/rkisp1/ipa_context.h | 3 +++ > > > 3 files changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 35440b67e999..17aaa259244b 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > { > > > /* Configure the default exposure and gain. */ > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > > + context.activeState.agc.automatic.ispGain = 1.0; > > > context.activeState.agc.automatic.exposure = > > > 10ms / context.configuration.sensor.lineDuration; > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > > > + context.activeState.agc.manual.ispGain = 1.0; > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > > > > > if (!frameContext.agc.autoExposureEnabled) > > > frameContext.agc.exposure = agc.manual.exposure; > > > - if (!frameContext.agc.autoGainEnabled) > > > + if (!frameContext.agc.autoGainEnabled) { > > > frameContext.agc.gain = agc.manual.gain; > > > + frameContext.agc.ispGain = agc.manual.ispGain; > > > + } > > > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > > if (meteringMode) { > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > { > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > > double activeAutoGain = context.activeState.agc.automatic.gain; > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > > > > > /* Populate exposure and gain in auto mode */ > > > if (frameContext.agc.autoExposureEnabled) > > > frameContext.agc.exposure = activeAutoExposure; > > > - if (frameContext.agc.autoGainEnabled) > > > + if (frameContext.agc.autoGainEnabled) { > > > frameContext.agc.gain = activeAutoGain; > > > + frameContext.agc.ispGain = activeAutoIspGain; > > > + } > > > > > > /* > > > * Populate manual exposure and gain from the active auto values when > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > /* Update the estimated exposure and gain. */ > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > > activeState.agc.automatic.gain = aGain; > > > + activeState.agc.automatic.ispGain = dGain; > > > > > > /* > > > * Expand the target frame duration so that we do not run faster than > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > index 399fb51be414..569cac4a3466 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > > > */ > > > if (frameContext.awb.autoEnabled) { > > > const auto &awb = context.activeState.awb; > > > - frameContext.awb.gains = awb.automatic.gains; > > > + const auto &agc = context.activeState.agc; > > > + > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > > > frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > } > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index 7ccc7b501aff..7180b0b7dff2 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -73,10 +73,12 @@ struct IPAActiveState { > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double ispGain; > > > } manual; > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double ispGain; > > > } automatic; > > > > > > bool autoExposureEnabled; > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > > > struct { > > > uint32_t exposure; > > > double gain; > > > + double ispGain; > > > double exposureValue; > > > uint32_t vblank; > > > bool autoExposureEnabled; > > > -- > > > 2.51.0 > > > > > -- > Kind Regards, > Niklas Söderlund
Hi Naushir, Thanks for the clarification. On 2025-08-28 13:12:32 +0100, Naushir Patuck wrote: > Hi Niklas and Kieran, > > On Wed, 27 Aug 2025 at 23:59, Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: > > > > Hi Kieran, > > > > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote: > > > Hi Niklas, > > > > > > Quoting Niklas Söderlund (2025-08-27 23:20:32) > > > > The digital gain is already calculated in the AGC algorithm but the > > > > value is not consumed by the AWB algorithm, which is where the gain > > > > stage is located. Add the needed plumbing to carry the value between the > > > > two algorithms. > > > > > > > > This is needed to get good images from sensors such as the IMX219 where > > > > large sensor frames needs a small VBLANK value. This results in images > > > > > > Do you really mean vblank here? Or is it just that you're hitting a > > > limitation on the exposure time or frame duration limit ? (and thus end > > > up with a small vblank...?) > > > > Yes, they are all intertwined in my head ;-) The default FrameLimits > > renders a VBLANK setting, this VBLANK setting limits the range of the > > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is > > very dark. From a .ko modules point of view the driving factor is the > > VBLANK setting. > > > > > > > > > that are so under exposed that even the sensors analog gain can't > > > > compensate. The digital gain stage of the ISP helps with this. > > > > > > This is interesting - but I think would soon be superceeded by the > > > capabilities we can use in the companding block or such with recent > > > development from Stefan in his WDR series ... > > > > I can't tell if it will, or not. > > > > > > > > See [0] and in particular [1] .... > > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 > > > [1] https://patchwork.libcamera.org/patch/24120/ > > > > > > Using AWB to apply a gain here can be problematic because we can > > > saturate a single channel > > > > I can set the gain on each channel separately, but I only get one value > > from libipa. Maybe I'm misunderstanding you, or maybe this is why it > > will be superseded by Stefan's work? > > > > > and I think the quantisation of the gain here > > > is quite coarse? > > > > The digital gain I setting used by this patch comes from libipa at the > > same time as the exposure and analog gain settings. It's just that the > > rksip1 IPA have never used the digital gain setting returned. > > > > > > > > But perhaps we'll end up with potentially multiple places we 'could' > > > apply the digital gain ... > > > > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and > > it applies DIGITAL_GAIN on the sensor. I have tried this and it works, > > but I gather using the sensor for DIGITAL_GAIN is not the best idea > > these days. And I could not find any other block in the ISP where this > > could be done. > > Just a quick correction, we don't apply digital gain on the sensor. > Instead, we only apply digital gain (if needed) in the ISP pipeline. I was just investigating my issue and found that the RPI/VC4 IPA is the only IPA that sets the V4L2_CID_DIGITAL_GAIN. I might have misunderstood how that is used. > > That said, we don't have any issues with setting up a bright image > with a shutter speed/analogue gain combination on the IMX219. With > digital gain, we only aim to correct the line length and/or gain code > quantization effects, so you would typically only add a few percent > gain. I suspect you might be encountering another problem in the IPA > perhaps that is limiting your shutter speed, likely based on VBLANK as > you mentioned earlier. I think so too. After digging more in this I don't think using digital gain the way I do in this patch is a good way forward. > > Naush > > > > > > > > > > Kieran > > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > > > > src/ipa/rkisp1/ipa_context.h | 3 +++ > > > > 3 files changed, 16 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > index 35440b67e999..17aaa259244b 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > > { > > > > /* Configure the default exposure and gain. */ > > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > > > + context.activeState.agc.automatic.ispGain = 1.0; > > > > context.activeState.agc.automatic.exposure = > > > > 10ms / context.configuration.sensor.lineDuration; > > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > > > > + context.activeState.agc.manual.ispGain = 1.0; > > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > > > > > > > if (!frameContext.agc.autoExposureEnabled) > > > > frameContext.agc.exposure = agc.manual.exposure; > > > > - if (!frameContext.agc.autoGainEnabled) > > > > + if (!frameContext.agc.autoGainEnabled) { > > > > frameContext.agc.gain = agc.manual.gain; > > > > + frameContext.agc.ispGain = agc.manual.ispGain; > > > > + } > > > > > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > > > if (meteringMode) { > > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > > { > > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > > > double activeAutoGain = context.activeState.agc.automatic.gain; > > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > > > > > > > /* Populate exposure and gain in auto mode */ > > > > if (frameContext.agc.autoExposureEnabled) > > > > frameContext.agc.exposure = activeAutoExposure; > > > > - if (frameContext.agc.autoGainEnabled) > > > > + if (frameContext.agc.autoGainEnabled) { > > > > frameContext.agc.gain = activeAutoGain; > > > > + frameContext.agc.ispGain = activeAutoIspGain; > > > > + } > > > > > > > > /* > > > > * Populate manual exposure and gain from the active auto values when > > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > > /* Update the estimated exposure and gain. */ > > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > > > activeState.agc.automatic.gain = aGain; > > > > + activeState.agc.automatic.ispGain = dGain; > > > > > > > > /* > > > > * Expand the target frame duration so that we do not run faster than > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > index 399fb51be414..569cac4a3466 100644 > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > > > > */ > > > > if (frameContext.awb.autoEnabled) { > > > > const auto &awb = context.activeState.awb; > > > > - frameContext.awb.gains = awb.automatic.gains; > > > > + const auto &agc = context.activeState.agc; > > > > + > > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > > > > frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > > } > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > index 7ccc7b501aff..7180b0b7dff2 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -73,10 +73,12 @@ struct IPAActiveState { > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > + double ispGain; > > > > } manual; > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > + double ispGain; > > > > } automatic; > > > > > > > > bool autoExposureEnabled; > > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > + double ispGain; > > > > double exposureValue; > > > > uint32_t vblank; > > > > bool autoExposureEnabled; > > > > -- > > > > 2.51.0 > > > > > > > > -- > > Kind Regards, > > Niklas Söderlund
Hi Kieran, On 2025-08-28 00:23:40 +0100, Kieran Bingham wrote: > Quoting Niklas Söderlund (2025-08-27 23:58:56) > > Hi Kieran, > > > > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote: > > > Hi Niklas, > > > > > > Quoting Niklas Söderlund (2025-08-27 23:20:32) > > > > The digital gain is already calculated in the AGC algorithm but the > > > > value is not consumed by the AWB algorithm, which is where the gain > > > > stage is located. Add the needed plumbing to carry the value between the > > > > two algorithms. > > > > > > > > This is needed to get good images from sensors such as the IMX219 where > > > > large sensor frames needs a small VBLANK value. This results in images > > > > > > Do you really mean vblank here? Or is it just that you're hitting a > > > limitation on the exposure time or frame duration limit ? (and thus end > > > up with a small vblank...?) > > > > Yes, they are all intertwined in my head ;-) The default FrameLimits > > renders a VBLANK setting, this VBLANK setting limits the range of the > > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is > > very dark. From a .ko modules point of view the driving factor is the > > VBLANK setting. > > > > > > > > > that are so under exposed that even the sensors analog gain can't > > > > compensate. The digital gain stage of the ISP helps with this. > > > > > > This is interesting - but I think would soon be superceeded by the > > > capabilities we can use in the companding block or such with recent > > > development from Stefan in his WDR series ... > > > > I can't tell if it will, or not. > > > > > > > > See [0] and in particular [1] .... > > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 > > > [1] https://patchwork.libcamera.org/patch/24120/ > > > > > > Using AWB to apply a gain here can be problematic because we can > > > saturate a single channel > > > > I can set the gain on each channel separately, but I only get one value > > from libipa. Maybe I'm misunderstanding you, or maybe this is why it > > will be superseded by Stefan's work? > > > > > and I think the quantisation of the gain here > > > is quite coarse? > > > > The digital gain I setting used by this patch comes from libipa at the > > same time as the exposure and analog gain settings. It's just that the > > rksip1 IPA have never used the digital gain setting returned. > > I mean the available values that can be applied through the AWB gains... > > > > But perhaps we'll end up with potentially multiple places we 'could' > > > apply the digital gain ... > > > > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and > > it applies DIGITAL_GAIN on the sensor. I have tried this and it works, > > but I gather using the sensor for DIGITAL_GAIN is not the best idea > > We haven't used digital gain on sensors yet - but perhaps that is indeed > another source that could be utilised. It feels a bit dangerous to do so, not all sensors expose digital gain controls. > > > these days. And I could not find any other block in the ISP where this > > could be done. > > The companding/compress block implemented by the WDR series has a > 'fine-grained/floating point' range gain that can be applied. I've > already been talking with Stefan to use this to similarly apply digital > gain - so I'm just waiting for his series to land ;-) Not all rkisp1 variations have this block, the one in R-Car don't so it would not help my use-case. That being said I dug some more in this and I think I found out what I can do differently without touching the rkisp1 IPA. I propose this patch be rejected. > > --- > Kieran > > > > > > > > > Kieran > > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > > > > src/ipa/rkisp1/ipa_context.h | 3 +++ > > > > 3 files changed, 16 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > index 35440b67e999..17aaa259244b 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > > { > > > > /* Configure the default exposure and gain. */ > > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > > > + context.activeState.agc.automatic.ispGain = 1.0; > > > > context.activeState.agc.automatic.exposure = > > > > 10ms / context.configuration.sensor.lineDuration; > > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > > > > + context.activeState.agc.manual.ispGain = 1.0; > > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > > > > > > > if (!frameContext.agc.autoExposureEnabled) > > > > frameContext.agc.exposure = agc.manual.exposure; > > > > - if (!frameContext.agc.autoGainEnabled) > > > > + if (!frameContext.agc.autoGainEnabled) { > > > > frameContext.agc.gain = agc.manual.gain; > > > > + frameContext.agc.ispGain = agc.manual.ispGain; > > > > + } > > > > > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > > > if (meteringMode) { > > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > > { > > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > > > double activeAutoGain = context.activeState.agc.automatic.gain; > > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > > > > > > > /* Populate exposure and gain in auto mode */ > > > > if (frameContext.agc.autoExposureEnabled) > > > > frameContext.agc.exposure = activeAutoExposure; > > > > - if (frameContext.agc.autoGainEnabled) > > > > + if (frameContext.agc.autoGainEnabled) { > > > > frameContext.agc.gain = activeAutoGain; > > > > + frameContext.agc.ispGain = activeAutoIspGain; > > > > + } > > > > > > > > /* > > > > * Populate manual exposure and gain from the active auto values when > > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > > /* Update the estimated exposure and gain. */ > > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > > > activeState.agc.automatic.gain = aGain; > > > > + activeState.agc.automatic.ispGain = dGain; > > > > > > > > /* > > > > * Expand the target frame duration so that we do not run faster than > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > index 399fb51be414..569cac4a3466 100644 > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > > > > */ > > > > if (frameContext.awb.autoEnabled) { > > > > const auto &awb = context.activeState.awb; > > > > - frameContext.awb.gains = awb.automatic.gains; > > > > + const auto &agc = context.activeState.agc; > > > > + > > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > > > > frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > > } > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > index 7ccc7b501aff..7180b0b7dff2 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -73,10 +73,12 @@ struct IPAActiveState { > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > + double ispGain; > > > > } manual; > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > + double ispGain; > > > > } automatic; > > > > > > > > bool autoExposureEnabled; > > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > + double ispGain; > > > > double exposureValue; > > > > uint32_t vblank; > > > > bool autoExposureEnabled; > > > > -- > > > > 2.51.0 > > > > > > > > -- > > Kind Regards, > > Niklas Söderlund
Hi Niklas, On Thu, 28 Aug 2025 at 14:52, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > > Hi Naushir, > > Thanks for the clarification. > > On 2025-08-28 13:12:32 +0100, Naushir Patuck wrote: > > Hi Niklas and Kieran, > > > > On Wed, 27 Aug 2025 at 23:59, Niklas Söderlund > > <niklas.soderlund+renesas@ragnatech.se> wrote: > > > > > > Hi Kieran, > > > > > > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote: > > > > Hi Niklas, > > > > > > > > Quoting Niklas Söderlund (2025-08-27 23:20:32) > > > > > The digital gain is already calculated in the AGC algorithm but the > > > > > value is not consumed by the AWB algorithm, which is where the gain > > > > > stage is located. Add the needed plumbing to carry the value between the > > > > > two algorithms. > > > > > > > > > > This is needed to get good images from sensors such as the IMX219 where > > > > > large sensor frames needs a small VBLANK value. This results in images > > > > > > > > Do you really mean vblank here? Or is it just that you're hitting a > > > > limitation on the exposure time or frame duration limit ? (and thus end > > > > up with a small vblank...?) > > > > > > Yes, they are all intertwined in my head ;-) The default FrameLimits > > > renders a VBLANK setting, this VBLANK setting limits the range of the > > > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is > > > very dark. From a .ko modules point of view the driving factor is the > > > VBLANK setting. > > > > > > > > > > > > that are so under exposed that even the sensors analog gain can't > > > > > compensate. The digital gain stage of the ISP helps with this. > > > > > > > > This is interesting - but I think would soon be superceeded by the > > > > capabilities we can use in the companding block or such with recent > > > > development from Stefan in his WDR series ... > > > > > > I can't tell if it will, or not. > > > > > > > > > > > See [0] and in particular [1] .... > > > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382 > > > > [1] https://patchwork.libcamera.org/patch/24120/ > > > > > > > > Using AWB to apply a gain here can be problematic because we can > > > > saturate a single channel > > > > > > I can set the gain on each channel separately, but I only get one value > > > from libipa. Maybe I'm misunderstanding you, or maybe this is why it > > > will be superseded by Stefan's work? > > > > > > > and I think the quantisation of the gain here > > > > is quite coarse? > > > > > > The digital gain I setting used by this patch comes from libipa at the > > > same time as the exposure and analog gain settings. It's just that the > > > rksip1 IPA have never used the digital gain setting returned. > > > > > > > > > > > But perhaps we'll end up with potentially multiple places we 'could' > > > > apply the digital gain ... > > > > > > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and > > > it applies DIGITAL_GAIN on the sensor. I have tried this and it works, > > > but I gather using the sensor for DIGITAL_GAIN is not the best idea > > > these days. And I could not find any other block in the ISP where this > > > could be done. > > > > Just a quick correction, we don't apply digital gain on the sensor. > > Instead, we only apply digital gain (if needed) in the ISP pipeline. > > I was just investigating my issue and found that the RPI/VC4 IPA is the > only IPA that sets the V4L2_CID_DIGITAL_GAIN. I might have > misunderstood how that is used. Ah, I understand now. Use of V4L2_CID_DIGITAL_GAIN in ipa/vc4.cpp is indeed used to set up the digital gain, but is directed to the ISP driver, not the sensor subdev. This is probably where the confusion comes from. Regards, Naush > > > > > That said, we don't have any issues with setting up a bright image > > with a shutter speed/analogue gain combination on the IMX219. With > > digital gain, we only aim to correct the line length and/or gain code > > quantization effects, so you would typically only add a few percent > > gain. I suspect you might be encountering another problem in the IPA > > perhaps that is limiting your shutter speed, likely based on VBLANK as > > you mentioned earlier. > > I think so too. After digging more in this I don't think using digital > gain the way I do in this patch is a good way forward. > > > > > Naush > > > > > > > > > > > > > > > Kieran > > > > > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- > > > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- > > > > > src/ipa/rkisp1/ipa_context.h | 3 +++ > > > > > 3 files changed, 16 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > index 35440b67e999..17aaa259244b 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > > > > { > > > > > /* Configure the default exposure and gain. */ > > > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; > > > > > + context.activeState.agc.automatic.ispGain = 1.0; > > > > > context.activeState.agc.automatic.exposure = > > > > > 10ms / context.configuration.sensor.lineDuration; > > > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; > > > > > + context.activeState.agc.manual.ispGain = 1.0; > > > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > > > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw; > > > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw; > > > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, > > > > > > > > > > if (!frameContext.agc.autoExposureEnabled) > > > > > frameContext.agc.exposure = agc.manual.exposure; > > > > > - if (!frameContext.agc.autoGainEnabled) > > > > > + if (!frameContext.agc.autoGainEnabled) { > > > > > frameContext.agc.gain = agc.manual.gain; > > > > > + frameContext.agc.ispGain = agc.manual.ispGain; > > > > > + } > > > > > > > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode); > > > > > if (meteringMode) { > > > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > > > > > { > > > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; > > > > > double activeAutoGain = context.activeState.agc.automatic.gain; > > > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; > > > > > > > > > > /* Populate exposure and gain in auto mode */ > > > > > if (frameContext.agc.autoExposureEnabled) > > > > > frameContext.agc.exposure = activeAutoExposure; > > > > > - if (frameContext.agc.autoGainEnabled) > > > > > + if (frameContext.agc.autoGainEnabled) { > > > > > frameContext.agc.gain = activeAutoGain; > > > > > + frameContext.agc.ispGain = activeAutoIspGain; > > > > > + } > > > > > > > > > > /* > > > > > * Populate manual exposure and gain from the active auto values when > > > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > > > > /* Update the estimated exposure and gain. */ > > > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration; > > > > > activeState.agc.automatic.gain = aGain; > > > > > + activeState.agc.automatic.ispGain = dGain; > > > > > > > > > > /* > > > > > * Expand the target frame duration so that we do not run faster than > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > > index 399fb51be414..569cac4a3466 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > > > > > */ > > > > > if (frameContext.awb.autoEnabled) { > > > > > const auto &awb = context.activeState.awb; > > > > > - frameContext.awb.gains = awb.automatic.gains; > > > > > + const auto &agc = context.activeState.agc; > > > > > + > > > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; > > > > > frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > > > } > > > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > > index 7ccc7b501aff..7180b0b7dff2 100644 > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > @@ -73,10 +73,12 @@ struct IPAActiveState { > > > > > struct { > > > > > uint32_t exposure; > > > > > double gain; > > > > > + double ispGain; > > > > > } manual; > > > > > struct { > > > > > uint32_t exposure; > > > > > double gain; > > > > > + double ispGain; > > > > > } automatic; > > > > > > > > > > bool autoExposureEnabled; > > > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { > > > > > struct { > > > > > uint32_t exposure; > > > > > double gain; > > > > > + double ispGain; > > > > > double exposureValue; > > > > > uint32_t vblank; > > > > > bool autoExposureEnabled; > > > > > -- > > > > > 2.51.0 > > > > > > > > > > > -- > > > Kind Regards, > > > Niklas Söderlund > > -- > Kind Regards, > Niklas Söderlund
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 35440b67e999..17aaa259244b 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) { /* Configure the default exposure and gain. */ context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain; + context.activeState.agc.automatic.ispGain = 1.0; context.activeState.agc.automatic.exposure = 10ms / context.configuration.sensor.lineDuration; context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain; + context.activeState.agc.manual.ispGain = 1.0; context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; context.activeState.agc.autoExposureEnabled = !context.configuration.raw; context.activeState.agc.autoGainEnabled = !context.configuration.raw; @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context, if (!frameContext.agc.autoExposureEnabled) frameContext.agc.exposure = agc.manual.exposure; - if (!frameContext.agc.autoGainEnabled) + if (!frameContext.agc.autoGainEnabled) { frameContext.agc.gain = agc.manual.gain; + frameContext.agc.ispGain = agc.manual.ispGain; + } const auto &meteringMode = controls.get(controls::AeMeteringMode); if (meteringMode) { @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, { uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure; double activeAutoGain = context.activeState.agc.automatic.gain; + double activeAutoIspGain = context.activeState.agc.automatic.ispGain; /* Populate exposure and gain in auto mode */ if (frameContext.agc.autoExposureEnabled) frameContext.agc.exposure = activeAutoExposure; - if (frameContext.agc.autoGainEnabled) + if (frameContext.agc.autoGainEnabled) { frameContext.agc.gain = activeAutoGain; + frameContext.agc.ispGain = activeAutoIspGain; + } /* * Populate manual exposure and gain from the active auto values when @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, /* Update the estimated exposure and gain. */ activeState.agc.automatic.exposure = newExposureTime / lineDuration; activeState.agc.automatic.gain = aGain; + activeState.agc.automatic.ispGain = dGain; /* * Expand the target frame duration so that we do not run faster than diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 399fb51be414..569cac4a3466 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, */ if (frameContext.awb.autoEnabled) { const auto &awb = context.activeState.awb; - frameContext.awb.gains = awb.automatic.gains; + const auto &agc = context.activeState.agc; + + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain; frameContext.awb.temperatureK = awb.automatic.temperatureK; } diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 7ccc7b501aff..7180b0b7dff2 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -73,10 +73,12 @@ struct IPAActiveState { struct { uint32_t exposure; double gain; + double ispGain; } manual; struct { uint32_t exposure; double gain; + double ispGain; } automatic; bool autoExposureEnabled; @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext { struct { uint32_t exposure; double gain; + double ispGain; double exposureValue; uint32_t vblank; bool autoExposureEnabled;
The digital gain is already calculated in the AGC algorithm but the value is not consumed by the AWB algorithm, which is where the gain stage is located. Add the needed plumbing to carry the value between the two algorithms. This is needed to get good images from sensors such as the IMX219 where large sensor frames needs a small VBLANK value. This results in images that are so under exposed that even the sensors analog gain can't compensate. The digital gain stage of the ISP helps with this. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++-- src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- src/ipa/rkisp1/ipa_context.h | 3 +++ 3 files changed, 16 insertions(+), 3 deletions(-)