[libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into algorithms
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 9 12:20:53 CEST 2021
Hi JM
Thank you for the patch
On 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:
> Use the Context class and algorithm interface to properly call the AWB
> 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)
Mapping old Initialise() directly to Algorithm::configure() ? If there
is a reason, I would expect something about it in the commit message maybe?
> {
> + ipu3_uapi_params ¶ms = context.params;
> + Size &bdsOutputSize = context.awb.grid.bdsOutputSize;
> +
> 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;
>
> 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;
hmm, no initialise() here? Doesn't seem sane to have a direct call to
configure(). I wonder, as in the last patch where there was only
Initialise() and no over-ridden configure()/process().
>
> 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;
> + } 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 */
> + 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)
> 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;
>
> configureAlgorithms();
>
> - awbAlgo_ = std::make_unique<IPU3Awb>();
> - awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
> -
> + bdsGrid_ = context_.awb.grid.bdsGrid;
> 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);
> -
> 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