[libcamera-devel] [PATCH] ipa: ipu3: Support return values from configure()

[paul.elder at ideasonboard.com]

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 at ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder at 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
>