[libcamera-devel,RFC,2/5] ipa: ipu3: Introduce modular algorithms
diff mbox series

Message ID 20210809092007.79067-3-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 
Provide a modular IPU3 specific algorithm, and frame context
structure so that algorithms can be built for the IPU3 in a
component based structure.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++
 src/ipa/ipu3/algorithms/meson.build |  4 +++
 src/ipa/ipu3/ipu3.cpp               | 47 +++++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3_agc.h             |  3 +-
 src/ipa/ipu3/ipu3_awb.h             |  3 +-
 src/ipa/ipu3/meson.build            |  4 +++
 6 files changed, 89 insertions(+), 2 deletions(-)
 create mode 100644 src/ipa/ipu3/algorithms/algorithm.h
 create mode 100644 src/ipa/ipu3/algorithms/meson.build

Comments

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

On 09/08/2021 10:20, Jean-Michel Hautbois wrote:
> Provide a modular IPU3 specific algorithm, and frame context

"s/and frame context structure/and make use of the IPAContext structure/"

> structure so that algorithms can be built for the IPU3 in a
> component based structure.
>


"""
The existing AWB and AGC algorithm's are moved to explicitly reference
the libipa ipa::Algorithm namespaced class and are converted separately.
"""

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++
>  src/ipa/ipu3/algorithms/meson.build |  4 +++
>  src/ipa/ipu3/ipu3.cpp               | 47 +++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3_agc.h             |  3 +-
>  src/ipa/ipu3/ipu3_awb.h             |  3 +-
>  src/ipa/ipu3/meson.build            |  4 +++
>  6 files changed, 89 insertions(+), 2 deletions(-)
>  create mode 100644 src/ipa/ipu3/algorithms/algorithm.h
>  create mode 100644 src/ipa/ipu3/algorithms/meson.build
> 
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> new file mode 100644
> index 00000000..bac82b7c
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * algorithm.h - IPU3 control algorithm interface
> + */
> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> +
> +#include <ipa_context.h>

I'm pretty sure I added this line, but it should be in "" not <>

But we might need to 'fix' meson to provide an equivalent to -iquote to
the top of the IPU3 IPA source tree (as separate patch). (As otherwise,
I think I wo0uld have used "")

> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3 {
> +
> +class Algorithm
> +{
> +public:
> +	virtual ~Algorithm() {}
> +
> +	virtual int initialise([[maybe_unused]] IPAContext &context) { return 0; }
> +	virtual int configure([[maybe_unused]] IPAContext &context) { return 0; }
> +	virtual void process([[maybe_unused]] IPAContext &context) {}
> +};
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> new file mode 100644
> index 00000000..67148333
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +ipu3_ipa_algorithms = files([
> +])
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index a714af85..55c4da72 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -22,6 +22,7 @@
>  
>  #include "libcamera/internal/framebuffer.h"
>  
> +#include "algorithms/algorithm.h"
>  #include "ipa_context.h"
>  
>  #include "ipu3_agc.h"
> @@ -53,6 +54,10 @@ public:
>  private:
>  	void processControls(unsigned int frame, const ControlList &controls);
>  	void fillParams(unsigned int frame, ipu3_uapi_params *params);
> +
> +	int initialiseAlgorithms();
> +	int configureAlgorithms();
> +	void processAlgorithms(const ipu3_uapi_stats_3a *stats);

new line here to keep the 'algorithm iteration' functions in their own
block.


>  	void parseStatistics(unsigned int frame,
>  			     int64_t frameTimestamp,
>  			     const ipu3_uapi_stats_3a *stats);
> @@ -82,6 +87,9 @@ private:
>  	/* Interface to the Camera Helper */
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
> +	/* Maintain the algorithms used by the IPA */
> +	std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
> +
>  	/* Local parameter storage */
>  	struct ipu3_uapi_grid_config bdsGrid_;
>  	struct IPAContext context_;
> @@ -98,6 +106,8 @@ int IPAIPU3::init(const IPASettings &settings)
>  	/* Reset all the hardware settings */
>  	context_.params = {};
>  
> +	initialiseAlgorithms();
> +
>  	return 0;
>  }
>  
> @@ -199,6 +209,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  
>  	calculateBdsGrid(configInfo.bdsOutputSize);
>  
> +	configureAlgorithms();
> +
>  	awbAlgo_ = std::make_unique<IPU3Awb>();
>  	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
>  
> @@ -288,12 +300,47 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  	queueFrameAction.emit(frame, op);
>  }
>  
> +int IPAIPU3::initialiseAlgorithms()
> +{
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->initialise(context_);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int IPAIPU3::configureAlgorithms()
> +{
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->configure(context_);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)
> +{
> +	/* It might be better to pass the stats in as a parameter to process() ? */

We either need to make a decision on this or prefix with "\todo"

> +	context_.stats = stats;
> +
> +	for (auto const &algo : algorithms_) {
> +		algo->process(context_);
> +	}
> +}
> +
>  void IPAIPU3::parseStatistics(unsigned int frame,
>  			      [[maybe_unused]] int64_t frameTimestamp,
>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>  {
>  	ControlList ctrls(controls::controls);
>  
> +	/* Run the process for each algorithm on the stats */
> +	processAlgorithms(stats);
> +
>  	double gain = camHelper_->gain(gain_);
>  	agcAlgo_->process(stats, exposure_, gain);
>  	gain_ = camHelper_->gainCode(gain);
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index 3deca3ae..e3e1d28b 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -16,6 +16,7 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "algorithms/algorithm.h"
>  #include "libipa/algorithm.h"
>  
>  namespace libcamera {
> @@ -26,7 +27,7 @@ namespace ipa::ipu3 {
>  
>  using utils::Duration;
>  
> -class IPU3Agc : public Algorithm
> +class IPU3Agc : public ipa::Algorithm
>  {
>  public:
>  	IPU3Agc();
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> index 122cf68c..284e3844 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -13,6 +13,7 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "algorithms/algorithm.h"
>  #include "libipa/algorithm.h"
>  
>  namespace libcamera {
> @@ -23,7 +24,7 @@ namespace ipa::ipu3 {
>  static constexpr uint32_t kAwbStatsSizeX = 16;
>  static constexpr uint32_t kAwbStatsSizeY = 12;
>  
> -class IPU3Awb : public Algorithm
> +class IPU3Awb : public ipa::Algorithm
>  {
>  public:
>  	IPU3Awb();
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index b6364190..fcb27d68 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
> +subdir('algorithms')
> +
>  ipa_name = 'ipa_ipu3'
>  
>  ipu3_ipa_sources = files([
> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([
>      'ipu3_awb.cpp',
>  ])
>  
> +ipu3_ipa_sources += ipu3_ipa_algorithms
> +
>  mod = shared_module(ipa_name,
>                      [ipu3_ipa_sources, libcamera_generated_ipa_headers],
>                      name_prefix : '',
>
Umang Jain Aug. 9, 2021, 9:47 a.m. UTC | #2 
Hi JM,

Thanks for the patch.

On 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:
> Provide a modular IPU3 specific algorithm, and frame context
> structure so that algorithms can be built for the IPU3 in a
> component based structure.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>   src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++
>   src/ipa/ipu3/algorithms/meson.build |  4 +++
>   src/ipa/ipu3/ipu3.cpp               | 47 +++++++++++++++++++++++++++++
>   src/ipa/ipu3/ipu3_agc.h             |  3 +-
>   src/ipa/ipu3/ipu3_awb.h             |  3 +-
>   src/ipa/ipu3/meson.build            |  4 +++
>   6 files changed, 89 insertions(+), 2 deletions(-)
>   create mode 100644 src/ipa/ipu3/algorithms/algorithm.h
>   create mode 100644 src/ipa/ipu3/algorithms/meson.build
>
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> new file mode 100644
> index 00000000..bac82b7c
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * algorithm.h - IPU3 control algorithm interface
> + */
> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> +
> +#include <ipa_context.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3 {
> +
> +class Algorithm
> +{
> +public:
> +	virtual ~Algorithm() {}
> +
> +	virtual int initialise([[maybe_unused]] IPAContext &context) { return 0; }
> +	virtual int configure([[maybe_unused]] IPAContext &context) { return 0; }
> +	virtual void process([[maybe_unused]] IPAContext &context) {}
Usually I have seen in libcamera, [[maybe_unused]] is part of function 
definition or declaration. There are some exceptions of course, so I 
won't worry about this too much
> +};
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> new file mode 100644
> index 00000000..67148333
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +ipu3_ipa_algorithms = files([
> +])
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index a714af85..55c4da72 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -22,6 +22,7 @@
>   
>   #include "libcamera/internal/framebuffer.h"
>   
> +#include "algorithms/algorithm.h"
>   #include "ipa_context.h"
>   
>   #include "ipu3_agc.h"
> @@ -53,6 +54,10 @@ public:
>   private:
>   	void processControls(unsigned int frame, const ControlList &controls);
>   	void fillParams(unsigned int frame, ipu3_uapi_params *params);
> +
> +	int initialiseAlgorithms();
> +	int configureAlgorithms();
> +	void processAlgorithms(const ipu3_uapi_stats_3a *stats);
>   	void parseStatistics(unsigned int frame,
>   			     int64_t frameTimestamp,
>   			     const ipu3_uapi_stats_3a *stats);
> @@ -82,6 +87,9 @@ private:
>   	/* Interface to the Camera Helper */
>   	std::unique_ptr<CameraSensorHelper> camHelper_;
>   
> +	/* Maintain the algorithms used by the IPA */
> +	std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
> +
>   	/* Local parameter storage */
>   	struct ipu3_uapi_grid_config bdsGrid_;
>   	struct IPAContext context_;
> @@ -98,6 +106,8 @@ int IPAIPU3::init(const IPASettings &settings)
>   	/* Reset all the hardware settings */
>   	context_.params = {};
>   
> +	initialiseAlgorithms();
> +
>   	return 0;
>   }
>   
> @@ -199,6 +209,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>   
>   	calculateBdsGrid(configInfo.bdsOutputSize);
>   
> +	configureAlgorithms();
> +
>   	awbAlgo_ = std::make_unique<IPU3Awb>();
>   	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
>   
> @@ -288,12 +300,47 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>   	queueFrameAction.emit(frame, op);
>   }
>   
> +int IPAIPU3::initialiseAlgorithms()
> +{
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->initialise(context_);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int IPAIPU3::configureAlgorithms()
> +{
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->configure(context_);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)
> +{
> +	/* It might be better to pass the stats in as a parameter to process() ? */
I think so yes,
> +	context_.stats = stats;
> +
> +	for (auto const &algo : algorithms_) {
> +		algo->process(context_);

I would expect context_ to remain unchanged during a single frame is 
processed. I would expect the context_ to change *between* two frames. 
So I guess, passing in stats explicitly and not populating inside 
context_, makes sense to me.

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

> +	}
> +}
> +
>   void IPAIPU3::parseStatistics(unsigned int frame,
>   			      [[maybe_unused]] int64_t frameTimestamp,
>   			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>   {
>   	ControlList ctrls(controls::controls);
>   
> +	/* Run the process for each algorithm on the stats */
> +	processAlgorithms(stats);
> +
>   	double gain = camHelper_->gain(gain_);
>   	agcAlgo_->process(stats, exposure_, gain);
>   	gain_ = camHelper_->gainCode(gain);
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index 3deca3ae..e3e1d28b 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -16,6 +16,7 @@
>   
>   #include <libcamera/geometry.h>
>   
> +#include "algorithms/algorithm.h"
>   #include "libipa/algorithm.h"
>   
>   namespace libcamera {
> @@ -26,7 +27,7 @@ namespace ipa::ipu3 {
>   
>   using utils::Duration;
>   
> -class IPU3Agc : public Algorithm
> +class IPU3Agc : public ipa::Algorithm
>   {
>   public:
>   	IPU3Agc();
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> index 122cf68c..284e3844 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -13,6 +13,7 @@
>   
>   #include <libcamera/geometry.h>
>   
> +#include "algorithms/algorithm.h"
>   #include "libipa/algorithm.h"
>   
>   namespace libcamera {
> @@ -23,7 +24,7 @@ namespace ipa::ipu3 {
>   static constexpr uint32_t kAwbStatsSizeX = 16;
>   static constexpr uint32_t kAwbStatsSizeY = 12;
>   
> -class IPU3Awb : public Algorithm
> +class IPU3Awb : public ipa::Algorithm
>   {
>   public:
>   	IPU3Awb();
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index b6364190..fcb27d68 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -1,5 +1,7 @@
>   # SPDX-License-Identifier: CC0-1.0
>   
> +subdir('algorithms')
> +
>   ipa_name = 'ipa_ipu3'
>   
>   ipu3_ipa_sources = files([
> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([
>       'ipu3_awb.cpp',
>   ])
>   
> +ipu3_ipa_sources += ipu3_ipa_algorithms
> +
>   mod = shared_module(ipa_name,
>                       [ipu3_ipa_sources, libcamera_generated_ipa_headers],
>                       name_prefix : '',
Jean-Michel Hautbois Aug. 10, 2021, 7:24 a.m. UTC | #3 
Hi Umang,

On 09/08/2021 11:47, Umang Jain wrote:
> Hi JM,
> 
> Thanks for the patch.
> 
> On 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:
>> Provide a modular IPU3 specific algorithm, and frame context
>> structure so that algorithms can be built for the IPU3 in a
>> component based structure.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Signed-off-by: Jean-Michel Hautbois
>> <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++
>>   src/ipa/ipu3/algorithms/meson.build |  4 +++
>>   src/ipa/ipu3/ipu3.cpp               | 47 +++++++++++++++++++++++++++++
>>   src/ipa/ipu3/ipu3_agc.h             |  3 +-
>>   src/ipa/ipu3/ipu3_awb.h             |  3 +-
>>   src/ipa/ipu3/meson.build            |  4 +++
>>   6 files changed, 89 insertions(+), 2 deletions(-)
>>   create mode 100644 src/ipa/ipu3/algorithms/algorithm.h
>>   create mode 100644 src/ipa/ipu3/algorithms/meson.build
>>
>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h
>> b/src/ipa/ipu3/algorithms/algorithm.h
>> new file mode 100644
>> index 00000000..bac82b7c
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/algorithm.h
>> @@ -0,0 +1,30 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * algorithm.h - IPU3 control algorithm interface
>> + */
>> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>> +
>> +#include <ipa_context.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::ipu3 {
>> +
>> +class Algorithm
>> +{
>> +public:
>> +    virtual ~Algorithm() {}
>> +
>> +    virtual int initialise([[maybe_unused]] IPAContext &context) {
>> return 0; }
>> +    virtual int configure([[maybe_unused]] IPAContext &context) {
>> return 0; }
>> +    virtual void process([[maybe_unused]] IPAContext &context) {}
> Usually I have seen in libcamera, [[maybe_unused]] is part of function
> definition or declaration. There are some exceptions of course, so I
> won't worry about this too much
>> +};
>> +
>> +} /* namespace ipa::ipu3 */
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */
>> diff --git a/src/ipa/ipu3/algorithms/meson.build
>> b/src/ipa/ipu3/algorithms/meson.build
>> new file mode 100644
>> index 00000000..67148333
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/meson.build
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +ipu3_ipa_algorithms = files([
>> +])
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index a714af85..55c4da72 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -22,6 +22,7 @@
>>     #include "libcamera/internal/framebuffer.h"
>>   +#include "algorithms/algorithm.h"
>>   #include "ipa_context.h"
>>     #include "ipu3_agc.h"
>> @@ -53,6 +54,10 @@ public:
>>   private:
>>       void processControls(unsigned int frame, const ControlList
>> &controls);
>>       void fillParams(unsigned int frame, ipu3_uapi_params *params);
>> +
>> +    int initialiseAlgorithms();
>> +    int configureAlgorithms();
>> +    void processAlgorithms(const ipu3_uapi_stats_3a *stats);
>>       void parseStatistics(unsigned int frame,
>>                    int64_t frameTimestamp,
>>                    const ipu3_uapi_stats_3a *stats);
>> @@ -82,6 +87,9 @@ private:
>>       /* Interface to the Camera Helper */
>>       std::unique_ptr<CameraSensorHelper> camHelper_;
>>   +    /* Maintain the algorithms used by the IPA */
>> +    std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
>> +
>>       /* Local parameter storage */
>>       struct ipu3_uapi_grid_config bdsGrid_;
>>       struct IPAContext context_;
>> @@ -98,6 +106,8 @@ int IPAIPU3::init(const IPASettings &settings)
>>       /* Reset all the hardware settings */
>>       context_.params = {};
>>   +    initialiseAlgorithms();
>> +
>>       return 0;
>>   }
>>   @@ -199,6 +209,8 @@ int IPAIPU3::configure(const IPAConfigInfo
>> &configInfo)
>>         calculateBdsGrid(configInfo.bdsOutputSize);
>>   +    configureAlgorithms();
>> +
>>       awbAlgo_ = std::make_unique<IPU3Awb>();
>>       awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize,
>> bdsGrid_);
>>   @@ -288,12 +300,47 @@ void IPAIPU3::fillParams(unsigned int frame,
>> ipu3_uapi_params *params)
>>       queueFrameAction.emit(frame, op);
>>   }
>>   +int IPAIPU3::initialiseAlgorithms()
>> +{
>> +    for (auto const &algo : algorithms_) {
>> +        int ret = algo->initialise(context_);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int IPAIPU3::configureAlgorithms()
>> +{
>> +    for (auto const &algo : algorithms_) {
>> +        int ret = algo->configure(context_);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)
>> +{
>> +    /* It might be better to pass the stats in as a parameter to
>> process() ? */
> I think so yes,
>> +    context_.stats = stats;
>> +
>> +    for (auto const &algo : algorithms_) {
>> +        algo->process(context_);
> 
> I would expect context_ to remain unchanged during a single frame is
> processed. I would expect the context_ to change *between* two frames.
> So I guess, passing in stats explicitly and not populating inside
> context_, makes sense to me.

Given that Context is not per-frame, but a "current IPA context"
structure, I think stats should be removed from it and passed to
process() yes.

> 
>  Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
>> +    }
>> +}
>> +
>>   void IPAIPU3::parseStatistics(unsigned int frame,
>>                     [[maybe_unused]] int64_t frameTimestamp,
>>                     [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>   {
>>       ControlList ctrls(controls::controls);
>>   +    /* Run the process for each algorithm on the stats */
>> +    processAlgorithms(stats);
>> +
>>       double gain = camHelper_->gain(gain_);
>>       agcAlgo_->process(stats, exposure_, gain);
>>       gain_ = camHelper_->gainCode(gain);
>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>> index 3deca3ae..e3e1d28b 100644
>> --- a/src/ipa/ipu3/ipu3_agc.h
>> +++ b/src/ipa/ipu3/ipu3_agc.h
>> @@ -16,6 +16,7 @@
>>     #include <libcamera/geometry.h>
>>   +#include "algorithms/algorithm.h"
>>   #include "libipa/algorithm.h"
>>     namespace libcamera {
>> @@ -26,7 +27,7 @@ namespace ipa::ipu3 {
>>     using utils::Duration;
>>   -class IPU3Agc : public Algorithm
>> +class IPU3Agc : public ipa::Algorithm
>>   {
>>   public:
>>       IPU3Agc();
>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>> index 122cf68c..284e3844 100644
>> --- a/src/ipa/ipu3/ipu3_awb.h
>> +++ b/src/ipa/ipu3/ipu3_awb.h
>> @@ -13,6 +13,7 @@
>>     #include <libcamera/geometry.h>
>>   +#include "algorithms/algorithm.h"
>>   #include "libipa/algorithm.h"
>>     namespace libcamera {
>> @@ -23,7 +24,7 @@ namespace ipa::ipu3 {
>>   static constexpr uint32_t kAwbStatsSizeX = 16;
>>   static constexpr uint32_t kAwbStatsSizeY = 12;
>>   -class IPU3Awb : public Algorithm
>> +class IPU3Awb : public ipa::Algorithm
>>   {
>>   public:
>>       IPU3Awb();
>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>> index b6364190..fcb27d68 100644
>> --- a/src/ipa/ipu3/meson.build
>> +++ b/src/ipa/ipu3/meson.build
>> @@ -1,5 +1,7 @@
>>   # SPDX-License-Identifier: CC0-1.0
>>   +subdir('algorithms')
>> +
>>   ipa_name = 'ipa_ipu3'
>>     ipu3_ipa_sources = files([
>> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([
>>       'ipu3_awb.cpp',
>>   ])
>>   +ipu3_ipa_sources += ipu3_ipa_algorithms
>> +
>>   mod = shared_module(ipa_name,
>>                       [ipu3_ipa_sources,
>> libcamera_generated_ipa_headers],
>>                       name_prefix : '',

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
new file mode 100644
index 00000000..bac82b7c
--- /dev/null
+++ b/src/ipa/ipu3/algorithms/algorithm.h
@@ -0,0 +1,30 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Ideas On Board
+ *
+ * algorithm.h - IPU3 control algorithm interface
+ */
+#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
+#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
+
+#include <ipa_context.h>
+
+namespace libcamera {
+
+namespace ipa::ipu3 {
+
+class Algorithm
+{
+public:
+	virtual ~Algorithm() {}
+
+	virtual int initialise([[maybe_unused]] IPAContext &context) { return 0; }
+	virtual int configure([[maybe_unused]] IPAContext &context) { return 0; }
+	virtual void process([[maybe_unused]] IPAContext &context) {}
+};
+
+} /* namespace ipa::ipu3 */
+
+} /* namespace libcamera */
+
+#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */
diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
new file mode 100644
index 00000000..67148333
--- /dev/null
+++ b/src/ipa/ipu3/algorithms/meson.build
@@ -0,0 +1,4 @@ 
+# SPDX-License-Identifier: CC0-1.0
+
+ipu3_ipa_algorithms = files([
+])
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index a714af85..55c4da72 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -22,6 +22,7 @@ 
 
 #include "libcamera/internal/framebuffer.h"
 
+#include "algorithms/algorithm.h"
 #include "ipa_context.h"
 
 #include "ipu3_agc.h"
@@ -53,6 +54,10 @@  public:
 private:
 	void processControls(unsigned int frame, const ControlList &controls);
 	void fillParams(unsigned int frame, ipu3_uapi_params *params);
+
+	int initialiseAlgorithms();
+	int configureAlgorithms();
+	void processAlgorithms(const ipu3_uapi_stats_3a *stats);
 	void parseStatistics(unsigned int frame,
 			     int64_t frameTimestamp,
 			     const ipu3_uapi_stats_3a *stats);
@@ -82,6 +87,9 @@  private:
 	/* Interface to the Camera Helper */
 	std::unique_ptr<CameraSensorHelper> camHelper_;
 
+	/* Maintain the algorithms used by the IPA */
+	std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
+
 	/* Local parameter storage */
 	struct ipu3_uapi_grid_config bdsGrid_;
 	struct IPAContext context_;
@@ -98,6 +106,8 @@  int IPAIPU3::init(const IPASettings &settings)
 	/* Reset all the hardware settings */
 	context_.params = {};
 
+	initialiseAlgorithms();
+
 	return 0;
 }
 
@@ -199,6 +209,8 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
 
 	calculateBdsGrid(configInfo.bdsOutputSize);
 
+	configureAlgorithms();
+
 	awbAlgo_ = std::make_unique<IPU3Awb>();
 	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
 
@@ -288,12 +300,47 @@  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
 	queueFrameAction.emit(frame, op);
 }
 
+int IPAIPU3::initialiseAlgorithms()
+{
+	for (auto const &algo : algorithms_) {
+		int ret = algo->initialise(context_);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+int IPAIPU3::configureAlgorithms()
+{
+	for (auto const &algo : algorithms_) {
+		int ret = algo->configure(context_);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)
+{
+	/* It might be better to pass the stats in as a parameter to process() ? */
+	context_.stats = stats;
+
+	for (auto const &algo : algorithms_) {
+		algo->process(context_);
+	}
+}
+
 void IPAIPU3::parseStatistics(unsigned int frame,
 			      [[maybe_unused]] int64_t frameTimestamp,
 			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
 {
 	ControlList ctrls(controls::controls);
 
+	/* Run the process for each algorithm on the stats */
+	processAlgorithms(stats);
+
 	double gain = camHelper_->gain(gain_);
 	agcAlgo_->process(stats, exposure_, gain);
 	gain_ = camHelper_->gainCode(gain);
diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
index 3deca3ae..e3e1d28b 100644
--- a/src/ipa/ipu3/ipu3_agc.h
+++ b/src/ipa/ipu3/ipu3_agc.h
@@ -16,6 +16,7 @@ 
 
 #include <libcamera/geometry.h>
 
+#include "algorithms/algorithm.h"
 #include "libipa/algorithm.h"
 
 namespace libcamera {
@@ -26,7 +27,7 @@  namespace ipa::ipu3 {
 
 using utils::Duration;
 
-class IPU3Agc : public Algorithm
+class IPU3Agc : public ipa::Algorithm
 {
 public:
 	IPU3Agc();
diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
index 122cf68c..284e3844 100644
--- a/src/ipa/ipu3/ipu3_awb.h
+++ b/src/ipa/ipu3/ipu3_awb.h
@@ -13,6 +13,7 @@ 
 
 #include <libcamera/geometry.h>
 
+#include "algorithms/algorithm.h"
 #include "libipa/algorithm.h"
 
 namespace libcamera {
@@ -23,7 +24,7 @@  namespace ipa::ipu3 {
 static constexpr uint32_t kAwbStatsSizeX = 16;
 static constexpr uint32_t kAwbStatsSizeY = 12;
 
-class IPU3Awb : public Algorithm
+class IPU3Awb : public ipa::Algorithm
 {
 public:
 	IPU3Awb();
diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
index b6364190..fcb27d68 100644
--- a/src/ipa/ipu3/meson.build
+++ b/src/ipa/ipu3/meson.build
@@ -1,5 +1,7 @@ 
 # SPDX-License-Identifier: CC0-1.0
 
+subdir('algorithms')
+
 ipa_name = 'ipa_ipu3'
 
 ipu3_ipa_sources = files([
@@ -8,6 +10,8 @@  ipu3_ipa_sources = files([
     'ipu3_awb.cpp',
 ])
 
+ipu3_ipa_sources += ipu3_ipa_algorithms
+
 mod = shared_module(ipa_name,
                     [ipu3_ipa_sources, libcamera_generated_ipa_headers],
                     name_prefix : '',