[libcamera-devel] ipa: ipu3: Support return values from configure()
diff mbox series

Message ID 20210611125239.1021944-1-kieran.bingham@ideasonboard.com
State Accepted
Commit 15196e5b76f184d3d27900749de0bf7010226261
Headers show
Series
  • [libcamera-devel] ipa: ipu3: Support return values from configure()
Related show

Commit Message

Kieran Bingham June 11, 2021, 12:52 p.m. UTC 
The IPU3 IPA interface does not define a return value from configure().
This prevents errors from being reported back to the pipeline handler
when they occur in the IPA.

Update the IPU3 IPA interface and add return values to the checks in
IPAIPU3::configure() accordingly

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/ipa/ipu3.mojom     |  2 +-
 src/ipa/ipu3/ipu3.cpp                | 18 +++++++++++-------
 src/libcamera/pipeline/ipu3/ipu3.cpp |  6 +++++-
 3 files changed, 17 insertions(+), 9 deletions(-)

Comments

Paul Elder June 14, 2021, 3:44 a.m. UTC | #1 
Hi Kieran,

On Fri, Jun 11, 2021 at 01:52:39PM +0100, Kieran Bingham wrote:
> The IPU3 IPA interface does not define a return value from configure().
> This prevents errors from being reported back to the pipeline handler
> when they occur in the IPA.
> 
> Update the IPU3 IPA interface and add return values to the checks in
> IPAIPU3::configure() accordingly
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  include/libcamera/ipa/ipu3.mojom     |  2 +-
>  src/ipa/ipu3/ipu3.cpp                | 18 +++++++++++-------
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  6 +++++-
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 000c494dbff9..911a3a072464 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -42,7 +42,7 @@ interface IPAIPU3Interface {
>  	start() => (int32 ret);
>  	stop();
>  
> -	configure(IPAConfigInfo configInfo) => ();
> +	configure(IPAConfigInfo configInfo) => (int32 ret);
>  
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 415ea9e58cf4..8b4c7351e9db 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -43,7 +43,7 @@ public:
>  	int start() override;
>  	void stop() override {}
>  
> -	void configure(const IPAConfigInfo &configInfo) override;
> +	int configure(const IPAConfigInfo &configInfo) override;
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> @@ -142,10 +142,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
>  }
>  
> -void IPAIPU3::configure(const IPAConfigInfo &configInfo)
> +int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  {
> -	if (configInfo.entityControls.empty())
> -		return;
> +	if (configInfo.entityControls.empty()) {
> +		LOG(IPAIPU3, Error) << "No controls provided";
> +		return -ENODATA;
> +	}
>  
>  	sensorInfo_ = configInfo.sensorInfo;
>  
> @@ -154,19 +156,19 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>  	if (itExp == ctrls_.end()) {
>  		LOG(IPAIPU3, Error) << "Can't find exposure control";
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>  	if (itGain == ctrls_.end()) {
>  		LOG(IPAIPU3, Error) << "Can't find gain control";
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);
>  	if (itVBlank == ctrls_.end()) {
>  		LOG(IPAIPU3, Error) << "Can't find VBLANK control";
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
> @@ -188,6 +190,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
>  	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> +
> +	return 0;
>  }
>  
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index b986bb7035fa..87f6bac83927 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -642,7 +642,11 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	configInfo.bdsOutputSize = config->imguConfig().bds;
>  	configInfo.iif = config->imguConfig().iif;
>  
> -	data->ipa_->configure(configInfo);
> +	ret = data->ipa_->configure(configInfo);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to configure IPA";
> +		return ret;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.30.2
>
Umang Jain June 14, 2021, 5:34 a.m. UTC | #2 
Hi Kieran

On 6/11/21 6:22 PM, Kieran Bingham wrote:
> The IPU3 IPA interface does not define a return value from configure().
> This prevents errors from being reported back to the pipeline handler
> when they occur in the IPA.
>
> Update the IPU3 IPA interface and add return values to the checks in
> IPAIPU3::configure() accordingly
s/accordingly/accordingly./
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   include/libcamera/ipa/ipu3.mojom     |  2 +-
>   src/ipa/ipu3/ipu3.cpp                | 18 +++++++++++-------
>   src/libcamera/pipeline/ipu3/ipu3.cpp |  6 +++++-
>   3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 000c494dbff9..911a3a072464 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -42,7 +42,7 @@ interface IPAIPU3Interface {
>   	start() => (int32 ret);
>   	stop();
>   
> -	configure(IPAConfigInfo configInfo) => ();
> +	configure(IPAConfigInfo configInfo) => (int32 ret);
>   
>   	mapBuffers(array<libcamera.IPABuffer> buffers);
>   	unmapBuffers(array<uint32> ids);
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 415ea9e58cf4..8b4c7351e9db 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -43,7 +43,7 @@ public:
>   	int start() override;
>   	void stop() override {}
>   
> -	void configure(const IPAConfigInfo &configInfo) override;
> +	int configure(const IPAConfigInfo &configInfo) override;
>   
>   	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>   	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> @@ -142,10 +142,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>   			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
>   }
>   
> -void IPAIPU3::configure(const IPAConfigInfo &configInfo)
> +int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>   {
> -	if (configInfo.entityControls.empty())
> -		return;
> +	if (configInfo.entityControls.empty()) {
> +		LOG(IPAIPU3, Error) << "No controls provided";
> +		return -ENODATA;
> +	}
>   
>   	sensorInfo_ = configInfo.sensorInfo;
>   
> @@ -154,19 +156,19 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>   	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>   	if (itExp == ctrls_.end()) {
>   		LOG(IPAIPU3, Error) << "Can't find exposure control";
> -		return;
> +		return -EINVAL;
>   	}
>   
>   	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>   	if (itGain == ctrls_.end()) {
>   		LOG(IPAIPU3, Error) << "Can't find gain control";
> -		return;
> +		return -EINVAL;
>   	}
>   
>   	const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);
>   	if (itVBlank == ctrls_.end()) {
>   		LOG(IPAIPU3, Error) << "Can't find VBLANK control";
> -		return;
> +		return -EINVAL;
>   	}
>   
>   	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
> @@ -188,6 +190,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>   
>   	agcAlgo_ = std::make_unique<IPU3Agc>();
>   	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> +
> +	return 0;
>   }
>   
>   void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index b986bb7035fa..87f6bac83927 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -642,7 +642,11 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>   	configInfo.bdsOutputSize = config->imguConfig().bds;
>   	configInfo.iif = config->imguConfig().iif;
>   
> -	data->ipa_->configure(configInfo);
> +	ret = data->ipa_->configure(configInfo);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to configure IPA";
> +		return ret;
> +	}
>   
>   	return 0;
>   }

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
index 000c494dbff9..911a3a072464 100644
--- a/include/libcamera/ipa/ipu3.mojom
+++ b/include/libcamera/ipa/ipu3.mojom
@@ -42,7 +42,7 @@  interface IPAIPU3Interface {
 	start() => (int32 ret);
 	stop();
 
-	configure(IPAConfigInfo configInfo) => ();
+	configure(IPAConfigInfo configInfo) => (int32 ret);
 
 	mapBuffers(array<libcamera.IPABuffer> buffers);
 	unmapBuffers(array<uint32> ids);
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 415ea9e58cf4..8b4c7351e9db 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -43,7 +43,7 @@  public:
 	int start() override;
 	void stop() override {}
 
-	void configure(const IPAConfigInfo &configInfo) override;
+	int configure(const IPAConfigInfo &configInfo) override;
 
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
@@ -142,10 +142,12 @@  void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
 			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
 }
 
