[libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in IPC

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Mon, Apr 26, 2021 at 10:46:08PM +0530, Umang Jain wrote:
> IPAConfigInfo is a consolidated data structure passed from IPU3
> pipeline-handler to IPU3 IPA. This provides a streamlined and
> extensible way to provide the configuration data to IPA interface.

The commit message should mention that you're adding more data to the
structure, and why.

> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  include/libcamera/ipa/ipu3.mojom     | 11 +++++++++--
>  src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  7 ++++++-
>  3 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index a717b1e6..88cbb403 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -25,13 +25,20 @@ struct IPU3Action {
>  	libcamera.ControlList controls;
>  };
>  
> +struct IPAConfigInfo {
> +	libcamera.CameraSensorInfo sensorInfo;
> +	map<uint32, ControlInfoMap> entityControls;
> +	libcamera.Size bdsOutputSize;
> +	libcamera.Size iif;
> +	libcamera.Size cropRegion;
> +};

Could you document this structure with doxygen comments ? We don't
generate documentation from mojom files yet, but we should still be
prepared.

> +
>  interface IPAIPU3Interface {
>  	init(libcamera.IPASettings settings) => (int32 ret);
>  	start() => (int32 ret);
>  	stop();
>  
> -	configure(map<uint32, libcamera.ControlInfoMap> entityControls,
> -		  libcamera.Size bdsOutputSize) => ();
> +	configure(IPAConfigInfo configInfo) => ();

While at it, I think you can drop the => ().

>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index f5343547..a77afd55 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -43,8 +43,7 @@ public:
>  	int start() override;
>  	void stop() override {}
>  
> -	void configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> -		       const Size &bdsOutputSize) override;
> +	void configure(const struct IPAConfigInfo &configInfo) override;
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
>  }
>  
> -void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> -			const Size &bdsOutputSize)
> +void IPAIPU3::configure(const struct IPAConfigInfo &configInfo)
>  {
> -	if (entityControls.empty())
> +	if (configInfo.entityControls.empty())
>  		return;
>  
> -	ctrls_ = entityControls.at(0);
> +	ctrls_ = configInfo.entityControls.at(0);
>  
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>  	if (itExp == ctrls_.end()) {
> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>  
>  	params_ = {};
>  
> -	calculateBdsGrid(bdsOutputSize);
> +	calculateBdsGrid(configInfo.bdsOutputSize);
>  
>  	awbAlgo_ = std::make_unique<IPU3Awb>();
> -	awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
> +	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
>  	agcAlgo_->initialise(bdsGrid_);
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 73306cea..be43d5a1 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -635,7 +635,12 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  
>  	std::map<uint32_t, ControlInfoMap> entityControls;
>  	entityControls.emplace(0, data->cio2_.sensor()->controls());
> -	data->ipa_->configure(entityControls, config->imguConfig().bds);
> +
> +	struct ipa::ipu3::IPAConfigInfo configInfo = {

You can drop the struct keyword.

> +		sensorInfo, entityControls, config->imguConfig().bds,
> +		config->imguConfig().iif, data->cropRegion_.size()
> +	};

Given that several members of this structures have the same type, it
would be safer to use named initializers.

	ipa::ipu3::IPAConfigInfo configInfo = {
		.sensorInfo = sensorInfo,
		.entityControls = entityControls,
		.bdsOutputSize = config->imguConfig().bds,
		.iif = config->imguConfig().iif,
		.cropRegion = data->cropRegion_.size(),
	};

> +	data->ipa_->configure(configInfo);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart