[libcamera-devel] [PATCH 1/7] ipa: Add sensor model string to IPASettings

[Laurent Pinchart]

Hi Naush,

On Tue, Mar 23, 2021 at 09:52:17AM +0000, Naushir Patuck wrote:
> On Mon, 22 Mar 2021 at 20:57, Laurent Pinchart wrote:
> > On Wed, Mar 17, 2021 at 10:02:05AM +0000, Naushir Patuck wrote:
> > > Pass the sensor model string to the IPA init() method through the
> > > IPASettings structure.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/core.mojom                   | 8 ++++++++
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++-
> > >  src/libcamera/pipeline/vimc/vimc.cpp               | 2 +-
> > >  test/ipa/ipa_interface_test.cpp                    | 2 +-
> > >  4 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > > index 5236a672461c..5363f1c5b48b 100644
> > > --- a/include/libcamera/ipa/core.mojom
> > > +++ b/include/libcamera/ipa/core.mojom
> > > @@ -145,8 +145,16 @@ struct IPABuffer {
> > >   * This field may be an empty string if the IPA doesn't require a configuration
> > >   * file.
> > >   */
> > > +
> > > + /**
> > > + * \var IPASettings::sensorModel
> > > + * \brief The sensor model name
> > > + *
> > > + * Provides the sensor model name to the IPA.
> > > + */
> > >  struct IPASettings {
> > >       string configurationFile;
> > > +     string sensorModel;
> > This makes complete sense for all IPAs. I wonder if we should remove it from CameraSensorInfo then, but that can be done on top of the series.
> >
> > >  };
> > >
> > >  /**
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 00fa62c972ed..2c8ae31a28ad 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1227,7 +1227,8 @@ int RPiCameraData::loadIPA()
> > >       ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> > >       ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
> > >
> > > -     IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
> > > +     IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"),
> > > +                          sensor_->model());
> > >
> > >       return ipa_->init(settings);
> > >  }
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 57c65b021c46..2dfb63ecf2ef 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -425,7 +425,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> > >       data->ipa_ = IPAManager::createIPA<ipa::vimc::IPAProxyVimc>(this, 0, 0);
> > >       if (data->ipa_ != nullptr) {
> > >               std::string conf = data->ipa_->configurationFile("vimc.conf");
> > > -             data->ipa_->init(IPASettings{ conf });
> > > +             data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
> > >       } else {
> > >               LOG(VIMC, Warning) << "no matching IPA found";
> > >       }
> > > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> > > index 4b88806f8f67..d6ca6f5137b0 100644
> > > --- a/test/ipa/ipa_interface_test.cpp
> > > +++ b/test/ipa/ipa_interface_test.cpp
> > > @@ -104,7 +104,7 @@ protected:
> > >
> > >               /* Test initialization of IPA module. */
> > >               std::string conf = ipa_->configurationFile("vimc.conf");
> > > -             int ret = ipa_->init(IPASettings{ conf });
> > > +             int ret = ipa_->init(IPASettings{ conf, "vimc" });
> > >               if (ret < 0) {
> > >                       cerr << "IPA interface init() failed" << endl;
> > >                       return TestFail;
> >
> > Could you please also update the ipu3 and rkisp1 pipeline handlers ?
> 
> I did intend on modifying them, but they call init() in slightly different
> ways:
> 
> ipu3: ipa_->init(IPASettings{});
> rkisp1: ipa_->init(hwRevision);
> 
> rkisp1 does not use IPASettings at all, so I left it out,

Fair eough :-)

> and ipu3 initialises an
> empty IPASettings object.  I could initialise ipu3 with the something like the
> following:
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2ea13ec9e1b9..0e9b05f2b771 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1144,7 +1144,8 @@ int IPU3CameraData::loadIPA()
> 
>         ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);
> 
> -       ipa_->init(IPASettings{});
> +       CameraSensor *sensor = cio2_.sensor();
> +       ipa_->init(IPASettings{"", sensor->model()});
> 
> Let me know what you think is appropriate.

That seems good to me. Thanks.

-- 
Regards,

Laurent Pinchart