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

Message ID 20210521132823.322076-6-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • External IPU3 IPA Support
Related show

Commit Message

Umang Jain May 21, 2021, 1:28 p.m. UTC 
IPAConfigInfo is a consolidated data structure passed from IPU3
pipeline-handler to IPU3 IPA. The structure can be extended with
additional parameters to accommodate the requirements of multiple
IPU3 IPA modules.

Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
 src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
 src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
 3 files changed, 21 insertions(+), 13 deletions(-)

Comments

Laurent Pinchart May 24, 2021, 1 a.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Fri, May 21, 2021 at 06:58:22PM +0530, Umang Jain wrote:
> IPAConfigInfo is a consolidated data structure passed from IPU3
> pipeline-handler to IPU3 IPA. The structure can be extended with
> additional parameters to accommodate the requirements of multiple
> IPU3 IPA modules.
> 
> Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
>  src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 9e3cd885..6b6b431f 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -30,13 +30,19 @@ struct IPU3Action {
>  	libcamera.ControlList controls;
>  };
>  
> +struct IPAConfigInfo {
> +	libcamera.IPACameraSensorInfo sensorInfo;
> +	map<uint32, ControlInfoMap> entityControls;

Should this be libcamera.ControlInfoMap ? Paul, what's the rule for the
'libcamera.' namespace prefix ?

With this address (if needed),

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

> +	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/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;
>  }
Paul Elder May 24, 2021, 5:43 a.m. UTC | #2 
Hi Umang and Laurent,

On Mon, May 24, 2021 at 04:00:01AM +0300, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Fri, May 21, 2021 at 06:58:22PM +0530, Umang Jain wrote:
> > IPAConfigInfo is a consolidated data structure passed from IPU3
> > pipeline-handler to IPU3 IPA. The structure can be extended with
> > additional parameters to accommodate the requirements of multiple
> > IPU3 IPA modules.
> > 
> > Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
> >  src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
> >  3 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > index 9e3cd885..6b6b431f 100644
> > --- a/include/libcamera/ipa/ipu3.mojom
> > +++ b/include/libcamera/ipa/ipu3.mojom
> > @@ -30,13 +30,19 @@ struct IPU3Action {
> >  	libcamera.ControlList controls;
> >  };
> >  
> > +struct IPAConfigInfo {
> > +	libcamera.IPACameraSensorInfo sensorInfo;
> > +	map<uint32, ControlInfoMap> entityControls;
> 
> Should this be libcamera.ControlInfoMap ? Paul, what's the rule for the
> 'libcamera.' namespace prefix ?

Here it's optional.

The code generator doesn't actually need the libcamera prefix anywhere
(because all generated files are in the libcamera namespace anyway), but
the mojo parser needs it for top-level types.


Paul

> 
> With this address (if needed),
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +	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/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;
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart May 24, 2021, 8:36 p.m. UTC | #3 
Hi Paul,

On Mon, May 24, 2021 at 02:43:54PM +0900, paul.elder@ideasonboard.com wrote:
> On Mon, May 24, 2021 at 04:00:01AM +0300, Laurent Pinchart wrote:
> > On Fri, May 21, 2021 at 06:58:22PM +0530, Umang Jain wrote:
> > > IPAConfigInfo is a consolidated data structure passed from IPU3
> > > pipeline-handler to IPU3 IPA. The structure can be extended with
> > > additional parameters to accommodate the requirements of multiple
> > > IPU3 IPA modules.
> > > 
> > > Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
> > > 
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
> > >  src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
> > >  3 files changed, 21 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > > index 9e3cd885..6b6b431f 100644
> > > --- a/include/libcamera/ipa/ipu3.mojom
> > > +++ b/include/libcamera/ipa/ipu3.mojom
> > > @@ -30,13 +30,19 @@ struct IPU3Action {
> > >  	libcamera.ControlList controls;
> > >  };
> > >  
> > > +struct IPAConfigInfo {
> > > +	libcamera.IPACameraSensorInfo sensorInfo;
> > > +	map<uint32, ControlInfoMap> entityControls;
> > 
> > Should this be libcamera.ControlInfoMap ? Paul, what's the rule for the
> > 'libcamera.' namespace prefix ?
> 
> Here it's optional.
> 
> The code generator doesn't actually need the libcamera prefix anywhere
> (because all generated files are in the libcamera namespace anyway), but
> the mojo parser needs it for top-level types.

What do you mean by "top-level types" ?

> > With this address (if needed),
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > +	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/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;
> > >  }
Paul Elder May 25, 2021, 9:09 a.m. UTC | #4 
Hi Laurent,

On Mon, May 24, 2021 at 11:36:14PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Mon, May 24, 2021 at 02:43:54PM +0900, paul.elder@ideasonboard.com wrote:
> > On Mon, May 24, 2021 at 04:00:01AM +0300, Laurent Pinchart wrote:
> > > On Fri, May 21, 2021 at 06:58:22PM +0530, Umang Jain wrote:
> > > > IPAConfigInfo is a consolidated data structure passed from IPU3
> > > > pipeline-handler to IPU3 IPA. The structure can be extended with
> > > > additional parameters to accommodate the requirements of multiple
> > > > IPU3 IPA modules.
> > > > 
> > > > Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
> > > > 
> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
> > > >  src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
> > > >  3 files changed, 21 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > > > index 9e3cd885..6b6b431f 100644
> > > > --- a/include/libcamera/ipa/ipu3.mojom
> > > > +++ b/include/libcamera/ipa/ipu3.mojom
> > > > @@ -30,13 +30,19 @@ struct IPU3Action {
> > > >  	libcamera.ControlList controls;
> > > >  };
> > > >  
> > > > +struct IPAConfigInfo {
> > > > +	libcamera.IPACameraSensorInfo sensorInfo;
> > > > +	map<uint32, ControlInfoMap> entityControls;
> > > 
> > > Should this be libcamera.ControlInfoMap ? Paul, what's the rule for the
> > > 'libcamera.' namespace prefix ?
> > 
> > Here it's optional.
> > 
> > The code generator doesn't actually need the libcamera prefix anywhere
> > (because all generated files are in the libcamera namespace anyway), but
> > the mojo parser needs it for top-level types.
> 
> What do you mean by "top-level types" ?

Types that are parameters of functions or that are members of structs.


Paul

> 
> > > With this address (if needed),
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > > +	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/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;
> > > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Umang Jain May 25, 2021, 2:56 p.m. UTC | #5 
Hi Paul,

On 5/25/21 2:39 PM, paul.elder@ideasonboard.com wrote:
> Hi Laurent,
>
> On Mon, May 24, 2021 at 11:36:14PM +0300, Laurent Pinchart wrote:
>> Hi Paul,
>>
>> On Mon, May 24, 2021 at 02:43:54PM +0900, paul.elder@ideasonboard.com wrote:
>>> On Mon, May 24, 2021 at 04:00:01AM +0300, Laurent Pinchart wrote:
>>>> On Fri, May 21, 2021 at 06:58:22PM +0530, Umang Jain wrote:
>>>>> IPAConfigInfo is a consolidated data structure passed from IPU3
>>>>> pipeline-handler to IPU3 IPA. The structure can be extended with
>>>>> additional parameters to accommodate the requirements of multiple
>>>>> IPU3 IPA modules.
>>>>>
>>>>> Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
>>>>>
>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>>>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>>>>> ---
>>>>>   include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
>>>>>   src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
>>>>>   src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
>>>>>   3 files changed, 21 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>>>>> index 9e3cd885..6b6b431f 100644
>>>>> --- a/include/libcamera/ipa/ipu3.mojom
>>>>> +++ b/include/libcamera/ipa/ipu3.mojom
>>>>> @@ -30,13 +30,19 @@ struct IPU3Action {
>>>>>   	libcamera.ControlList controls;
>>>>>   };
>>>>>   
>>>>> +struct IPAConfigInfo {
>>>>> +	libcamera.IPACameraSensorInfo sensorInfo;
>>>>> +	map<uint32, ControlInfoMap> entityControls;
>>>> Should this be libcamera.ControlInfoMap ? Paul, what's the rule for the
>>>> 'libcamera.' namespace prefix ?
>>> Here it's optional.
>>>
>>> The code generator doesn't actually need the libcamera prefix anywhere
>>> (because all generated files are in the libcamera namespace anyway), but
>>> the mojo parser needs it for top-level types.
>> What do you mean by "top-level types" ?
> Types that are parameters of functions or that are members of structs.
Omitting `libcamera.` for entityControls introduced a FATAL breakage 
while running the IPA from master:

[0:52:11.842213069] [7830] FATAL IPADataSerializer 
ipa_data_serializer.cpp:437 ControlSerializer not provided for 
serialization of ControlInfoMap

Weird that it never was seen before by me (or Kieran (assuming that 
these patches were applied to his tree in development for testing Intel 
IPA))  :-/

The below diff fixes the FATAL error:

diff --git a/include/libcamera/ipa/ipu3.mojom 
b/include/libcamera/ipa/ipu3.mojom
index 6b6b431f..32c046ad 100644
--- a/include/libcamera/ipa/ipu3.mojom
+++ b/include/libcamera/ipa/ipu3.mojom
@@ -32,7 +32,7 @@ struct IPU3Action {

  struct IPAConfigInfo {
         libcamera.IPACameraSensorInfo sensorInfo;
-       map<uint32, ControlInfoMap> entityControls;
+       map<uint32, libcamera.ControlInfoMap> entityControls;
         libcamera.Size bdsOutputSize;
         libcamera.Size iif;


>
>
> Paul
>
>>>> With this address (if needed),
>>>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>>> +	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/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;
>>>>>   }
>> -- 
>> Regards,
>>
>> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
index 9e3cd885..6b6b431f 100644
--- a/include/libcamera/ipa/ipu3.mojom
+++ b/include/libcamera/ipa/ipu3.mojom
@@ -30,13 +30,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/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;
 }