| Message ID | 20210819145759.184192-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
On 19/08/2021 15:57, Jean-Michel Hautbois wrote: > Introduce three functions in the Algorithm class to manage algorithms: > - configure which is called when IPA is configured only > - prepare called on EventFillParams event at each frame when the request > is queued > - process called on EventStatReady event at each frame completion when > the statistics have been generated. > > The existing AGC implementation already has a function named process(), > though it has different arguments. Adding the new virtual process() > interface causes a compiler warning due to the AGC implementation > overloading a virtual function, even though the overload can be resolved > correctly. > > Temporarily disable the warning in this commit to maintain bisection > until the AGC is converted to the new interface. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > meson.build | 4 ++ > src/ipa/ipu3/algorithms/algorithm.cpp | 72 +++++++++++++++++++++++++++ > src/ipa/ipu3/algorithms/algorithm.h | 9 ++++ > 3 files changed, 85 insertions(+) > > diff --git a/meson.build b/meson.build > index a49c484f..4a10e2b6 100644 > --- a/meson.build > +++ b/meson.build > @@ -108,6 +108,10 @@ if cc.has_argument('-Wno-c99-designator') > ] > endif > > +# Do not warn when a function declaration hides virtual functions from > +# a base class > +cpp_arguments += '-Wno-overloaded-virtual' > + > c_arguments += common_arguments > cpp_arguments += common_arguments > > diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/ipu3/algorithms/algorithm.cpp > index dd46846a..ec3c152e 100644 > --- a/src/ipa/ipu3/algorithms/algorithm.cpp > +++ b/src/ipa/ipu3/algorithms/algorithm.cpp > @@ -25,6 +25,78 @@ namespace ipa::ipu3 { > * to manage algorithms regardless of their specific type. > */ > > +/** > + * \param[in] context The shared IPA context > + * \param[in] configInfo The IPAConfigInfo structure reference > + * \return 0 if successful, an error code otherwise > + * > + * \brief Configure the Algorithm given an IPAConfigInfo > + * Algorithms may implement a configure operation to pre-calculate > + * parameters prior to commencing streaming. > + * > + * Configuration state may be stored in the IPASessionConfiguration structure of > + * the IPAContext. > + */ > +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) > +{ > + return 0; > +} > + > +/** > + * \param[in] context The shared IPA context > + * \param[in] params The IPU3 specific parameters. params is an [out] not an in. > + * > + * \brief Fill the parameter buffer with the updated context values > + * > + * While streaming, the IPA will configure the IPU3 IMGU through it's > + * parameter structure. > + * > + * Algorithms shall fill in the parameter structure fields appropriately to > + * configure algorithm blocks that they are responsible for. > + * The structure should be updated to set the enable fields and use flags > + * of any algorithms they are responsible for. > + */ > +void Algorithm::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) I think params should be a pointer here, not a reference. > +{ > +} > + > +/** > + * \param[in] context The shared IPA context > + * \param[in] stats The IPU3 statistics and ISP results > + * > + * \brief Process ISP statistics, and run algorithm operations. > + * > + * While streaming, the IPA will receive the generated statistics from the > + * IPU3 IMGU of processed frames. > + * > + * Algorithms shall use this data to run calculations and update their > + * state accordingly. > + * > + * Processing shall not take an undue amount of time, and any extended or > + * computationally expensive calculations or operations must be handled > + * asynchronously in a separate thread. > + * > + * Algorithms can store state in their respective IPAFrameContext > + * structures, and reference state from the IPAFrameContext of other > + * algorithms. > + * > + * \todo Historical data may be required as part of the processing. > + * > + * Either the previous frame, or the IPAFrameContext state of the frame > + * that generated the statistics for this operation may be required for > + * some advanced algorithms to prevent oscillations or support control > + * loops correctly. Only a single IPAFrameContext is available currently, > + * and so any data stored may represent the results of the previously > + * completed operations. > + * > + * Care shall be taken to ensure the ordering of access to the information > + * such that the algorithms use up to date state as required. > + */ > +void Algorithm::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a &stats) And stats should be a pointer here, not a reference too. With those, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > +{ > +} > + > } /* namespace ipa::ipu3 */ > > } /* namespace libcamera */ > + > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > index 072f01c4..89431005 100644 > --- a/src/ipa/ipu3/algorithms/algorithm.h > +++ b/src/ipa/ipu3/algorithms/algorithm.h > @@ -7,6 +7,9 @@ > #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > > +#include <libcamera/ipa/ipu3_ipa_interface.h> > + > +#include "ipa_context.h" > namespace libcamera { > > namespace ipa::ipu3 { > @@ -15,6 +18,12 @@ class Algorithm > { > public: > virtual ~Algorithm() {} > + > + virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo); > + > + virtual void prepare(IPAContext &context, ipu3_uapi_params ¶ms); > + > + virtual void process(IPAContext &context, const ipu3_uapi_stats_3a &stats); > }; > > } /* namespace ipa::ipu3 */ >
Hi Jean-Michel, Thank you for the patch. On Thu, Aug 19, 2021 at 04:22:49PM +0100, Kieran Bingham wrote: > On 19/08/2021 15:57, Jean-Michel Hautbois wrote: > > Introduce three functions in the Algorithm class to manage algorithms: > > - configure which is called when IPA is configured only > > - prepare called on EventFillParams event at each frame when the request > > is queued > > - process called on EventStatReady event at each frame completion when > > the statistics have been generated. > > > > The existing AGC implementation already has a function named process(), > > though it has different arguments. Adding the new virtual process() > > interface causes a compiler warning due to the AGC implementation > > overloading a virtual function, even though the overload can be resolved > > correctly. > > > > Temporarily disable the warning in this commit to maintain bisection > > until the AGC is converted to the new interface. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > meson.build | 4 ++ > > src/ipa/ipu3/algorithms/algorithm.cpp | 72 +++++++++++++++++++++++++++ > > src/ipa/ipu3/algorithms/algorithm.h | 9 ++++ > > 3 files changed, 85 insertions(+) > > > > diff --git a/meson.build b/meson.build > > index a49c484f..4a10e2b6 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -108,6 +108,10 @@ if cc.has_argument('-Wno-c99-designator') > > ] > > endif > > > > +# Do not warn when a function declaration hides virtual functions from > > +# a base class > > +cpp_arguments += '-Wno-overloaded-virtual' > > + > > c_arguments += common_arguments > > cpp_arguments += common_arguments > > > > diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/ipu3/algorithms/algorithm.cpp > > index dd46846a..ec3c152e 100644 > > --- a/src/ipa/ipu3/algorithms/algorithm.cpp > > +++ b/src/ipa/ipu3/algorithms/algorithm.cpp > > @@ -25,6 +25,78 @@ namespace ipa::ipu3 { > > * to manage algorithms regardless of their specific type. > > */ > > > > +/** > > + * \param[in] context The shared IPA context > > + * \param[in] configInfo The IPAConfigInfo structure reference Instead of describing the variable type, which is already conveyed by the type, I think it would be better to describe its purpose: * \param[in] configInfo The IPA configuration data, received from the pipeline * handler > > + * \return 0 if successful, an error code otherwise > > + * > > + * \brief Configure the Algorithm given an IPAConfigInfo > > + * Algorithms may implement a configure operation to pre-calculate > > + * parameters prior to commencing streaming. > > + * > > + * Configuration state may be stored in the IPASessionConfiguration structure of > > + * the IPAContext. We normally format doxygen blocks as /** * \brief ... * \param ... * * Documentation body * * \return ... */ Same for the functions below. > > + */ > > +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) You really don't like line wraps, do you ? :-) > > +{ > > + return 0; > > +} > > + > > +/** > > + * \param[in] context The shared IPA context > > + * \param[in] params The IPU3 specific parameters. > > params is an [out] not an in. > > > + * > > + * \brief Fill the parameter buffer with the updated context values Let's be more precise. * \brief Fill the \a params buffer with ISP processing parameters for a frame > > + * > > + * While streaming, the IPA will configure the IPU3 IMGU through it's It's usually spelled "ImgU", not "IMGU". s/it's/its/ > > + * parameter structure. How about making this describe when this function is called ? * This function is called while streaming for every frame before it is * processed by the ImgU, to prepare the ImgU processing parameters for that * frame. By the way, libcamera doesn't define a "streaming" concept, so at some point we'll either have to define "streaming" and apply it consistently through the documentation, or replace it with something else. > > + * > > + * Algorithms shall fill in the parameter structure fields appropriately to > > + * configure algorithm blocks that they are responsible for. Maybe "the ImgU processing blocks" instead of "algorithms block" ? I was about to write "hardware blocks", but it's sometimes firmware too. > > + * The structure should be updated to set the enable fields and use flags > > + * of any algorithms they are responsible for. Same comment about paragraph reflow as earlier in the series. To avoid repeating "they are responsible for", how about the following ? * Algorithms shall fill in the parameter structure fields appropriately to * configure the ImgU processing blocks that they are responsible for. This * includes setting fields and flags that enable those processing blocks. > > + */ > > +void Algorithm::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) > > I think params should be a pointer here, not a reference. > > > +{ > > +} > > + > > +/** > > + * \param[in] context The shared IPA context > > + * \param[in] stats The IPU3 statistics and ISP results > > + * > > + * \brief Process ISP statistics, and run algorithm operations. s/.$// > > + * > > + * While streaming, the IPA will receive the generated statistics from the > > + * IPU3 IMGU of processed frames. Similarly as above, * This function is called while streaming for every frame processed by the * ImgU, to process statistics generated from that frame by the ImgU. > > + * > > + * Algorithms shall use this data to run calculations and update their > > + * state accordingly. You could merge the above two paragraphs. > > + * > > + * Processing shall not take an undue amount of time, and any extended or > > + * computationally expensive calculations or operations must be handled > > + * asynchronously in a separate thread. > > + * > > + * Algorithms can store state in their respective IPAFrameContext > > + * structures, and reference state from the IPAFrameContext of other > > + * algorithms. You can reflow to 80 columns. > > + * > > + * \todo Historical data may be required as part of the processing. \todo covers a single paragraph only (that's how Doxygen processes them, it's not a coding rule from libcamera), so the blank line below needs to be removed otherwise the \todo item will only be the above line, and the next paragraph will be part of the documentation of the function. > > + * > > + * Either the previous frame, or the IPAFrameContext state of the frame > > + * that generated the statistics for this operation may be required for > > + * some advanced algorithms to prevent oscillations or support control > > + * loops correctly. Only a single IPAFrameContext is available currently, > > + * and so any data stored may represent the results of the previously > > + * completed operations. > > + * > > + * Care shall be taken to ensure the ordering of access to the information > > + * such that the algorithms use up to date state as required. > > + */ > > +void Algorithm::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a &stats) > > And stats should be a pointer here, not a reference too. > > With those, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +{ > > +} > > + > > } /* namespace ipa::ipu3 */ > > > > } /* namespace libcamera */ > > + Extra blank line. I've seen at least another instance of this when applying the previous version of the series with 'git am'. Could you please check all patches ? > > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > > index 072f01c4..89431005 100644 > > --- a/src/ipa/ipu3/algorithms/algorithm.h > > +++ b/src/ipa/ipu3/algorithms/algorithm.h > > @@ -7,6 +7,9 @@ > > #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > > #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > > > > +#include <libcamera/ipa/ipu3_ipa_interface.h> > > + > > +#include "ipa_context.h" Missing blank line. > > namespace libcamera { > > > > namespace ipa::ipu3 { > > @@ -15,6 +18,12 @@ class Algorithm > > { > > public: > > virtual ~Algorithm() {} > > + > > + virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo); > > + > > + virtual void prepare(IPAContext &context, ipu3_uapi_params ¶ms); > > + > > + virtual void process(IPAContext &context, const ipu3_uapi_stats_3a &stats); You could drop the blank lines between those three functions, up to you. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > }; > > > > } /* namespace ipa::ipu3 */
Hi Jean-Michel, Thank you for the patch. On Thu, Aug 19, 2021 at 04:57:55PM +0200, Jean-Michel Hautbois wrote: > Introduce a new algorithm to manage the tone mapping handling of the > IPU3. > > The initial algorithm is chosen to configure the gamma contrast curve > which moves the implementation out of AWB for simplicity. As it is > initialised with a default gamma value of 1.1, there is no need to use > the default table at initialisation anymore. > > This demonstrates the way to use process() call when the EventStatReady > comes in. The function calculates the LUT in the context of a frame, and > when prepare() is called, the parameters are filled with the updated > values. > > AGC is modified to take the new process interface into account. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/meson.build | 1 + > src/ipa/ipu3/algorithms/tone_mapping.cpp | 58 ++++++++++++++++++++++++ > src/ipa/ipu3/algorithms/tone_mapping.h | 32 +++++++++++++ > src/ipa/ipu3/ipa_context.h | 3 ++ > src/ipa/ipu3/ipu3.cpp | 17 ++++++- > src/ipa/ipu3/ipu3_agc.cpp | 5 +- > src/ipa/ipu3/ipu3_agc.h | 3 -- > src/ipa/ipu3/ipu3_awb.cpp | 41 +---------------- > src/ipa/ipu3/ipu3_awb.h | 2 +- > 9 files changed, 114 insertions(+), 48 deletions(-) > create mode 100644 src/ipa/ipu3/algorithms/tone_mapping.cpp > create mode 100644 src/ipa/ipu3/algorithms/tone_mapping.h > > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build > index dc538b79..71eedfa4 100644 > --- a/src/ipa/ipu3/algorithms/meson.build > +++ b/src/ipa/ipu3/algorithms/meson.build > @@ -2,4 +2,5 @@ > > ipu3_ipa_algorithms = files([ > 'algorithm.cpp', > + 'tone_mapping.cpp', > ]) > diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp > new file mode 100644 > index 00000000..0dd6755e > --- /dev/null > +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp > @@ -0,0 +1,58 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google inc. > + * > + * tone_mapping.cpp - IPU3 ToneMapping and Gamma control > + */ > + > +#include "tone_mapping.h" > + > +#include <cmath> > +#include <string.h> > + > +namespace libcamera { > + > +namespace ipa::ipu3::algorithms { > + > +ToneMapping::ToneMapping() > + : gamma_(1.0) > +{ > +} > + > +void ToneMapping::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params ¶ms) > +{ > + /* Copy the calculated LUT into the parameters buffer. */ > + memcpy(params.acc_param.gamma.gc_lut.lut, > + context.frameContext.toneMapping.gammaCorrection.lut, > + IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * > + sizeof(params.acc_param.gamma.gc_lut.lut[0])); > + > + /* Enable the custom gamma table. */ > + params.use.acc_gamma = 1; > + params.acc_param.gamma.gc_ctrl.enable = 1; > +} > + > +void ToneMapping::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *&stats) Line wrap. > +{ > + /* > + * Hardcode gamma to 1.1 as a default for now. > + * > + * \todo Expose gamma control setting through the libcamera control API > + */ > + gamma_ = 1.1; > + > + struct ipu3_uapi_gamma_corr_lut &lut = > + context.frameContext.toneMapping.gammaCorrection; > + > + for (uint32_t i = 0; i < std::size(lut.lut); i++) { > + double j = static_cast<double>(i) / (std::size(lut.lut) - 1); > + double gamma = std::pow(j, 1.0 / gamma_); > + > + /* The output value is expressed on 13 bits. */ > + lut.lut[i] = gamma * 8191; > + } > +} > + > +} /* namespace ipa::ipu3::algorithms */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/ipu3/algorithms/tone_mapping.h b/src/ipa/ipu3/algorithms/tone_mapping.h > new file mode 100644 > index 00000000..24176728 > --- /dev/null > +++ b/src/ipa/ipu3/algorithms/tone_mapping.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google inc. > + * > + * tone_mapping.h - IPU3 ToneMapping and Gamma control > + */ > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_TONE_MAPPING_H__ > +#define __LIBCAMERA_IPU3_ALGORITHMS_TONE_MAPPING_H__ > + > +#include "algorithm.h" > + > +namespace libcamera { > + > +namespace ipa::ipu3::algorithms { > + > +class ToneMapping : public Algorithm > +{ > +public: > + ToneMapping(); > + > + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override; > + void process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) override; As commented by Kieran on the previous patch, the second argument should be either a const pointer or a const reference, not a reference to a pointer. > + > +private: > + double gamma_; > +}; > + > +} /* namespace ipa::ipu3::algorithms */ > + > +} /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_TONE_MAPPING_H__ */ > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index a031ab83..4b12f129 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -24,6 +24,9 @@ struct IPASessionConfiguration { > }; > > struct IPAFrameContext { > + struct { > + struct ipu3_uapi_gamma_corr_lut gammaCorrection; > + } toneMapping; > }; > > struct IPAContext { > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index fa09d9f2..edc2d911 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -30,6 +30,7 @@ > #include "libcamera/internal/mapped_framebuffer.h" > > #include "algorithms/algorithm.h" > +#include "algorithms/tone_mapping.h" > #include "ipu3_agc.h" > #include "ipu3_awb.h" > #include "libipa/camera_sensor_helper.h" > @@ -94,6 +95,17 @@ static constexpr uint32_t kMaxCellHeightPerSet = 56; > * \brief BDS output size configured by the pipeline handler > */ > > +/** > + * \struct IPAFrameContext::toneMapping > + * \brief Context for ToneMapping and Gamma control > + * > + * \var IPAFrameContext::toneMapping::gammaCorrection > + * \brief Per-pixel tone mapping implemented as a LUT > + * > + * The LUT structure is defined by the IPU3 kernel interface. See > + * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details. I'm not sure this paragraph brings much, given that the type of the field is ipu3_uapi_gamma_corr_lut. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(IPAIPU3) > @@ -225,6 +237,9 @@ int IPAIPU3::init(const IPASettings &settings, > > *ipaControls = ControlInfoMap(std::move(controls), controls::controls); > > + /* Construct our Algorithms */ > + algorithms_.push_back(std::make_unique<algorithms::ToneMapping>()); > + > return 0; > } > > @@ -422,7 +437,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > algo->prepare(context_, params_); > > if (agcAlgo_->updateControls()) > - awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma()); > + awbAlgo_->updateWbParameters(params_); > > *params = params_; > > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp > index 408eb849..20c2d3b4 100644 > --- a/src/ipa/ipu3/ipu3_agc.cpp > +++ b/src/ipa/ipu3/ipu3_agc.cpp > @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8; > > IPU3Agc::IPU3Agc() > : frameCount_(0), lastFrame_(0), converged_(false), > - updateControls_(false), iqMean_(0.0), gamma_(1.0), > + updateControls_(false), iqMean_(0.0), > lineDuration_(0s), maxExposureTime_(0s), > prevExposure_(0s), prevExposureNoDg_(0s), > currentExposure_(0s), currentExposureNoDg_(0s) > @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats) > } > } > > - /* Limit the gamma effect for now */ > - gamma_ = 1.1; > - > /* Estimate the quantile mean of the top 2% of the histogram */ > iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0); > } > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h > index f00b98d6..2d86127d 100644 > --- a/src/ipa/ipu3/ipu3_agc.h > +++ b/src/ipa/ipu3/ipu3_agc.h > @@ -33,8 +33,6 @@ public: > void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain); > bool converged() { return converged_; } > bool updateControls() { return updateControls_; } > - /* \todo Use a metadata exchange between IPAs */ > - double gamma() { return gamma_; } > > private: > void processBrightness(const ipu3_uapi_stats_3a *stats); > @@ -50,7 +48,6 @@ private: > bool updateControls_; > > double iqMean_; > - double gamma_; > > Duration lineDuration_; > Duration maxExposureTime_; > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp > index 4bb321b3..c2d9e0c1 100644 > --- a/src/ipa/ipu3/ipu3_awb.cpp > +++ b/src/ipa/ipu3/ipu3_awb.cpp > @@ -133,31 +133,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = { > 0, 0, 8191, 0 > }; > > -/* Default settings for Gamma correction */ > -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { { > - 63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287, > - 303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511, > - 527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735, > - 751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959, > - 975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151, > - 1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343, > - 1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535, > - 1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727, > - 1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919, > - 1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111, > - 2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495, > - 2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879, > - 2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263, > - 3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647, > - 3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031, > - 4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671, > - 4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439, > - 5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207, > - 6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975, > - 7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743, > - 7807, 7871, 7935, 7999, 8063, 8127, 8191 > -} }; > - > IPU3Awb::IPU3Awb() > : Algorithm() > { > @@ -197,10 +172,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st > params.use.acc_ccm = 1; > params.acc_param.ccm = imguCssCcmDefault; > > - params.use.acc_gamma = 1; > - params.acc_param.gamma.gc_lut = imguCssGammaLut; > - params.acc_param.gamma.gc_ctrl.enable = 1; > - > zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY); > } > > @@ -350,7 +321,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats) > } > } > > -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma) > +void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms) > { > /* > * Green gains should not be touched and considered 1. > @@ -362,18 +333,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma) > params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain; > params.acc_param.bnr.wb_gains.gb = 16; > > - LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK > - << " and gamma calculated: " << agcGamma; > + LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK; > > /* The CCM matrix may change when color temperature will be used */ > params.acc_param.ccm = imguCssCcmDefault; > - > - for (uint32_t i = 0; i < 256; i++) { > - double j = i / 255.0; > - double gamma = std::pow(j, 1.0 / agcGamma); > - /* The maximum value 255 is represented on 13 bits in the IPU3 */ > - params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191; > - } > } > > } /* namespace ipa::ipu3 */ > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h > index ea2d4320..eeb2920b 100644 > --- a/src/ipa/ipu3/ipu3_awb.h > +++ b/src/ipa/ipu3/ipu3_awb.h > @@ -31,7 +31,7 @@ public: > > 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, double agcGamma); > + void updateWbParameters(ipu3_uapi_params ¶ms); > > struct Ipu3AwbCell { > unsigned char greenRedAvg;
Hi Jean-Michel, Thank you for the patch. On Thu, Aug 19, 2021 at 04:57:57PM +0200, Jean-Michel Hautbois wrote: > In preparation for using the AGC through the new algorithm interfaces, > convert the existing code to use the new function types. > > Now that the process call is rewritten, re-enable the compiler flag to > warn when a function declaration hides virtual functions from a base class > (-Woverloaded-virtual). > > We never use converged_ so remove its declaration. > The controls may not need to be updated at each call, but it should be > decided on the context side and not by a specific call by using a lock > status in the Agc structure for instance. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > meson.build | 4 ---- > src/ipa/ipu3/ipa_context.h | 5 +++++ > src/ipa/ipu3/ipu3.cpp | 33 +++++++++++++++++++++++++-------- > src/ipa/ipu3/ipu3_agc.cpp | 24 +++++++++++------------- > src/ipa/ipu3/ipu3_agc.h | 9 ++------- > 5 files changed, 43 insertions(+), 32 deletions(-) > > diff --git a/meson.build b/meson.build > index 4a10e2b6..a49c484f 100644 > --- a/meson.build > +++ b/meson.build > @@ -108,10 +108,6 @@ if cc.has_argument('-Wno-c99-designator') > ] > endif > > -# Do not warn when a function declaration hides virtual functions from > -# a base class > -cpp_arguments += '-Wno-overloaded-virtual' > - > c_arguments += common_arguments > cpp_arguments += common_arguments > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index 24dd8bf7..9d9444dc 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -24,6 +24,11 @@ struct IPASessionConfiguration { > }; > > struct IPAFrameContext { > + struct { > + uint32_t exposure; > + double gain; > + } agc; > + > struct { > struct { > double red; > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index cede897e..33e21993 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -95,6 +95,22 @@ static constexpr uint32_t kMaxCellHeightPerSet = 56; > * \brief BDS output size configured by the pipeline handler > */ > > +/** > + * \struct IPAFrameContext::agc > + * \brief Context for the Automatic Gain Control algorithm > + * > + * The exposure and gain determined are expected to be applied to the sensor > + * at the earliest opportunity. > + * > + * \var IPAFrameContext::agc::exposure > + * \brief Exposure time expressed as a number of lines > + * > + * \var IPAFrameContext::agc::gain > + * \brief Analogue gain multiplier > + * > + * The gain should be adapted to the sensor specific gain code before applying. > + */ > + > /** > * \struct IPAFrameContext::awb > * \brief Context for the Automatic White Balance algorithm > @@ -375,7 +391,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) > > awbAlgo_ = std::make_unique<IPU3Awb>(); > agcAlgo_ = std::make_unique<IPU3Agc>(); > - agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_); > + agcAlgo_->configure(context_, configInfo); > > return 0; > } > @@ -452,8 +468,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > for (auto const &algo : algorithms_) > algo->prepare(context_, params_); > > - if (agcAlgo_->updateControls()) > - awbAlgo_->prepare(context_, params_); > + awbAlgo_->prepare(context_, params_); > > *params = params_; > > @@ -472,14 +487,16 @@ void IPAIPU3::parseStatistics(unsigned int frame, > for (auto const &algo : algorithms_) > algo->process(context_, stats); > > - double gain = camHelper_->gain(gain_); > - agcAlgo_->process(stats, exposure_, gain); > - gain_ = camHelper_->gainCode(gain); > + /* \todo These fields should not be written by the IPAIPU3 layer */ > + context_.frameContext.agc.gain = camHelper_->gain(gain_); > + context_.frameContext.agc.exposure = exposure_; > + agcAlgo_->process(context_, stats); > + exposure_ = context_.frameContext.agc.exposure; > + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); > > awbAlgo_->process(context_, stats); > > - if (agcAlgo_->updateControls()) > - setControls(frame); > + setControls(frame); > > /* \todo Use VBlank value calculated from each frame exposure. */ > int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) / > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp > index 20c2d3b4..30888e10 100644 > --- a/src/ipa/ipu3/ipu3_agc.cpp > +++ b/src/ipa/ipu3/ipu3_agc.cpp > @@ -51,20 +51,21 @@ static constexpr double kEvGainTarget = 0.5; > static constexpr uint8_t kCellSize = 8; > > IPU3Agc::IPU3Agc() > - : frameCount_(0), lastFrame_(0), converged_(false), > - updateControls_(false), iqMean_(0.0), > - lineDuration_(0s), maxExposureTime_(0s), > - prevExposure_(0s), prevExposureNoDg_(0s), > + : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), > + maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s), > currentExposure_(0s), currentExposureNoDg_(0s) > { > } > > -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo) > +int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > { > - aeGrid_ = bdsGrid; > + aeGrid_ = context.configuration.grid.bdsGrid; Do we need to keep a copy of the grid, can't we git it from the context when needed ? It doesn't have to be addressed in this patch, could be done on top if easier. > > - lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; > + lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s > + / configInfo.sensorInfo.pixelRate; > maxExposureTime_ = kMaxExposure * lineDuration_; > + > + return 0; > } > > void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats) > @@ -144,8 +145,6 @@ void IPU3Agc::filterExposure() > > void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) > { > - updateControls_ = false; > - > /* Algorithm initialization should wait for first valid frames */ > /* \todo - have a number of frames given by DelayedControls ? > * - implement a function for IIR */ > @@ -155,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) > /* Are we correctly exposed ? */ > if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { > LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_; > - converged_ = true; > } else { > double newGain = kEvGainTarget * knumHistogramBins / iqMean_; > > @@ -178,20 +176,20 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) > exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure); > newExposure = currentExposure_ / exposure; > gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); > - updateControls_ = true; > } else if (currentShutter >= maxExposureTime_) { > gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); > newExposure = currentExposure_ / gain; > exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure); > - updateControls_ = true; > } > LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain; > } > lastFrame_ = frameCount_; > } > > -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain) > +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) > { > + uint32_t &exposure = context.frameContext.agc.exposure; > + double &gain = context.frameContext.agc.gain; > processBrightness(stats); > lockExposureGain(exposure, gain); > frameCount_++; > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h > index 2d86127d..0bd70021 100644 > --- a/src/ipa/ipu3/ipu3_agc.h > +++ b/src/ipa/ipu3/ipu3_agc.h > @@ -29,10 +29,8 @@ public: > IPU3Agc(); > ~IPU3Agc() = default; > > - void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo); > - void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain); > - bool converged() { return converged_; } > - bool updateControls() { return updateControls_; } > + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > + void process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) override; > > private: > void processBrightness(const ipu3_uapi_stats_3a *stats); > @@ -44,9 +42,6 @@ private: > uint64_t frameCount_; > uint64_t lastFrame_; > > - bool converged_; > - bool updateControls_; > - > double iqMean_; > > Duration lineDuration_;
Hi Laurent, On 20/08/2021 03:12, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Thu, Aug 19, 2021 at 04:57:57PM +0200, Jean-Michel Hautbois wrote: >> In preparation for using the AGC through the new algorithm interfaces, >> convert the existing code to use the new function types. >> >> Now that the process call is rewritten, re-enable the compiler flag to >> warn when a function declaration hides virtual functions from a base class >> (-Woverloaded-virtual). >> >> We never use converged_ so remove its declaration. >> The controls may not need to be updated at each call, but it should be >> decided on the context side and not by a specific call by using a lock >> status in the Agc structure for instance. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> meson.build | 4 ---- >> src/ipa/ipu3/ipa_context.h | 5 +++++ >> src/ipa/ipu3/ipu3.cpp | 33 +++++++++++++++++++++++++-------- >> src/ipa/ipu3/ipu3_agc.cpp | 24 +++++++++++------------- >> src/ipa/ipu3/ipu3_agc.h | 9 ++------- >> 5 files changed, 43 insertions(+), 32 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index 4a10e2b6..a49c484f 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -108,10 +108,6 @@ if cc.has_argument('-Wno-c99-designator') >> ] >> endif >> >> -# Do not warn when a function declaration hides virtual functions from >> -# a base class >> -cpp_arguments += '-Wno-overloaded-virtual' >> - >> c_arguments += common_arguments >> cpp_arguments += common_arguments >> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h >> index 24dd8bf7..9d9444dc 100644 >> --- a/src/ipa/ipu3/ipa_context.h >> +++ b/src/ipa/ipu3/ipa_context.h >> @@ -24,6 +24,11 @@ struct IPASessionConfiguration { >> }; >> >> struct IPAFrameContext { >> + struct { >> + uint32_t exposure; >> + double gain; >> + } agc; >> + >> struct { >> struct { >> double red; >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index cede897e..33e21993 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -95,6 +95,22 @@ static constexpr uint32_t kMaxCellHeightPerSet = 56; >> * \brief BDS output size configured by the pipeline handler >> */ >> >> +/** >> + * \struct IPAFrameContext::agc >> + * \brief Context for the Automatic Gain Control algorithm >> + * >> + * The exposure and gain determined are expected to be applied to the sensor >> + * at the earliest opportunity. >> + * >> + * \var IPAFrameContext::agc::exposure >> + * \brief Exposure time expressed as a number of lines >> + * >> + * \var IPAFrameContext::agc::gain >> + * \brief Analogue gain multiplier >> + * >> + * The gain should be adapted to the sensor specific gain code before applying. >> + */ >> + >> /** >> * \struct IPAFrameContext::awb >> * \brief Context for the Automatic White Balance algorithm >> @@ -375,7 +391,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) >> >> awbAlgo_ = std::make_unique<IPU3Awb>(); >> agcAlgo_ = std::make_unique<IPU3Agc>(); >> - agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_); >> + agcAlgo_->configure(context_, configInfo); >> >> return 0; >> } >> @@ -452,8 +468,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >> for (auto const &algo : algorithms_) >> algo->prepare(context_, params_); >> >> - if (agcAlgo_->updateControls()) >> - awbAlgo_->prepare(context_, params_); >> + awbAlgo_->prepare(context_, params_); >> >> *params = params_; >> >> @@ -472,14 +487,16 @@ void IPAIPU3::parseStatistics(unsigned int frame, >> for (auto const &algo : algorithms_) >> algo->process(context_, stats); >> >> - double gain = camHelper_->gain(gain_); >> - agcAlgo_->process(stats, exposure_, gain); >> - gain_ = camHelper_->gainCode(gain); >> + /* \todo These fields should not be written by the IPAIPU3 layer */ >> + context_.frameContext.agc.gain = camHelper_->gain(gain_); >> + context_.frameContext.agc.exposure = exposure_; >> + agcAlgo_->process(context_, stats); >> + exposure_ = context_.frameContext.agc.exposure; >> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); >> >> awbAlgo_->process(context_, stats); >> >> - if (agcAlgo_->updateControls()) >> - setControls(frame); >> + setControls(frame); >> >> /* \todo Use VBlank value calculated from each frame exposure. */ >> int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) / >> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp >> index 20c2d3b4..30888e10 100644 >> --- a/src/ipa/ipu3/ipu3_agc.cpp >> +++ b/src/ipa/ipu3/ipu3_agc.cpp >> @@ -51,20 +51,21 @@ static constexpr double kEvGainTarget = 0.5; >> static constexpr uint8_t kCellSize = 8; >> >> IPU3Agc::IPU3Agc() >> - : frameCount_(0), lastFrame_(0), converged_(false), >> - updateControls_(false), iqMean_(0.0), >> - lineDuration_(0s), maxExposureTime_(0s), >> - prevExposure_(0s), prevExposureNoDg_(0s), >> + : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), >> + maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s), >> currentExposure_(0s), currentExposureNoDg_(0s) >> { >> } >> >> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo) >> +int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >> { >> - aeGrid_ = bdsGrid; >> + aeGrid_ = context.configuration.grid.bdsGrid; > > Do we need to keep a copy of the grid, can't we git it from the context > when needed ? It doesn't have to be addressed in this patch, could be > done on top if easier. > Yes, I want it to be done on top because this is more than what this patch does (it is converting AGC to the interface, and as the grid is used in two places it would be a bit too much, according to me :-)). >> >> - lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; >> + lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s >> + / configInfo.sensorInfo.pixelRate; >> maxExposureTime_ = kMaxExposure * lineDuration_; >> + >> + return 0; >> } >> >> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats) >> @@ -144,8 +145,6 @@ void IPU3Agc::filterExposure() >> >> void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) >> { >> - updateControls_ = false; >> - >> /* Algorithm initialization should wait for first valid frames */ >> /* \todo - have a number of frames given by DelayedControls ? >> * - implement a function for IIR */ >> @@ -155,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) >> /* Are we correctly exposed ? */ >> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { >> LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_; >> - converged_ = true; >> } else { >> double newGain = kEvGainTarget * knumHistogramBins / iqMean_; >> >> @@ -178,20 +176,20 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) >> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure); >> newExposure = currentExposure_ / exposure; >> gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); >> - updateControls_ = true; >> } else if (currentShutter >= maxExposureTime_) { >> gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); >> newExposure = currentExposure_ / gain; >> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure); >> - updateControls_ = true; >> } >> LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain; >> } >> lastFrame_ = frameCount_; >> } >> >> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain) >> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) >> { >> + uint32_t &exposure = context.frameContext.agc.exposure; >> + double &gain = context.frameContext.agc.gain; >> processBrightness(stats); >> lockExposureGain(exposure, gain); >> frameCount_++; >> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h >> index 2d86127d..0bd70021 100644 >> --- a/src/ipa/ipu3/ipu3_agc.h >> +++ b/src/ipa/ipu3/ipu3_agc.h >> @@ -29,10 +29,8 @@ public: >> IPU3Agc(); >> ~IPU3Agc() = default; >> >> - void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo); >> - void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain); >> - bool converged() { return converged_; } >> - bool updateControls() { return updateControls_; } >> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >> + void process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) override; >> >> private: >> void processBrightness(const ipu3_uapi_stats_3a *stats); >> @@ -44,9 +42,6 @@ private: >> uint64_t frameCount_; >> uint64_t lastFrame_; >> >> - bool converged_; >> - bool updateControls_; >> - >> double iqMean_; >> >> Duration lineDuration_; >
Hi Laurent, On 20/08/2021 02:50, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Thu, Aug 19, 2021 at 04:22:49PM +0100, Kieran Bingham wrote: >> On 19/08/2021 15:57, Jean-Michel Hautbois wrote: >>> Introduce three functions in the Algorithm class to manage algorithms: >>> - configure which is called when IPA is configured only >>> - prepare called on EventFillParams event at each frame when the request >>> is queued >>> - process called on EventStatReady event at each frame completion when >>> the statistics have been generated. >>> >>> The existing AGC implementation already has a function named process(), >>> though it has different arguments. Adding the new virtual process() >>> interface causes a compiler warning due to the AGC implementation >>> overloading a virtual function, even though the overload can be resolved >>> correctly. >>> >>> Temporarily disable the warning in this commit to maintain bisection >>> until the AGC is converted to the new interface. >>> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> meson.build | 4 ++ >>> src/ipa/ipu3/algorithms/algorithm.cpp | 72 +++++++++++++++++++++++++++ >>> src/ipa/ipu3/algorithms/algorithm.h | 9 ++++ >>> 3 files changed, 85 insertions(+) >>> >>> diff --git a/meson.build b/meson.build >>> index a49c484f..4a10e2b6 100644 >>> --- a/meson.build >>> +++ b/meson.build >>> @@ -108,6 +108,10 @@ if cc.has_argument('-Wno-c99-designator') >>> ] >>> endif >>> >>> +# Do not warn when a function declaration hides virtual functions from >>> +# a base class >>> +cpp_arguments += '-Wno-overloaded-virtual' >>> + >>> c_arguments += common_arguments >>> cpp_arguments += common_arguments >>> >>> diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/ipu3/algorithms/algorithm.cpp >>> index dd46846a..ec3c152e 100644 >>> --- a/src/ipa/ipu3/algorithms/algorithm.cpp >>> +++ b/src/ipa/ipu3/algorithms/algorithm.cpp >>> @@ -25,6 +25,78 @@ namespace ipa::ipu3 { >>> * to manage algorithms regardless of their specific type. >>> */ >>> >>> +/** >>> + * \param[in] context The shared IPA context >>> + * \param[in] configInfo The IPAConfigInfo structure reference > > Instead of describing the variable type, which is already conveyed by > the type, I think it would be better to describe its purpose: > > * \param[in] configInfo The IPA configuration data, received from the pipeline > * handler > >>> + * \return 0 if successful, an error code otherwise >>> + * >>> + * \brief Configure the Algorithm given an IPAConfigInfo >>> + * Algorithms may implement a configure operation to pre-calculate >>> + * parameters prior to commencing streaming. >>> + * >>> + * Configuration state may be stored in the IPASessionConfiguration structure of >>> + * the IPAContext. > > We normally format doxygen blocks as > > /** > * \brief ... > * \param ... > * > * Documentation body > * > * \return ... > */ > > Same for the functions below. > >>> + */ >>> +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) > > You really don't like line wraps, do you ? :-) > >>> +{ >>> + return 0; >>> +} >>> + >>> +/** >>> + * \param[in] context The shared IPA context >>> + * \param[in] params The IPU3 specific parameters. >> >> params is an [out] not an in. >> >>> + * >>> + * \brief Fill the parameter buffer with the updated context values > > Let's be more precise. > > * \brief Fill the \a params buffer with ISP processing parameters for a frame > >>> + * >>> + * While streaming, the IPA will configure the IPU3 IMGU through it's > > It's usually spelled "ImgU", not "IMGU". > > s/it's/its/ > >>> + * parameter structure. > > How about making this describe when this function is called ? > > * This function is called while streaming for every frame before it is > * processed by the ImgU, to prepare the ImgU processing parameters for that > * frame. > > By the way, libcamera doesn't define a "streaming" concept, so at some > point we'll either have to define "streaming" and apply it consistently > through the documentation, or replace it with something else. > OK, should I change it maybe then :-). The camera is, when this is called, in running state, so I can still mention that with: * This function is called for every frame when the camera is running * before it is processed by the ImgU to prepare the ImgU processing * parameters for that frame." What do you think ? >>> + * >>> + * Algorithms shall fill in the parameter structure fields appropriately to >>> + * configure algorithm blocks that they are responsible for. > > Maybe "the ImgU processing blocks" instead of "algorithms block" ? I was > about to write "hardware blocks", but it's sometimes firmware too. > >>> + * The structure should be updated to set the enable fields and use flags >>> + * of any algorithms they are responsible for. > > Same comment about paragraph reflow as earlier in the series. > > To avoid repeating "they are responsible for", how about the following ? > > * Algorithms shall fill in the parameter structure fields appropriately to > * configure the ImgU processing blocks that they are responsible for. This > * includes setting fields and flags that enable those processing blocks. > >>> + */ >>> +void Algorithm::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) >> >> I think params should be a pointer here, not a reference. >> >>> +{ >>> +} >>> + >>> +/** >>> + * \param[in] context The shared IPA context >>> + * \param[in] stats The IPU3 statistics and ISP results >>> + * >>> + * \brief Process ISP statistics, and run algorithm operations. > > s/.$// > >>> + * >>> + * While streaming, the IPA will receive the generated statistics from the >>> + * IPU3 IMGU of processed frames. > > Similarly as above, > > * This function is called while streaming for every frame processed by the > * ImgU, to process statistics generated from that frame by the ImgU. > >>> + * >>> + * Algorithms shall use this data to run calculations and update their >>> + * state accordingly. > > You could merge the above two paragraphs. > >>> + * >>> + * Processing shall not take an undue amount of time, and any extended or >>> + * computationally expensive calculations or operations must be handled >>> + * asynchronously in a separate thread. >>> + * >>> + * Algorithms can store state in their respective IPAFrameContext >>> + * structures, and reference state from the IPAFrameContext of other >>> + * algorithms. > > You can reflow to 80 columns. > >>> + * >>> + * \todo Historical data may be required as part of the processing. > > \todo covers a single paragraph only (that's how Doxygen processes them, > it's not a coding rule from libcamera), so the blank line below needs to > be removed otherwise the \todo item will only be the above line, and the > next paragraph will be part of the documentation of the function. > >>> + * >>> + * Either the previous frame, or the IPAFrameContext state of the frame >>> + * that generated the statistics for this operation may be required for >>> + * some advanced algorithms to prevent oscillations or support control >>> + * loops correctly. Only a single IPAFrameContext is available currently, >>> + * and so any data stored may represent the results of the previously >>> + * completed operations. >>> + * >>> + * Care shall be taken to ensure the ordering of access to the information >>> + * such that the algorithms use up to date state as required. >>> + */ >>> +void Algorithm::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a &stats) >> >> And stats should be a pointer here, not a reference too. >> >> With those, >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> +{ >>> +} >>> + >>> } /* namespace ipa::ipu3 */ >>> >>> } /* namespace libcamera */ >>> + > > Extra blank line. > > I've seen at least another instance of this when applying the previous > version of the series with 'git am'. Could you please check all patches > ? > >>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h >>> index 072f01c4..89431005 100644 >>> --- a/src/ipa/ipu3/algorithms/algorithm.h >>> +++ b/src/ipa/ipu3/algorithms/algorithm.h >>> @@ -7,6 +7,9 @@ >>> #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ >>> #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ >>> >>> +#include <libcamera/ipa/ipu3_ipa_interface.h> >>> + >>> +#include "ipa_context.h" > > Missing blank line. > >>> namespace libcamera { >>> >>> namespace ipa::ipu3 { >>> @@ -15,6 +18,12 @@ class Algorithm >>> { >>> public: >>> virtual ~Algorithm() {} >>> + >>> + virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo); >>> + >>> + virtual void prepare(IPAContext &context, ipu3_uapi_params ¶ms); >>> + >>> + virtual void process(IPAContext &context, const ipu3_uapi_stats_3a &stats); > > You could drop the blank lines between those three functions, up to you. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> }; >>> >>> } /* namespace ipa::ipu3 */ >
Hi Jean-Michel, On Fri, Aug 20, 2021 at 07:33:02AM +0200, Jean-Michel Hautbois wrote: > On 20/08/2021 03:12, Laurent Pinchart wrote: > > On Thu, Aug 19, 2021 at 04:57:57PM +0200, Jean-Michel Hautbois wrote: > >> In preparation for using the AGC through the new algorithm interfaces, > >> convert the existing code to use the new function types. > >> > >> Now that the process call is rewritten, re-enable the compiler flag to > >> warn when a function declaration hides virtual functions from a base class > >> (-Woverloaded-virtual). > >> > >> We never use converged_ so remove its declaration. > >> The controls may not need to be updated at each call, but it should be > >> decided on the context side and not by a specific call by using a lock > >> status in the Agc structure for instance. > >> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> meson.build | 4 ---- > >> src/ipa/ipu3/ipa_context.h | 5 +++++ > >> src/ipa/ipu3/ipu3.cpp | 33 +++++++++++++++++++++++++-------- > >> src/ipa/ipu3/ipu3_agc.cpp | 24 +++++++++++------------- > >> src/ipa/ipu3/ipu3_agc.h | 9 ++------- > >> 5 files changed, 43 insertions(+), 32 deletions(-) > >> > >> diff --git a/meson.build b/meson.build > >> index 4a10e2b6..a49c484f 100644 > >> --- a/meson.build > >> +++ b/meson.build > >> @@ -108,10 +108,6 @@ if cc.has_argument('-Wno-c99-designator') > >> ] > >> endif > >> > >> -# Do not warn when a function declaration hides virtual functions from > >> -# a base class > >> -cpp_arguments += '-Wno-overloaded-virtual' > >> - > >> c_arguments += common_arguments > >> cpp_arguments += common_arguments > >> > >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > >> index 24dd8bf7..9d9444dc 100644 > >> --- a/src/ipa/ipu3/ipa_context.h > >> +++ b/src/ipa/ipu3/ipa_context.h > >> @@ -24,6 +24,11 @@ struct IPASessionConfiguration { > >> }; > >> > >> struct IPAFrameContext { > >> + struct { > >> + uint32_t exposure; > >> + double gain; > >> + } agc; > >> + > >> struct { > >> struct { > >> double red; > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index cede897e..33e21993 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -95,6 +95,22 @@ static constexpr uint32_t kMaxCellHeightPerSet = 56; > >> * \brief BDS output size configured by the pipeline handler > >> */ > >> > >> +/** > >> + * \struct IPAFrameContext::agc > >> + * \brief Context for the Automatic Gain Control algorithm > >> + * > >> + * The exposure and gain determined are expected to be applied to the sensor > >> + * at the earliest opportunity. > >> + * > >> + * \var IPAFrameContext::agc::exposure > >> + * \brief Exposure time expressed as a number of lines > >> + * > >> + * \var IPAFrameContext::agc::gain > >> + * \brief Analogue gain multiplier > >> + * > >> + * The gain should be adapted to the sensor specific gain code before applying. > >> + */ > >> + > >> /** > >> * \struct IPAFrameContext::awb > >> * \brief Context for the Automatic White Balance algorithm > >> @@ -375,7 +391,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) > >> > >> awbAlgo_ = std::make_unique<IPU3Awb>(); > >> agcAlgo_ = std::make_unique<IPU3Agc>(); > >> - agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_); > >> + agcAlgo_->configure(context_, configInfo); > >> > >> return 0; > >> } > >> @@ -452,8 +468,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > >> for (auto const &algo : algorithms_) > >> algo->prepare(context_, params_); > >> > >> - if (agcAlgo_->updateControls()) > >> - awbAlgo_->prepare(context_, params_); > >> + awbAlgo_->prepare(context_, params_); > >> > >> *params = params_; > >> > >> @@ -472,14 +487,16 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >> for (auto const &algo : algorithms_) > >> algo->process(context_, stats); > >> > >> - double gain = camHelper_->gain(gain_); > >> - agcAlgo_->process(stats, exposure_, gain); > >> - gain_ = camHelper_->gainCode(gain); > >> + /* \todo These fields should not be written by the IPAIPU3 layer */ > >> + context_.frameContext.agc.gain = camHelper_->gain(gain_); > >> + context_.frameContext.agc.exposure = exposure_; > >> + agcAlgo_->process(context_, stats); > >> + exposure_ = context_.frameContext.agc.exposure; > >> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); > >> > >> awbAlgo_->process(context_, stats); > >> > >> - if (agcAlgo_->updateControls()) > >> - setControls(frame); > >> + setControls(frame); > >> > >> /* \todo Use VBlank value calculated from each frame exposure. */ > >> int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) / > >> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp > >> index 20c2d3b4..30888e10 100644 > >> --- a/src/ipa/ipu3/ipu3_agc.cpp > >> +++ b/src/ipa/ipu3/ipu3_agc.cpp > >> @@ -51,20 +51,21 @@ static constexpr double kEvGainTarget = 0.5; > >> static constexpr uint8_t kCellSize = 8; > >> > >> IPU3Agc::IPU3Agc() > >> - : frameCount_(0), lastFrame_(0), converged_(false), > >> - updateControls_(false), iqMean_(0.0), > >> - lineDuration_(0s), maxExposureTime_(0s), > >> - prevExposure_(0s), prevExposureNoDg_(0s), > >> + : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s), > >> + maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s), > >> currentExposure_(0s), currentExposureNoDg_(0s) > >> { > >> } > >> > >> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo) > >> +int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >> { > >> - aeGrid_ = bdsGrid; > >> + aeGrid_ = context.configuration.grid.bdsGrid; > > > > Do we need to keep a copy of the grid, can't we git it from the context > > when needed ? It doesn't have to be addressed in this patch, could be > > done on top if easier. > > Yes, I want it to be done on top because this is more than what this > patch does (it is converting AGC to the interface, and as the grid is > used in two places it would be a bit too much, according to me :-)). Sure, no problem. Please record it so it's not forgotten. > >> > >> - lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; > >> + lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s > >> + / configInfo.sensorInfo.pixelRate; > >> maxExposureTime_ = kMaxExposure * lineDuration_; > >> + > >> + return 0; > >> } > >> > >> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats) > >> @@ -144,8 +145,6 @@ void IPU3Agc::filterExposure() > >> > >> void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) > >> { > >> - updateControls_ = false; > >> - > >> /* Algorithm initialization should wait for first valid frames */ > >> /* \todo - have a number of frames given by DelayedControls ? > >> * - implement a function for IIR */ > >> @@ -155,7 +154,6 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) > >> /* Are we correctly exposed ? */ > >> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) { > >> LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_; > >> - converged_ = true; > >> } else { > >> double newGain = kEvGainTarget * knumHistogramBins / iqMean_; > >> > >> @@ -178,20 +176,20 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain) > >> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure); > >> newExposure = currentExposure_ / exposure; > >> gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain); > >> - updateControls_ = true; > >> } else if (currentShutter >= maxExposureTime_) { > >> gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain); > >> newExposure = currentExposure_ / gain; > >> exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure); > >> - updateControls_ = true; > >> } > >> LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain; > >> } > >> lastFrame_ = frameCount_; > >> } > >> > >> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain) > >> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) > >> { > >> + uint32_t &exposure = context.frameContext.agc.exposure; > >> + double &gain = context.frameContext.agc.gain; > >> processBrightness(stats); > >> lockExposureGain(exposure, gain); > >> frameCount_++; > >> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h > >> index 2d86127d..0bd70021 100644 > >> --- a/src/ipa/ipu3/ipu3_agc.h > >> +++ b/src/ipa/ipu3/ipu3_agc.h > >> @@ -29,10 +29,8 @@ public: > >> IPU3Agc(); > >> ~IPU3Agc() = default; > >> > >> - void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo); > >> - void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain); > >> - bool converged() { return converged_; } > >> - bool updateControls() { return updateControls_; } > >> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > >> + void process(IPAContext &context, const ipu3_uapi_stats_3a *&stats) override; > >> > >> private: > >> void processBrightness(const ipu3_uapi_stats_3a *stats); > >> @@ -44,9 +42,6 @@ private: > >> uint64_t frameCount_; > >> uint64_t lastFrame_; > >> > >> - bool converged_; > >> - bool updateControls_; > >> - > >> double iqMean_; > >> > >> Duration lineDuration_;
Hi Jean-Michel, On Fri, Aug 20, 2021 at 08:13:04AM +0200, Jean-Michel Hautbois wrote: > On 20/08/2021 02:50, Laurent Pinchart wrote: > > On Thu, Aug 19, 2021 at 04:22:49PM +0100, Kieran Bingham wrote: > >> On 19/08/2021 15:57, Jean-Michel Hautbois wrote: > >>> Introduce three functions in the Algorithm class to manage algorithms: > >>> - configure which is called when IPA is configured only > >>> - prepare called on EventFillParams event at each frame when the request > >>> is queued > >>> - process called on EventStatReady event at each frame completion when > >>> the statistics have been generated. > >>> > >>> The existing AGC implementation already has a function named process(), > >>> though it has different arguments. Adding the new virtual process() > >>> interface causes a compiler warning due to the AGC implementation > >>> overloading a virtual function, even though the overload can be resolved > >>> correctly. > >>> > >>> Temporarily disable the warning in this commit to maintain bisection > >>> until the AGC is converted to the new interface. > >>> > >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >>> --- > >>> meson.build | 4 ++ > >>> src/ipa/ipu3/algorithms/algorithm.cpp | 72 +++++++++++++++++++++++++++ > >>> src/ipa/ipu3/algorithms/algorithm.h | 9 ++++ > >>> 3 files changed, 85 insertions(+) > >>> > >>> diff --git a/meson.build b/meson.build > >>> index a49c484f..4a10e2b6 100644 > >>> --- a/meson.build > >>> +++ b/meson.build > >>> @@ -108,6 +108,10 @@ if cc.has_argument('-Wno-c99-designator') > >>> ] > >>> endif > >>> > >>> +# Do not warn when a function declaration hides virtual functions from > >>> +# a base class > >>> +cpp_arguments += '-Wno-overloaded-virtual' > >>> + > >>> c_arguments += common_arguments > >>> cpp_arguments += common_arguments > >>> > >>> diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/ipu3/algorithms/algorithm.cpp > >>> index dd46846a..ec3c152e 100644 > >>> --- a/src/ipa/ipu3/algorithms/algorithm.cpp > >>> +++ b/src/ipa/ipu3/algorithms/algorithm.cpp > >>> @@ -25,6 +25,78 @@ namespace ipa::ipu3 { > >>> * to manage algorithms regardless of their specific type. > >>> */ > >>> > >>> +/** > >>> + * \param[in] context The shared IPA context > >>> + * \param[in] configInfo The IPAConfigInfo structure reference > > > > Instead of describing the variable type, which is already conveyed by > > the type, I think it would be better to describe its purpose: > > > > * \param[in] configInfo The IPA configuration data, received from the pipeline > > * handler > > > >>> + * \return 0 if successful, an error code otherwise > >>> + * > >>> + * \brief Configure the Algorithm given an IPAConfigInfo > >>> + * Algorithms may implement a configure operation to pre-calculate > >>> + * parameters prior to commencing streaming. > >>> + * > >>> + * Configuration state may be stored in the IPASessionConfiguration structure of > >>> + * the IPAContext. > > > > We normally format doxygen blocks as > > > > /** > > * \brief ... > > * \param ... > > * > > * Documentation body > > * > > * \return ... > > */ > > > > Same for the functions below. > > > >>> + */ > >>> +int Algorithm::configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) > > > > You really don't like line wraps, do you ? :-) > > > >>> +{ > >>> + return 0; > >>> +} > >>> + > >>> +/** > >>> + * \param[in] context The shared IPA context > >>> + * \param[in] params The IPU3 specific parameters. > >> > >> params is an [out] not an in. > >> > >>> + * > >>> + * \brief Fill the parameter buffer with the updated context values > > > > Let's be more precise. > > > > * \brief Fill the \a params buffer with ISP processing parameters for a frame > > > >>> + * > >>> + * While streaming, the IPA will configure the IPU3 IMGU through it's > > > > It's usually spelled "ImgU", not "IMGU". > > > > s/it's/its/ > > > >>> + * parameter structure. > > > > How about making this describe when this function is called ? > > > > * This function is called while streaming for every frame before it is > > * processed by the ImgU, to prepare the ImgU processing parameters for that > > * frame. > > > > By the way, libcamera doesn't define a "streaming" concept, so at some > > point we'll either have to define "streaming" and apply it consistently > > through the documentation, or replace it with something else. > > > > OK, should I change it maybe then :-). The camera is, when this is > called, in running state, so I can still mention that with: > > * This function is called for every frame when the camera is running > * before it is processed by the ImgU to prepare the ImgU processing > * parameters for that frame." > > What do you think ? Sounds good. > >>> + * > >>> + * Algorithms shall fill in the parameter structure fields appropriately to > >>> + * configure algorithm blocks that they are responsible for. > > > > Maybe "the ImgU processing blocks" instead of "algorithms block" ? I was > > about to write "hardware blocks", but it's sometimes firmware too. > > > >>> + * The structure should be updated to set the enable fields and use flags > >>> + * of any algorithms they are responsible for. > > > > Same comment about paragraph reflow as earlier in the series. > > > > To avoid repeating "they are responsible for", how about the following ? > > > > * Algorithms shall fill in the parameter structure fields appropriately to > > * configure the ImgU processing blocks that they are responsible for. This > > * includes setting fields and flags that enable those processing blocks. > > > >>> + */ > >>> +void Algorithm::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) > >> > >> I think params should be a pointer here, not a reference. > >> > >>> +{ > >>> +} > >>> + > >>> +/** > >>> + * \param[in] context The shared IPA context > >>> + * \param[in] stats The IPU3 statistics and ISP results > >>> + * > >>> + * \brief Process ISP statistics, and run algorithm operations. > > > > s/.$// > > > >>> + * > >>> + * While streaming, the IPA will receive the generated statistics from the > >>> + * IPU3 IMGU of processed frames. > > > > Similarly as above, > > > > * This function is called while streaming for every frame processed by the > > * ImgU, to process statistics generated from that frame by the ImgU. > > > >>> + * > >>> + * Algorithms shall use this data to run calculations and update their > >>> + * state accordingly. > > > > You could merge the above two paragraphs. > > > >>> + * > >>> + * Processing shall not take an undue amount of time, and any extended or > >>> + * computationally expensive calculations or operations must be handled > >>> + * asynchronously in a separate thread. > >>> + * > >>> + * Algorithms can store state in their respective IPAFrameContext > >>> + * structures, and reference state from the IPAFrameContext of other > >>> + * algorithms. > > > > You can reflow to 80 columns. > > > >>> + * > >>> + * \todo Historical data may be required as part of the processing. > > > > \todo covers a single paragraph only (that's how Doxygen processes them, > > it's not a coding rule from libcamera), so the blank line below needs to > > be removed otherwise the \todo item will only be the above line, and the > > next paragraph will be part of the documentation of the function. > > > >>> + * > >>> + * Either the previous frame, or the IPAFrameContext state of the frame > >>> + * that generated the statistics for this operation may be required for > >>> + * some advanced algorithms to prevent oscillations or support control > >>> + * loops correctly. Only a single IPAFrameContext is available currently, > >>> + * and so any data stored may represent the results of the previously > >>> + * completed operations. > >>> + * > >>> + * Care shall be taken to ensure the ordering of access to the information > >>> + * such that the algorithms use up to date state as required. > >>> + */ > >>> +void Algorithm::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a &stats) > >> > >> And stats should be a pointer here, not a reference too. > >> > >> With those, > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> +{ > >>> +} > >>> + > >>> } /* namespace ipa::ipu3 */ > >>> > >>> } /* namespace libcamera */ > >>> + > > > > Extra blank line. > > > > I've seen at least another instance of this when applying the previous > > version of the series with 'git am'. Could you please check all patches > > ? > > > >>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > >>> index 072f01c4..89431005 100644 > >>> --- a/src/ipa/ipu3/algorithms/algorithm.h > >>> +++ b/src/ipa/ipu3/algorithms/algorithm.h > >>> @@ -7,6 +7,9 @@ > >>> #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > >>> #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > >>> > >>> +#include <libcamera/ipa/ipu3_ipa_interface.h> > >>> + > >>> +#include "ipa_context.h" > > > > Missing blank line. > > > >>> namespace libcamera { > >>> > >>> namespace ipa::ipu3 { > >>> @@ -15,6 +18,12 @@ class Algorithm > >>> { > >>> public: > >>> virtual ~Algorithm() {} > >>> + > >>> + virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo); > >>> + > >>> + virtual void prepare(IPAContext &context, ipu3_uapi_params ¶ms); > >>> + > >>> + virtual void process(IPAContext &context, const ipu3_uapi_stats_3a &stats); > > > > You could drop the blank lines between those three functions, up to you. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >>> }; > >>> > >>> } /* namespace ipa::ipu3 */
This patch series splits the algorithms in IPU3 into a modular form. When the Metadata class was discussed a proposal from Kieran was to have a fully-defined structure which would be used to pass the context between algorithms and the IPAIPU3 class, as well as between the algorithms themselves. The algorithms are statically emplaced in a list for the moment, and a registration system may be used later. In order to demonstrate how to add a new algorithm, patch 5/9 moves the contrast algorithm from AWB into its own algorithm. Patch 6/9 and 7/9 change the AWB and AGC to use the new interface. And the two latest patches are indeed the usage of the new modular interface applied to AWB and AGC. Jean-Michel Hautbois (9): ipa: move libipa::Algorithm to ipa/ipu3/algorithms ipa: ipu3: Introduce a Context structure ipa: ipu3: Add the functions to the Algorithm class ipa: ipu3: Introduce modular algorithm ipa: ipu3: Introduce a modular tone mapping algorithm ipa: ipu3: convert AWB to the new algorithm interface ipa: ipu3: convert AGC to the new algorithm interface ipa: ipu3: Move IPU3 awb into algorithms ipa: ipu3: Move IPU3 agc into algorithms .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 41 ++--- src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 32 ++-- src/ipa/ipu3/algorithms/algorithm.cpp | 102 +++++++++++ src/ipa/ipu3/algorithms/algorithm.h | 33 ++++ .../ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} | 166 +++++++---------- src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} | 26 +-- src/ipa/ipu3/algorithms/meson.build | 8 + src/ipa/ipu3/algorithms/tone_mapping.cpp | 58 ++++++ src/ipa/ipu3/algorithms/tone_mapping.h | 32 ++++ src/ipa/ipu3/ipa_context.h | 54 ++++++ src/ipa/ipu3/ipu3.cpp | 172 +++++++++++++++--- src/ipa/ipu3/meson.build | 6 +- src/ipa/libipa/algorithm.cpp | 39 ---- src/ipa/libipa/algorithm.h | 24 --- src/ipa/libipa/libipa.cpp | 22 +++ src/ipa/libipa/meson.build | 5 +- 16 files changed, 565 insertions(+), 255 deletions(-) rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (87%) rename src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} (51%) create mode 100644 src/ipa/ipu3/algorithms/algorithm.cpp create mode 100644 src/ipa/ipu3/algorithms/algorithm.h rename src/ipa/ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} (71%) rename src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} (77%) create mode 100644 src/ipa/ipu3/algorithms/meson.build create mode 100644 src/ipa/ipu3/algorithms/tone_mapping.cpp create mode 100644 src/ipa/ipu3/algorithms/tone_mapping.h create mode 100644 src/ipa/ipu3/ipa_context.h delete mode 100644 src/ipa/libipa/algorithm.cpp delete mode 100644 src/ipa/libipa/algorithm.h create mode 100644 src/ipa/libipa/libipa.cpp