[4/4] ipa: rkisp1: agc: Add support for sync
diff mbox series

Message ID 20250829091011.2628954-5-paul.elder@ideasonboard.com
State New
Headers show
Series
  • Add Sync Layer
Related show

Commit Message

Paul Elder Aug. 29, 2025, 9:10 a.m. UTC 
Add sync support to the RkISP1 IPA using the SyncHelper from libipa. The
syncAdjustment is saved in the frameContext (as opposed to getting it
from the SyncHelper) to avoid potential races and ensure that the same
value that was set into vblank will be returned in metadata.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 22 +++++++++++++++++++++-
 src/ipa/rkisp1/algorithms/agc.h   |  3 ++-
 src/ipa/rkisp1/ipa_context.h      |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

Comments

Barnabás Pőcze Sept. 25, 2025, 11:15 a.m. UTC | #1 
Hi

2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:
> Add sync support to the RkISP1 IPA using the SyncHelper from libipa. The
> syncAdjustment is saved in the frameContext (as opposed to getting it
> from the SyncHelper) to avoid potential races and ensure that the same
> value that was set into vblank will be returned in metadata.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>   src/ipa/rkisp1/algorithms/agc.cpp | 22 +++++++++++++++++++++-
>   src/ipa/rkisp1/algorithms/agc.h   |  3 ++-
>   src/ipa/rkisp1/ipa_context.h      |  1 +
>   3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index bb1558df5422..9a27696ff868 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -156,6 +156,15 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>   				ControlValue(controls::AnalogueGainModeManual) } },
>   			    ControlValue(controls::AnalogueGainModeAuto));
>   	context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> +	/*
> +	 * Insert the controlInfo for sync. Since FrameDurationLimits is only
> +	 * set *after* the algorithms are initialized, we have no information
> +	 * on it here. Use a sensible default here and update it later in
> +	 * configure().
> +	 * \todo Move FrameDurationLimits from the base IPA to AGC
> +	 */
> +	context.ctrlMap[&controls::SyncAdjustment] = controlInfo(120000);

Just a small thing, but could you write 120'000? It's easier to count the zeros that way.



