[v2,10/12] ipa: ipu3: Don't pass sensorInfo to updateControls()
diff mbox series

Message ID 20260626-ipu3-libipa-rework-v2-10-41546e23de3e@ideasonboard.com
State New
Headers show
Series
  • libipa: Re-work IPU3 IPA to use libipa algorithms
Related show

Commit Message

Dan Scally June 26, 2026, 1:05 p.m. UTC 
The updateControls() function uses the configured output size of the
sensor to set the ranges for vertical blanking and frame duration
controls. We currently pass sensorInfo as a parameter, but it's also
stored as a member of class IPAIPU3 and so doesn't need to be passed
to updateControls at all().

Drop the parameter and just use the class member instead.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

        - New patch
---
 src/ipa/ipu3/ipu3.cpp | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Kieran Bingham June 29, 2026, 9:58 a.m. UTC | #1 
Quoting Daniel Scally (2026-06-26 14:05:57)
> The updateControls() function uses the configured output size of the
> sensor to set the ranges for vertical blanking and frame duration
> controls. We currently pass sensorInfo as a parameter, but it's also
> stored as a member of class IPAIPU3 and so doesn't need to be passed
> to updateControls at all().
> 
> Drop the parameter and just use the class member instead.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
> 
>         - New patch

Is this the same pattern as the other libipa modules ?

I'd be keen to work towards consistency here throughout.