-void IPAIPU3::configure(const IPAConfigInfo &configInfo)
+int IPAIPU3::configure(const IPAConfigInfo &configInfo)
 {
-	if (configInfo.entityControls.empty())
-		return;
+	if (configInfo.entityControls.empty()) {
+		LOG(IPAIPU3, Error) << "No controls provided";
+		return -ENODATA;
+	}
 
 	sensorInfo_ = configInfo.sensorInfo;
 
@@ -154,19 +156,19 @@  void IPAIPU3::configure(const IPAConfigInfo &configInfo)
 	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
 	if (itExp == ctrls_.end()) {
 		LOG(IPAIPU3, Error) << "Can't find exposure control";
-		return;
+		return -EINVAL;
 	}
 
 	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
 	if (itGain == ctrls_.end()) {
 		LOG(IPAIPU3, Error) << "Can't find gain control";
-		return;
+		return -EINVAL;
 	}
 
 	const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);
 	if (itVBlank == ctrls_.end()) {
 		LOG(IPAIPU3, Error) << "Can't find VBLANK control";
-		return;
+		return -EINVAL;
 	}
 
 	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
@@ -188,6 +190,8 @@  void IPAIPU3::configure(const IPAConfigInfo &configInfo)
 
 	agcAlgo_ = std::make_unique<IPU3Agc>();
 	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
+
+	return 0;
 }
 
 void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index b986bb7035fa..87f6bac83927 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -642,7 +642,11 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 	configInfo.bdsOutputSize = config->imguConfig().bds;
 	configInfo.iif = config->imguConfig().iif;
 
-	data->ipa_->configure(configInfo);
+	ret = data->ipa_->configure(configInfo);
+	if (ret) {
+		LOG(IPU3, Error) << "Failed to configure IPA";
+		return ret;
+	}
 
 	return 0;
 }