[libcamera-devel,v3,3/3] ipa: ipu3: Ensure controls exists in updateSessionConfiguration()
diff mbox series

Message ID 20220324142035.47338-4-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • ipa: ipu3: Consolidate querying of exposure and gain limits
Related show

Commit Message

Umang Jain March 24, 2022, 2:20 p.m. UTC 
Add a control-not-found error checking for the controls being
queried in IPAIPU3::updateSessionConfiguration(). If the control
is not found, progagate the error back to the caller. This requires
a change in the function signature of private member function
IPAIPU3::updateSessionConfiguration().

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart March 24, 2022, 3:57 p.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Thu, Mar 24, 2022 at 07:50:35PM +0530, Umang Jain via libcamera-devel wrote:
> Add a control-not-found error checking for the controls being
> queried in IPAIPU3::updateSessionConfiguration(). If the control
> is not found, progagate the error back to the caller. This requires
> a change in the function signature of private member function
> IPAIPU3::updateSessionConfiguration().
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 3717d893..66346728 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -149,7 +149,7 @@ private:
>  	void updateControls(const IPACameraSensorInfo &sensorInfo,
>  			    const ControlInfoMap &sensorControls,
>  			    ControlInfoMap *ipaControls);
> -	void updateSessionConfiguration(const ControlInfoMap &sensorControls);
> +	int updateSessionConfiguration(const ControlInfoMap &sensorControls);
>  	void processControls(unsigned int frame, const ControlList &controls);
>  	void fillParams(unsigned int frame, ipu3_uapi_params *params);
>  	void parseStatistics(unsigned int frame,
> @@ -180,18 +180,33 @@ private:
>   * \brief Compute IPASessionConfiguration using the sensor information and the
>   * sensor V4L2 controls
>   */
> -void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
> +int IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  {
> -	const ControlInfo vBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> -	context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
> +	const auto itVBlank = sensorControls.find(V4L2_CID_VBLANK);
> +	if (itVBlank == sensorControls.end()) {
> +		LOG(IPAIPU3, Error) << "Can't find vblank control";
> +		return -EINVAL;
> +	}

This answers my question in 2/3 :-)

I think it's good to be defensive.

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

>  
> -	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> -	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
> -	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
> +	context_.configuration.sensor.defVBlank = itVBlank->second.def().get<int32_t>();
> +
> +	const auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);
> +	if (itExp == sensorControls.end()) {
> +		LOG(IPAIPU3, Error) << "Can't find exposure control";
> +		return -EINVAL;
> +	}
> +
> +	int32_t minExposure = itExp->second.min().get<int32_t>();
> +	int32_t maxExposure = itExp->second.max().get<int32_t>();
>  
> -	const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
> -	int32_t minGain = v4l2Gain.min().get<int32_t>();
> -	int32_t maxGain = v4l2Gain.max().get<int32_t>();
> +	const auto itGain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);
> +	if (itGain == sensorControls.end()) {
> +		LOG(IPAIPU3, Error) << "Can't find analogue gain control";
> +		return -EINVAL;
> +	}
> +
> +	int32_t minGain = itGain->second.min().get<int32_t>();
> +	int32_t maxGain = itGain->second.max().get<int32_t>();
>  
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
> @@ -204,6 +219,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
>  	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
>  	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -437,10 +454,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  	updateControls(sensorInfo_, sensorCtrls_, ipaControls);
>  
>  	/* Update the IPASessionConfiguration using the sensor settings. */
> -	updateSessionConfiguration(sensorCtrls_);
> +	int ret = updateSessionConfiguration(sensorCtrls_);
> +	if (ret < 0)
> +		return ret;
> +
>  
>  	for (auto const &algo : algorithms_) {
> -		int ret = algo->configure(context_, configInfo);
> +		ret = algo->configure(context_, configInfo);
>  		if (ret)
>  			return ret;
>  	}
Kieran Bingham March 25, 2022, 9:08 a.m. UTC | #2 
Hi Umang,

Quoting Umang Jain via libcamera-devel (2022-03-24 14:20:35)
> Add a control-not-found error checking for the controls being
> queried in IPAIPU3::updateSessionConfiguration(). If the control
> is not found, progagate the error back to the caller. This requires
> a change in the function signature of private member function
> IPAIPU3::updateSessionConfiguration().

In RPi IPA this is handled with a dedicated function that can check a
list of controls, which helps make it extendable and keep the list of
required controls centralised.

Look at
 IPARPi::validateSensorControls()
 IPARPi::validateIspControls()
 IPARPi::validateLensControls()

A loop or dedicated validate function might make sense, but I think this
is also valid. At least this implementation means once the ctrl is
found, it's used without having to search again later.

So either way:

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


> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 3717d893..66346728 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -149,7 +149,7 @@ private:
>         void updateControls(const IPACameraSensorInfo &sensorInfo,
>                             const ControlInfoMap &sensorControls,
>                             ControlInfoMap *ipaControls);
> -       void updateSessionConfiguration(const ControlInfoMap &sensorControls);
> +       int updateSessionConfiguration(const ControlInfoMap &sensorControls);
>         void processControls(unsigned int frame, const ControlList &controls);
>         void fillParams(unsigned int frame, ipu3_uapi_params *params);
>         void parseStatistics(unsigned int frame,
> @@ -180,18 +180,33 @@ private:
>   * \brief Compute IPASessionConfiguration using the sensor information and the
>   * sensor V4L2 controls
>   */
> -void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
> +int IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>  {
> -       const ControlInfo vBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> -       context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
> +       const auto itVBlank = sensorControls.find(V4L2_CID_VBLANK);
> +       if (itVBlank == sensorControls.end()) {
> +               LOG(IPAIPU3, Error) << "Can't find vblank control";
> +               return -EINVAL;
> +       }
>  
> -       const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> -       int32_t minExposure = v4l2Exposure.min().get<int32_t>();
> -       int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
> +       context_.configuration.sensor.defVBlank = itVBlank->second.def().get<int32_t>();
> +
> +       const auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);
> +       if (itExp == sensorControls.end()) {
> +               LOG(IPAIPU3, Error) << "Can't find exposure control";
> +               return -EINVAL;
> +       }
> +
> +       int32_t minExposure = itExp->second.min().get<int32_t>();
> +       int32_t maxExposure = itExp->second.max().get<int32_t>();
>  
> -       const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
> -       int32_t minGain = v4l2Gain.min().get<int32_t>();
> -       int32_t maxGain = v4l2Gain.max().get<int32_t>();
> +       const auto itGain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);
> +       if (itGain == sensorControls.end()) {
> +               LOG(IPAIPU3, Error) << "Can't find analogue gain control";
> +               return -EINVAL;
> +       }
> +
> +       int32_t minGain = itGain->second.min().get<int32_t>();
> +       int32_t maxGain = itGain->second.max().get<int32_t>();
>  
>         /*
>          * When the AGC computes the new exposure values for a frame, it needs
> @@ -204,6 +219,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>         context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
>         context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
>         context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> +
> +       return 0;
>  }
>  
>  /**
> @@ -437,10 +454,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>         updateControls(sensorInfo_, sensorCtrls_, ipaControls);
>  
>         /* Update the IPASessionConfiguration using the sensor settings. */
> -       updateSessionConfiguration(sensorCtrls_);
> +       int ret = updateSessionConfiguration(sensorCtrls_);
> +       if (ret < 0)
> +               return ret;
> +
>  
>         for (auto const &algo : algorithms_) {
> -               int ret = algo->configure(context_, configInfo);
> +               ret = algo->configure(context_, configInfo);
>                 if (ret)
>                         return ret;
>         }
> -- 
> 2.31.1
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 3717d893..66346728 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -149,7 +149,7 @@  private:
 	void updateControls(const IPACameraSensorInfo &sensorInfo,
 			    const ControlInfoMap &sensorControls,
 			    ControlInfoMap *ipaControls);
-	void updateSessionConfiguration(const ControlInfoMap &sensorControls);
+	int updateSessionConfiguration(const ControlInfoMap &sensorControls);
 	void processControls(unsigned int frame, const ControlList &controls);
 	void fillParams(unsigned int frame, ipu3_uapi_params *params);
 	void parseStatistics(unsigned int frame,
@@ -180,18 +180,33 @@  private:
  * \brief Compute IPASessionConfiguration using the sensor information and the
  * sensor V4L2 controls
  */
-void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
+int IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
 {
-	const ControlInfo vBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
-	context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
+	const auto itVBlank = sensorControls.find(V4L2_CID_VBLANK);
+	if (itVBlank == sensorControls.end()) {
+		LOG(IPAIPU3, Error) << "Can't find vblank control";
+		return -EINVAL;
+	}
 
-	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
-	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
-	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
+	context_.configuration.sensor.defVBlank = itVBlank->second.def().get<int32_t>();
+
+	const auto itExp = sensorControls.find(V4L2_CID_EXPOSURE);
+	if (itExp == sensorControls.end()) {
+		LOG(IPAIPU3, Error) << "Can't find exposure control";
+		return -EINVAL;
+	}
+
+	int32_t minExposure = itExp->second.min().get<int32_t>();
+	int32_t maxExposure = itExp->second.max().get<int32_t>();
 
-	const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
-	int32_t minGain = v4l2Gain.min().get<int32_t>();
-	int32_t maxGain = v4l2Gain.max().get<int32_t>();
+	const auto itGain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);
+	if (itGain == sensorControls.end()) {
+		LOG(IPAIPU3, Error) << "Can't find analogue gain control";
+		return -EINVAL;
+	}
+
+	int32_t minGain = itGain->second.min().get<int32_t>();
+	int32_t maxGain = itGain->second.max().get<int32_t>();
 
 	/*
 	 * When the AGC computes the new exposure values for a frame, it needs
@@ -204,6 +219,8 @@  void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
 	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
 	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
 	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
+
+	return 0;
 }
 
 /**
@@ -437,10 +454,13 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo,
 	updateControls(sensorInfo_, sensorCtrls_, ipaControls);
 
 	/* Update the IPASessionConfiguration using the sensor settings. */
-	updateSessionConfiguration(sensorCtrls_);
+	int ret = updateSessionConfiguration(sensorCtrls_);
+	if (ret < 0)
+		return ret;
+
 
 	for (auto const &algo : algorithms_) {
-		int ret = algo->configure(context_, configInfo);
+		ret = algo->configure(context_, configInfo);
 		if (ret)
 			return ret;
 	}