So if it's the right way:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/ipu3/ipu3.cpp | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 4bdc4b7677fe9703fb968ca91967c68742454514..b1c176e68098b24d8fd37e7a0e278cabf0e1c3b2 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -165,8 +165,7 @@ protected:
>         std::string logPrefix() const override;
>  
>  private:
> -       void updateControls(const IPACameraSensorInfo &sensorInfo,
> -                           const ControlInfoMap &sensorControls,
> +       void updateControls(const ControlInfoMap &sensorControls,
>                             ControlInfoMap *ipaControls);
>         void updateSessionConfiguration(const ControlInfoMap &sensorControls);
>  
> @@ -239,8 +238,7 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>   * - controls::ExposureTime
>   * - controls::FrameDurationLimits
>   */
> -void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> -                            const ControlInfoMap &sensorControls,
> +void IPAIPU3::updateControls(const ControlInfoMap &sensorControls,
>                              ControlInfoMap *ipaControls)
>  {
>         ControlInfoMap::Map controls{};
> @@ -267,19 +265,19 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>          */
>         const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
>         uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> -       uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> +       uint32_t lineLength = sensorInfo_.outputSize.width + hblank;
>  
>         const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
>         std::array<uint32_t, 3> frameHeights{
> -               v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> -               v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> -               v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> +               v4l2VBlank.min().get<int32_t>() + sensorInfo_.outputSize.height,
> +               v4l2VBlank.max().get<int32_t>() + sensorInfo_.outputSize.height,
> +               v4l2VBlank.def().get<int32_t>() + sensorInfo_.outputSize.height,
>         };
>  
>         std::array<int64_t, 3> frameDurations;
>         for (unsigned int i = 0; i < frameHeights.size(); ++i) {
>                 uint64_t frameSize = lineLength * frameHeights[i];
> -               frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> +               frameDurations[i] = frameSize / (sensorInfo_.pixelRate / 1000000U);
>         }
>         controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
>                                                                frameDurations[1],
> @@ -313,6 +311,7 @@ int IPAIPU3::init(const IPASettings &settings,
>         context_.configuration = {};
>         context_.configuration.sensor.lineDuration =
>                 sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;
> +       sensorInfo_ = sensorInfo;
>  
>         /* Load the tuning data file. */
>         File file(settings.configurationFile);
> @@ -346,7 +345,7 @@ int IPAIPU3::init(const IPASettings &settings,
>                 return ret;
>  
>         /* Initialize controls. */
> -       updateControls(sensorInfo, sensorControls, ipaControls);
> +       updateControls(sensorControls, ipaControls);
>  
>         return 0;
>  }
> @@ -488,7 +487,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>         calculateBdsGrid(configInfo.bdsOutputSize);
>  
>         /* Update the camera controls using the new sensor settings. */
> -       updateControls(sensorInfo_, sensorCtrls_, ipaControls);
> +       updateControls(sensorCtrls_, ipaControls);
>  
>         /* Update the IPASessionConfiguration using the sensor settings. */
>         updateSessionConfiguration(sensorCtrls_);
> 
> -- 
> 2.43.0
>
Barnabás Pőcze June 30, 2026, 11:36 a.m. UTC | #2 
2026. 06. 26. 15:05 keltezéssel, Daniel Scally írta:
> The updateControls() function uses the configured output size of the
> sensor to set the ranges for vertical blanking and frame duration
> controls. We currently pass sensorInfo as a parameter, but it's also
> stored as a member of class IPAIPU3 and so doesn't need to be passed
> to updateControls at all().
> 
> Drop the parameter and just use the class member instead.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
> 
>          - New patch
> ---

Looks ok to me.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   src/ipa/ipu3/ipu3.cpp | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 4bdc4b7677fe9703fb968ca91967c68742454514..b1c176e68098b24d8fd37e7a0e278cabf0e1c3b2 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -165,8 +165,7 @@ protected:
>   	std::string logPrefix() const override;
>   
>   private:
> -	void updateControls(const IPACameraSensorInfo &sensorInfo,
> -			    const ControlInfoMap &sensorControls,
> +	void updateControls(const ControlInfoMap &sensorControls,
>   			    ControlInfoMap *ipaControls);
>   	void updateSessionConfiguration(const ControlInfoMap &sensorControls);
>   
> @@ -239,8 +238,7 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>    * - controls::ExposureTime
>    * - controls::FrameDurationLimits
>    */
> -void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> -			     const ControlInfoMap &sensorControls,
> +void IPAIPU3::updateControls(const ControlInfoMap &sensorControls,
>   			     ControlInfoMap *ipaControls)
>   {
>   	ControlInfoMap::Map controls{};
> @@ -267,19 +265,19 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>   	 */
>   	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
>   	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> -	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> +	uint32_t lineLength = sensorInfo_.outputSize.width + hblank;
>   
>   	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
>   	std::array<uint32_t, 3> frameHeights{
> -		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> -		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> -		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> +		v4l2VBlank.min().get<int32_t>() + sensorInfo_.outputSize.height,
> +		v4l2VBlank.max().get<int32_t>() + sensorInfo_.outputSize.height,
> +		v4l2VBlank.def().get<int32_t>() + sensorInfo_.outputSize.height,
>   	};
>   
>   	std::array<int64_t, 3> frameDurations;
>   	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
>   		uint64_t frameSize = lineLength * frameHeights[i];
> -		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> +		frameDurations[i] = frameSize / (sensorInfo_.pixelRate / 1000000U);
>   	}
>   	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
>   							       frameDurations[1],
> @@ -313,6 +311,7 @@ int IPAIPU3::init(const IPASettings &settings,
>   	context_.configuration = {};
>   	context_.configuration.sensor.lineDuration =
>   		sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;
> +	sensorInfo_ = sensorInfo;
>   
>   	/* Load the tuning data file. */
>   	File file(settings.configurationFile);
> @@ -346,7 +345,7 @@ int IPAIPU3::init(const IPASettings &settings,
>   		return ret;
>   
>   	/* Initialize controls. */
> -	updateControls(sensorInfo, sensorControls, ipaControls);
> +	updateControls(sensorControls, ipaControls);
>   
>   	return 0;
>   }
> @@ -488,7 +487,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   	calculateBdsGrid(configInfo.bdsOutputSize);
>   
>   	/* Update the camera controls using the new sensor settings. */
> -	updateControls(sensorInfo_, sensorCtrls_, ipaControls);
> +	updateControls(sensorCtrls_, ipaControls);
>   
>   	/* Update the IPASessionConfiguration using the sensor settings. */
>   	updateSessionConfiguration(sensorCtrls_);
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 4bdc4b7677fe9703fb968ca91967c68742454514..b1c176e68098b24d8fd37e7a0e278cabf0e1c3b2 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -165,8 +165,7 @@  protected:
 	std::string logPrefix() const override;
 
 private:
-	void updateControls(const IPACameraSensorInfo &sensorInfo,
-			    const ControlInfoMap &sensorControls,
+	void updateControls(const ControlInfoMap &sensorControls,
 			    ControlInfoMap *ipaControls);
 	void updateSessionConfiguration(const ControlInfoMap &sensorControls);
 
@@ -239,8 +238,7 @@  void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
  * - controls::ExposureTime
  * - controls::FrameDurationLimits
  */
-void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
-			     const ControlInfoMap &sensorControls,
+void IPAIPU3::updateControls(const ControlInfoMap &sensorControls,
 			     ControlInfoMap *ipaControls)
 {
 	ControlInfoMap::Map controls{};
@@ -267,19 +265,19 @@  void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
 	 */
 	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
 	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
-	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
+	uint32_t lineLength = sensorInfo_.outputSize.width + hblank;
 
 	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
 	std::array<uint32_t, 3> frameHeights{
-		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
-		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
-		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
+		v4l2VBlank.min().get<int32_t>() + sensorInfo_.outputSize.height,
+		v4l2VBlank.max().get<int32_t>() + sensorInfo_.outputSize.height,
+		v4l2VBlank.def().get<int32_t>() + sensorInfo_.outputSize.height,
 	};
 
 	std::array<int64_t, 3> frameDurations;
 	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
 		uint64_t frameSize = lineLength * frameHeights[i];
-		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
+		frameDurations[i] = frameSize / (sensorInfo_.pixelRate / 1000000U);
 	}
 	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
 							       frameDurations[1],
@@ -313,6 +311,7 @@  int IPAIPU3::init(const IPASettings &settings,
 	context_.configuration = {};
 	context_.configuration.sensor.lineDuration =
 		sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;
+	sensorInfo_ = sensorInfo;
 
 	/* Load the tuning data file. */
 	File file(settings.configurationFile);
@@ -346,7 +345,7 @@  int IPAIPU3::init(const IPASettings &settings,
 		return ret;
 
 	/* Initialize controls. */
-	updateControls(sensorInfo, sensorControls, ipaControls);
+	updateControls(sensorControls, ipaControls);
 
 	return 0;
 }
@@ -488,7 +487,7 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo,
 	calculateBdsGrid(configInfo.bdsOutputSize);
 
 	/* Update the camera controls using the new sensor settings. */
-	updateControls(sensorInfo_, sensorCtrls_, ipaControls);
+	updateControls(sensorCtrls_, ipaControls);
 
 	/* Update the IPASessionConfiguration using the sensor settings. */
 	updateSessionConfiguration(sensorCtrls_);