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

[Umang Jain]

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 at ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain 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;
>   }