> +	/* Insert the controls for agc */
>   	context.ctrlMap.merge(controls());
>   
>   	return 0;
> @@ -202,7 +211,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>   		  context.configuration.sensor.minAnalogueGain,
>   		  context.configuration.sensor.maxAnalogueGain);
>   
> +	context.ctrlMap[&controls::SyncAdjustment] =
> +		controlInfo(frameDurationLimits.max().get<int64_t>());
> +
>   	resetFrameCount();
> +	resetSync();
>   
>   	return 0;
>   }
> @@ -324,6 +337,10 @@ void Agc::queueRequest(IPAContext &context,
>   	}
>   	frameContext.agc.minFrameDuration = agc.minFrameDuration;
>   	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> +
> +	const auto &sync = controls.get(controls::SyncAdjustment);
> +	if (sync)
> +		setSync(*sync, frameContext.agc.minFrameDuration);
>   }
>   
>   /**
> @@ -413,6 +430,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>   	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
>   	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
>   	metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
> +	metadata.set(controls::SyncAdjustment, frameContext.agc.syncAdjustment.count());
>   }
>   
>   /**
> @@ -471,7 +489,9 @@ void Agc::processFrameDuration(IPAContext &context,
>   	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
>   	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
>   
> -	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> +	utils::Duration sync = getSync();
> +	frameContext.agc.vblank = ((frameDuration + sync) / lineDuration) - sensorInfo.outputSize.height;
> +	frameContext.agc.syncAdjustment = sync;
>   
>   	/* Update frame duration accounting for line length quantization. */
>   	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index 7867eed9c4e3..4441a519e857 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -15,6 +15,7 @@
>   #include <libcamera/geometry.h>
>   
>   #include "libipa/agc_mean_luminance.h"
> +#include "libipa/sync_helper.h"
>   
>   #include "algorithm.h"
>   
> @@ -22,7 +23,7 @@ namespace libcamera {
>   
>   namespace ipa::rkisp1::algorithms {
>   
> -class Agc : public Algorithm, public AgcMeanLuminance
> +class Agc : public Algorithm, public AgcMeanLuminance, public SyncHelper

I think we could (should?) avoid inheritance here, and simply store a `SyncHelper sync`
member somewhere in `Agc`. This would allow you to drop the "Sync" suffix from the
functions in `SyncHelper`. Or is there anything planned that will need virtual
functions, etc?


Regards,
Barnabás Pőcze

>   {
>   public:
>   	Agc();
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 7ccc7b501aff..c21ab77b2ba5 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -143,6 +143,7 @@ struct IPAFrameContext : public FrameContext {
>   		bool updateMetering;
>   		bool autoExposureModeChange;
>   		bool autoGainModeChange;
> +		utils::Duration syncAdjustment;
>   	} agc;
>   
>   	struct {
Paul Elder Sept. 26, 2025, 8:15 a.m. UTC | #2 
Hi Barnabás,

Thanks for the review.

Quoting Barnabás Pőcze (2025-09-25 20:15:31)
> Hi
> 
> 2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:
> > Add sync support to the RkISP1 IPA using the SyncHelper from libipa. The
> > syncAdjustment is saved in the frameContext (as opposed to getting it
> > from the SyncHelper) to avoid potential races and ensure that the same
> > value that was set into vblank will be returned in metadata.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >   src/ipa/rkisp1/algorithms/agc.cpp | 22 +++++++++++++++++++++-
> >   src/ipa/rkisp1/algorithms/agc.h   |  3 ++-
> >   src/ipa/rkisp1/ipa_context.h      |  1 +
> >   3 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index bb1558df5422..9a27696ff868 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -156,6 +156,15 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> >                               ControlValue(controls::AnalogueGainModeManual) } },
> >                           ControlValue(controls::AnalogueGainModeAuto));
> >       context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
> > +     /*
> > +      * Insert the controlInfo for sync. Since FrameDurationLimits is only
> > +      * set *after* the algorithms are initialized, we have no information
> > +      * on it here. Use a sensible default here and update it later in
> > +      * configure().
> > +      * \todo Move FrameDurationLimits from the base IPA to AGC
> > +      */
> > +     context.ctrlMap[&controls::SyncAdjustment] = controlInfo(120000);
> 
> Just a small thing, but could you write 120'000? It's easier to count the zeros that way.
> 
> 
> 
> > +     /* Insert the controls for agc */
> >       context.ctrlMap.merge(controls());
> >   
> >       return 0;
> > @@ -202,7 +211,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >                 context.configuration.sensor.minAnalogueGain,
> >                 context.configuration.sensor.maxAnalogueGain);
> >   
> > +     context.ctrlMap[&controls::SyncAdjustment] =
> > +             controlInfo(frameDurationLimits.max().get<int64_t>());
> > +
> >       resetFrameCount();
> > +     resetSync();
> >   
> >       return 0;
> >   }
> > @@ -324,6 +337,10 @@ void Agc::queueRequest(IPAContext &context,
> >       }
> >       frameContext.agc.minFrameDuration = agc.minFrameDuration;
> >       frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> > +
> > +     const auto &sync = controls.get(controls::SyncAdjustment);
> > +     if (sync)
> > +             setSync(*sync, frameContext.agc.minFrameDuration);
> >   }
> >   
> >   /**
> > @@ -413,6 +430,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> >       metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> >       metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> >       metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
> > +     metadata.set(controls::SyncAdjustment, frameContext.agc.syncAdjustment.count());
> >   }
> >   
> >   /**
> > @@ -471,7 +489,9 @@ void Agc::processFrameDuration(IPAContext &context,
> >       IPACameraSensorInfo &sensorInfo = context.sensorInfo;
> >       utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> >   
> > -     frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > +     utils::Duration sync = getSync();
> > +     frameContext.agc.vblank = ((frameDuration + sync) / lineDuration) - sensorInfo.outputSize.height;
> > +     frameContext.agc.syncAdjustment = sync;
> >   
> >       /* Update frame duration accounting for line length quantization. */
> >       frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index 7867eed9c4e3..4441a519e857 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -15,6 +15,7 @@
> >   #include <libcamera/geometry.h>
> >   
> >   #include "libipa/agc_mean_luminance.h"
> > +#include "libipa/sync_helper.h"
> >   
> >   #include "algorithm.h"
> >   
> > @@ -22,7 +23,7 @@ namespace libcamera {
> >   
> >   namespace ipa::rkisp1::algorithms {
> >   
> > -class Agc : public Algorithm, public AgcMeanLuminance
> > +class Agc : public Algorithm, public AgcMeanLuminance, public SyncHelper
> 
> I think we could (should?) avoid inheritance here, and simply store a `SyncHelper sync`
> member somewhere in `Agc`. This would allow you to drop the "Sync" suffix from the
> functions in `SyncHelper`. Or is there anything planned that will need virtual
> functions, etc?

The intention was that in the (hopefully near) future, IPAs/algorithms could
inherit functionality from libipa and not even have to plumb anything like this
patch does. But I suppose until that future comes, it might indeed be cleaner
to store a SyncHelper member.


Thanks,

Paul

> 
> 
> Regards,
> Barnabás Pőcze
> 
> >   {
> >   public:
> >       Agc();
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 7ccc7b501aff..c21ab77b2ba5 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -143,6 +143,7 @@ struct IPAFrameContext : public FrameContext {
> >               bool updateMetering;
> >               bool autoExposureModeChange;
> >               bool autoGainModeChange;
> > +             utils::Duration syncAdjustment;
> >       } agc;
> >   
> >       struct {
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index bb1558df5422..9a27696ff868 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -156,6 +156,15 @@  int Agc::init(IPAContext &context, const YamlObject &tuningData)
 				ControlValue(controls::AnalogueGainModeManual) } },
 			    ControlValue(controls::AnalogueGainModeAuto));
 	context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);
