[libcamera-devel] [RFC PATCH 4/4] ipa: ipu3: Add YAML tuning file support

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2022-08-02 11:09:55)
> Replace the manual instantiation of algorithms with an automatic
> mechanism based on a tuning data file, provided by the Module base
> class. This brings the IPU3 IPA module in line with the RkISP1 IPA
> module.

I like this a lot - it's really pulling out the benefits of the modular
components, and finally lets us get some custom data into the IPU3 IPA!

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 70 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672f7bb..e37b2fa0f30e 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -18,6 +18,7 @@
>  #include <linux/intel-ipu3.h>
>  #include <linux/v4l2-controls.h>
>  
> +#include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
>  
> @@ -29,6 +30,7 @@
>  #include <libcamera/request.h>
>  
>  #include "libcamera/internal/mapped_framebuffer.h"
> +#include "libcamera/internal/yaml_parser.h"
>  
>  #include "algorithms/af.h"
>  #include "algorithms/agc.h"
> @@ -71,7 +73,7 @@ namespace ipa::ipu3 {
>   *
>   * At initialisation time, a CameraSensorHelper is instantiated to support
>   * camera-specific calculations, while the default controls are computed, and
> - * the algorithms are constructed and placed in an ordered list.
> + * the algorithms are instantiated from the tuning data file.
>   *
>   * The IPU3 ImgU operates with a grid layout to divide the overall frame into
>   * rectangular cells of pixels. When the IPA is configured, we determine the
> @@ -92,12 +94,14 @@ namespace ipa::ipu3 {
>   * fillParamsBuffer() call.
>   *
>   * The individual algorithms are split into modular components that are called
> - * iteratively to allow them to process statistics from the ImgU in a defined
> - * order.
> + * iteratively to allow them to process statistics from the ImgU in the order
> + * defined in the tuning data file.
>   *
> - * The current implementation supports three core algorithms:
> - * - Automatic white balance (AWB)
> + * The current implementation supports five core algorithms:
> + *
> + * - Auto focus (AF)
>   * - Automatic gain and exposure control (AGC)
> + * - Automatic white balance (AWB)
>   * - Black level correction (BLC)
>   * - Tone mapping (Gamma)
>   *
> @@ -128,7 +132,7 @@ namespace ipa::ipu3 {
>   * sensor-specific tuning to adapt for Black Level compensation (BLC), Lens
>   * shading correction (SHD) and Color correction (CCM).
>   */
> -class IPAIPU3 : public IPAIPU3Interface
> +class IPAIPU3 : public IPAIPU3Interface, public Module
>  {
>  public:
>         int init(const IPASettings &settings,
> @@ -150,6 +154,10 @@ public:
>         void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,
>                                 const uint32_t bufferId,
>                                 const ControlList &sensorControls) override;
> +
> +protected:
> +       std::string logPrefix() const override;
> +
>  private:
>         void updateControls(const IPACameraSensorInfo &sensorInfo,
>                             const ControlInfoMap &sensorControls,
> @@ -171,13 +179,15 @@ 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_;
>  };
>  
> +std::string IPAIPU3::logPrefix() const
> +{
> +       return "ipu3";
> +}
> +
>  /**
>   * \brief Compute IPASessionConfiguration using the sensor information and the
>   * sensor V4L2 controls
> @@ -316,12 +326,36 @@ int IPAIPU3::init(const IPASettings &settings,
>         context_.configuration = {};
>         context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>  
> -       /* Construct our Algorithms */
> -       algorithms_.push_back(std::make_unique<algorithms::Af>());
> -       algorithms_.push_back(std::make_unique<algorithms::Agc>());
> -       algorithms_.push_back(std::make_unique<algorithms::Awb>());
> -       algorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());
> -       algorithms_.push_back(std::make_unique<algorithms::ToneMapping>());
> +       /* Load the tuning data file. */
> +       File file(settings.configurationFile.c_str());
> +       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> +               int ret = file.error();
> +               LOG(IPAIPU3, Error)
> +                       << "Failed to open configuration file "
> +                       << settings.configurationFile << ": " << strerror(-ret);
> +               return ret;
> +       }
> +
> +       std::unique_ptr<libcamera::YamlObject> data = YamlParser::parse(file);
> +       if (!data)
> +               return -EINVAL;
> +
> +       unsigned int version = (*data)["version"].get<uint32_t>(0);
> +       if (version != 1) {
> +               LOG(IPAIPU3, Error)
> +                       << "Invalid tuning file version " << version;
> +               return -EINVAL;
> +       }
> +
> +       if (!data->contains("algorithms")) {
> +               LOG(IPAIPU3, Error)
> +                       << "Tuning file doesn't contain any algorithm";
> +               return -EINVAL;
> +       }
> +
> +       int ret = createAlgorithms(context_, (*data)["algorithms"]);
> +       if (ret)
> +               return ret;

I'm curious about how we fail here.

I added an extra 'algorithm' to the tuning file (before I added it to
the build) and it causes the IPA to fail to load.

That seems like it might be a bit much to fail for, but we certainly
need to make sure we convey errors to the user if the tuning file isn't
expected. 

Do we have an existing statement of what failure expectations are for
tuning files? Should any failure to parse cause the IPA to fail to load?
or should there be a best effort at times (i.e. referencing an algorithm
that doesn't exist, could still operate, and that algorithm data
wouldn't be read...?)


Anyway, even with that failure - I expect this matches the RKISP
implementation - so keeping them consistent is expected, and if we
change the failure operations - that should be on top and cover both.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  
>         /* Initialize controls. */
>         updateControls(sensorInfo, sensorControls, ipaControls);
> @@ -470,7 +504,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>         /* Update the IPASessionConfiguration using the sensor settings. */
>         updateSessionConfiguration(sensorCtrls_);
>  
> -       for (auto const &algo : algorithms_) {
> +       for (auto const &algo : algorithms()) {
>                 int ret = algo->configure(context_, configInfo);
>                 if (ret)
>                         return ret;
> @@ -538,7 +572,7 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>          */
>         params->use = {};
>  
> -       for (auto const &algo : algorithms_)
> +       for (auto const &algo : algorithms())
>                 algo->prepare(context_, params);
>  
>         paramsBufferReady.emit(frame);
> @@ -581,7 +615,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>         int32_t vBlank = context_.configuration.sensor.defVBlank;
>         ControlList ctrls(controls::controls);
>  
> -       for (auto const &algo : algorithms_)
> +       for (auto const &algo : algorithms())
>                 algo->process(context_, &frameContext, stats);
>  
>         setControls(frame);
> -- 
> Regards,
> 
> Laurent Pinchart
>