[v2,4/7] ipa: rpi: Advance the delay context counter even when IPAs don't run

23863 diff mbox series

From: David Plowman <david.plowman@raspberrypi.com>

The delay context counter must be advanced even when we decide not to
run prepare/process. Otherwise, when we skip them at higher
framerates, the current IPA context counter will catch up and
overwrite the delay context.

It's safe to advance the counter because the metadata is always copied
forward a slot when we decide not to run the IPAs.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Quoting Naushir Patuck (2025-07-21 08:47:24)
> From: David Plowman <david.plowman@raspberrypi.com>
> 
> The delay context counter must be advanced even when we decide not to
> run prepare/process. Otherwise, when we skip them at higher
> framerates, the current IPA context counter will catch up and
> overwrite the delay context.
> 
> It's safe to advance the counter because the metadata is always copied
> forward a slot when we decide not to run the IPAs.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 8b4eb75e7e6b..98690b80d5d3 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -501,10 +501,11 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>  void IpaBase::processStats(const ProcessParams &params)
>  {
>         unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
> +       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
>  
> -       if (processPending_ && frameCount_ >= mistrustCount_) {
> -               RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> +       Duration offset(0s);

This is never used as far as I can see - and it's breaking CI:

[389/648] Compiling C++ object src/ipa/rpi/common/librpi_ipa_common.a.p/ipa_base.cpp.o
FAILED: src/ipa/rpi/common/librpi_ipa_common.a.p/ipa_base.cpp.o
clang++ -Isrc/ipa/rpi/common/librpi_ipa_common.a.p -Isrc/ipa/rpi/common -I../src/ipa/rpi/common -Isrc/ipa/rpi -I../src/ipa/rpi -Iinclude -I../include -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -fsanitize=address -fno-omit-frame-pointer -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c++17 -O0 -g -Wnon-virtual-dtor -stdlib=libc++ -Wextra-semi -Wthread-safety -Wmissing-declarations -Wshadow -include /builds/camera/libcamera/build/config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/rpi/common/librpi_ipa_common.a.p/ipa_base.cpp.o -MF src/ipa/rpi/common/librpi_ipa_common.a.p/ipa_base.cpp.o.d -o src/ipa/rpi/common/librpi_ipa_common.a.p/ipa_base.cpp.o -c ../src/ipa/rpi/common/ipa_base.cpp
../src/ipa/rpi/common/ipa_base.cpp:511:11: error: unused variable 'offset' [-Werror,-Wunused-variable]
        Duration offset(0s);
                 ^
1 error generated.


I think it's safe to remove so I'll try that and repush to gitlab.


Ok - now it's through:
https://gitlab.freedesktop.org/camera/libcamera/-/jobs/80899046


I'll also fix this up and then push:

---------------------------------------------------------------------------------------------------
70a9f8917e0166cba04429f07010caf51c709188 ipa: rpi: agc: Change handling of colour gains less than 1
---------------------------------------------------------------------------------------------------
--- src/ipa/rpi/controller/rpi/agc_channel.cpp
+++ src/ipa/rpi/controller/rpi/agc_channel.cpp
@@ -711,7 +710,7 @@
 	}

 	/* Factor in the AWB correction if needed. */
-	if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb)  {
+	if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) {
 		double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 });
 		minColourGain = std::max(minColourGain, 1.0);
 		RGB<double> colourGains{ { awb.gainR, awb.gainG, awb.gainB } };
---

--
Kieran


>  
> +       if (processPending_ && frameCount_ >= mistrustCount_) {
>                 auto it = buffers_.find(params.buffers.stats);
>                 if (it == buffers_.end()) {
>                         LOG(IPARPI, Error) << "Could not find stats buffer!";
> @@ -518,14 +519,14 @@ void IpaBase::processStats(const ProcessParams &params)
>  
>                 helper_->process(statistics, rpiMetadata);
>                 controller_.process(statistics, &rpiMetadata);
> +       }
>  
> -               struct AgcStatus agcStatus;
> -               if (rpiMetadata.get("agc.status", agcStatus) == 0) {
> -                       ControlList ctrls(sensorCtrls_);
> -                       applyAGC(&agcStatus, ctrls);
> -                       setDelayedControls.emit(ctrls, ipaContext);
> -                       setCameraTimeoutValue();
> -               }
> +       struct AgcStatus agcStatus;
> +       if (rpiMetadata.get("agc.status", agcStatus) == 0) {
> +               ControlList ctrls(sensorCtrls_);
> +               applyAGC(&agcStatus, ctrls);
> +               setDelayedControls.emit(ctrls, ipaContext);
> +               setCameraTimeoutValue();
>         }
>  
>         /*
> -- 
> 2.43.0
>