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

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

Commit Message

Umang Jain May 19, 2021, 10:19 a.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>
---
 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

Paul Elder May 20, 2021, 2:54 a.m. UTC | #1 
On Wed, May 19, 2021 at 03:49:52PM +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: 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 a717b1e6..e78f9cbb 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/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;
>  }
> -- 
> 2.26.2
>
Jean-Michel Hautbois May 20, 2021, 8:52 a.m. UTC | #2 
Hi Umang,

Thanks for the patch !

On 19/05/2021 12:19, 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: Jean-Michel Hautbois <jeanmichel.hautbois@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 a717b1e6..e78f9cbb 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/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;
>  }
>
Jacopo Mondi May 21, 2021, 9:36 a.m. UTC | #3 
Hello,
   this all looks good, but I have a kind of unrelated question...

On Wed, May 19, 2021 at 03:49:52PM +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>

> ---
>  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(-)
>

[snip]

> --- 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);

Are we aware the if raw-only the function bails out earlier and the
ipa is never configured, right ?

Thanks
  j

>
>  	return 0;
>  }
> --
> 2.26.2
>
Kieran Bingham May 21, 2021, 10:32 a.m. UTC | #4 
Hi Jacopo,

On 21/05/2021 10:36, Jacopo Mondi wrote:
> Hello,
>    this all looks good, but I have a kind of unrelated question...
> 
> On Wed, May 19, 2021 at 03:49:52PM +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>
> 
>> ---
>>  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(-)
>>
> 
> [snip]
> 
>> --- 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);
> 
> Are we aware the if raw-only the function bails out earlier and the
> ipa is never configured, right ?

That sounds bad, but indeed an unrelated bug. If we don't have a bug
report for that could you create one?

--
Kieran


> 
> Thanks
>   j
> 
>>
>>  	return 0;
>>  }
>> --
>> 2.26.2
>>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
index a717b1e6..e78f9cbb 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/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;
 }