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

[Laurent Pinchart]

On Thu, Aug 04, 2022 at 10:10:01AM +0100, Kieran Bingham wrote:
> 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...?)

You ask very good questions :-)

I've so far considered that most errors should be fatal (not in the
sense of causing the IPA to crash, but making the camera initialization
to fail), with clear messages to help debugging, because the tuning data
file is supposed to be generated by a tool and not by a human. There's
of course room for human edition of the file, given that we use YAML,
that we have no tool at the moment, and that removing or adding
algorithms manually is a very easy way to tune the behaviour of the IPA
module. I could consider turning high-level errors into warnings (for
instance when an algorithm doesn't exist), but for low-level parameters
(e.g. the values in a lens shading table) I think we should consider
errors fatal (but still log them properly as much as possible, to help
debugging). I'm not entirely sure where to set the limit though, the
issue with warnings is that they're often overlooked.

> 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.

And it would only need to be done once,  in the base Module class :-)

> 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