[libcamera-devel] [PATCH v2 2/3] src: rkisp1: return error from ipa's configure method if it fails

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Thu, Feb 25, 2021 at 06:12:04PM +0100, Dafna Hirschfeld wrote:
> The IPA of rkisp1 relay on some of the camera's controls.

s/relay/relies/

> Therefore it can't work if those controls are not given.
> Return -EINVAL from 'configure' in that case.
> Also return error from the pipeline's 'configure' method
> if the IPA configure fails.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

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

> ---
>  include/libcamera/ipa/rkisp1.mojom       |  2 +-
>  src/ipa/rkisp1/rkisp1.cpp                | 11 ++++++-----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 +++++--
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 9270f9c7..95fa0d93 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -31,7 +31,7 @@ interface IPARkISP1Interface {
>  
>  	configure(CameraSensorInfo sensorInfo,
>  		  map<uint32, IPAStream> streamConfig,
> -		  map<uint32, ControlInfoMap> entityControls) => ();
> +		  map<uint32, ControlInfoMap> entityControls) => (int32 ret);
>  
>  	mapBuffers(array<IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 67bac986..70e8d8f7 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -38,7 +38,7 @@ public:
>  	int start() override { return 0; }
>  	void stop() override {}
>  
> -	void configure(const CameraSensorInfo &info,
> +	int configure(const CameraSensorInfo &info,
>  		       const std::map<uint32_t, IPAStream> &streamConfig,
>  		       const std::map<uint32_t, ControlInfoMap> &entityControls) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> @@ -75,25 +75,25 @@ private:
>   * assemble one. Make sure the reported sensor information are relevant
>   * before accessing them.
>   */
> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>  			  [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
>  			  const std::map<uint32_t, ControlInfoMap> &entityControls)
>  {
>  	if (entityControls.empty())
> -		return;
> +		return -EINVAL;
>  
>  	ctrls_ = entityControls.at(0);
>  
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>  	if (itExp == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find exposure control";
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>  	if (itGain == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find gain control";
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	autoExposure_ = true;
> @@ -111,6 +111,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>  		<< " Gain: " << minGain_ << "-" << maxGain_;
>  
>  	setControls(0);
> +	return 0;
>  }
>  
>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 538c0139..34814f62 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -653,8 +653,11 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	std::map<uint32_t, ControlInfoMap> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>  
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> -
> +	ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	if (ret) {
> +		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> +		return ret;
> +	}
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart