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

[Jean-Michel Hautbois]

Hi Umang,

Thanks for the patch !

On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and
> open-source IPU3 IPAs. Hence, the structure encompasses additional
> configuration information required to init and configure the
> closed-source IPA as well.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> Changes in v2:
> - Mark as RFC(do not merge) as this is built upon Paul's
>   [Fix support for core.mojom structs v3] - waiting for merge to master
> - Drop cropRegion_  from structure - this is provided from CameraSensorInfo
>   itself.
> - Doxygen documentation of IPAConfigInfo.
> ---
>  include/libcamera/ipa/ipu3.mojom     | 46 ++++++++++++++++++++++++++--
>  src/ipa/ipu3/ipu3.cpp                | 14 ++++-----
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  9 +++++-
>  3 files changed, 58 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index a717b1e6..acd5cfa4 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -25,13 +25,55 @@ struct IPU3Action {
>  	libcamera.ControlList controls;
>  };
>  
> +/**
> + * \struct IPAConfigInfo
> + * \brief Information to be passed from IPU3 pipeline-handler to IPA
> + *
> + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler
> + * to the IPAIPU3Interface::configure(). The structure provides extensibility
> + * for additional configuration data as required, for closed-source or
> + * open-source IPAs' configure() phases.
> + */
> +
> +/**
> + * \var IPAConfigInfo::sensorInfo
> + * \brief Camera sensor information
> + *
> + * Provides the camera sensor information such as model name, pixel-rate,
> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further
> + * details.
> + */
> +
> + /**
> + * \var IPAConfigInfo::entityControls
> + * \brief Controls supported by the sensor
> + *
> + * A map of V4L2 controls supported by the sensor in order to read or write
> + * control values to them.
> + */
> +
> + /**
> + * \var IPAConfigInfo::bdsOutputSize
> + * \brief Size of ImgU BDS rectangle
> + */
> +
> + /**
> + * \var IPAConfigInfo::iif
> + * \brief Size of ImgU input-feeder rectangle
> + */
> +struct IPAConfigInfo {
> +	libcamera.CameraSensorInfo sensorInfo;
> +	map<uint32, ControlInfoMap> entityControls;
> +	libcamera.Size bdsOutputSize;
> +	libcamera.Size iif;
> +};
> +

I think it is interesting, and would like your advice: I would need this
same structure for rkisp1 platform, except that the sizes are obviously
not the same.
And so, I am wondering if we could imagine a more general structure like:
struct IPAConfigInfo {
	libcamera.CameraSensorInfo sensorInfo;
	map<uint32, ControlInfoMap> entityControls;
	libcamera.Size ispInputSize;
	libcamera.Size ispOutputSize;
};

names and all may have to be reworked but that is the base idea.
There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least
you have an input an output pad size for the ISP node.

>  interface IPAIPU3Interface {
>  	init(libcamera.IPASettings settings) => (int32 ret);
>  	start() => (int32 ret);
>  	stop();
>  
> -	configure(map<uint32, libcamera.ControlInfoMap> entityControls,
> -		  libcamera.Size bdsOutputSize) => ();
> +	configure(IPAConfigInfo configInfo);
>  
>  	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..769c24d3 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 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 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..b1ff1dbe 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -635,7 +635,14 @@ 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);
> +
> +	ipa::ipu3::IPAConfigInfo configInfo;
> +	configInfo.sensorInfo = sensorInfo;
> +	configInfo.entityControls = entityControls;
> +	configInfo.bdsOutputSize = config->imguConfig().bds;
> +	configInfo.iif = config->imguConfig().iif;
> +
> +	data->ipa_->configure(configInfo);
>  
>  	return 0;
>  }
>