| Message ID | 9c7e79d2-1b12-4a30-9126-019607673bb7@oss.qualcomm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, On Mon, Jul 27, 2026 at 01:58:23PM +0200, Hans de Goede wrote: > Hi All, > > As discussed I've been looking into loading / creating IPAs by (proxy) type name. > > A way to do this was proposed by Barnabás here: > > https://patchwork.libcamera.org/patch/26503/ > > Barnabás Pőcze wrote: > > """ > > --- a/include/libcamera/internal/ipa_manager.h > > +++ b/include/libcamera/internal/ipa_manager.h > > @@ -34,12 +34,13 @@ public: > > > > template<typename T> > > static std::unique_ptr<T> createIPA(PipelineHandler *pipe, > > + const char *name, > > uint32_t minVersion, > > uint32_t maxVersion) > > { > > I agree with this change, but I wish that the name could be derived from `T`. > And that is quite easy to do, just adding a > > static constexpr const char *name() { return "..."; } > > and then using `T::name()` here works mostly. > > But unfortunately the raspberry pi ipa modules are not compatible at the moment > with this approach. I have tried to come up with something, but so for has failed. > > Maybe the `name` argument could be kept like this: > > const char *name = T::name() > > and then the rpi pipeline handler could be changed to use the specific names. Not > a fan of this in any case. > """ > > This sounds nice and simple, but the <typename T> being passed to > the template here is an IPA proxy which gets auto-generated by mojo. > > mojo does support const variables so I've tried this: As discussed on IRC, you don't need to modify the .mojom files. The proxy class is generated by a jinja2 template, see utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl. Adding a static name() function there should be enough. > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index d9a50b01d..b430ae5fd 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -9,6 +9,8 @@ module ipa.ipu3; > import "include/libcamera/ipa/core.mojom"; > > struct IPAConfigInfo { > + const string kInterfaceName = "ipu3"; > + > libcamera.IPACameraSensorInfo sensorInfo; > libcamera.ControlInfoMap sensorControls; > libcamera.ControlInfoMap lensControls; > @@ -17,6 +19,8 @@ struct IPAConfigInfo { > }; > > interface IPAIPU3Interface { > + const string kInterfaceName = "ipu3"; > + > init(libcamera.IPASettings settings, > libcamera.IPACameraSensorInfo sensorInfo, > libcamera.ControlInfoMap sensorControls) > > > Unfortunately this results in the follow autogenerated code: > > struct IPAConfigInfo > { > public: > #ifndef __DOXYGEN__ > IPAConfigInfo() = default; > > template< > typename T1 = IPACameraSensorInfo, > typename T2 = ControlInfoMap, > typename T3 = ControlInfoMap, > typename T4 = Size, > typename T5 = Size, > std::enable_if_t<std::is_convertible_v<T1&&, IPACameraSensorInfo>> * = nullptr, > std::enable_if_t<std::is_convertible_v<T2&&, ControlInfoMap>> * = nullptr, > std::enable_if_t<std::is_convertible_v<T3&&, ControlInfoMap>> * = nullptr, > std::enable_if_t<std::is_convertible_v<T4&&, Size>> * = nullptr, > std::enable_if_t<std::is_convertible_v<T5&&, Size>> * = nullptr > > > IPAConfigInfo(T1 &&_sensorInfo, T2 &&_sensorControls, T3 &&_lensControls, T4 &&_bdsOutputSize, T5 &&_iif) > : sensorInfo(std::forward<T1>(_sensorInfo)) > , sensorControls(std::forward<T2>(_sensorControls)) > , lensControls(std::forward<T3>(_lensControls)) > , bdsOutputSize(std::forward<T4>(_bdsOutputSize)) > , iif(std::forward<T5>(_iif)) > { > } > #endif > > > IPACameraSensorInfo sensorInfo; > ControlInfoMap sensorControls; > ControlInfoMap lensControls; > Size bdsOutputSize; > Size iif; > }; > > class IPAIPU3Interface : public IPAInterface > { > public: > > virtual int32_t init( > const IPASettings &settings, > const IPACameraSensorInfo &sensorInfo, > const ControlInfoMap &sensorControls, > ControlInfoMap *ipaControls) = 0; > > virtual int32_t start() = 0; > > virtual void stop() = 0; > > virtual int32_t configure( > const IPAConfigInfo &configInfo, > ControlInfoMap *ipaControls) = 0; > > virtual void mapBuffers( > const std::vector<libcamera::IPABuffer> &buffers) = 0; > > virtual void unmapBuffers( > const std::vector<uint32_t> &ids) = 0; > > virtual void queueRequest( > const uint32_t frame, > const ControlList &controls) = 0; > > virtual void computeParams( > const uint32_t frame, > const uint32_t bufferId) = 0; > > virtual void processStats( > const uint32_t frame, > const int64_t frameTimestamp, > const uint32_t bufferId, > const ControlList &sensorControls) = 0; > > Signal<uint32_t, const ControlList &, const ControlList &> setSensorControls; > > Signal<uint32_t> paramsComputed; > > Signal<uint32_t, const ControlList &> metadataReady; > }; > > which does not change by adding the const lines to the .mojo file. > > it seems the const declarations inside mojo files are only intended for use > inside the mojo files (for e.g. arg default values) and are nor propagated > to the generic code. > > So AFAICT this means that the whole load/create IPA by IPA-proxy type idea > will not work.
Quoting Laurent Pinchart (2026-07-27 22:14:55) > Hi Hans, > > On Mon, Jul 27, 2026 at 01:58:23PM +0200, Hans de Goede wrote: > > Hi All, > > > > As discussed I've been looking into loading / creating IPAs by (proxy) type name. > > > > A way to do this was proposed by Barnabás here: > > > > https://patchwork.libcamera.org/patch/26503/ > > > > Barnabás Pőcze wrote: > > > > """ > > > --- a/include/libcamera/internal/ipa_manager.h > > > +++ b/include/libcamera/internal/ipa_manager.h > > > @@ -34,12 +34,13 @@ public: > > > > > > template<typename T> > > > static std::unique_ptr<T> createIPA(PipelineHandler *pipe, > > > + const char *name, > > > uint32_t minVersion, > > > uint32_t maxVersion) > > > { > > > > I agree with this change, but I wish that the name could be derived from `T`. > > And that is quite easy to do, just adding a > > > > static constexpr const char *name() { return "..."; } > > > > and then using `T::name()` here works mostly. > > > > But unfortunately the raspberry pi ipa modules are not compatible at the moment > > with this approach. I have tried to come up with something, but so for has failed. > > > > Maybe the `name` argument could be kept like this: > > > > const char *name = T::name() > > > > and then the rpi pipeline handler could be changed to use the specific names. Not > > a fan of this in any case. > > """ > > > > This sounds nice and simple, but the <typename T> being passed to > > the template here is an IPA proxy which gets auto-generated by mojo. > > > > mojo does support const variables so I've tried this: Yes the language does, but our code generator doesn't (although I see a comment in core.mojom that says we do... maybe I need to fix that). > As discussed on IRC, you don't need to modify the .mojom files. The > proxy class is generated by a jinja2 template, see > utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl. > Adding a static name() function there should be enough. Yes, the IPA implementation name does not belong in the mojom file as that is the definition of the interface between the pipeline handler and the IPA. I haven't been following the discussion so I'm probably missing a lot of context, but I'd have put IPA implementation name in IPAModuleInfo. Paul > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > index d9a50b01d..b430ae5fd 100644 > > --- a/include/libcamera/ipa/ipu3.mojom > > +++ b/include/libcamera/ipa/ipu3.mojom > > @@ -9,6 +9,8 @@ module ipa.ipu3; > > import "include/libcamera/ipa/core.mojom"; > > > > struct IPAConfigInfo { > > + const string kInterfaceName = "ipu3"; > > + > > libcamera.IPACameraSensorInfo sensorInfo; > > libcamera.ControlInfoMap sensorControls; > > libcamera.ControlInfoMap lensControls; > > @@ -17,6 +19,8 @@ struct IPAConfigInfo { > > }; > > > > interface IPAIPU3Interface { > > + const string kInterfaceName = "ipu3"; > > + > > init(libcamera.IPASettings settings, > > libcamera.IPACameraSensorInfo sensorInfo, > > libcamera.ControlInfoMap sensorControls) > > > > > > Unfortunately this results in the follow autogenerated code: > > > > struct IPAConfigInfo > > { > > public: > > #ifndef __DOXYGEN__ > > IPAConfigInfo() = default; > > > > template< > > typename T1 = IPACameraSensorInfo, > > typename T2 = ControlInfoMap, > > typename T3 = ControlInfoMap, > > typename T4 = Size, > > typename T5 = Size, > > std::enable_if_t<std::is_convertible_v<T1&&, IPACameraSensorInfo>> * = nullptr, > > std::enable_if_t<std::is_convertible_v<T2&&, ControlInfoMap>> * = nullptr, > > std::enable_if_t<std::is_convertible_v<T3&&, ControlInfoMap>> * = nullptr, > > std::enable_if_t<std::is_convertible_v<T4&&, Size>> * = nullptr, > > std::enable_if_t<std::is_convertible_v<T5&&, Size>> * = nullptr > > > > > IPAConfigInfo(T1 &&_sensorInfo, T2 &&_sensorControls, T3 &&_lensControls, T4 &&_bdsOutputSize, T5 &&_iif) > > : sensorInfo(std::forward<T1>(_sensorInfo)) > > , sensorControls(std::forward<T2>(_sensorControls)) > > , lensControls(std::forward<T3>(_lensControls)) > > , bdsOutputSize(std::forward<T4>(_bdsOutputSize)) > > , iif(std::forward<T5>(_iif)) > > { > > } > > #endif > > > > > > IPACameraSensorInfo sensorInfo; > > ControlInfoMap sensorControls; > > ControlInfoMap lensControls; > > Size bdsOutputSize; > > Size iif; > > }; > > > > class IPAIPU3Interface : public IPAInterface > > { > > public: > > > > virtual int32_t init( > > const IPASettings &settings, > > const IPACameraSensorInfo &sensorInfo, > > const ControlInfoMap &sensorControls, > > ControlInfoMap *ipaControls) = 0; > > > > virtual int32_t start() = 0; > > > > virtual void stop() = 0; > > > > virtual int32_t configure( > > const IPAConfigInfo &configInfo, > > ControlInfoMap *ipaControls) = 0; > > > > virtual void mapBuffers( > > const std::vector<libcamera::IPABuffer> &buffers) = 0; > > > > virtual void unmapBuffers( > > const std::vector<uint32_t> &ids) = 0; > > > > virtual void queueRequest( > > const uint32_t frame, > > const ControlList &controls) = 0; > > > > virtual void computeParams( > > const uint32_t frame, > > const uint32_t bufferId) = 0; > > > > virtual void processStats( > > const uint32_t frame, > > const int64_t frameTimestamp, > > const uint32_t bufferId, > > const ControlList &sensorControls) = 0; > > > > Signal<uint32_t, const ControlList &, const ControlList &> setSensorControls; > > > > Signal<uint32_t> paramsComputed; > > > > Signal<uint32_t, const ControlList &> metadataReady; > > }; > > > > which does not change by adding the const lines to the .mojo file. > > > > it seems the const declarations inside mojo files are only intended for use > > inside the mojo files (for e.g. arg default values) and are nor propagated > > to the generic code. > > > > So AFAICT this means that the whole load/create IPA by IPA-proxy type idea > > will not work. > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index d9a50b01d..b430ae5fd 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -9,6 +9,8 @@ module ipa.ipu3; import "include/libcamera/ipa/core.mojom"; struct IPAConfigInfo { + const string kInterfaceName = "ipu3"; + libcamera.IPACameraSensorInfo sensorInfo; libcamera.ControlInfoMap sensorControls; libcamera.ControlInfoMap lensControls; @@ -17,6 +19,8 @@ struct IPAConfigInfo { }; interface IPAIPU3Interface { + const string kInterfaceName = "ipu3"; + init(libcamera.IPASettings settings, libcamera.IPACameraSensorInfo sensorInfo, libcamera.ControlInfoMap sensorControls)