[libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular algorithms
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 9 11:47:44 CEST 2021
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 at ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at 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 at 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 : '',
More information about the libcamera-devel
mailing list