[libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap to IPA module

[Florian Sylvestre]

Hi Laurent,

On Wed, 3 Aug 2022 at 15:46, <paul.elder at ideasonboard.com> wrote:
>
> On Tue, Aug 02, 2022 at 03:13:30AM +0300, Laurent Pinchart wrote:
> > Currently the pipeline handler advertises controls handled by the IPA
> > from a ControlInfoMap it manually constructs. This is wrong, as the IPA
> > module is the component that knows what controls it supports. Fix this
> > by moving the ControlInfoMap construction to the IPA module, and pass it
> > to the pipeline handler as a return value from the IPA init() function.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
Reviewed-by: Florian Sylvestre <fsylvestre at baylibre.com>
>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  2 +-
> >  src/ipa/rkisp1/rkisp1.cpp                | 30 ++++++++++++++++++++---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----------------------
> >  3 files changed, 30 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index e35373852b7c..eaf3955e4096 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -11,7 +11,7 @@ import "include/libcamera/ipa/core.mojom";
> >  interface IPARkISP1Interface {
> >       init(libcamera.IPASettings settings,
> >            uint32 hwRevision)
> > -             => (int32 ret);
> > +             => (int32 ret, libcamera.ControlInfoMap ipaControls);
> >       start() => (int32 ret);
> >       stop();
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index e5010f6ab629..52f7143980fe 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -46,7 +46,8 @@ namespace ipa::rkisp1 {
> >  class IPARkISP1 : public IPARkISP1Interface, public Module
> >  {
> >  public:
> > -     int init(const IPASettings &settings, unsigned int hwRevision) override;
> > +     int init(const IPASettings &settings, unsigned int hwRevision,
> > +              ControlInfoMap *ipaControls) override;
> >       int start() override;
> >       void stop() override {}
> >
> > @@ -89,12 +90,27 @@ private:
> >       struct IPAContext context_;
> >  };
> >
> > +namespace {
> > +
> > +/* List of controls handled by the RkISP1 IPA */
> > +const ControlInfoMap::Map rkisp1Controls{
> > +     { &controls::AeEnable, ControlInfo(false, true) },
> > +     { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
> > +     { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> > +     { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> > +     { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> > +     { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> > +};
> > +
> > +} /* namespace */
> > +
> >  std::string IPARkISP1::logPrefix() const
> >  {
> >       return "rkisp1";
> >  }
> >
> > -int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> > +int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > +                 ControlInfoMap *ipaControls)
> >  {
> >       /* \todo Add support for other revisions */
> >       switch (hwRevision) {
> > @@ -156,7 +172,15 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> >               return -EINVAL;
> >       }
> >
> > -     return createAlgorithms(context_, (*data)["algorithms"]);
> > +     int ret = createAlgorithms(context_, (*data)["algorithms"]);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Return the controls handled by the IPA. */
> > +     ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > +     *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > +
> > +     return 0;
> >  }
> >
> >  int IPARkISP1::start()
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index de687f4dc6b0..932873329e89 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -344,7 +344,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >               ipaTuningFile = std::string(configFromEnv);
> >       }
> >
> > -     int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision);
> > +     int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > +                          &controlInfo_);
> >       if (ret < 0) {
> >               LOG(RkISP1, Error) << "IPA initialization failure";
> >               return ret;
> > @@ -967,34 +968,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >               std::make_unique<RkISP1CameraData>(this, &mainPath_,
> >                                                  hasSelfPath_ ? &selfPath_ : nullptr);
> >
> > -     ControlInfoMap::Map ctrls;
> > -     ctrls.emplace(std::piecewise_construct,
> > -                   std::forward_as_tuple(&controls::Sharpness),
> > -                   std::forward_as_tuple(0.0f, 10.0f, 1.0f));
> > -
> > -     ctrls.emplace(std::piecewise_construct,
> > -                   std::forward_as_tuple(&controls::Brightness),
> > -                   std::forward_as_tuple(-1.0f, 0.993f));
> > -
> > -     ctrls.emplace(std::piecewise_construct,
> > -                   std::forward_as_tuple(&controls::Contrast),
> > -                   std::forward_as_tuple(0.0f, 1.993f));
> > -
> > -     ctrls.emplace(std::piecewise_construct,
> > -                   std::forward_as_tuple(&controls::Saturation),
> > -                   std::forward_as_tuple(0.0f, 1.993f));
> > -
> > -     ctrls.emplace(std::piecewise_construct,
> > -                   std::forward_as_tuple(&controls::draft::NoiseReductionMode),
> > -                   std::forward_as_tuple(controls::draft::NoiseReductionModeValues));
> > -
> > -     ctrls.emplace(std::piecewise_construct,
> > -                   std::forward_as_tuple(&controls::AeEnable),
> > -                   std::forward_as_tuple(false, true));
> > -
> > -     data->controlInfo_ = ControlInfoMap(std::move(ctrls),
> > -                                         controls::controls);
> > -
> >       data->sensor_ = std::make_unique<CameraSensor>(sensor);
> >       ret = data->sensor_->init();
> >       if (ret)
> > --
> > Regards,
> >
> > Laurent Pinchart
> >



-- 
Florian Sylvestre