[libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into algorithms
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 9 12:22:52 CEST 2021
On 09/08/2021 10:20, Jean-Michel Hautbois wrote:
> Use the Context class and algorithm interface to properly call the AWB
s/Context/IPAContext/
s/algorithm/Algorithm/
and instead of 'properly call' I'd say "to call the AWB algorithm using
the new modular interface"
> algorithm from IPAIPU3.
> We need to pass the BDS grid size calculated, pass those through the Context
> class in a dedicated Awb structure.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> .../ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} | 42 ++++++++++++-------
> src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} | 29 ++++++-------
> src/ipa/ipu3/algorithms/meson.build | 1 +
> src/ipa/ipu3/ipa_context.h | 12 ++++++
> src/ipa/ipu3/ipu3.cpp | 30 ++++++-------
> src/ipa/ipu3/meson.build | 1 -
> 6 files changed, 66 insertions(+), 49 deletions(-)
> rename src/ipa/ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} (92%)
> rename src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} (79%)
>
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> similarity index 92%
> rename from src/ipa/ipu3/ipu3_awb.cpp
> rename to src/ipa/ipu3/algorithms/awb.cpp
> index 043c3838..92a41b65 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -2,9 +2,9 @@
> /*
> * Copyright (C) 2021, Ideas On Board
> *
> - * ipu3_awb.cpp - AWB control algorithm
> + * awb.cpp - AWB control algorithm
> */
> -#include "ipu3_awb.h"
> +#include "awb.h"
>
> #include <cmath>
> #include <numeric>
> @@ -14,7 +14,7 @@
>
> namespace libcamera {
>
> -namespace ipa::ipu3 {
> +namespace ipa::ipu3::algorithms {
>
> LOG_DEFINE_CATEGORY(IPU3Awb)
>
> @@ -134,7 +134,7 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> 0, 0, 8191, 0
> };
>
> -IPU3Awb::IPU3Awb()
> +Awb::Awb()
> : Algorithm()
> {
> asyncResults_.blueGain = 1.0;
> @@ -143,17 +143,19 @@ IPU3Awb::IPU3Awb()
> asyncResults_.temperatureK = 4500;
> }
>
> -IPU3Awb::~IPU3Awb()
> +Awb::~Awb()
> {
> }
>
> -void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
> +int Awb::configure(IPAContext &context)
> {
> + ipu3_uapi_params ¶ms = context.params;
> + Size &bdsOutputSize = context.awb.grid.bdsOutputSize;
> +
We'll need to give more thought as to what parameters are coming in as
Configuration parameters, and they might get passed in (perhaps as a
separate structure, or perhaps still using the IPAContext).
But we've already discussed whether the Grid configuration would be
something calculated by the top level ipu3.cpp and passed in, or if it
should be it's own 'algorithm/module' and handled as part of the
processing chain.
But for now, I think this is fine.
> params.use.acc_awb = 1;
> params.acc_param.awb.config = imguCssAwbDefaults;
>
> - awbGrid_ = bdsGrid;
> - params.acc_param.awb.config.grid = awbGrid_;
> + awbGrid_ = context.awb.grid.bdsGrid;
does this Awb class still need a local copy of the grid in awbGrid_ ?
Perhaps it does if it's something that will be passed in, and
potentially not available in the IPAContext at process() time ... but at
the moment, it 'could' always get it from the Context ...
>
> params.use.acc_bnr = 1;
> params.acc_param.bnr = imguCssBnrDefaults;
> @@ -174,6 +176,8 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st
> params.acc_param.ccm = imguCssCcmDefault;
>
> zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> +
> + return 0;
> }
>
> /**
> @@ -190,7 +194,7 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st
> * More detailed information can be found in:
> * https://en.wikipedia.org/wiki/Color_temperature#Approximation
> */
> -uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
> +uint32_t Awb::estimateCCT(double red, double green, double blue)
> {
> /* Convert the RGB values to CIE tristimulus values (XYZ) */
> double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
> @@ -207,7 +211,7 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
> }
>
> /* Generate an RGB vector with the average values for each region */
> -void IPU3Awb::generateZones(std::vector<RGB> &zones)
> +void Awb::generateZones(std::vector<RGB> &zones)
> {
> for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> RGB zone;
> @@ -224,7 +228,7 @@ void IPU3Awb::generateZones(std::vector<RGB> &zones)
> }
>
> /* Translate the IPU3 statistics into the default statistics region array */
> -void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
> +void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
> {
> uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
> uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
> @@ -256,7 +260,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
> }
> }
>
> -void IPU3Awb::clearAwbStats()
> +void Awb::clearAwbStats()
> {
> for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> awbStats_[i].bSum = 0;
> @@ -267,7 +271,7 @@ void IPU3Awb::clearAwbStats()
> }
> }
>
> -void IPU3Awb::awbGreyWorld()
> +void Awb::awbGreyWorld()
> {
> LOG(IPU3Awb, Debug) << "Grey world AWB";
> /*
> @@ -307,7 +311,7 @@ void IPU3Awb::awbGreyWorld()
> asyncResults_.blueGain = blueGain;
> }
>
> -void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> +void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> {
> ASSERT(stats->stats_3a_status.awb_en);
> zones_.clear();
> @@ -322,7 +326,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> }
> }
>
> -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)
> +void Awb::updateWbParameters(ipu3_uapi_params ¶ms)
> {
> /*
> * Green gains should not be touched and considered 1.
> @@ -340,6 +344,12 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)
> params.acc_param.ccm = imguCssCcmDefault;
> }
>
> -} /* namespace ipa::ipu3 */
> +void Awb::process(IPAContext &context)
> +{
> + calculateWBGains(context.stats);
> + updateWbParameters(context.params);
> +}
> +
> +} /* namespace ipa::ipu3::algorithms */
>
> } /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/algorithms/awb.h
> similarity index 79%
> rename from src/ipa/ipu3/ipu3_awb.h
> rename to src/ipa/ipu3/algorithms/awb.h
> index 8b05ac03..37690b37 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -2,10 +2,10 @@
> /*
> * Copyright (C) 2021, Ideas On Board
> *
> - * ipu3_awb.h - IPU3 AWB control algorithm
> + * awb.h - IPU3 AWB control algorithm
> */
> -#ifndef __LIBCAMERA_IPU3_AWB_H__
> -#define __LIBCAMERA_IPU3_AWB_H__
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__
>
> #include <vector>
>
> @@ -13,26 +13,24 @@
>
> #include <libcamera/geometry.h>
>
> -#include "algorithms/algorithm.h"
> -#include "libipa/algorithm.h"
> +#include "algorithm.h"
>
> namespace libcamera {
>
> -namespace ipa::ipu3 {
> +namespace ipa::ipu3::algorithms {
>
> /* Region size for the statistics generation algorithm */
> static constexpr uint32_t kAwbStatsSizeX = 16;
> static constexpr uint32_t kAwbStatsSizeY = 12;
>
> -class IPU3Awb : public ipa::Algorithm
> +class Awb : public Algorithm
> {
> public:
> - IPU3Awb();
> - ~IPU3Awb();
> + Awb();
> + ~Awb();
>
> - void initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
> - void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> - void updateWbParameters(ipu3_uapi_params ¶ms);
> + int configure(IPAContext &context) override;
> + void process(IPAContext &context) override;
>
> struct Ipu3AwbCell {
> unsigned char greenRedAvg;
> @@ -73,6 +71,9 @@ public:
> };
>
> private:
> + void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> + void updateWbParameters(ipu3_uapi_params ¶ms);
> +
> void generateZones(std::vector<RGB> &zones);
> void generateAwbStats(const ipu3_uapi_stats_3a *stats);
> void clearAwbStats();
> @@ -86,7 +87,7 @@ private:
> AwbStatus asyncResults_;
> };
>
> -} /* namespace ipa::ipu3 */
> +} /* namespace ipa::ipu3::algorithms */
>
> } /* namespace libcamera*/
> -#endif /* __LIBCAMERA_IPU3_AWB_H__ */
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index f71d1e61..df36d719 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: CC0-1.0
>
> ipu3_ipa_algorithms = files([
> + 'awb.cpp',
> 'contrast.cpp',
> ])
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 5d717be5..c43b275b 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -11,6 +11,8 @@
>
> #include <linux/intel-ipu3.h>
>
> +#include <libcamera/geometry.h>
> +
> namespace libcamera {
>
> namespace ipa::ipu3 {
> @@ -21,6 +23,16 @@ struct IPAContext {
>
> /* Output Parameters which will be written to the hardware */
> ipu3_uapi_params params;
> +
> + /* AWB specific parameters to share */
> + struct Awb {
> + struct Grid {
> + /* BDS grid plane config used by the kernel */
> + ipu3_uapi_grid_config bdsGrid;
> + /* BDS output size configured by the pipeline handler */
> + Size bdsOutputSize;
> + } grid;
Discussed off-list, but re-discussing here for the list:
I think the BDS grid sounds like it's own module. It references 'BDS'
which is a distinct entity on the IPU3, and it does a calculation at
configure time. The Grid isn't a property of the AWB, but something that
it uses, (as does the AGC, I believe).
So I'd expect to see this stored outside of the Awb context structure.
However - As this is currently calculated /inside/ the Awb algorithm
module - I'm fine with keeping it here to show that.
If or rather when, the grid calculation gets moved out - I would expect
that this will likely move out of this structure and to a structure that
matches the component which generates it.
> + } awb;
> };
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 7035802f..82506461 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -23,11 +23,11 @@
> #include "libcamera/internal/framebuffer.h"
>
> #include "algorithms/algorithm.h"
> +#include "algorithms/awb.h"
> #include "algorithms/contrast.h"
> #include "ipa_context.h"
>
> #include "ipu3_agc.h"
> -#include "ipu3_awb.h"
> #include "libipa/camera_sensor_helper.h"
>
> static constexpr uint32_t kMaxCellWidthPerSet = 160;
> @@ -81,8 +81,6 @@ private:
> uint32_t minGain_;
> uint32_t maxGain_;
>
> - /* Interface to the AWB algorithm */
> - std::unique_ptr<IPU3Awb> awbAlgo_;
> /* Interface to the AEC/AGC algorithm */
> std::unique_ptr<IPU3Agc> agcAlgo_;
> /* Interface to the Camera Helper */
> @@ -105,6 +103,7 @@ int IPAIPU3::init(const IPASettings &settings)
> }
>
> /* Construct our Algorithms */
Perhaps in the previous patch we should state here that algorithms will
be initialised, configured, and processed in the order that they are
added to this list.
> + algorithms_.emplace_back(new algorithms::Awb());
> algorithms_.emplace_back(new algorithms::Contrast());
>
> /* Reset all the hardware settings */
> @@ -138,7 +137,7 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
Oh! The grid *isn't* calculated in the Awb currently, so it really isn't
an Awb structure ?
It's just that the Awb uses the structure ? is that right?
It looks like this is something that the IPU3 top level code does
currently - and really could be moved out to an 'algorithm' to handle
during the Configure stage.
I would be quite tempted to see that done as a patch /before/ converting
the Awb given it's a pre-requisite to handling the Awb ...
It might also show that we should pass "const IPAConfigInfo &configInfo"
into the Configure() operations of the modules too.
Alternatively, if you really believe this is part of Awb and not it's
own 'module' - should/could it move into the Configure phase of the Awb
/in this patch/ ?
If the grid configuration is used only for Awb, then it can be part of
the Awb configure.
If it is used for any other algorithm stage (and I mean on the IPU3, not
just our current algorithms) then I think it deserves it's own module.
> uint32_t minError = std::numeric_limits<uint32_t>::max();
> Size best;
> Size bestLog2;
> - bdsGrid_ = {};
> + ipu3_uapi_grid_config &bdsGrid = context_.awb.grid.bdsGrid;
>
> for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> uint32_t width = std::min(kMaxCellWidthPerSet,
> @@ -162,14 +161,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> }
> }
>
> - bdsGrid_.width = best.width >> bestLog2.width;
> - bdsGrid_.block_width_log2 = bestLog2.width;
> - bdsGrid_.height = best.height >> bestLog2.height;
> - bdsGrid_.block_height_log2 = bestLog2.height;
> + bdsGrid.width = best.width >> bestLog2.width;
> + bdsGrid.block_width_log2 = bestLog2.width;
> + bdsGrid.height = best.height >> bestLog2.height;
> + bdsGrid.block_height_log2 = bestLog2.height;
>
> LOG(IPAIPU3, Debug) << "Best grid found is: ("
> - << (int)bdsGrid_.width << " << " << (int)bdsGrid_.block_width_log2 << ") x ("
> - << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
> + << (int)bdsGrid.width << " << " << (int)bdsGrid.block_width_log2 << ") x ("
> + << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
> }
>
> int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> @@ -211,13 +210,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>
> defVBlank_ = itVBlank->second.def().get<int32_t>();
>
> + /* Prepare AWB parameters */
> calculateBdsGrid(configInfo.bdsOutputSize);
> + context_.awb.grid.bdsOutputSize = configInfo.bdsOutputSize;
This looks like evidence suggesting we need an algorithm to handle the
configuration of the BDS which stores it's 'output' in our IPAContext as
a BDS specific data structure.
>
> configureAlgorithms();
>
> - awbAlgo_ = std::make_unique<IPU3Awb>();
> - awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
> -
> + bdsGrid_ = context_.awb.grid.bdsGrid;
Is this just a copy of the grid in the top level IPU3 framework ? I
suspect it shouldn't be needed when we have it on the IPAContext...
Is it still used somehow?
Anyway, that might be all irrelevant if you create a BDS algorithm that
runs first.
> agcAlgo_ = std::make_unique<IPU3Agc>();
> agcAlgo_->initialise(bdsGrid_, sensorInfo_);
>
> @@ -293,9 +292,6 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>
> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> {
> - if (agcAlgo_->updateControls())
> - awbAlgo_->updateWbParameters(context_.params);
> -
> *params = context_.params;
>
> IPU3Action op;
> @@ -349,8 +345,6 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> agcAlgo_->process(stats, exposure_, gain);
> gain_ = camHelper_->gainCode(gain);
>
> - awbAlgo_->calculateWBGains(stats);
> -
Very happy to see these algorithm specific hooks being removed though.
> if (agcAlgo_->updateControls())
> setControls(frame);
>
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index fcb27d68..d1126947 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -7,7 +7,6 @@ ipa_name = 'ipa_ipu3'
> ipu3_ipa_sources = files([
> 'ipu3.cpp',
> 'ipu3_agc.cpp',
> - 'ipu3_awb.cpp',
> ])
>
> ipu3_ipa_sources += ipu3_ipa_algorithms
>
More information about the libcamera-devel
mailing list