[libcamera-devel,v1,3/6] ipa: ipu3: Introduce IPAConfigInfo in IPC
diff mbox series

Message ID 20210514075808.282479-4-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • External IPU3 IPA support
Related show

Commit Message

Umang Jain May 14, 2021, 7:58 a.m. UTC 
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
IPU3 IPA modules for e.g., an open-source IPA (in-tree) and a closed
source IPA (for ChromeOS).

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@ideasonboard.com>
---
 include/libcamera/ipa/ipu3.mojom             | 10 ++++-
 include/libcamera/ipa/ipu3_ipa_interface.cpp | 39 ++++++++++++++++++++
 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

Comments

Kieran Bingham May 14, 2021, 9:57 a.m. UTC | #1 
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@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;
>  }
>
Laurent Pinchart May 15, 2021, 4:30 p.m. UTC | #2 
Hi Kieran,

On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> 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@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.

I'm beginning to wonder if we should write a simple python script that
would copy comments from a mojom file to a cpp file, and keep the
documentation in the .mojom file. What do you think ?

> >  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;
> >  }
Paul Elder May 18, 2021, 9:43 a.m. UTC | #3 
Hi Laurent,

On Sat, May 15, 2021 at 07:30:13PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> > 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@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.
> 
> I'm beginning to wonder if we should write a simple python script that
> would copy comments from a mojom file to a cpp file, and keep the
> documentation in the .mojom file. What do you think ?

tbh that's what I wanted to do, but I thought that was over-engineering
it. Maybe it's worth it to stop the clutter from docs-only cpp files?


Paul

> 
> > >  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;
> > >  }
Laurent Pinchart May 18, 2021, 9:47 a.m. UTC | #4 
Hi Paul,

On Tue, May 18, 2021 at 06:43:32PM +0900, paul.elder@ideasonboard.com wrote:
> On Sat, May 15, 2021 at 07:30:13PM +0300, Laurent Pinchart wrote:
> > On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> > > 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@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.
> > 
> > I'm beginning to wonder if we should write a simple python script that
> > would copy comments from a mojom file to a cpp file, and keep the
> > documentation in the .mojom file. What do you think ?
> 
> tbh that's what I wanted to do, but I thought that was over-engineering
> it. Maybe it's worth it to stop the clutter from docs-only cpp files?

Can the mojo parser help there, could it be leveraged to extract
comments, or does it ignore them completely ?

> > > >  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;
> > > >  }
Paul Elder May 18, 2021, 10:06 a.m. UTC | #5 
Hi Laurent,

On Tue, May 18, 2021 at 12:47:24PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Tue, May 18, 2021 at 06:43:32PM +0900, paul.elder@ideasonboard.com wrote:
> > On Sat, May 15, 2021 at 07:30:13PM +0300, Laurent Pinchart wrote:
> > > On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> > > > 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@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.
> > > 
> > > I'm beginning to wonder if we should write a simple python script that
> > > would copy comments from a mojom file to a cpp file, and keep the
> > > documentation in the .mojom file. What do you think ?
> > 
> > tbh that's what I wanted to do, but I thought that was over-engineering
> > it. Maybe it's worth it to stop the clutter from docs-only cpp files?
> 
> Can the mojo parser help there, could it be leveraged to extract
> comments, or does it ignore them completely ?

That's why I didn't pursue that route; the mojo parser ignores comments
completely :/

Actually it's before that, they're dropped by the lexer.


Paul

<snip>
Laurent Pinchart May 18, 2021, 10:27 a.m. UTC | #6 
Hi Paul,

On Tue, May 18, 2021 at 07:06:40PM +0900, paul.elder@ideasonboard.com wrote:
> On Tue, May 18, 2021 at 12:47:24PM +0300, Laurent Pinchart wrote:
> > On Tue, May 18, 2021 at 06:43:32PM +0900, paul.elder@ideasonboard.com wrote:
> > > On Sat, May 15, 2021 at 07:30:13PM +0300, Laurent Pinchart wrote:
> > > > On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> > > > > 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@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.
> > > > 
> > > > I'm beginning to wonder if we should write a simple python script that
> > > > would copy comments from a mojom file to a cpp file, and keep the
> > > > documentation in the .mojom file. What do you think ?
> > > 
> > > tbh that's what I wanted to do, but I thought that was over-engineering
> > > it. Maybe it's worth it to stop the clutter from docs-only cpp files?
> > 
> > Can the mojo parser help there, could it be leveraged to extract
> > comments, or does it ignore them completely ?
> 
> That's why I didn't pursue that route; the mojo parser ignores comments
> completely :/
> 
> Actually it's before that, they're dropped by the lexer.

Let's not go down that rabbit hole then. Should you however file a bug
report against mojom to ask for this feature ? If you think it's a good
idea, could you do so ?
Paul Elder May 19, 2021, 2:51 a.m. UTC | #7 
Hi Laurent,

On Tue, May 18, 2021 at 01:27:00PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Tue, May 18, 2021 at 07:06:40PM +0900, paul.elder@ideasonboard.com wrote:
> > On Tue, May 18, 2021 at 12:47:24PM +0300, Laurent Pinchart wrote:
> > > On Tue, May 18, 2021 at 06:43:32PM +0900, paul.elder@ideasonboard.com wrote:
> > > > On Sat, May 15, 2021 at 07:30:13PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> > > > > > 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@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.
> > > > > 
> > > > > I'm beginning to wonder if we should write a simple python script that
> > > > > would copy comments from a mojom file to a cpp file, and keep the
> > > > > documentation in the .mojom file. What do you think ?
> > > > 
> > > > tbh that's what I wanted to do, but I thought that was over-engineering
> > > > it. Maybe it's worth it to stop the clutter from docs-only cpp files?
> > > 
> > > Can the mojo parser help there, could it be leveraged to extract
> > > comments, or does it ignore them completely ?
> > 
> > That's why I didn't pursue that route; the mojo parser ignores comments
> > completely :/
> > 
> > Actually it's before that, they're dropped by the lexer.
> 
> Let's not go down that rabbit hole then. Should you however file a bug
> report against mojom to ask for this feature ? If you think it's a good
> idea, could you do so ?

No, I don't think it's a good idea. The job of the lexer is to turn the
sequence of characters into a sequence of tokens to feed to the parser,
and comments don't belong as a token.

It's probably better just to have a simple python script to extract the
comments.


Paul

Patch
diff mbox series 

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',
 ])
 
 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);
 
 	return 0;
 }