[v3,04/19] ipa: mali: Move exposure limits to sensor config
diff mbox series

Message ID 20251114-exposure-limits-v3-4-b7c07feba026@ideasonboard.com
State New
Headers show
Series
  • libipa: agc: Calculate exposure limits
Related show

Commit Message

Jacopo Mondi Nov. 14, 2025, 2:16 p.m. UTC 
Move the exposure limits (shutter time and gains) in the IPAContext
sensor configuration and not in the 'agc' member.

This aligns the Mali IPA with the RkISP1 and will help unifying the
handling of sensor configuration data through a common type.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/mali-c55/algorithms/agc.cpp | 12 ++++++------
 src/ipa/mali-c55/ipa_context.h      |  8 ++++----
 src/ipa/mali-c55/mali-c55.cpp       |  9 +++++----
 3 files changed, 15 insertions(+), 14 deletions(-)

Comments

Laurent Pinchart Nov. 19, 2025, 4:27 a.m. UTC | #1 
On Fri, Nov 14, 2025 at 03:16:59PM +0100, Jacopo Mondi wrote:
> Move the exposure limits (shutter time and gains) in the IPAContext
> sensor configuration and not in the 'agc' member.
> 
> This aligns the Mali IPA with the RkISP1 and will help unifying the
> handling of sensor configuration data through a common type.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/mali-c55/algorithms/agc.cpp | 12 ++++++------
>  src/ipa/mali-c55/ipa_context.h      |  8 ++++----
>  src/ipa/mali-c55/mali-c55.cpp       |  9 +++++----
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> index f60fddac3f04fd6f09dc782e929ff1593758c29b..4fa00769d201d906be357809f5af02c71201a4f1 100644
> --- a/src/ipa/mali-c55/algorithms/agc.cpp
> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> @@ -160,20 +160,20 @@ int Agc::configure(IPAContext &context,
>  	 * minimum analogue gain. AEGC is _active_ by default.
>  	 */
>  	context.activeState.agc.autoEnabled = true;
> -	context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
> +	context.activeState.agc.automatic.sensorGain = context.configuration.sensor.minAnalogueGain;
>  	context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
>  	context.activeState.agc.automatic.ispGain = kMinDigitalGain;
> -	context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
> +	context.activeState.agc.manual.sensorGain = context.configuration.sensor.minAnalogueGain;
>  	context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
>  	context.activeState.agc.manual.ispGain = kMinDigitalGain;
>  	context.activeState.agc.constraintMode = constraintModes().begin()->first;
>  	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>  
>  	/* \todo Run this again when FrameDurationLimits is passed in */
> -	setLimits(context.configuration.agc.minShutterSpeed,
> -		  context.configuration.agc.maxShutterSpeed,
> -		  context.configuration.agc.minAnalogueGain,
> -		  context.configuration.agc.maxAnalogueGain,
> +	setLimits(context.configuration.sensor.minShutterSpeed,
> +		  context.configuration.sensor.maxShutterSpeed,
> +		  context.configuration.sensor.minAnalogueGain,
> +		  context.configuration.sensor.maxAnalogueGain,
>  		  {});
>  
>  	resetFrameCount();
> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
> index bfa805c7b93f313dda2497365e83542bbc39e291..fe75590ec93302e61a31e4832f5c497aab80cf4d 100644
> --- a/src/ipa/mali-c55/ipa_context.h
> +++ b/src/ipa/mali-c55/ipa_context.h
> @@ -21,17 +21,17 @@ namespace ipa::mali_c55 {
>  
>  struct IPASessionConfiguration {
>  	struct {
> -		utils::Duration minShutterSpeed;
> -		utils::Duration maxShutterSpeed;
>  		uint32_t defaultExposure;
> -		double minAnalogueGain;
> -		double maxAnalogueGain;
>  	} agc;
>  
>  	struct {
>  		BayerFormat::Order bayerOrder;
>  		utils::Duration lineDuration;
>  		uint32_t blackLevel;
> +		utils::Duration minShutterSpeed;
> +		utils::Duration maxShutterSpeed;

It would be nice to rename this to minExposureTime and maxExposureTime
to align the names with rkisp1. Maybe that's done later in this series.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		double minAnalogueGain;
> +		double maxAnalogueGain;
>  	} sensor;
>  };
>  
> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
> index 0751513dc584ca84dd212bf8c1469dd4b40c053d..9375facf7ab559853986f66634d4e36b896361c8 100644
> --- a/src/ipa/mali-c55/mali-c55.cpp
> +++ b/src/ipa/mali-c55/mali-c55.cpp
> @@ -185,11 +185,12 @@ void IPAMaliC55::updateSessionConfiguration(const IPACameraSensorInfo &info,
>  	 * \todo take VBLANK into account for maximum shutter speed
>  	 */
>  	context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;
> -	context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> -	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.sensor.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.sensor.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.sensor.minAnalogueGain = context_.camHelper->gain(minGain);
> +	context_.configuration.sensor.maxAnalogueGain = context_.camHelper->gain(maxGain);
> +
>  	context_.configuration.agc.defaultExposure = defExposure;
> -	context_.configuration.agc.minAnalogueGain = context_.camHelper->gain(minGain);
> -	context_.configuration.agc.maxAnalogueGain = context_.camHelper->gain(maxGain);
>  
>  	if (context_.camHelper->blackLevel().has_value()) {
>  		/*
Jacopo Mondi Nov. 19, 2025, 6:54 p.m. UTC | #2 
Hi Laurent

On Wed, Nov 19, 2025 at 01:27:20PM +0900, Laurent Pinchart wrote:
> On Fri, Nov 14, 2025 at 03:16:59PM +0100, Jacopo Mondi wrote:
> > Move the exposure limits (shutter time and gains) in the IPAContext
> > sensor configuration and not in the 'agc' member.
> >
> > This aligns the Mali IPA with the RkISP1 and will help unifying the
> > handling of sensor configuration data through a common type.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/ipa/mali-c55/algorithms/agc.cpp | 12 ++++++------
> >  src/ipa/mali-c55/ipa_context.h      |  8 ++++----
> >  src/ipa/mali-c55/mali-c55.cpp       |  9 +++++----
> >  3 files changed, 15 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..4fa00769d201d906be357809f5af02c71201a4f1 100644
> > --- a/src/ipa/mali-c55/algorithms/agc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> > @@ -160,20 +160,20 @@ int Agc::configure(IPAContext &context,
> >  	 * minimum analogue gain. AEGC is _active_ by default.
> >  	 */
> >  	context.activeState.agc.autoEnabled = true;
> > -	context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
> > +	context.activeState.agc.automatic.sensorGain = context.configuration.sensor.minAnalogueGain;
> >  	context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
> >  	context.activeState.agc.automatic.ispGain = kMinDigitalGain;
> > -	context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
> > +	context.activeState.agc.manual.sensorGain = context.configuration.sensor.minAnalogueGain;
> >  	context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
> >  	context.activeState.agc.manual.ispGain = kMinDigitalGain;
> >  	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >  	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >
> >  	/* \todo Run this again when FrameDurationLimits is passed in */
> > -	setLimits(context.configuration.agc.minShutterSpeed,
> > -		  context.configuration.agc.maxShutterSpeed,
> > -		  context.configuration.agc.minAnalogueGain,
> > -		  context.configuration.agc.maxAnalogueGain,
> > +	setLimits(context.configuration.sensor.minShutterSpeed,
> > +		  context.configuration.sensor.maxShutterSpeed,
> > +		  context.configuration.sensor.minAnalogueGain,
> > +		  context.configuration.sensor.maxAnalogueGain,
> >  		  {});
> >
> >  	resetFrameCount();
> > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
> > index bfa805c7b93f313dda2497365e83542bbc39e291..fe75590ec93302e61a31e4832f5c497aab80cf4d 100644
> > --- a/src/ipa/mali-c55/ipa_context.h
> > +++ b/src/ipa/mali-c55/ipa_context.h
> > @@ -21,17 +21,17 @@ namespace ipa::mali_c55 {
> >
> >  struct IPASessionConfiguration {
> >  	struct {
> > -		utils::Duration minShutterSpeed;
> > -		utils::Duration maxShutterSpeed;
> >  		uint32_t defaultExposure;
> > -		double minAnalogueGain;
> > -		double maxAnalogueGain;
> >  	} agc;
> >
> >  	struct {
> >  		BayerFormat::Order bayerOrder;
> >  		utils::Duration lineDuration;
> >  		uint32_t blackLevel;
> > +		utils::Duration minShutterSpeed;
> > +		utils::Duration maxShutterSpeed;
>
> It would be nice to rename this to minExposureTime and maxExposureTime
> to align the names with rkisp1. Maybe that's done later in this series.

You know, Stefan pointed out the same, and I really thought we were
going into the opposite direction: replacing ExposureTime with
ShutterTime! I actually started converting the AgcMeanLuminance class,
fortunately I stopped and thought I could have done on top of this series...

I'll add a patch on the next version to s/ShutterSpeed/ExposureTime in
the Mali IPA.

Thanks
  j

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +		double minAnalogueGain;
> > +		double maxAnalogueGain;
> >  	} sensor;
> >  };
> >
> > diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
> > index 0751513dc584ca84dd212bf8c1469dd4b40c053d..9375facf7ab559853986f66634d4e36b896361c8 100644
> > --- a/src/ipa/mali-c55/mali-c55.cpp
> > +++ b/src/ipa/mali-c55/mali-c55.cpp
> > @@ -185,11 +185,12 @@ void IPAMaliC55::updateSessionConfiguration(const IPACameraSensorInfo &info,
> >  	 * \todo take VBLANK into account for maximum shutter speed
> >  	 */
> >  	context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;
> > -	context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> > -	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> > +	context_.configuration.sensor.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> > +	context_.configuration.sensor.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> > +	context_.configuration.sensor.minAnalogueGain = context_.camHelper->gain(minGain);
> > +	context_.configuration.sensor.maxAnalogueGain = context_.camHelper->gain(maxGain);
> > +
> >  	context_.configuration.agc.defaultExposure = defExposure;
> > -	context_.configuration.agc.minAnalogueGain = context_.camHelper->gain(minGain);
> > -	context_.configuration.agc.maxAnalogueGain = context_.camHelper->gain(maxGain);
> >
> >  	if (context_.camHelper->blackLevel().has_value()) {
> >  		/*
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
index f60fddac3f04fd6f09dc782e929ff1593758c29b..4fa00769d201d906be357809f5af02c71201a4f1 100644
--- a/src/ipa/mali-c55/algorithms/agc.cpp
+++ b/src/ipa/mali-c55/algorithms/agc.cpp
@@ -160,20 +160,20 @@  int Agc::configure(IPAContext &context,
 	 * minimum analogue gain. AEGC is _active_ by default.
 	 */
 	context.activeState.agc.autoEnabled = true;
-	context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
+	context.activeState.agc.automatic.sensorGain = context.configuration.sensor.minAnalogueGain;
 	context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
 	context.activeState.agc.automatic.ispGain = kMinDigitalGain;
-	context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
+	context.activeState.agc.manual.sensorGain = context.configuration.sensor.minAnalogueGain;
 	context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
 	context.activeState.agc.manual.ispGain = kMinDigitalGain;
 	context.activeState.agc.constraintMode = constraintModes().begin()->first;
 	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
 
 	/* \todo Run this again when FrameDurationLimits is passed in */
-	setLimits(context.configuration.agc.minShutterSpeed,
-		  context.configuration.agc.maxShutterSpeed,
-		  context.configuration.agc.minAnalogueGain,
-		  context.configuration.agc.maxAnalogueGain,
+	setLimits(context.configuration.sensor.minShutterSpeed,
+		  context.configuration.sensor.maxShutterSpeed,
+		  context.configuration.sensor.minAnalogueGain,
+		  context.configuration.sensor.maxAnalogueGain,
 		  {});
 
 	resetFrameCount();
diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
index bfa805c7b93f313dda2497365e83542bbc39e291..fe75590ec93302e61a31e4832f5c497aab80cf4d 100644
--- a/src/ipa/mali-c55/ipa_context.h
+++ b/src/ipa/mali-c55/ipa_context.h
@@ -21,17 +21,17 @@  namespace ipa::mali_c55 {
 
 struct IPASessionConfiguration {
 	struct {
-		utils::Duration minShutterSpeed;
-		utils::Duration maxShutterSpeed;
 		uint32_t defaultExposure;
-		double minAnalogueGain;
-		double maxAnalogueGain;
 	} agc;
 
 	struct {
 		BayerFormat::Order bayerOrder;
 		utils::Duration lineDuration;
 		uint32_t blackLevel;
+		utils::Duration minShutterSpeed;
+		utils::Duration maxShutterSpeed;
+		double minAnalogueGain;
+		double maxAnalogueGain;
 	} sensor;
 };
 
diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
index 0751513dc584ca84dd212bf8c1469dd4b40c053d..9375facf7ab559853986f66634d4e36b896361c8 100644
--- a/src/ipa/mali-c55/mali-c55.cpp
+++ b/src/ipa/mali-c55/mali-c55.cpp
@@ -185,11 +185,12 @@  void IPAMaliC55::updateSessionConfiguration(const IPACameraSensorInfo &info,
 	 * \todo take VBLANK into account for maximum shutter speed
 	 */
 	context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;
-	context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
-	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
+	context_.configuration.sensor.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
+	context_.configuration.sensor.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
+	context_.configuration.sensor.minAnalogueGain = context_.camHelper->gain(minGain);
+	context_.configuration.sensor.maxAnalogueGain = context_.camHelper->gain(maxGain);
+
 	context_.configuration.agc.defaultExposure = defExposure;
-	context_.configuration.agc.minAnalogueGain = context_.camHelper->gain(minGain);
-	context_.configuration.agc.maxAnalogueGain = context_.camHelper->gain(maxGain);
 
 	if (context_.camHelper->blackLevel().has_value()) {
 		/*