[libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular algorithms
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Aug 10 09:24:08 CEST 2021
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 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.
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 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