| Message ID | 20251017-exposure-limits-v1-1-6288cd86e719@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. Quoting Jacopo Mondi (2025-10-17 11:00:05) > The AGC algorithm implementation of the RkISP1 IPA is implemented > deriving the generic AgcMeanLuminance class, which limits the > achievable exposure time using a maxExposureTime parameter provided by > the IPA module. > > The RkISP1 IPA fetches the maxExposureTime value from the IPAContext > sensor.maxExposureTime value, which is initialized at IPA init() > and configure() time, but never updated later on, effectively limiting > the achievable exposure time to the frame duration programmed at > startup time. > > Whenever the frame duration is changed, the maximum exposure time > should change accordingly, but in the current implementation this > doesn't happen. > > Fix this by updating the sensor.maxExposureTime value when a new set of > controls is sent to the pipeline handler. Store the current VBLANK value > in the IPAContext by replacing 'defVBlank' which is never used and use > it to detect changes in the blanking value. Whenever the blanking > changes, update the maximum exposure limit accordingly. > > As reported in a comment in the code, even if the new sensor controls > will be programmed in the sensor with a delay of a few frames on the > pipeline handler side, the limits for the algorithms should be updated > immediately. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/rkisp1/ipa_context.h | 2 +- > src/ipa/rkisp1/rkisp1.cpp | 31 ++++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index f85a130d9c23dba7987f388e395239e4b141d776..af66a749052bc82bbbe7fbb0c4626f2422700926 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -65,7 +65,7 @@ struct IPASessionConfiguration { > double minAnalogueGain; > double maxAnalogueGain; > > - int32_t defVBlank; > + unsigned int vBlank; Why the change to unsigned? The V4L2 control is also int32_t. Now that this becomes a dynamic value should it be moved to active state? Same would then also apply to maxExposureTime. > utils::Duration lineDuration; > Size size; > } sensor; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index fa22bfc349043002345d275b11a60ac983e329d7..54bd1434e0f4e34834beb1f9e9c39b77590f8b34 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -250,7 +250,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > - context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); > + context_.configuration.sensor.vBlank = vBlank.def().get<int32_t>(); > context_.configuration.sensor.size = info.outputSize; > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate; > > @@ -261,8 +261,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > * When the AGC computes the new exposure values for a frame, it needs > * to know the limits for exposure time and analogue gain. As it depends > * on the sensor, update it with the controls. > - * > - * \todo take VBLANK into account for maximum exposure time > */ > context_.configuration.sensor.minExposureTime = > minExposure * context_.configuration.sensor.lineDuration; > @@ -457,6 +455,33 @@ void IPARkISP1::setControls(unsigned int frame) > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > uint32_t vblank = frameContext.agc.vblank; > > + /* > + * Update the exposure limits if vblank has changed. Even if the controls > + * will actually be applied to the sensor with some frames of latency > + * by DelayedControls, all the algorithms calculations from now on should > + * use the new limits. > + * > + * \todo Sensors usually have a margin that limits the max exposure to > + * be shorter by the full frame length: > + * > + * (max_exposure_lines = height + vblank - margin) > + * > + * As the margin is a sensor-specific parameter either: > + * - Ignore the margin and rely on the driver clamping the exposure > + * value correctly > + * - Defer to the sensor helpers by creating an exposure() function that > + * subtract the margin from the frame length > + * > + * For the time being ignore the margins and rely on the driver doing > + * the adjustment. This could actually be a problem. In the reworked regulation we reached a high level of precision. Even digital gain is used to mitigate quantization in exposure/gain and the AEGC needs to precisely predict which exposure time will be applied. So in this case we will see oscillations at the point where AEGC is maxing out exposure time and then switches to the next gain value. What about deducing this margin from the initial set of sensor config (defVBlank, maxDuration and lineDuration)? Possibly inside the sensor helper so it can be overwritten if the margin is more than a constant. > + */ > + if (vblank != context_.configuration.sensor.vBlank) { What is the idea behind this if()? I think you could just set the new maximum unconditionally. Best regards, Stefan > + context_.configuration.sensor.vBlank = vblank; > + context_.configuration.sensor.maxExposureTime = > + (vblank + context_.configuration.sensor.size.height) * > + context_.configuration.sensor.lineDuration; > + } > + > LOG(IPARkISP1, Debug) > << "Set controls for frame " << frame << ": exposure " << exposure > << ", gain " << frameContext.agc.gain << ", vblank " << vblank; > > -- > 2.51.0 >
Hi Stefan On Mon, Oct 20, 2025 at 02:11:05PM +0200, Stefan Klug wrote: > Hi Jacopo, > > Thank you for the patch. > > Quoting Jacopo Mondi (2025-10-17 11:00:05) > > The AGC algorithm implementation of the RkISP1 IPA is implemented > > deriving the generic AgcMeanLuminance class, which limits the > > achievable exposure time using a maxExposureTime parameter provided by > > the IPA module. > > > > The RkISP1 IPA fetches the maxExposureTime value from the IPAContext > > sensor.maxExposureTime value, which is initialized at IPA init() > > and configure() time, but never updated later on, effectively limiting > > the achievable exposure time to the frame duration programmed at > > startup time. > > > > Whenever the frame duration is changed, the maximum exposure time > > should change accordingly, but in the current implementation this > > doesn't happen. > > > > Fix this by updating the sensor.maxExposureTime value when a new set of > > controls is sent to the pipeline handler. Store the current VBLANK value > > in the IPAContext by replacing 'defVBlank' which is never used and use > > it to detect changes in the blanking value. Whenever the blanking > > changes, update the maximum exposure limit accordingly. > > > > As reported in a comment in the code, even if the new sensor controls > > will be programmed in the sensor with a delay of a few frames on the > > pipeline handler side, the limits for the algorithms should be updated > > immediately. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/ipa/rkisp1/ipa_context.h | 2 +- > > src/ipa/rkisp1/rkisp1.cpp | 31 ++++++++++++++++++++++++++++--- > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index f85a130d9c23dba7987f388e395239e4b141d776..af66a749052bc82bbbe7fbb0c4626f2422700926 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -65,7 +65,7 @@ struct IPASessionConfiguration { > > double minAnalogueGain; > > double maxAnalogueGain; > > > > - int32_t defVBlank; > > + unsigned int vBlank; > > Why the change to unsigned? The V4L2 control is also int32_t. Because I assumed vblank cannot be negative ? v4l2-ctrl uses signed integers, but here I think it's safe to implicit cast it to unsigned ? > > Now that this becomes a dynamic value should it be moved to active > state? Same would then also apply to maxExposureTime. mmm, good question. AS they change dynamically I agree they could fit into the active state. At least for vblank I think it is fine to move it to the ActiveState. maxExposureTime is already part of the session configuration along with minExposureTime and the analogue gain limits. Do you think they should be moved too ? > > > utils::Duration lineDuration; > > Size size; > > } sensor; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index fa22bfc349043002345d275b11a60ac983e329d7..54bd1434e0f4e34834beb1f9e9c39b77590f8b34 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -250,7 +250,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > > - context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); > > + context_.configuration.sensor.vBlank = vBlank.def().get<int32_t>(); > > context_.configuration.sensor.size = info.outputSize; > > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate; > > > > @@ -261,8 +261,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > * When the AGC computes the new exposure values for a frame, it needs > > * to know the limits for exposure time and analogue gain. As it depends > > * on the sensor, update it with the controls. > > - * > > - * \todo take VBLANK into account for maximum exposure time > > */ > > context_.configuration.sensor.minExposureTime = > > minExposure * context_.configuration.sensor.lineDuration; > > @@ -457,6 +455,33 @@ void IPARkISP1::setControls(unsigned int frame) > > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > uint32_t vblank = frameContext.agc.vblank; > > > > + /* > > + * Update the exposure limits if vblank has changed. Even if the controls > > + * will actually be applied to the sensor with some frames of latency > > + * by DelayedControls, all the algorithms calculations from now on should > > + * use the new limits. > > + * > > + * \todo Sensors usually have a margin that limits the max exposure to > > + * be shorter by the full frame length: > > + * > > + * (max_exposure_lines = height + vblank - margin) > > + * > > + * As the margin is a sensor-specific parameter either: > > + * - Ignore the margin and rely on the driver clamping the exposure > > + * value correctly > > + * - Defer to the sensor helpers by creating an exposure() function that > > + * subtract the margin from the frame length > > + * > > + * For the time being ignore the margins and rely on the driver doing > > + * the adjustment. > > This could actually be a problem. In the reworked regulation we reached > a high level of precision. Even digital gain is used to mitigate > quantization in exposure/gain and the AEGC needs to precisely predict > which exposure time will be applied. So in this case we will see > oscillations at the point where AEGC is maxing out exposure time and > then switches to the next gain value. > > What about deducing this margin from the initial set of sensor config > (defVBlank, maxDuration and lineDuration)? Possibly inside the sensor I don't think we can use defVBlank as there is no guarantee that it will be initialized to the maximum possible value by the driver. > helper so it can be overwritten if the margin is more than a constant. > What do you mean with "more than a constant" ? If we want to take margin into account (we're talking about 4 lines or something here usually) I think the best would be to add a function to CameraSensorHelper where the per-sensor margin will be subtracted from the calculated max_exposure. > > + */ > > + if (vblank != context_.configuration.sensor.vBlank) { > > What is the idea behind this if()? I think you could just set the new > maximum unconditionally. Why should I if it didn't change ? Thanks j > > Best regards, > Stefan > > > + context_.configuration.sensor.vBlank = vblank; > > + context_.configuration.sensor.maxExposureTime = > > + (vblank + context_.configuration.sensor.size.height) * > > + context_.configuration.sensor.lineDuration; > > + } > > + > > LOG(IPARkISP1, Debug) > > << "Set controls for frame " << frame << ": exposure " << exposure > > << ", gain " << frameContext.agc.gain << ", vblank " << vblank; > > > > -- > > 2.51.0 > >
Hi Jacopo, Quoting Jacopo Mondi (2025-10-21 08:47:25) > Hi Stefan > > On Mon, Oct 20, 2025 at 02:11:05PM +0200, Stefan Klug wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > Quoting Jacopo Mondi (2025-10-17 11:00:05) > > > The AGC algorithm implementation of the RkISP1 IPA is implemented > > > deriving the generic AgcMeanLuminance class, which limits the > > > achievable exposure time using a maxExposureTime parameter provided by > > > the IPA module. > > > > > > The RkISP1 IPA fetches the maxExposureTime value from the IPAContext > > > sensor.maxExposureTime value, which is initialized at IPA init() > > > and configure() time, but never updated later on, effectively limiting > > > the achievable exposure time to the frame duration programmed at > > > startup time. > > > > > > Whenever the frame duration is changed, the maximum exposure time > > > should change accordingly, but in the current implementation this > > > doesn't happen. > > > > > > Fix this by updating the sensor.maxExposureTime value when a new set of > > > controls is sent to the pipeline handler. Store the current VBLANK value > > > in the IPAContext by replacing 'defVBlank' which is never used and use > > > it to detect changes in the blanking value. Whenever the blanking > > > changes, update the maximum exposure limit accordingly. > > > > > > As reported in a comment in the code, even if the new sensor controls > > > will be programmed in the sensor with a delay of a few frames on the > > > pipeline handler side, the limits for the algorithms should be updated > > > immediately. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/ipa_context.h | 2 +- > > > src/ipa/rkisp1/rkisp1.cpp | 31 ++++++++++++++++++++++++++++--- > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index f85a130d9c23dba7987f388e395239e4b141d776..af66a749052bc82bbbe7fbb0c4626f2422700926 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -65,7 +65,7 @@ struct IPASessionConfiguration { > > > double minAnalogueGain; > > > double maxAnalogueGain; > > > > > > - int32_t defVBlank; > > > + unsigned int vBlank; > > > > Why the change to unsigned? The V4L2 control is also int32_t. > > Because I assumed vblank cannot be negative ? v4l2-ctrl uses signed > integers, but here I think it's safe to implicit cast it to unsigned ? True, they shouldn't be negative assuming the drivers are bugfree. Maybe that is personal preference. In my experience unsigned ints create more harm and hidden bugs than they improve things, so I prefer signed ints when possible. But that is just an opinion. So either way is fine. > > > > > Now that this becomes a dynamic value should it be moved to active > > state? Same would then also apply to maxExposureTime. > > mmm, good question. AS they change dynamically I agree they could fit > into the active state. At least for vblank I think it is fine to move > it to the ActiveState. > > maxExposureTime is already part of the session configuration along > with minExposureTime and the analogue gain limits. Do you think they > should be moved too ? Yes, I think as soon as things change during the session they should be moved out of the session config. Gain limits do not change, so they can stay. > > > > > > utils::Duration lineDuration; > > > Size size; > > > } sensor; > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index fa22bfc349043002345d275b11a60ac983e329d7..54bd1434e0f4e34834beb1f9e9c39b77590f8b34 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -250,7 +250,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > > > > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > > > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > > > - context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); > > > + context_.configuration.sensor.vBlank = vBlank.def().get<int32_t>(); > > > context_.configuration.sensor.size = info.outputSize; > > > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate; > > > > > > @@ -261,8 +261,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > * When the AGC computes the new exposure values for a frame, it needs > > > * to know the limits for exposure time and analogue gain. As it depends > > > * on the sensor, update it with the controls. > > > - * > > > - * \todo take VBLANK into account for maximum exposure time > > > */ > > > context_.configuration.sensor.minExposureTime = > > > minExposure * context_.configuration.sensor.lineDuration; > > > @@ -457,6 +455,33 @@ void IPARkISP1::setControls(unsigned int frame) > > > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > > uint32_t vblank = frameContext.agc.vblank; > > > > > > + /* > > > + * Update the exposure limits if vblank has changed. Even if the controls > > > + * will actually be applied to the sensor with some frames of latency > > > + * by DelayedControls, all the algorithms calculations from now on should > > > + * use the new limits. > > > + * > > > + * \todo Sensors usually have a margin that limits the max exposure to > > > + * be shorter by the full frame length: > > > + * > > > + * (max_exposure_lines = height + vblank - margin) > > > + * > > > + * As the margin is a sensor-specific parameter either: > > > + * - Ignore the margin and rely on the driver clamping the exposure > > > + * value correctly > > > + * - Defer to the sensor helpers by creating an exposure() function that > > > + * subtract the margin from the frame length > > > + * > > > + * For the time being ignore the margins and rely on the driver doing > > > + * the adjustment. > > > > This could actually be a problem. In the reworked regulation we reached > > a high level of precision. Even digital gain is used to mitigate > > quantization in exposure/gain and the AEGC needs to precisely predict > > which exposure time will be applied. So in this case we will see > > oscillations at the point where AEGC is maxing out exposure time and > > then switches to the next gain value. > > > > What about deducing this margin from the initial set of sensor config > > (defVBlank, maxDuration and lineDuration)? Possibly inside the sensor > > I don't think we can use defVBlank as there is no guarantee that it > will be initialized to the maximum possible value by the driver. No, but I assume that defVBlank and maxExposureTime correspond to each other. So margin = frameHeight + defVBlank - maxExposureTime We have access to these values in IPARkISP1::updateControls(). > > > helper so it can be overwritten if the margin is more than a constant. > > > > What do you mean with "more than a constant" ? Maybe I meant "not just a constant", meaning something that changes depending on external factors like the frame height. > > If we want to take margin into account (we're talking about 4 lines or > something here usually) I think the best would be to add a function to > CameraSensorHelper where the per-sensor margin will be subtracted from > the calculated max_exposure. Yes, I introduced the digital gain for WDR because the jump between two lines was too big for a stable image. I agree it is not visible in most normal cases, but then there is this specific light condition where it suddenly starts to oscillate. Moving that to the CameraSensorHelper makes sense. > > > > + */ > > > + if (vblank != context_.configuration.sensor.vBlank) { > > > > What is the idea behind this if()? I think you could just set the new > > maximum unconditionally. > > Why should I if it didn't change ? I don't expect it to help a lot in terms of cpu usage. If the lower FrameDurationLimit is not set, vBlank will change on every frame anyways. So I'd go for the simpler code. Best regards, Stefan > > Thanks > j > > > > > Best regards, > > Stefan > > > > > + context_.configuration.sensor.vBlank = vblank; > > > + context_.configuration.sensor.maxExposureTime = > > > + (vblank + context_.configuration.sensor.size.height) * > > > + context_.configuration.sensor.lineDuration; > > > + } > > > + > > > LOG(IPARkISP1, Debug) > > > << "Set controls for frame " << frame << ": exposure " << exposure > > > << ", gain " << frameContext.agc.gain << ", vblank " << vblank; > > > > > > -- > > > 2.51.0 > > >
Hi Stefan On Tue, Oct 21, 2025 at 09:50:50AM +0200, Stefan Klug wrote: > Hi Jacopo, > > Quoting Jacopo Mondi (2025-10-21 08:47:25) > > Hi Stefan > > > > On Mon, Oct 20, 2025 at 02:11:05PM +0200, Stefan Klug wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > Quoting Jacopo Mondi (2025-10-17 11:00:05) > > > > The AGC algorithm implementation of the RkISP1 IPA is implemented > > > > deriving the generic AgcMeanLuminance class, which limits the > > > > achievable exposure time using a maxExposureTime parameter provided by > > > > the IPA module. > > > > > > > > The RkISP1 IPA fetches the maxExposureTime value from the IPAContext > > > > sensor.maxExposureTime value, which is initialized at IPA init() > > > > and configure() time, but never updated later on, effectively limiting > > > > the achievable exposure time to the frame duration programmed at > > > > startup time. > > > > > > > > Whenever the frame duration is changed, the maximum exposure time > > > > should change accordingly, but in the current implementation this > > > > doesn't happen. > > > > > > > > Fix this by updating the sensor.maxExposureTime value when a new set of > > > > controls is sent to the pipeline handler. Store the current VBLANK value > > > > in the IPAContext by replacing 'defVBlank' which is never used and use > > > > it to detect changes in the blanking value. Whenever the blanking > > > > changes, update the maximum exposure limit accordingly. > > > > > > > > As reported in a comment in the code, even if the new sensor controls > > > > will be programmed in the sensor with a delay of a few frames on the > > > > pipeline handler side, the limits for the algorithms should be updated > > > > immediately. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/ipa_context.h | 2 +- > > > > src/ipa/rkisp1/rkisp1.cpp | 31 ++++++++++++++++++++++++++++--- > > > > 2 files changed, 29 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > index f85a130d9c23dba7987f388e395239e4b141d776..af66a749052bc82bbbe7fbb0c4626f2422700926 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -65,7 +65,7 @@ struct IPASessionConfiguration { > > > > double minAnalogueGain; > > > > double maxAnalogueGain; > > > > > > > > - int32_t defVBlank; > > > > + unsigned int vBlank; > > > > > > Why the change to unsigned? The V4L2 control is also int32_t. > > > > Because I assumed vblank cannot be negative ? v4l2-ctrl uses signed > > integers, but here I think it's safe to implicit cast it to unsigned ? > > True, they shouldn't be negative assuming the drivers are bugfree. Maybe > that is personal preference. In my experience unsigned ints create more > harm and hidden bugs than they improve things, so I prefer signed ints > when possible. But that is just an opinion. So either way is fine. > Ack, I'll use int. > > > > > > > > Now that this becomes a dynamic value should it be moved to active > > > state? Same would then also apply to maxExposureTime. > > > > mmm, good question. AS they change dynamically I agree they could fit > > into the active state. At least for vblank I think it is fine to move > > it to the ActiveState. > > > > maxExposureTime is already part of the session configuration along > > with minExposureTime and the analogue gain limits. Do you think they > > should be moved too ? > > Yes, I think as soon as things change during the session they should be > moved out of the session config. Gain limits do not change, so they can > stay. > Ack, I'll move to the active state. > > > > > > > > > utils::Duration lineDuration; > > > > Size size; > > > > } sensor; > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index fa22bfc349043002345d275b11a60ac983e329d7..54bd1434e0f4e34834beb1f9e9c39b77590f8b34 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -250,7 +250,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > > > > > > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > > > > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > > > > - context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); > > > > + context_.configuration.sensor.vBlank = vBlank.def().get<int32_t>(); > > > > context_.configuration.sensor.size = info.outputSize; > > > > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate; > > > > > > > > @@ -261,8 +261,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > > * When the AGC computes the new exposure values for a frame, it needs > > > > * to know the limits for exposure time and analogue gain. As it depends > > > > * on the sensor, update it with the controls. > > > > - * > > > > - * \todo take VBLANK into account for maximum exposure time > > > > */ > > > > context_.configuration.sensor.minExposureTime = > > > > minExposure * context_.configuration.sensor.lineDuration; > > > > @@ -457,6 +455,33 @@ void IPARkISP1::setControls(unsigned int frame) > > > > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > > > uint32_t vblank = frameContext.agc.vblank; > > > > > > > > + /* > > > > + * Update the exposure limits if vblank has changed. Even if the controls > > > > + * will actually be applied to the sensor with some frames of latency > > > > + * by DelayedControls, all the algorithms calculations from now on should > > > > + * use the new limits. > > > > + * > > > > + * \todo Sensors usually have a margin that limits the max exposure to > > > > + * be shorter by the full frame length: > > > > + * > > > > + * (max_exposure_lines = height + vblank - margin) > > > > + * > > > > + * As the margin is a sensor-specific parameter either: > > > > + * - Ignore the margin and rely on the driver clamping the exposure > > > > + * value correctly > > > > + * - Defer to the sensor helpers by creating an exposure() function that > > > > + * subtract the margin from the frame length > > > > + * > > > > + * For the time being ignore the margins and rely on the driver doing > > > > + * the adjustment. > > > > > > This could actually be a problem. In the reworked regulation we reached > > > a high level of precision. Even digital gain is used to mitigate > > > quantization in exposure/gain and the AEGC needs to precisely predict > > > which exposure time will be applied. So in this case we will see > > > oscillations at the point where AEGC is maxing out exposure time and > > > then switches to the next gain value. > > > > > > What about deducing this margin from the initial set of sensor config > > > (defVBlank, maxDuration and lineDuration)? Possibly inside the sensor > > > > I don't think we can use defVBlank as there is no guarantee that it > > will be initialized to the maximum possible value by the driver. > > No, but I assume that defVBlank and maxExposureTime correspond to each > other. So > > margin = frameHeight + defVBlank - maxExposureTime > > We have access to these values in IPARkISP1::updateControls(). > mmm, you're right here. I could check if the calculated margin matches the driver's defined one and see if I can use this method. It's certainly easier than having to instrument one more function for each CameraSensorHelper instance just to do a subtraction > > > > > helper so it can be overwritten if the margin is more than a constant. > > > > > > > What do you mean with "more than a constant" ? > > Maybe I meant "not just a constant", meaning something that changes > depending on external factors like the frame height. > As far as I know, the margin is a constant > > > > If we want to take margin into account (we're talking about 4 lines or > > something here usually) I think the best would be to add a function to > > CameraSensorHelper where the per-sensor margin will be subtracted from > > the calculated max_exposure. > > Yes, I introduced the digital gain for WDR because the jump between two > lines was too big for a stable image. I agree it is not visible in most > normal cases, but then there is this specific light condition where it > suddenly starts to oscillate. Moving that to the CameraSensorHelper > makes sense. > > > > > > > + */ > > > > + if (vblank != context_.configuration.sensor.vBlank) { > > > > > > What is the idea behind this if()? I think you could just set the new > > > maximum unconditionally. > > > > Why should I if it didn't change ? > > I don't expect it to help a lot in terms of cpu usage. If the lower > FrameDurationLimit is not set, vBlank will change on every frame > anyways. So I'd go for the simpler code. Does it ? To me, it's still worth avoiding extra calculations if all it takes is a if (vblank != context_.configuration.sensor.vBlank) > > Best regards, > Stefan > > > > > Thanks > > j > > > > > > > > Best regards, > > > Stefan > > > > > > > + context_.configuration.sensor.vBlank = vblank; > > > > + context_.configuration.sensor.maxExposureTime = > > > > + (vblank + context_.configuration.sensor.size.height) * > > > > + context_.configuration.sensor.lineDuration; > > > > + } > > > > + > > > > LOG(IPARkISP1, Debug) > > > > << "Set controls for frame " << frame << ": exposure " << exposure > > > > << ", gain " << frameContext.agc.gain << ", vblank " << vblank; > > > > > > > > -- > > > > 2.51.0 > > > >
I tested this patch on a PinePhone Pro and it fixes (way too) low frame rates for me - very glad to see, thanks a lot! Tested-by: Robert Mader<robert.mader@collabora.com> On 10/17/25 11:00, Jacopo Mondi wrote: > The AGC algorithm implementation of the RkISP1 IPA is implemented > deriving the generic AgcMeanLuminance class, which limits the > achievable exposure time using a maxExposureTime parameter provided by > the IPA module. > > The RkISP1 IPA fetches the maxExposureTime value from the IPAContext > sensor.maxExposureTime value, which is initialized at IPA init() > and configure() time, but never updated later on, effectively limiting > the achievable exposure time to the frame duration programmed at > startup time. > > Whenever the frame duration is changed, the maximum exposure time > should change accordingly, but in the current implementation this > doesn't happen. > > Fix this by updating the sensor.maxExposureTime value when a new set of > controls is sent to the pipeline handler. Store the current VBLANK value > in the IPAContext by replacing 'defVBlank' which is never used and use > it to detect changes in the blanking value. Whenever the blanking > changes, update the maximum exposure limit accordingly. > > As reported in a comment in the code, even if the new sensor controls > will be programmed in the sensor with a delay of a few frames on the > pipeline handler side, the limits for the algorithms should be updated > immediately. > > Signed-off-by: Jacopo Mondi<jacopo.mondi@ideasonboard.com> > --- > src/ipa/rkisp1/ipa_context.h | 2 +- > src/ipa/rkisp1/rkisp1.cpp | 31 ++++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index f85a130d9c23dba7987f388e395239e4b141d776..af66a749052bc82bbbe7fbb0c4626f2422700926 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -65,7 +65,7 @@ struct IPASessionConfiguration { > double minAnalogueGain; > double maxAnalogueGain; > > - int32_t defVBlank; > + unsigned int vBlank; > utils::Duration lineDuration; > Size size; > } sensor; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index fa22bfc349043002345d275b11a60ac983e329d7..54bd1434e0f4e34834beb1f9e9c39b77590f8b34 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -250,7 +250,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > - context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); > + context_.configuration.sensor.vBlank = vBlank.def().get<int32_t>(); > context_.configuration.sensor.size = info.outputSize; > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate; > > @@ -261,8 +261,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > * When the AGC computes the new exposure values for a frame, it needs > * to know the limits for exposure time and analogue gain. As it depends > * on the sensor, update it with the controls. > - * > - * \todo take VBLANK into account for maximum exposure time > */ > context_.configuration.sensor.minExposureTime = > minExposure * context_.configuration.sensor.lineDuration; > @@ -457,6 +455,33 @@ void IPARkISP1::setControls(unsigned int frame) > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > uint32_t vblank = frameContext.agc.vblank; > > + /* > + * Update the exposure limits if vblank has changed. Even if the controls > + * will actually be applied to the sensor with some frames of latency > + * by DelayedControls, all the algorithms calculations from now on should > + * use the new limits. > + * > + * \todo Sensors usually have a margin that limits the max exposure to > + * be shorter by the full frame length: > + * > + * (max_exposure_lines = height + vblank - margin) > + * > + * As the margin is a sensor-specific parameter either: > + * - Ignore the margin and rely on the driver clamping the exposure > + * value correctly > + * - Defer to the sensor helpers by creating an exposure() function that > + * subtract the margin from the frame length > + * > + * For the time being ignore the margins and rely on the driver doing > + * the adjustment. > + */ > + if (vblank != context_.configuration.sensor.vBlank) { > + context_.configuration.sensor.vBlank = vblank; > + context_.configuration.sensor.maxExposureTime = > + (vblank + context_.configuration.sensor.size.height) * > + context_.configuration.sensor.lineDuration; > + } > + > LOG(IPARkISP1, Debug) > << "Set controls for frame " << frame << ": exposure " << exposure > << ", gain " << frameContext.agc.gain << ", vblank " << vblank; >
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index f85a130d9c23dba7987f388e395239e4b141d776..af66a749052bc82bbbe7fbb0c4626f2422700926 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -65,7 +65,7 @@ struct IPASessionConfiguration { double minAnalogueGain; double maxAnalogueGain; - int32_t defVBlank; + unsigned int vBlank; utils::Duration lineDuration; Size size; } sensor; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index fa22bfc349043002345d275b11a60ac983e329d7..54bd1434e0f4e34834beb1f9e9c39b77590f8b34 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -250,7 +250,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, const IPACameraSensorInfo &info = ipaConfig.sensorInfo; const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; - context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); + context_.configuration.sensor.vBlank = vBlank.def().get<int32_t>(); context_.configuration.sensor.size = info.outputSize; context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate; @@ -261,8 +261,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, * When the AGC computes the new exposure values for a frame, it needs * to know the limits for exposure time and analogue gain. As it depends * on the sensor, update it with the controls. - * - * \todo take VBLANK into account for maximum exposure time */ context_.configuration.sensor.minExposureTime = minExposure * context_.configuration.sensor.lineDuration; @@ -457,6 +455,33 @@ void IPARkISP1::setControls(unsigned int frame) uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); uint32_t vblank = frameContext.agc.vblank; + /* + * Update the exposure limits if vblank has changed. Even if the controls + * will actually be applied to the sensor with some frames of latency + * by DelayedControls, all the algorithms calculations from now on should + * use the new limits. + * + * \todo Sensors usually have a margin that limits the max exposure to + * be shorter by the full frame length: + * + * (max_exposure_lines = height + vblank - margin) + * + * As the margin is a sensor-specific parameter either: + * - Ignore the margin and rely on the driver clamping the exposure + * value correctly + * - Defer to the sensor helpers by creating an exposure() function that + * subtract the margin from the frame length + * + * For the time being ignore the margins and rely on the driver doing + * the adjustment. + */ + if (vblank != context_.configuration.sensor.vBlank) { + context_.configuration.sensor.vBlank = vblank; + context_.configuration.sensor.maxExposureTime = + (vblank + context_.configuration.sensor.size.height) * + context_.configuration.sensor.lineDuration; + } + LOG(IPARkISP1, Debug) << "Set controls for frame " << frame << ": exposure " << exposure << ", gain " << frameContext.agc.gain << ", vblank " << vblank;
The AGC algorithm implementation of the RkISP1 IPA is implemented deriving the generic AgcMeanLuminance class, which limits the achievable exposure time using a maxExposureTime parameter provided by the IPA module. The RkISP1 IPA fetches the maxExposureTime value from the IPAContext sensor.maxExposureTime value, which is initialized at IPA init() and configure() time, but never updated later on, effectively limiting the achievable exposure time to the frame duration programmed at startup time. Whenever the frame duration is changed, the maximum exposure time should change accordingly, but in the current implementation this doesn't happen. Fix this by updating the sensor.maxExposureTime value when a new set of controls is sent to the pipeline handler. Store the current VBLANK value in the IPAContext by replacing 'defVBlank' which is never used and use it to detect changes in the blanking value. Whenever the blanking changes, update the maximum exposure limit accordingly. As reported in a comment in the code, even if the new sensor controls will be programmed in the sensor with a delay of a few frames on the pipeline handler side, the limits for the algorithms should be updated immediately. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/ipa/rkisp1/ipa_context.h | 2 +- src/ipa/rkisp1/rkisp1.cpp | 31 ++++++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-)