[libcamera-devel] [PATCH v2 03/10] ipa: ipu3: Introduce modular grid algorithm
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Aug 13 10:40:09 CEST 2021
Hi JM,
On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> Implement a new modular framework for algorithms with a common context
> structure that is passed to each algorithm through a common API.
>
> The initial algorithm is chosen to configure the Bayer Down Scaler grid
> which is moved from the IPAIPU3 class.
>
> This patch:
> - adds a configure() function to the Algorithm interface
> - creates a new Grid class implementing the computation at configure
> call
> - removes all the local references from IPAIPU3
> - implements the list of pointers and the loop at configure call on each
> algorithm (right now, only Grid is in the list)
> - removes the imguCssAwbDefaults structure as it is now configured
> properly
>
A busy patch, but I think in this instance it is reasonable to do all of
that in a single patch.
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/algorithm.h | 11 ++++
> src/ipa/ipu3/algorithms/grid.cpp | 83 +++++++++++++++++++++++++++++
> src/ipa/ipu3/algorithms/grid.h | 29 ++++++++++
> src/ipa/ipu3/algorithms/meson.build | 1 +
> src/ipa/ipu3/ipa_context.h | 8 +++
> src/ipa/ipu3/ipu3.cpp | 75 ++++++--------------------
> src/ipa/ipu3/ipu3_awb.cpp | 27 ++--------
> 7 files changed, 154 insertions(+), 80 deletions(-)
> create mode 100644 src/ipa/ipu3/algorithms/grid.cpp
> create mode 100644 src/ipa/ipu3/algorithms/grid.h
>
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index 072f01c4..c1b37276 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.h
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -7,6 +7,12 @@
> #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>
> +#include <iostream>
> +
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
> +
> +#include "ipa_context.h"
> +
> namespace libcamera {
>
> namespace ipa::ipu3 {
> @@ -15,6 +21,11 @@ class Algorithm
> {
> public:
> virtual ~Algorithm() {}
> +
> + virtual int configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> + {
> + return 0;
> + }
> };
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp
> new file mode 100644
> index 00000000..3578f41b
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/grid.cpp
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * grid.cpp - IPU3 grid configuration
> + */
> +
> +#include "grid.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +LOG_DEFINE_CATEGORY(IPU3Grid)
> +
> +/* Maximum number of cells on a row */
> +static constexpr uint32_t kMaxCellWidthPerSet = 160;
> +/* Maximum number of cells on a column */
> +static constexpr uint32_t kMaxCellHeightPerSet = 56;
> +
> +/**
> + * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> + * Its input is the BDS output size calculated in the ImgU.
> + * It is limited for now to the simplest method: find the lesser error
> + * with the width/height and respective log2 width/height of the cells.
> + *
> + * \todo The frame is divided into cells which can be 8x8 => 128x128.
> + * As a smaller cell improves the algorithm precision, adapting the
> + * x_start and y_start parameters of the grid would provoke a loss of
> + * some pixels but would also result in more accurate algorithms.
> + */
> +int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> +{
> + uint32_t minError = std::numeric_limits<uint32_t>::max();
> + Size best;
> + Size bestLog2;
> + ipu3_uapi_grid_config &bdsGrid = context.configuration.grid.bdsGrid;
> +
> + context.configuration.grid.bdsOutputSize = configInfo.bdsOutputSize;
> + Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;
> +
> + bdsGrid.x_start = 0;
> + bdsGrid.y_start = 0;
> +
> + for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> + uint32_t width = std::min(kMaxCellWidthPerSet,
> + bdsOutputSize.width >> widthShift);
> + width = width << widthShift;
> + for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> + int32_t height = std::min(kMaxCellHeightPerSet,
> + bdsOutputSize.height >> heightShift);
> + height = height << heightShift;
> + uint32_t error = std::abs(static_cast<int>(width - bdsOutputSize.width))
> + + std::abs(static_cast<int>(height - bdsOutputSize.height));
> +
> + if (error > minError)
> + continue;
> +
> + minError = error;
> + best.width = width;
> + best.height = height;
> + bestLog2.width = widthShift;
> + bestLog2.height = heightShift;
> + }
> + }
> +
> + 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(IPU3Grid, Debug) << "Best grid found is: ("
> + << (int)bdsGrid.width << " << " << (int)bdsGrid.block_width_log2 << ") x ("
> + << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
> +
> + return 0;
> +}
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h
> new file mode 100644
> index 00000000..b4a51b42
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/grid.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google
> + *
> + * grid.h - IPU3 grid configuration
> + */
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +class Grid : public Algorithm
> +{
> +public:
> + ~Grid() = default;
> +
> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +};
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> +
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index 67148333..3fb3ce56 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: CC0-1.0
>
> ipu3_ipa_algorithms = files([
> + 'grid.cpp',
> ])
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 0a197a41..90b2f2c2 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -11,12 +11,20 @@
>
> #include <linux/intel-ipu3.h>
>
> +#include <libcamera/geometry.h>
> +
> namespace libcamera {
>
> namespace ipa::ipu3 {
>
> /* Fixed configuration of the IPA */
> struct IPAConfiguration {
> + struct Grid {
> + /* Bayer Down Scaler grid plane config used by the kernel */
> + ipu3_uapi_grid_config bdsGrid;
I always find structures with line comments above better with a blank
line in-between, otherwise they 'merge together' in my eyes. But that's
just personal preference, so not an issue.
> + /* BDS output size configured by the pipeline handler */
> + Size bdsOutputSize;
> + } grid;
> };
>
> /*
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index c34fa460..ef7fec86 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -29,13 +29,12 @@
>
> #include "libcamera/internal/mapped_framebuffer.h"
>
> +#include "algorithms/algorithm.h"
> +#include "algorithms/grid.h"
> #include "ipu3_agc.h"
> #include "ipu3_awb.h"
> #include "libipa/camera_sensor_helper.h"
>
> -static constexpr uint32_t kMaxCellWidthPerSet = 160;
> -static constexpr uint32_t kMaxCellHeightPerSet = 56;
> -
> namespace libcamera {
>
> LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -91,10 +90,12 @@ 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 IPAContext context_;
> struct ipu3_uapi_params params_;
> -
> - struct ipu3_uapi_grid_config bdsGrid_;
> };
>
> /**
> @@ -164,6 +165,8 @@ int IPAIPU3::init(const IPASettings &settings,
> frameDurations[2]);
>
> *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
I think a new line here is deserved though, Adding algorithms to the
list, is unrelated to initialising the controls - so they should be
separated with a blank line.
Two trivial blank lines is the most I can say ... so I think that
satisfies a:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + /* Construct our Algorithms */
> + algorithms_.emplace_back(new algorithms::Grid());
>
> return 0;
> }
> @@ -175,56 +178,6 @@ int IPAIPU3::start()
> return 0;
> }
>
> -/**
> - * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> - * Its input is the BDS output size calculated in the ImgU.
> - * It is limited for now to the simplest method: find the lesser error
> - * with the width/height and respective log2 width/height of the cells.
> - *
> - * \todo The frame is divided into cells which can be 8x8 => 128x128.
> - * As a smaller cell improves the algorithm precision, adapting the
> - * x_start and y_start parameters of the grid would provoke a loss of
> - * some pixels but would also result in more accurate algorithms.
> - */
> -void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> -{
> - uint32_t minError = std::numeric_limits<uint32_t>::max();
> - Size best;
> - Size bestLog2;
> - bdsGrid_ = {};
> -
> - for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> - uint32_t width = std::min(kMaxCellWidthPerSet,
> - bdsOutputSize.width >> widthShift);
> - width = width << widthShift;
> - for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> - int32_t height = std::min(kMaxCellHeightPerSet,
> - bdsOutputSize.height >> heightShift);
> - height = height << heightShift;
> - uint32_t error = std::abs(static_cast<int>(width - bdsOutputSize.width))
> - + std::abs(static_cast<int>(height - bdsOutputSize.height));
> -
> - if (error > minError)
> - continue;
> -
> - minError = error;
> - best.width = width;
> - best.height = height;
> - bestLog2.width = widthShift;
> - bestLog2.height = heightShift;
> - }
> - }
> -
> - 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 IPAIPU3::configure(const IPAConfigInfo &configInfo)
> {
> if (configInfo.entityControls.empty()) {
> @@ -264,15 +217,21 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>
> defVBlank_ = itVBlank->second.def().get<int32_t>();
>
> + /* Clean context and IPU3 parameters at configuration */
> params_ = {};
> + context_ = {};
>
> - calculateBdsGrid(configInfo.bdsOutputSize);
> + for (auto const &algo : algorithms_) {
> + int ret = algo->configure(context_, configInfo);
> + if (ret)
> + return ret;
> + }
>
> awbAlgo_ = std::make_unique<IPU3Awb>();
> - awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> + awbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);
>
> agcAlgo_ = std::make_unique<IPU3Agc>();
> - agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> + agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
>
> return 0;
> }
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> index 4bb321b3..4ee5ee6f 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -107,25 +107,6 @@ static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
> .opt_center_sqr = { 419904, 133956 },
> };
>
> -/* Default settings for Auto White Balance replicated from the Kernel*/
> -static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
> - .rgbs_thr_gr = 8191,
> - .rgbs_thr_r = 8191,
> - .rgbs_thr_gb = 8191,
> - .rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> - .grid = {
> - .width = 160,
> - .height = 36,
> - .block_width_log2 = 3,
> - .block_height_log2 = 4,
> - .height_per_slice = 1, /* Overridden by kernel. */
> - .x_start = 0,
> - .y_start = 0,
> - .x_end = 0,
> - .y_end = 0,
> - },
> -};
> -
> /* Default color correction matrix defined as an identity matrix */
> static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> 8191, 0, 0, 0,
> @@ -174,10 +155,12 @@ IPU3Awb::~IPU3Awb()
> void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
> {
> params.use.acc_awb = 1;
> - params.acc_param.awb.config = imguCssAwbDefaults;
> -
> + params.acc_param.awb.config.rgbs_thr_gr = 8191;
> + params.acc_param.awb.config.rgbs_thr_r = 8191;
> + params.acc_param.awb.config.rgbs_thr_gb = 8191;
> + params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;
> + params.acc_param.awb.config.grid = bdsGrid;
> awbGrid_ = bdsGrid;
> - params.acc_param.awb.config.grid = awbGrid_;
>
> params.use.acc_bnr = 1;
> params.acc_param.bnr = imguCssBnrDefaults;
>
More information about the libcamera-devel
mailing list