| Message ID | 20210427063527.219509-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thanks for the patch ! On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and > open-source IPU3 IPAs. Hence, the structure encompasses additional > configuration information required to init and configure the > closed-source IPA as well. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes in v2: > - Mark as RFC(do not merge) as this is built upon Paul's > [Fix support for core.mojom structs v3] - waiting for merge to master > - Drop cropRegion_ from structure - this is provided from CameraSensorInfo > itself. > - Doxygen documentation of IPAConfigInfo. > --- > include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- > src/ipa/ipu3/ipu3.cpp | 14 ++++----- > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- > 3 files changed, 58 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index a717b1e6..acd5cfa4 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -25,13 +25,55 @@ struct IPU3Action { > libcamera.ControlList controls; > }; > > +/** > + * \struct IPAConfigInfo > + * \brief Information to be passed from IPU3 pipeline-handler to 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 > + * > + * Provides the camera sensor information such as model name, pixel-rate, > + * output-size and so on. Refer to libcamera::CameraSensorInfo for further > + * details. > + */ > + > + /** > + * \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 > + */ > +struct IPAConfigInfo { > + libcamera.CameraSensorInfo sensorInfo; > + map<uint32, ControlInfoMap> entityControls; > + libcamera.Size bdsOutputSize; > + libcamera.Size iif; > +}; > + I think it is interesting, and would like your advice: I would need this same structure for rkisp1 platform, except that the sizes are obviously not the same. And so, I am wondering if we could imagine a more general structure like: struct IPAConfigInfo { libcamera.CameraSensorInfo sensorInfo; map<uint32, ControlInfoMap> entityControls; libcamera.Size ispInputSize; libcamera.Size ispOutputSize; }; names and all may have to be reworked but that is the base idea. There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least you have an input an output pad size for the ISP node. > 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..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 73306cea..b1ff1dbe 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -635,7 +635,14 @@ 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); > + > + 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 JM On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote: > Hi Umang, > > Thanks for the patch ! > > On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and >> open-source IPU3 IPAs. Hence, the structure encompasses additional >> configuration information required to init and configure the >> closed-source IPA as well. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> Changes in v2: >> - Mark as RFC(do not merge) as this is built upon Paul's >> [Fix support for core.mojom structs v3] - waiting for merge to master >> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo >> itself. >> - Doxygen documentation of IPAConfigInfo. >> --- >> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- >> src/ipa/ipu3/ipu3.cpp | 14 ++++----- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- >> 3 files changed, 58 insertions(+), 11 deletions(-) >> >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom >> index a717b1e6..acd5cfa4 100644 >> --- a/include/libcamera/ipa/ipu3.mojom >> +++ b/include/libcamera/ipa/ipu3.mojom >> @@ -25,13 +25,55 @@ struct IPU3Action { >> libcamera.ControlList controls; >> }; >> >> +/** >> + * \struct IPAConfigInfo >> + * \brief Information to be passed from IPU3 pipeline-handler to 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 >> + * >> + * Provides the camera sensor information such as model name, pixel-rate, >> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further >> + * details. >> + */ >> + >> + /** >> + * \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 >> + */ >> +struct IPAConfigInfo { >> + libcamera.CameraSensorInfo sensorInfo; >> + map<uint32, ControlInfoMap> entityControls; >> + libcamera.Size bdsOutputSize; >> + libcamera.Size iif; >> +}; >> + > I think it is interesting, and would like your advice: I would need this > same structure for rkisp1 platform, except that the sizes are obviously > not the same. > And so, I am wondering if we could imagine a more general structure like: > struct IPAConfigInfo { > libcamera.CameraSensorInfo sensorInfo; > map<uint32, ControlInfoMap> entityControls; > libcamera.Size ispInputSize; > libcamera.Size ispOutputSize; > }; > > names and all may have to be reworked but that is the base idea. > There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least > you have an input an output pad size for the ISP node. Certainly, this might be a good idea. I am not opposed to it. The question here is, that is the structure core.mojom worthy, in order to be shared between multiple IPAs right? If not, I think we need to keep them in <platform IPA>.mojom for now. And if we want to share the data-structures, we need to work out how all existing IPAs (for e.g. Raspberry Pi) can be ported to used that IPAConfig structure too. We probably, should address this by looking at all the common parameters we need for all the existing IPAs and have a patch series that ports every IPA to use that data-structure. However, there are some risky? situations that can happen, for e.g. - An IPA X requiring a parameter Y for configuration and the common struct IPAConfig doesn't have a placeholder - Parameter Y is added to common struct IPAConfig - Parameter Y is unused/nullptr for all IPAs except IPA X I do not know if adding placeholder for Y is fine in the common struct, if fine or not, in the codebase purview. Thinking about it, the chances of various IPA, having exact configuration data (common IPAConfig) are slim. Hence, the IPA authors will try hacks for other parameters they need for IPA configuration, either as function-params or introducing new structures and passing alongside with common struct IPAConfig to the IPA. That would not be nice, isn't it? I might be wrong, so I shall keep an open mind about it. Maybe we can track this as a task somewhere too? > >> 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..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 73306cea..b1ff1dbe 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -635,7 +635,14 @@ 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); >> + >> + 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; >> } >>
On 27/04/2021 14:54, Umang Jain wrote: > Hi JM > > On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote: >> Hi Umang, >> >> Thanks for the patch ! >> >> On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and >>> open-source IPU3 IPAs. Hence, the structure encompasses additional >>> configuration information required to init and configure the >>> closed-source IPA as well. >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> Changes in v2: >>> - Mark as RFC(do not merge) as this is built upon Paul's >>> [Fix support for core.mojom structs v3] - waiting for merge to master >>> - Drop cropRegion_ from structure - this is provided from >>> CameraSensorInfo >>> itself. >>> - Doxygen documentation of IPAConfigInfo. >>> --- >>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- >>> src/ipa/ipu3/ipu3.cpp | 14 ++++----- >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- >>> 3 files changed, 58 insertions(+), 11 deletions(-) >>> >>> diff --git a/include/libcamera/ipa/ipu3.mojom >>> b/include/libcamera/ipa/ipu3.mojom >>> index a717b1e6..acd5cfa4 100644 >>> --- a/include/libcamera/ipa/ipu3.mojom >>> +++ b/include/libcamera/ipa/ipu3.mojom >>> @@ -25,13 +25,55 @@ struct IPU3Action { >>> libcamera.ControlList controls; >>> }; >>> +/** >>> + * \struct IPAConfigInfo >>> + * \brief Information to be passed from IPU3 pipeline-handler to 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 >>> + * >>> + * Provides the camera sensor information such as model name, >>> pixel-rate, >>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for >>> further >>> + * details. >>> + */ >>> + >>> + /** >>> + * \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 >>> + */ >>> +struct IPAConfigInfo { >>> + libcamera.CameraSensorInfo sensorInfo; >>> + map<uint32, ControlInfoMap> entityControls; >>> + libcamera.Size bdsOutputSize; >>> + libcamera.Size iif; >>> +}; >>> + >> I think it is interesting, and would like your advice: I would need this >> same structure for rkisp1 platform, except that the sizes are obviously >> not the same. >> And so, I am wondering if we could imagine a more general structure like: >> struct IPAConfigInfo { >> libcamera.CameraSensorInfo sensorInfo; >> map<uint32, ControlInfoMap> entityControls; >> libcamera.Size ispInputSize; >> libcamera.Size ispOutputSize; >> }; >> >> names and all may have to be reworked but that is the base idea. >> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least >> you have an input an output pad size for the ISP node. > Certainly, this might be a good idea. I am not opposed to it. > > The question here is, that is the structure core.mojom worthy, in order > to be shared between multiple IPAs right? If not, I think we need to > keep them in <platform IPA>.mojom for now. > > And if we want to share the data-structures, we need to work out how all > existing IPAs (for e.g. Raspberry Pi) can be ported to used that > IPAConfig structure too. We probably, should address this by looking at > all the common parameters we need for all the existing IPAs and have a > patch series that ports every IPA to use that data-structure. > > However, there are some risky? situations that can happen, for e.g. > - An IPA X requiring a parameter Y for configuration and the common > struct IPAConfig doesn't have a placeholder > - Parameter Y is added to common struct IPAConfig > - Parameter Y is unused/nullptr for all IPAs except IPA X > > I do not know if adding placeholder for Y is fine in the common struct, > if fine or not, in the codebase purview. > > Thinking about it, the chances of various IPA, having exact > configuration data (common IPAConfig) are slim. Hence, the IPA authors > will try hacks for other parameters they need for IPA configuration, > either as function-params or introducing new structures and passing > alongside with common struct IPAConfig to the IPA. That would not be > nice, isn't it? > > I might be wrong, so I shall keep an open mind about it. Maybe we can > track this as a task somewhere too? > The idea I had is to have a common structure, and having the IPA specific ones inherit from this common one. You would have something like IPAIPU3ConfigInfo: IPAConfigInfo { /* myGreatParameters */ } That one may not make sense though :-). >> >>> 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..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 73306cea..b1ff1dbe 100644 >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>> @@ -635,7 +635,14 @@ 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); >>> + >>> + 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 12:05:27PM +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 IPA interface should be common to both closed-source and > open-source IPU3 IPAs. Hence, the structure encompasses additional > configuration information required to init and configure the > closed-source IPA as well. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes in v2: > - Mark as RFC(do not merge) as this is built upon Paul's > [Fix support for core.mojom structs v3] - waiting for merge to master > - Drop cropRegion_ from structure - this is provided from CameraSensorInfo > itself. > - Doxygen documentation of IPAConfigInfo. > --- > include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- > src/ipa/ipu3/ipu3.cpp | 14 ++++----- > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- > 3 files changed, 58 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index a717b1e6..acd5cfa4 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -25,13 +25,55 @@ struct IPU3Action { > libcamera.ControlList controls; > }; > > +/** > + * \struct IPAConfigInfo > + * \brief Information to be passed from IPU3 pipeline-handler to IPA 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. What do you mean with extensibility ? That one can add fields to the structure, like to all structures ? :) Or are you thinking at some inheritance mechanism ? In the former case I would say "The structure can be extended with additional parameters to accommodate the requirements of different IPA modules" > + */ > + > +/** > + * \var IPAConfigInfo::sensorInfo > + * \brief Camera sensor information > + * > + * Provides the camera sensor information such as model name, pixel-rate, > + * output-size and so on. Refer to libcamera::CameraSensorInfo for further > + * details. I would just: \sa CameraSensorInfo As duplicating the description has limited value imho > + */ > + > + /** > + * \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. > + */ The existing entityControls is described as \param[in] entityControls Controls provided by the pipeline entities and it does not only include the sensor controls (otherwise it would have been just a ControlInfoMap not a map). The idea was that each ph-IPA defines it's own mapping. Ie entityControls[0] = sensor controls, entityControls[1] = someother entity controls, etc etc). So far I think only this is only been effectively used to transport sensor controls, and I would be fine making this a ControlList and name it 'sensorControls'. Even if we later need to pass to IPA the ControlInfoMap of another entity, we can add it to this structure with a more explicit name (much better than hiding it in a map and establishing what an index maps to like we're doing now) > + > + /** > + * \var IPAConfigInfo::bdsOutputSize > + * \brief Size of ImgU BDS rectangle > + */ > + > + /** > + * \var IPAConfigInfo::iif > + * \brief Size of ImgU input-feeder rectangle > + */ Empty line maybe. I wonder if it wouldn't be better to pass the full ImgUDevice::PipeConfig. True that the less data we transport over IPC the better, so this makes sense. Is it worth wrapping thise two sizes in an 'ImguSize' structure ? BTW I don't see iif being used currently. Will you need it in future ? > +struct IPAConfigInfo { > + libcamera.CameraSensorInfo 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); No idea how that works for IPC, but if this was a regular function I would have made this configure(const 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..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; Ah Why the two prototypes are different ? > > 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 73306cea..b1ff1dbe 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -635,7 +635,14 @@ 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); > + > + ipa::ipu3::IPAConfigInfo configInfo; > + configInfo.sensorInfo = sensorInfo; > + configInfo.entityControls = entityControls; Why a copy when you can populate this one directly ? Overall the idea is good. I would refrain from premature optimizations like the one suggested by Jean-Micheal for the moment though, it's a bit too early in my opinion, and the effort for each pipeline handler to define its own exchange structures is limited. Thanks j > + configInfo.bdsOutputSize = config->imguConfig().bds; > + configInfo.iif = config->imguConfig().iif; > + > + data->ipa_->configure(configInfo); > > return 0; > } > -- > 2.26.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for comments, On 4/29/21 1:15 PM, Jacopo Mondi wrote: > Hi Umang, > > On Tue, Apr 27, 2021 at 12:05:27PM +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 IPA interface should be common to both closed-source and >> open-source IPU3 IPAs. Hence, the structure encompasses additional >> configuration information required to init and configure the >> closed-source IPA as well. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> Changes in v2: >> - Mark as RFC(do not merge) as this is built upon Paul's >> [Fix support for core.mojom structs v3] - waiting for merge to master >> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo >> itself. >> - Doxygen documentation of IPAConfigInfo. >> --- >> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- >> src/ipa/ipu3/ipu3.cpp | 14 ++++----- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- >> 3 files changed, 58 insertions(+), 11 deletions(-) >> >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom >> index a717b1e6..acd5cfa4 100644 >> --- a/include/libcamera/ipa/ipu3.mojom >> +++ b/include/libcamera/ipa/ipu3.mojom >> @@ -25,13 +25,55 @@ struct IPU3Action { >> libcamera.ControlList controls; >> }; >> >> +/** >> + * \struct IPAConfigInfo >> + * \brief Information to be passed from IPU3 pipeline-handler to IPA > 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. > What do you mean with extensibility ? That one can add fields to the > structure, like to all structures ? :) > Or are you thinking at some inheritance mechanism ? > > In the former case I would say > "The structure can be extended with additional parameters to > accommodate the requirements of different IPA modules" Ok, I meant the former, I shall re-word in next iteration > > >> + */ >> + >> +/** >> + * \var IPAConfigInfo::sensorInfo >> + * \brief Camera sensor information >> + * >> + * Provides the camera sensor information such as model name, pixel-rate, >> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further >> + * details. > I would just: > \sa CameraSensorInfo > > As duplicating the description has limited value imho Agreed > >> + */ >> + >> + /** >> + * \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. >> + */ > The existing entityControls is described as > \param[in] entityControls Controls provided by the pipeline entities > > and it does not only include the sensor controls (otherwise it would > have been just a ControlInfoMap not a map). The idea was that each ph-IPA > defines it's own mapping. Ie entityControls[0] = sensor controls, > entityControls[1] = someother entity controls, etc etc). > > So far I think only this is only been effectively used to transport > sensor controls, and I would be fine making this a ControlList and > name it 'sensorControls'. Even if we later need to pass to IPA the > ControlInfoMap of another entity, we can add it to this structure with > a more explicit name (much better than hiding it in a map and > establishing what an index maps to like we're doing now) Oh, thanks for the context. I'll take a note of it and tinker on how we can address this change and adapt the documentation. Might need some cycle of discussion in general with Kieran too! > >> + >> + /** >> + * \var IPAConfigInfo::bdsOutputSize >> + * \brief Size of ImgU BDS rectangle >> + */ >> + >> + /** >> + * \var IPAConfigInfo::iif >> + * \brief Size of ImgU input-feeder rectangle >> + */ > Empty line maybe. > > I wonder if it wouldn't be better to pass the full > ImgUDevice::PipeConfig. True that the less data we transport over IPC > the better, so this makes sense. Is it worth wrapping thise two sizes > in an 'ImguSize' structure ? Maybe - I see this as an optimization point, so not my highest priority as of now. > > BTW I don't see iif being used currently. Will you need it in future ? `iif` is used for closed source IPA. So, the current issue is we have two competing IPAs - Open IPU3 IPA (already merged in-tree) and a closed one(probably will be residing in it's own git repo). But the ph-IPA interface has to be common to both. This is why you don't see `iif` usage here(in open IPA), but is need for the closed one. > >> +struct IPAConfigInfo { >> + libcamera.CameraSensorInfo 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); > No idea how that works for IPC, but if this was a regular function I > would have made this > configure(const IPAConfigInfo &configInfo) These are mojom declarations - hence I believe the framework by-default enforces that all function parameters are `const` and passed by-reference. These mojom files are used to generated the .cpp interface code (see include/libcamera/ipa/meson.build), which matches the prototype below (where you have comment (`Why the two prototypes are different`). > >> 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..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; > Ah > > Why the two prototypes are different ? > >> 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 73306cea..b1ff1dbe 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -635,7 +635,14 @@ 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); >> + >> + ipa::ipu3::IPAConfigInfo configInfo; >> + configInfo.sensorInfo = sensorInfo; >> + configInfo.entityControls = entityControls; > Why a copy when you can populate this one directly ? Originally it was your suggested way only, but Laurent wanted to use named initializers for safety - as there are couple of `Sizes` parameters and one can get the ordering wrong in directly populating them. IPAConfigInfo is generated from mojom framework and doesn't yet support named initiazers hence, this is what I ended up as closest implementation. > > Overall the idea is good. I would refrain from premature optimizations > like the one suggested by Jean-Micheal for the moment though, it's a > bit too early in my opinion, and the effort for each pipeline handler > to define its own exchange structures is limited. Yes - overall idea is good, but I would be comfortable doing that when we have got some level of stability for the closed source IPA > > Thanks > j > >> + configInfo.bdsOutputSize = config->imguConfig().bds; >> + configInfo.iif = config->imguConfig().iif; >> + >> + data->ipa_->configure(configInfo); >> >> return 0; >> } >> -- >> 2.26.2 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Umang, On Thu, Apr 29, 2021 at 01:55:57PM +0530, Umang Jain wrote: [snip] > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -635,7 +635,14 @@ 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); > > > + > > > + ipa::ipu3::IPAConfigInfo configInfo; > > > + configInfo.sensorInfo = sensorInfo; > > > + configInfo.entityControls = entityControls; > > Why a copy when you can populate this one directly ? > Originally it was your suggested way only, but Laurent wanted to use named > initializers for safety - as there are couple of `Sizes` parameters and one > can get the ordering wrong in directly populating them. IPAConfigInfo is > generated from mojom framework and doesn't yet support named initiazers > hence, this is what I ended up as closest implementation. What I meant was: why do this std::map<uint32_t, ControlInfoMap> entityControls; entityControls.emplace(0, data->cio2_.sensor()->controls()); .... configInfo.entityControls = entityControls; Instead of configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); and avoid the copy ? Thanks j > > > > Overall the idea is good. I would refrain from premature optimizations > > like the one suggested by Jean-Micheal for the moment though, it's a > > bit too early in my opinion, and the effort for each pipeline handler > > to define its own exchange structures is limited. > Yes - overall idea is good, but I would be comfortable doing that when we > have got some level of stability for the closed source IPA > > > > Thanks > > j > > > > > + configInfo.bdsOutputSize = config->imguConfig().bds; > > > + configInfo.iif = config->imguConfig().iif; > > > + > > > + data->ipa_->configure(configInfo); > > > > > > return 0; > > > } > > > -- > > > 2.26.2 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Jean-Michel, On Thu, Apr 29, 2021 at 07:30:47AM +0200, Jean-Michel Hautbois wrote: > On 27/04/2021 14:54, Umang Jain wrote: > > On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote: > >> On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and > >>> open-source IPU3 IPAs. Hence, the structure encompasses additional > >>> configuration information required to init and configure the > >>> closed-source IPA as well. > >>> > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>> --- > >>> Changes in v2: > >>> - Mark as RFC(do not merge) as this is built upon Paul's > >>> [Fix support for core.mojom structs v3] - waiting for merge to master > >>> - Drop cropRegion_ from structure - this is provided from > >>> CameraSensorInfo > >>> itself. > >>> - Doxygen documentation of IPAConfigInfo. > >>> --- > >>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- > >>> src/ipa/ipu3/ipu3.cpp | 14 ++++----- > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- > >>> 3 files changed, 58 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/include/libcamera/ipa/ipu3.mojom > >>> b/include/libcamera/ipa/ipu3.mojom > >>> index a717b1e6..acd5cfa4 100644 > >>> --- a/include/libcamera/ipa/ipu3.mojom > >>> +++ b/include/libcamera/ipa/ipu3.mojom > >>> @@ -25,13 +25,55 @@ struct IPU3Action { > >>> libcamera.ControlList controls; > >>> }; > >>> +/** > >>> + * \struct IPAConfigInfo > >>> + * \brief Information to be passed from IPU3 pipeline-handler to 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 > >>> + * > >>> + * Provides the camera sensor information such as model name, > >>> pixel-rate, > >>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for > >>> further > >>> + * details. > >>> + */ > >>> + > >>> + /** > >>> + * \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 > >>> + */ > >>> +struct IPAConfigInfo { > >>> + libcamera.CameraSensorInfo sensorInfo; > >>> + map<uint32, ControlInfoMap> entityControls; > >>> + libcamera.Size bdsOutputSize; > >>> + libcamera.Size iif; > >>> +}; > >>> + > >> I think it is interesting, and would like your advice: I would need this > >> same structure for rkisp1 platform, except that the sizes are obviously > >> not the same. > >> And so, I am wondering if we could imagine a more general structure like: > >> struct IPAConfigInfo { > >> libcamera.CameraSensorInfo sensorInfo; > >> map<uint32, ControlInfoMap> entityControls; > >> libcamera.Size ispInputSize; > >> libcamera.Size ispOutputSize; > >> }; > >> > >> names and all may have to be reworked but that is the base idea. > >> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least > >> you have an input an output pad size for the ISP node. > > Certainly, this might be a good idea. I am not opposed to it. > > > > The question here is, that is the structure core.mojom worthy, in order > > to be shared between multiple IPAs right? If not, I think we need to > > keep them in <platform IPA>.mojom for now. > > > > And if we want to share the data-structures, we need to work out how all > > existing IPAs (for e.g. Raspberry Pi) can be ported to used that > > IPAConfig structure too. We probably, should address this by looking at > > all the common parameters we need for all the existing IPAs and have a > > patch series that ports every IPA to use that data-structure. > > > > However, there are some risky? situations that can happen, for e.g. > > - An IPA X requiring a parameter Y for configuration and the common > > struct IPAConfig doesn't have a placeholder > > - Parameter Y is added to common struct IPAConfig > > - Parameter Y is unused/nullptr for all IPAs except IPA X > > > > I do not know if adding placeholder for Y is fine in the common struct, > > if fine or not, in the codebase purview. > > > > Thinking about it, the chances of various IPA, having exact > > configuration data (common IPAConfig) are slim. Hence, the IPA authors > > will try hacks for other parameters they need for IPA configuration, > > either as function-params or introducing new structures and passing > > alongside with common struct IPAConfig to the IPA. That would not be > > nice, isn't it? > > > > I might be wrong, so I shall keep an open mind about it. Maybe we can > > track this as a task somewhere too? > > > > The idea I had is to have a common structure, and having the IPA > specific ones inherit from this common one. > You would have something like > IPAIPU3ConfigInfo: IPAConfigInfo { > /* myGreatParameters */ > } > > That one may not make sense though :-). IPA parameters are very platform-specific, I don't think this would bring much benefit. > >>> 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..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 73306cea..b1ff1dbe 100644 > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> @@ -635,7 +635,14 @@ 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); > >>> + > >>> + 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; > >>> } > >>>
Hello Umang and Jean-Michel, On Thu, Apr 29, 2021 at 07:30:47AM +0200, Jean-Michel Hautbois wrote: > > > On 27/04/2021 14:54, Umang Jain wrote: > > Hi JM > > > > On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote: > >> Hi Umang, > >> > >> Thanks for the patch ! > >> > >> On 27/04/2021 08:35, 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 IPA interface should be common to both closed-source and > >>> open-source IPU3 IPAs. Hence, the structure encompasses additional > >>> configuration information required to init and configure the > >>> closed-source IPA as well. > >>> > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>> --- > >>> Changes in v2: > >>> - Mark as RFC(do not merge) as this is built upon Paul's > >>> [Fix support for core.mojom structs v3] - waiting for merge to master This has been merged already. > >>> - Drop cropRegion_ from structure - this is provided from > >>> CameraSensorInfo > >>> itself. > >>> - Doxygen documentation of IPAConfigInfo. > >>> --- > >>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- > >>> src/ipa/ipu3/ipu3.cpp | 14 ++++----- > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- > >>> 3 files changed, 58 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/include/libcamera/ipa/ipu3.mojom > >>> b/include/libcamera/ipa/ipu3.mojom > >>> index a717b1e6..acd5cfa4 100644 > >>> --- a/include/libcamera/ipa/ipu3.mojom > >>> +++ b/include/libcamera/ipa/ipu3.mojom > >>> @@ -25,13 +25,55 @@ struct IPU3Action { > >>> libcamera.ControlList controls; > >>> }; > >>> +/** > >>> + * \struct IPAConfigInfo > >>> + * \brief Information to be passed from IPU3 pipeline-handler to 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 > >>> + * > >>> + * Provides the camera sensor information such as model name, > >>> pixel-rate, > >>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for > >>> further > >>> + * details. > >>> + */ > >>> + > >>> + /** > >>> + * \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 > >>> + */ > >>> +struct IPAConfigInfo { > >>> + libcamera.CameraSensorInfo sensorInfo; > >>> + map<uint32, ControlInfoMap> entityControls; > >>> + libcamera.Size bdsOutputSize; > >>> + libcamera.Size iif; > >>> +}; > >>> + > >> I think it is interesting, and would like your advice: I would need this > >> same structure for rkisp1 platform, except that the sizes are obviously > >> not the same. > >> And so, I am wondering if we could imagine a more general structure like: > >> struct IPAConfigInfo { > >> libcamera.CameraSensorInfo sensorInfo; > >> map<uint32, ControlInfoMap> entityControls; > >> libcamera.Size ispInputSize; > >> libcamera.Size ispOutputSize; > >> }; > >> > >> names and all may have to be reworked but that is the base idea. > >> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least > >> you have an input an output pad size for the ISP node. > > Certainly, this might be a good idea. I am not opposed to it. > > > > The question here is, that is the structure core.mojom worthy, in order > > to be shared between multiple IPAs right? If not, I think we need to > > keep them in <platform IPA>.mojom for now. > > > > And if we want to share the data-structures, we need to work out how all > > existing IPAs (for e.g. Raspberry Pi) can be ported to used that > > IPAConfig structure too. We probably, should address this by looking at > > all the common parameters we need for all the existing IPAs and have a > > patch series that ports every IPA to use that data-structure. > > > > However, there are some risky? situations that can happen, for e.g. > > - An IPA X requiring a parameter Y for configuration and the common > > struct IPAConfig doesn't have a placeholder > > - Parameter Y is added to common struct IPAConfig > > - Parameter Y is unused/nullptr for all IPAs except IPA X > > > > I do not know if adding placeholder for Y is fine in the common struct, > > if fine or not, in the codebase purview. > > > > Thinking about it, the chances of various IPA, having exact > > configuration data (common IPAConfig) are slim. Hence, the IPA authors > > will try hacks for other parameters they need for IPA configuration, > > either as function-params or introducing new structures and passing > > alongside with common struct IPAConfig to the IPA. That would not be > > nice, isn't it? We could have a greatest common divisor for the IPA config info struct in core.mojom, and then the IPAs could define their own additional struct. > > > > I might be wrong, so I shall keep an open mind about it. Maybe we can > > track this as a task somewhere too? > > > > The idea I had is to have a common structure, and having the IPA > specific ones inherit from this common one. > You would have something like > IPAIPU3ConfigInfo: IPAConfigInfo { > /* myGreatParameters */ > } Because mojo doesn't support inheritance :/ Well okay, if you *really* wanted to, in theory, we could use an attribute to declare inheritance, and then I'd have to expand the code generator for that (just keeping doors and minds open). Paul > > That one may not make sense though :-). > > >> > >>> 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..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 73306cea..b1ff1dbe 100644 > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> @@ -635,7 +635,14 @@ 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); > >>> + > >>> + 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; > >>> } > >>> > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index a717b1e6..acd5cfa4 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -25,13 +25,55 @@ struct IPU3Action { libcamera.ControlList controls; }; +/** + * \struct IPAConfigInfo + * \brief Information to be passed from IPU3 pipeline-handler to 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 + * + * Provides the camera sensor information such as model name, pixel-rate, + * output-size and so on. Refer to libcamera::CameraSensorInfo for further + * details. + */ + + /** + * \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 + */ +struct IPAConfigInfo { + libcamera.CameraSensorInfo 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/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 73306cea..b1ff1dbe 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -635,7 +635,14 @@ 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); + + 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; }
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 IPA interface should be common to both closed-source and open-source IPU3 IPAs. Hence, the structure encompasses additional configuration information required to init and configure the closed-source IPA as well. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- Changes in v2: - Mark as RFC(do not merge) as this is built upon Paul's [Fix support for core.mojom structs v3] - waiting for merge to master - Drop cropRegion_ from structure - this is provided from CameraSensorInfo itself. - Doxygen documentation of IPAConfigInfo. --- include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++-- src/ipa/ipu3/ipu3.cpp | 14 ++++----- src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++- 3 files changed, 58 insertions(+), 11 deletions(-)