[libcamera-devel] [PATCH 2/4] ipa: rkisp1: Use IPAConfig in IPA::configure()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Nov 04, 2022 at 11:50:02AM +0100, Jacopo Mondi via libcamera-devel wrote:
> The RkISP1 implementation of IPA::configure() still uses the legacy
> interface where sensor controls (and eventually lens controls) are
> passed from the pipeline handler to the IPA in a map.
> 
> Since the introduction of mojom-based IPA interface definition, it is
> possible to define custom data types and use them in the interface
> definition between the pipeline handler and the IPA.
> 
> Align the RkISP1 IPA::configure() implementation with the one in the
> IPU3 IPA module by using a custom data type instead of relying on a map
> to pass controls to the IPA.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

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

> ---
>  include/libcamera/ipa/rkisp1.mojom       | 10 ++++++---
>  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++--------------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++------
>  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index eaf3955e4096..36bf291e8a51 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -8,6 +8,11 @@ module ipa.rkisp1;
>  
>  import "include/libcamera/ipa/core.mojom";
>  
> +struct IPAConfigInfo {
> +	libcamera.IPACameraSensorInfo sensorInfo;
> +	libcamera.ControlInfoMap sensorControls;
> +};
> +
>  interface IPARkISP1Interface {
>  	init(libcamera.IPASettings settings,
>  	     uint32 hwRevision)
> @@ -15,9 +20,8 @@ interface IPARkISP1Interface {
>  	start() => (int32 ret);
>  	stop();
>  
> -	configure(libcamera.IPACameraSensorInfo sensorInfo,
> -		  map<uint32, libcamera.IPAStream> streamConfig,
> -		  map<uint32, libcamera.ControlInfoMap> entityControls)
> +	configure(IPAConfigInfo configInfo,
> +		  map<uint32, libcamera.IPAStream> streamConfig)
>  		=> (int32 ret);
>  
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index c64cf380445e..e6e58feee6b0 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -53,9 +53,8 @@ public:
>  	int start() override;
>  	void stop() override;
>  
> -	int configure(const IPACameraSensorInfo &info,
> -		      const std::map<uint32_t, IPAStream> &streamConfig,
> -		      const std::map<uint32_t, ControlInfoMap> &entityControls) override;
> +	int configure(const IPAConfigInfo &ipaConfig,
> +		      const std::map<uint32_t, IPAStream> &streamConfig) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
> @@ -73,7 +72,7 @@ private:
>  	std::map<unsigned int, FrameBuffer> buffers_;
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>  
> -	ControlInfoMap ctrls_;
> +	ControlInfoMap sensorControls_;
>  
>  	/* revision-specific data */
>  	rkisp1_cif_isp_version hwRevision_;
> @@ -205,20 +204,16 @@ void IPARkISP1::stop()
>   * assemble one. Make sure the reported sensor information are relevant
>   * before accessing them.
>   */
> -int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> -			 [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
> -			 const std::map<uint32_t, ControlInfoMap> &entityControls)
> +int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> +			 [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig)
>  {
> -	if (entityControls.empty())
> -		return -EINVAL;
> -
> -	ctrls_ = entityControls.at(0);
> +	sensorControls_ = ipaConfig.sensorControls;
>  
> -	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> +	const auto itExp = sensorControls_.find(V4L2_CID_EXPOSURE);
>  	int32_t minExposure = itExp->second.min().get<int32_t>();
>  	int32_t maxExposure = itExp->second.max().get<int32_t>();
>  
> -	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> +	const auto itGain = sensorControls_.find(V4L2_CID_ANALOGUE_GAIN);
>  	int32_t minGain = itGain->second.min().get<int32_t>();
>  	int32_t maxGain = itGain->second.max().get<int32_t>();
>  
> @@ -234,7 +229,8 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  	/* Set the hardware revision for the algorithms. */
>  	context_.configuration.hw.revision = hwRevision_;
>  
> -	const ControlInfo vBlank = ctrls_.find(V4L2_CID_VBLANK)->second;
> +	const IPACameraSensorInfo &info = ipaConfig.sensorInfo;
> +	const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second;
>  	context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
>  	context_.configuration.sensor.size = info.outputSize;
>  	context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;
> @@ -350,7 +346,7 @@ void IPARkISP1::setControls(unsigned int frame)
>  	uint32_t exposure = frameContext.agc.exposure;
>  	uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);
>  
> -	ControlList ctrls(ctrls_);
> +	ControlList ctrls(sensorControls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 455ee2a0a711..e345718bd305 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -714,18 +714,18 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>  
>  	/* Inform IPA of stream configuration and sensor controls. */
> -	IPACameraSensorInfo sensorInfo = {};
> -	ret = data->sensor_->sensorInfo(&sensorInfo);
> +	ipa::rkisp1::IPAConfigInfo ipaConfig{};
> +
> +	ret = data->sensor_->sensorInfo(&ipaConfig.sensorInfo);
>  	if (ret) {
>  		/* \todo Turn this into a hard failure. */
>  		LOG(RkISP1, Warning) << "Camera sensor information not available";
> -		sensorInfo = {};
> +		ipaConfig.sensorInfo = {};
>  	}
>  
> -	std::map<uint32_t, ControlInfoMap> entityControls;
> -	entityControls.emplace(0, data->sensor_->controls());
> +	ipaConfig.sensorControls = data->sensor_->controls();
>  
> -	ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	ret = data->ipa_->configure(ipaConfig, streamConfig);
>  	if (ret) {
>  		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
>  		return ret;

-- 
Regards,

Laurent Pinchart