[libcamera-devel] [PATCH v3 5/6] ipa: ipu3: Introduce IPAConfigInfo in IPC

[paul.elder at ideasonboard.com]

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 at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder at 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 at 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