[libcamera-devel] [PATCH v1 3/6] ipa: ipu3: Introduce IPAConfigInfo in IPC
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue May 18 11:43:32 CEST 2021
Hi Laurent,
On Sat, May 15, 2021 at 07:30:13PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Fri, May 14, 2021 at 10:57:24AM +0100, Kieran Bingham wrote:
> > On 14/05/2021 08:58, Umang Jain wrote:
> > > IPAConfigInfo is a consolidated data structure passed from IPU3
> > > pipeline-handler to IPU3 IPA. The structure can be extended with
> > > additional parameiters to accommodate the requirements of multiple
> >
> > s/parameiters/parameters/
> >
> > > IPU3 IPA modules for e.g., an open-source IPA (in-tree) and a closed
> > > source IPA (for ChromeOS).
> >
> > No need to have the expression after the e.g. It's simply to accommodate
> > the parameters for the IPU3 IPAs.
> >
> > In fact, this isn't related to supporting multiple IPAs at all. This
> > simply defines the interface. The fact that we have multiple
> > implementations is just a fact on top of that.
> >
> > > As usual, the documentation for .mojom files are kept in a separate
> > > .cpp file. Hence, create and document IPAConfigInfo in
> > > ipu3_ipa_interface.cpp.
> > >
> > > Finally, adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > include/libcamera/ipa/ipu3.mojom | 10 ++++-
> > > include/libcamera/ipa/ipu3_ipa_interface.cpp | 39 ++++++++++++++++++++
> >
> > Ok, now I really think we need to have the ipa directory under
> > src/libcamera/ipa as a prerequisite to this series.
> >
> > Let me know if you'll do that or if you prefer me to do it.
>
> I'm beginning to wonder if we should write a simple python script that
> would copy comments from a mojom file to a cpp file, and keep the
> documentation in the .mojom file. What do you think ?
tbh that's what I wanted to do, but I thought that was over-engineering
it. Maybe it's worth it to stop the clutter from docs-only cpp files?
Paul
>
> > > include/libcamera/ipa/meson.build | 1 +
> > > src/ipa/ipu3/ipu3.cpp | 14 +++----
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++--
> > > 5 files changed, 61 insertions(+), 13 deletions(-)
> > > create mode 100644 include/libcamera/ipa/ipu3_ipa_interface.cpp
> > >
> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > > index a717b1e6..22c4ce1b 100644
> > > --- a/include/libcamera/ipa/ipu3.mojom
> > > +++ b/include/libcamera/ipa/ipu3.mojom
> > > @@ -25,13 +25,19 @@ struct IPU3Action {
> > > libcamera.ControlList controls;
> > > };
> > >
> > > +struct IPAConfigInfo {
> > > + libcamera.IPACameraSensorInfo sensorInfo;
> > > + map<uint32, ControlInfoMap> entityControls;
> > > + libcamera.Size bdsOutputSize;
> > > + libcamera.Size iif;
> > > +};
> > > +
> > > interface IPAIPU3Interface {
> > > init(libcamera.IPASettings settings) => (int32 ret);
> > > start() => (int32 ret);
> > > stop();
> > >
> > > - configure(map<uint32, libcamera.ControlInfoMap> entityControls,
> > > - libcamera.Size bdsOutputSize) => ();
> > > + configure(IPAConfigInfo configInfo);
> > >
> > > mapBuffers(array<libcamera.IPABuffer> buffers);
> > > unmapBuffers(array<uint32> ids);
> > > diff --git a/include/libcamera/ipa/ipu3_ipa_interface.cpp b/include/libcamera/ipa/ipu3_ipa_interface.cpp
> > > new file mode 100644
> > > index 00000000..9aafbcdb
> > > --- /dev/null
> > > +++ b/include/libcamera/ipa/ipu3_ipa_interface.cpp
> > > @@ -0,0 +1,39 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \struct IPAConfigInfo
> > > + * \brief Information to be passed from IPU3 pipeline-handler to the IPA
> > > + *
> > > + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler
> > > + * to the IPAIPU3Interface::configure(). The structure provides extensibility
> > > + * for additional configuration data as required, for closed-source or
> > > + * open-source IPAs' configure() phases.
> > > + */
> > > +
> > > +/**
> > > + * \var IPAConfigInfo::sensorInfo
> > > + * \brief Camera sensor information
> > > + *
> > > + * \sa IPACameraSensorInfo
> > > + */
> > > +
> > > +/**
> > > + * \var IPAConfigInfo::entityControls
> > > + * \brief Controls supported by the sensor
> > > + *
> > > + * A map of V4L2 controls supported by the sensor in order to read or write
> > > + * control values to them.
> > > + */
> > > +
> > > +/**
> > > + * \var IPAConfigInfo::bdsOutputSize
> > > + * \brief Size of ImgU BDS rectangle
> > > + */
> > > +
> > > +/**
> > > + * \var IPAConfigInfo::iif
> > > + * \brief Size of ImgU input-feeder rectangle
> > > + */
> > > +} /* namespace libcamera */
> > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > > index da60aa68..9684a34f 100644
> > > --- a/include/libcamera/ipa/meson.build
> > > +++ b/include/libcamera/ipa/meson.build
> > > @@ -10,6 +10,7 @@ libcamera_ipa_headers = files([
> > >
> > > libcamera_ipa_docs = files([
> > > 'core_ipa_interface.cpp',
> > > + 'ipu3_ipa_interface.cpp',
> >
> > And of course then this would get added to the libcamera sources as part
> > of src/libcamera/ipa/meson.build which would then get passed into doxygen.
> >
> > > ])
> > >
> > > install_headers(libcamera_ipa_headers,
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index f5343547..769c24d3 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -43,8 +43,7 @@ public:
> > > int start() override;
> > > void stop() override {}
> > >
> > > - void configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> > > - const Size &bdsOutputSize) override;
> > > + void configure(const IPAConfigInfo &configInfo) override;
> > >
> > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> > > << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
> > > }
> > >
> > > -void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> > > - const Size &bdsOutputSize)
> > > +void IPAIPU3::configure(const IPAConfigInfo &configInfo)
> > > {
> > > - if (entityControls.empty())
> > > + if (configInfo.entityControls.empty())
> > > return;
> > >
> > > - ctrls_ = entityControls.at(0);
> > > + ctrls_ = configInfo.entityControls.at(0);
> > >
> > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> > > if (itExp == ctrls_.end()) {
> > > @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
> > >
> > > params_ = {};
> > >
> > > - calculateBdsGrid(bdsOutputSize);
> > > + calculateBdsGrid(configInfo.bdsOutputSize);
> > >
> > > awbAlgo_ = std::make_unique<IPU3Awb>();
> > > - awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
> > > + awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> > >
> > > agcAlgo_ = std::make_unique<IPU3Agc>();
> > > agcAlgo_->initialise(bdsGrid_);
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 98c6160f..5b15ca90 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -633,9 +633,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > > return ret;
> > > }
> > >
> > > - std::map<uint32_t, ControlInfoMap> entityControls;
> > > - entityControls.emplace(0, data->cio2_.sensor()->controls());
> > > - data->ipa_->configure(entityControls, config->imguConfig().bds);
> > > + ipa::ipu3::IPAConfigInfo configInfo;
> > > + configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());
> > > + configInfo.sensorInfo = sensorInfo;
> > > + configInfo.bdsOutputSize = config->imguConfig().bds;
> > > + configInfo.iif = config->imguConfig().iif;
> > > +
> > > + data->ipa_->configure(configInfo);
> >
> > Otherwise, the code move looks ok to me.
> >
> > >
> > > return 0;
> > > }
More information about the libcamera-devel
mailing list