[libcamera-devel] [PATCH v2 03/10] ipa: ipu3: Introduce modular grid algorithm
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Aug 15 22:34:16 CEST 2021
Hello JM,
Thanks for your work.
On 2021-08-15 20:58:27 +0300, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Fri, Aug 13, 2021 at 09:40:09AM +0100, Kieran Bingham wrote:
> > 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.
>
> I'm sure Niklas would have disagreed ;-) I tend to like splitting things
> up too, possibly not as much as he does though.
So this was the burning sensation I had all day :-)
Maybe the addition of the Grid class could be broken out to a separate
patch? Over all I like the idea of the algorithms helpers.
>
> > > 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)
>
> Line wrap.
>
> > > + {
> > > + 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)
>
> I don't think this qualifies as an "algorithm" I'm afraid. I'd keep it
> in the IPU3IPA class.
>
> > > +{
> > > + 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.
>
> As this is meant to be a reference implementation, documentation is
> fairly important, and I'd prefer if we used Doxygen formatting. It thus
> brings the question of whether we want to keep it here or move it to a
> .cpp file.
>
> > > + /* 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;
>
> Line wrap here too.
>
> > > + 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;
> > >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list