[libcamera-devel,RFC,1/5] ipa: ipu3: Introduce a Context structure
diff mbox series

Message ID 20210809092007.79067-2-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • IPU3: Introduce modularity for algorithms
Related show

Commit Message

Jean-Michel Hautbois Aug. 9, 2021, 9:20 a.m. UTC 
Passing parameters, statistics and all specific data each algorithm shares.
As the ipu3_uapi_params are part of the IPA context referenced by the
algorithms, move the structure into the IPAContext directly and adapt existing
algorithm implementations.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipa_context.h | 30 ++++++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3.cpp      | 16 +++++++++-------
 2 files changed, 39 insertions(+), 7 deletions(-)
 create mode 100644 src/ipa/ipu3/ipa_context.h

Comments

Kieran Bingham Aug. 9, 2021, 9:31 a.m. UTC | #1 
Hi JM,

On 09/08/2021 10:20, Jean-Michel Hautbois wrote:
> Passing parameters, statistics and all specific data each algorithm shares.

We're introducing something new, so we should describe the need first.

"""
An increasing amount of data and information needs to be shared between
the components that build up to implement image processing algorithms.

Create a context structure which will allow us to work towards calling
algorithms in a modular way, and sharing information between the modules.
"""


> As the ipu3_uapi_params are part of the IPA context referenced by the
> algorithms, move the structure into the IPAContext directly and adapt existing
> algorithm implementations.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.h | 30 ++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3.cpp      | 16 +++++++++-------
>  2 files changed, 39 insertions(+), 7 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipa_context.h
> 
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> new file mode 100644
> index 00000000..5d717be5
> --- /dev/null
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_ipa_context.h - IPU3 IPA Context
> + *
> + * Context information shared between the algorithms
> + */
> +#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3 {
> +
> +struct IPAContext {
> +	/* Input statistics from the previous frame */
> +	const ipu3_uapi_stats_3a *stats;

It looks like we don't use this yet in this patch.

Given that we also question in patch 2/5 if this should be used in here,
I think it should be introduced there too.


Other than that,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
> +	/* Output Parameters which will be written to the hardware */
> +	ipu3_uapi_params params;
> +};
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera*/
> +
> +#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36..a714af85 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -22,6 +22,8 @@
>  
>  #include "libcamera/internal/framebuffer.h"
>  
> +#include "ipa_context.h"
> +
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
>  #include "libipa/camera_sensor_helper.h"
> @@ -81,9 +83,8 @@ private:
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
>  	/* Local parameter storage */
> -	struct ipu3_uapi_params params_;
> -
>  	struct ipu3_uapi_grid_config bdsGrid_;
> +	struct IPAContext context_;
>  };
>  
>  int IPAIPU3::init(const IPASettings &settings)
> @@ -94,6 +95,9 @@ int IPAIPU3::init(const IPASettings &settings)
>  		return -ENODEV;
>  	}
>  
> +	/* Reset all the hardware settings */
> +	context_.params = {};
> +
>  	return 0;
>  }
>  
> @@ -193,12 +197,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  
>  	defVBlank_ = itVBlank->second.def().get<int32_t>();
>  
> -	params_ = {};
> -
>  	calculateBdsGrid(configInfo.bdsOutputSize);
>  
>  	awbAlgo_ = std::make_unique<IPU3Awb>();
> -	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> +	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
>  	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> @@ -276,9 +278,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
>  	if (agcAlgo_->updateControls())
> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> +		awbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());
>  
> -	*params = params_;
> +	*params = context_.params;
>  
>  	IPU3Action op;
>  	op.op = ActionParamFilled;
>
Umang Jain Aug. 9, 2021, 9:35 a.m. UTC | #2 
Hi JM,

