[libcamera-devel] [PATCH v1 3/6] ipa: ipu3: Introduce IPAConfigInfo in IPC

[Kieran Bingham]

Hi Umang,

On 14/05/2021 08:58, Umang Jain wrote:
> IPAConfigInfo is a consolidated data structure passed from IPU3
> pipeline-handler to IPU3 IPA. The structure can be extended with
> additional parameiters to accommodate the requirements of multiple

s/parameiters/parameters/

> IPU3 IPA modules for e.g., an open-source IPA (in-tree) and a closed
> source IPA (for ChromeOS).


No need to have the expression after the e.g. It's simply to accommodate
the parameters for the IPU3 IPAs.

In fact, this isn't related to supporting multiple IPAs at all. This
simply defines the interface. The fact that we have multiple
implementations is just a fact on top of that.



> As usual, the documentation for .mojom files are kept in a separate
> .cpp file. Hence, create and document IPAConfigInfo in
> ipu3_ipa_interface.cpp.
> 
> Finally, adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  include/libcamera/ipa/ipu3.mojom             | 10 ++++-
>  include/libcamera/ipa/ipu3_ipa_interface.cpp | 39 ++++++++++++++++++++

Ok, now I really think we need to have the ipa directory under
src/libcamera/ipa as a prerequisite to this series.

Let me know if you'll do that or if you prefer me to do it.



>  include/libcamera/ipa/meson.build            |  1 +
>  src/ipa/ipu3/ipu3.cpp                        | 14 +++----
>  src/libcamera/pipeline/ipu3/ipu3.cpp         | 10 +++--
>  5 files changed, 61 insertions(+), 13 deletions(-)
>  create mode 100644 include/libcamera/ipa/ipu3_ipa_interface.cpp
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index a717b1e6..22c4ce1b 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -25,13 +25,19 @@ struct IPU3Action {
>  	libcamera.ControlList controls;
>  };
>  
> +struct IPAConfigInfo {
> +	libcamera.IPACameraSensorInfo sensorInfo;
> +	map<uint32, ControlInfoMap> entityControls;
> +	libcamera.Size bdsOutputSize;
> +	libcamera.Size iif;
> +};
> +
>  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/include/libcamera/ipa/ipu3_ipa_interface.cpp b/include/libcamera/ipa/ipu3_ipa_interface.cpp
> new file mode 100644
> index 00000000..9aafbcdb
> --- /dev/null
> +++ b/include/libcamera/ipa/ipu3_ipa_interface.cpp
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +namespace libcamera {
> +
> +/**
> + * \struct IPAConfigInfo
> + * \brief Information to be passed from IPU3 pipeline-handler to the 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
> + *
> + * \sa IPACameraSensorInfo
> + */
> +
> +/**
> + * \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
> + */
> +} /* namespace libcamera */
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index da60aa68..9684a34f 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -10,6 +10,7 @@ libcamera_ipa_headers = files([
>  
>  libcamera_ipa_docs = files([
>      'core_ipa_interface.cpp',
> +    'ipu3_ipa_interface.cpp',

And of course then this would get added to the libcamera sources as part
of src/libcamera/ipa/meson.build which would then get passed into doxygen.


>  ])
>  
>  install_headers(libcamera_ipa_headers,
> 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 98c6160f..5b15ca90 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -633,9 +633,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>  	}
>  
> -	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.entityControls.emplace(0, data->cio2_.sensor()->controls());
> +	configInfo.sensorInfo = sensorInfo;
> +	configInfo.bdsOutputSize = config->imguConfig().bds;
> +	configInfo.iif = config->imguConfig().iif;
> +
> +	data->ipa_->configure(configInfo);

Otherwise, the code move looks ok to me.

>  
>  	return 0;
>  }
> 

-- 
Regards
--
Kieran