+	/*
+	 * Insert the controlInfo for sync. Since FrameDurationLimits is only
+	 * set *after* the algorithms are initialized, we have no information
+	 * on it here. Use a sensible default here and update it later in
+	 * configure().
+	 * \todo Move FrameDurationLimits from the base IPA to AGC
+	 */
+	context.ctrlMap[&controls::SyncAdjustment] = controlInfo(120000);
+	/* Insert the controls for agc */
 	context.ctrlMap.merge(controls());
 
 	return 0;
@@ -202,7 +211,11 @@  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
 		  context.configuration.sensor.minAnalogueGain,
 		  context.configuration.sensor.maxAnalogueGain);
 
+	context.ctrlMap[&controls::SyncAdjustment] =
+		controlInfo(frameDurationLimits.max().get<int64_t>());
+
 	resetFrameCount();
+	resetSync();
 
 	return 0;
 }
@@ -324,6 +337,10 @@  void Agc::queueRequest(IPAContext &context,
 	}
 	frameContext.agc.minFrameDuration = agc.minFrameDuration;
 	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
+
+	const auto &sync = controls.get(controls::SyncAdjustment);
+	if (sync)
+		setSync(*sync, frameContext.agc.minFrameDuration);
 }
 
 /**
@@ -413,6 +430,7 @@  void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
 	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
 	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
 	metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);
+	metadata.set(controls::SyncAdjustment, frameContext.agc.syncAdjustment.count());
 }
 
 /**
@@ -471,7 +489,9 @@  void Agc::processFrameDuration(IPAContext &context,
 	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
 	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
 
-	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
+	utils::Duration sync = getSync();
+	frameContext.agc.vblank = ((frameDuration + sync) / lineDuration) - sensorInfo.outputSize.height;
+	frameContext.agc.syncAdjustment = sync;
 
 	/* Update frame duration accounting for line length quantization. */
 	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
index 7867eed9c4e3..4441a519e857 100644
--- a/src/ipa/rkisp1/algorithms/agc.h
+++ b/src/ipa/rkisp1/algorithms/agc.h
@@ -15,6 +15,7 @@ 
 #include <libcamera/geometry.h>
 
 #include "libipa/agc_mean_luminance.h"
+#include "libipa/sync_helper.h"
 
 #include "algorithm.h"
 
@@ -22,7 +23,7 @@  namespace libcamera {
 
 namespace ipa::rkisp1::algorithms {
 
-class Agc : public Algorithm, public AgcMeanLuminance
+class Agc : public Algorithm, public AgcMeanLuminance, public SyncHelper
 {
 public:
 	Agc();
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 7ccc7b501aff..c21ab77b2ba5 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -143,6 +143,7 @@  struct IPAFrameContext : public FrameContext {
 		bool updateMetering;
 		bool autoExposureModeChange;
 		bool autoGainModeChange;
+		utils::Duration syncAdjustment;
 	} agc;
 
 	struct {