[libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in IPC
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Apr 27 09:54:28 CEST 2021
Hi Umang and Laurent,
On Tue, Apr 27, 2021 at 09:37:24AM +0300, Laurent Pinchart wrote:
> Hi Umang,
>
> On Tue, Apr 27, 2021 at 11:40:25AM +0530, Umang Jain wrote:
> > On 4/27/21 8:27 AM, Laurent Pinchart wrote:
> > > On Mon, Apr 26, 2021 at 10:46:08PM +0530, Umang Jain wrote:
> > >> IPAConfigInfo is a consolidated data structure passed from IPU3
> > >> pipeline-handler to IPU3 IPA. This provides a streamlined and
> > >> extensible way to provide the configuration data to IPA interface.
> > > The commit message should mention that you're adding more data to the
> > > structure, and why.
> > >
> > >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > >> ---
> > >> include/libcamera/ipa/ipu3.mojom | 11 +++++++++--
> > >> src/ipa/ipu3/ipu3.cpp | 14 ++++++--------
> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 ++++++-
> > >> 3 files changed, 21 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > >> index a717b1e6..88cbb403 100644
> > >> --- a/include/libcamera/ipa/ipu3.mojom
> > >> +++ b/include/libcamera/ipa/ipu3.mojom
> > >> @@ -25,13 +25,20 @@ struct IPU3Action {
> > >> libcamera.ControlList controls;
> > >> };
> > >>
> > >> +struct IPAConfigInfo {
> > >> + libcamera.CameraSensorInfo sensorInfo;
> > >> + map<uint32, ControlInfoMap> entityControls;
> > >> + libcamera.Size bdsOutputSize;
> > >> + libcamera.Size iif;
> > >> + libcamera.Size cropRegion;
> > >> +};
> > > Could you document this structure with doxygen comments ? We don't
> > > generate documentation from mojom files yet, but we should still be
> > > prepared.
> > >
> > >> +
> > >> interface IPAIPU3Interface {
> > >> init(libcamera.IPASettings settings) => (int32 ret);
> > >> start() => (int32 ret);
> > >> stop();
> > >>
> > >> - configure(map<uint32, libcamera.ControlInfoMap> entityControls,
> > >> - libcamera.Size bdsOutputSize) => ();
> > >> + configure(IPAConfigInfo configInfo) => ();
> > > While at it, I think you can drop the => ().
> > >
> > >> mapBuffers(array<libcamera.IPABuffer> buffers);
> > >> unmapBuffers(array<uint32> ids);
> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > >> index f5343547..a77afd55 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 struct 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 struct 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 73306cea..be43d5a1 100644
> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> @@ -635,7 +635,12 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >>
> > >> std::map<uint32_t, ControlInfoMap> entityControls;
> > >> entityControls.emplace(0, data->cio2_.sensor()->controls());
> > >> - data->ipa_->configure(entityControls, config->imguConfig().bds);
> > >> +
> > >> + struct ipa::ipu3::IPAConfigInfo configInfo = {
> > > You can drop the struct keyword.
> > >
> > >> + sensorInfo, entityControls, config->imguConfig().bds,
> > >> + config->imguConfig().iif, data->cropRegion_.size()
> > >> + };
> > > Given that several members of this structures have the same type, it
> > > would be safer to use named initializers.
> > >
> > > ipa::ipu3::IPAConfigInfo configInfo = {
> > > .sensorInfo = sensorInfo,
> > > .entityControls = entityControls,
> > > .bdsOutputSize = config->imguConfig().bds,
> > > .iif = config->imguConfig().iif,
> > > .cropRegion = data->cropRegion_.size(),
> > > };
> > This throws a compile-time error because the generated
> > ipu3-ipa-interface.h code doesn't suppport named initializers for
> > IPAConfigInfo I think. I think we need to support in mojom first?
I put in the user-provided constructors to support setting default
values in mojom, so I don't think named initializers will be supported.
Paul
> >
> > FAILED: src/libcamera/4ab8042@@camera at sha/pipeline_ipu3_ipu3.cpp.o
> > ../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/src/libcamera/pipeline/ipu3/ipu3.cpp:639:27:
> > error: no matching constructor for initialization of
> > 'ipa::ipu3::IPAConfigInfo'
> > ipa::ipu3::IPAConfigInfo configInfo = {
> > ^ ~
> > include/libcamera/ipa/ipu3_ipa_interface.h:96:2: note: candidate
> > constructor not viable: cannot convert argument of incomplete type
> > 'void' to 'const libcamera::CameraSensorInfo' for 1st argument
> > IPAConfigInfo(const CameraSensorInfo &_sensorInfo, const
> > std::map<uint32_t, ControlInfoMap> &_entityControls, const Size
> > &_bdsOutputSize, const Size &_iif)
> > ^
> > include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate
> > constructor (the implicit copy constructor) not viable: requires 1
> > argument, but 4 were provided
> > struct IPAConfigInfo
> > ^
> > include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate
> > constructor (the implicit move constructor) not viable: requires 1
> > argument, but 4 were provided
> > include/libcamera/ipa/ipu3_ipa_interface.h:92:2: note: candidate
> > constructor not viable: requires 0 arguments, but 4 were provided
> > IPAConfigInfo()
> > ^
> > 1 error generated.
>
> Good point, we can't use aggregate initialization when user-provided
> constructors are present. I had overlooked that.
>
> > Meanwhile, in order to address your concerns, maybe this?
> >
> > + ipa::ipu3::IPAConfigInfo configInfo;
> > + configInfo.sensorInfo = sensorInfo;
> > + configInfo.entityControls = entityControls;
> > + configInfo.bdsOutputSize = config->imguConfig().bds;
> > + configInfo.iif = config->imguConfig().iif;
>
> Up to you, I'm fine either way.
>
> > >> + data->ipa_->configure(configInfo);
> > >>
> > >> return 0;
> > >> }
More information about the libcamera-devel
mailing list