Message ID | 20210426171608.145784-1-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Delegated to: | Umang Jain |
Headers | show |
Series |
|
Related | show |
Hi Umang, Thank you for the patch. 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@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(), }; > + data->ipa_->configure(configInfo); > > return 0; > }
Hi Laurent, Thanks for the reviews. On 4/27/21 8:27 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > 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@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? FAILED: src/libcamera/4ab8042@@camera@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. 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; >> + data->ipa_->configure(configInfo); >> >> return 0; >> }
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@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? > > FAILED: src/libcamera/4ab8042@@camera@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; > >> }
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@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@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; > > >> }
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; +}; + 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/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 = { + sensorInfo, entityControls, config->imguConfig().bds, + config->imguConfig().iif, data->cropRegion_.size() + }; + data->ipa_->configure(configInfo); return 0; }
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. Signed-off-by: Umang Jain <umang.jain@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(-)