On 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:
> Passing parameters, statistics and all specific data each algorithm shares.
I would re-phrase this sentence to sound more complete.
> As the ipu3_uapi_params are part of the IPA context referenced by the
> algorithms, move the structure into the IPAContext directly and adapt existing
> algorithm implementations.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>   src/ipa/ipu3/ipa_context.h | 30 ++++++++++++++++++++++++++++++
>   src/ipa/ipu3/ipu3.cpp      | 16 +++++++++-------
>   2 files changed, 39 insertions(+), 7 deletions(-)
>   create mode 100644 src/ipa/ipu3/ipa_context.h
>
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> new file mode 100644
> index 00000000..5d717be5
> --- /dev/null
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_ipa_context.h - IPU3 IPA Context
> + *
> + * Context information shared between the algorithms
> + */
> +#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3 {
> +
> +struct IPAContext {
> +	/* Input statistics from the previous frame */
> +	const ipu3_uapi_stats_3a *stats;

Is there used somewhere in the series? If yes, I suggest moving this to 
the patch where this is used, as I can't see in this patch.

With that addressed,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +
> +	/* Output Parameters which will be written to the hardware */
> +	ipu3_uapi_params params;
> +};
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera*/
> +
> +#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36..a714af85 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -22,6 +22,8 @@
>   
>   #include "libcamera/internal/framebuffer.h"
>   
> +#include "ipa_context.h"
> +
>   #include "ipu3_agc.h"
>   #include "ipu3_awb.h"
>   #include "libipa/camera_sensor_helper.h"
> @@ -81,9 +83,8 @@ private:
>   	std::unique_ptr<CameraSensorHelper> camHelper_;
>   
>   	/* Local parameter storage */
> -	struct ipu3_uapi_params params_;
> -
>   	struct ipu3_uapi_grid_config bdsGrid_;
> +	struct IPAContext context_;
>   };
>   
>   int IPAIPU3::init(const IPASettings &settings)
> @@ -94,6 +95,9 @@ int IPAIPU3::init(const IPASettings &settings)
>   		return -ENODEV;
>   	}
>   
> +	/* Reset all the hardware settings */
> +	context_.params = {};
> +
>   	return 0;
>   }
>   
> @@ -193,12 +197,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>   
>   	defVBlank_ = itVBlank->second.def().get<int32_t>();
>   
> -	params_ = {};
> -
>   	calculateBdsGrid(configInfo.bdsOutputSize);
>   
>   	awbAlgo_ = std::make_unique<IPU3Awb>();
> -	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> +	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
>   
>   	agcAlgo_ = std::make_unique<IPU3Agc>();
>   	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> @@ -276,9 +278,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>   void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>   {
>   	if (agcAlgo_->updateControls())
> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> +		awbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());
>   
> -	*params = params_;
> +	*params = context_.params;
>   
>   	IPU3Action op;
>   	op.op = ActionParamFilled;

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
new file mode 100644
index 00000000..5d717be5
--- /dev/null
+++ b/src/ipa/ipu3/ipa_context.h
@@ -0,0 +1,30 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Ideas On Board
+ *
+ * ipu3_ipa_context.h - IPU3 IPA Context
+ *
+ * Context information shared between the algorithms
+ */
+#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__
+#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__
+
+#include <linux/intel-ipu3.h>
+
+namespace libcamera {
+
+namespace ipa::ipu3 {
+
+struct IPAContext {
+	/* Input statistics from the previous frame */
+	const ipu3_uapi_stats_3a *stats;
+
+	/* Output Parameters which will be written to the hardware */
+	ipu3_uapi_params params;
+};
+
+} /* namespace ipa::ipu3 */
+
+} /* namespace libcamera*/
+
+#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 71698d36..a714af85 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -22,6 +22,8 @@ 
 
 #include "libcamera/internal/framebuffer.h"
 
+#include "ipa_context.h"
+
 #include "ipu3_agc.h"
 #include "ipu3_awb.h"
 #include "libipa/camera_sensor_helper.h"
@@ -81,9 +83,8 @@  private:
 	std::unique_ptr<CameraSensorHelper> camHelper_;
 
 	/* Local parameter storage */
-	struct ipu3_uapi_params params_;
-
 	struct ipu3_uapi_grid_config bdsGrid_;
+	struct IPAContext context_;
 };
 
 int IPAIPU3::init(const IPASettings &settings)
@@ -94,6 +95,9 @@  int IPAIPU3::init(const IPASettings &settings)
 		return -ENODEV;
 	}
 
+	/* Reset all the hardware settings */
+	context_.params = {};
+
 	return 0;
 }
 
@@ -193,12 +197,10 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
 
 	defVBlank_ = itVBlank->second.def().get<int32_t>();
 
-	params_ = {};
-
 	calculateBdsGrid(configInfo.bdsOutputSize);
 
 	awbAlgo_ = std::make_unique<IPU3Awb>();
-	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
+	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
 
 	agcAlgo_ = std::make_unique<IPU3Agc>();
 	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
@@ -276,9 +278,9 @@  void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
 void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
 {
 	if (agcAlgo_->updateControls())
-		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
+		awbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());
 
-	*params = params_;
+	*params = context_.params;
 
 	IPU3Action op;
 	op.op = ActionParamFilled;