| Message ID | 20260728172447.68551-6-johannes.goede@oss.qualcomm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans On Tue, Jul 28, 2026 at 07:24:47PM +0200, Hans de Goede wrote: > Currently createIPA() / IPAManager::module() assume that there is a 1:1 > relationship between pipeline handlers and IPAs and IPA matching is done > based on matching the pipe's name to ipaModuleInfo.name[]. > > This coupling between pipeline name and which IPA to create is undesirable > and makes it impossible to for example use the softISP as a fallback in > other pipeline handlers then the simple pipeline handler. > > The actual API contract between the IPA and the pipeline handler is given > by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated > IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead > of using PipelineHandler::name(). > > This requires changing the name, as well as the subdirectory of the softISP > IPA from "simple" to "soft", which is the more sensible name anyway. Took me a bit to get why the IPA was named "simple" and found out it's because the pipeline handler name used to create SoftwareISP (which calls createIPA<>) is the "simple" one. Of course this breaks once you need other pipelines to use the soft IPA, showing that it's not the pipeline that matters but the IPAProxy, so I think this really goes in the right direction. > > Note: this means that softISP IPA config files will now be looked for under > /usr/share/libcamera/ipa/soft instead of /usr/share/libcamera/ipa/simple! Updating libcamera to a new version that contains this patch will also re-install the tuning file. The only possible issue is if a user has manually installed files in ipa/simple/ as they will be ignored now. > > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > --- > include/libcamera/internal/pipeline_handler.h | 2 +- > meson_options.txt | 2 +- > src/ipa/meson.build | 2 +- > src/ipa/{simple => soft}/algorithms/adjust.cpp | 0 > src/ipa/{simple => soft}/algorithms/adjust.h | 0 > src/ipa/{simple => soft}/algorithms/agc.cpp | 0 > src/ipa/{simple => soft}/algorithms/agc.h | 0 > src/ipa/{simple => soft}/algorithms/algorithm.h | 0 > src/ipa/{simple => soft}/algorithms/awb.cpp | 2 +- > src/ipa/{simple => soft}/algorithms/awb.h | 0 > src/ipa/{simple => soft}/algorithms/blc.cpp | 0 > src/ipa/{simple => soft}/algorithms/blc.h | 0 > src/ipa/{simple => soft}/algorithms/ccm.cpp | 0 > src/ipa/{simple => soft}/algorithms/ccm.h | 0 > src/ipa/{simple => soft}/algorithms/meson.build | 0 > src/ipa/{simple => soft}/data/meson.build | 2 +- > src/ipa/{simple => soft}/data/uncalibrated.yaml | 0 > src/ipa/{simple => soft}/ipa_context.cpp | 0 > src/ipa/{simple => soft}/ipa_context.h | 0 > src/ipa/{simple => soft}/meson.build | 0 > src/ipa/{simple => soft}/module.h | 0 > src/ipa/{simple => soft}/soft_simple.cpp | 2 +- > src/libcamera/pipeline_handler.cpp | 6 +++--- > .../generators/libcamera_templates/module_ipa_proxy.h.tmpl | 2 ++ > 24 files changed, 11 insertions(+), 9 deletions(-) > rename src/ipa/{simple => soft}/algorithms/adjust.cpp (100%) > rename src/ipa/{simple => soft}/algorithms/adjust.h (100%) > rename src/ipa/{simple => soft}/algorithms/agc.cpp (100%) > rename src/ipa/{simple => soft}/algorithms/agc.h (100%) > rename src/ipa/{simple => soft}/algorithms/algorithm.h (100%) > rename src/ipa/{simple => soft}/algorithms/awb.cpp (98%) > rename src/ipa/{simple => soft}/algorithms/awb.h (100%) > rename src/ipa/{simple => soft}/algorithms/blc.cpp (100%) > rename src/ipa/{simple => soft}/algorithms/blc.h (100%) > rename src/ipa/{simple => soft}/algorithms/ccm.cpp (100%) > rename src/ipa/{simple => soft}/algorithms/ccm.h (100%) > rename src/ipa/{simple => soft}/algorithms/meson.build (100%) > rename src/ipa/{simple => soft}/data/meson.build (80%) > rename src/ipa/{simple => soft}/data/uncalibrated.yaml (100%) > rename src/ipa/{simple => soft}/ipa_context.cpp (100%) > rename src/ipa/{simple => soft}/ipa_context.h (100%) > rename src/ipa/{simple => soft}/meson.build (100%) > rename src/ipa/{simple => soft}/module.h (100%) > rename src/ipa/{simple => soft}/soft_simple.cpp (99%) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index b60c07b13..2a2ba95a4 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -76,7 +76,7 @@ public: > std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) > { > IPAManager *ipaManager = manager_->_d()->ipaManager(); > - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); > + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); > } > > template<typename T> > diff --git a/meson_options.txt b/meson_options.txt > index 20baacc4f..953b093e3 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -48,7 +48,7 @@ option('gstreamer', > > option('ipas', > type : 'array', > - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', > + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'soft', > 'vimc'], > description : 'Select which IPA modules to build') > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > index c583c7efd..2ec9d7c4b 100644 > --- a/src/ipa/meson.build > +++ b/src/ipa/meson.build > @@ -30,7 +30,7 @@ supported_ipas = { > 'rkisp1': 'rkisp1', > 'rpi/pisp': 'rpi/pisp', > 'rpi/vc4': 'rpi/vc4', > - 'simple': 'simple', > + 'simple': 'soft', > 'vimc': 'vimc' > } These two last hunk will also break pre-configured meson build directories. I think it's fine, there is no way around it anyway > > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/soft/algorithms/adjust.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/adjust.cpp > rename to src/ipa/soft/algorithms/adjust.cpp > diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/soft/algorithms/adjust.h > similarity index 100% > rename from src/ipa/simple/algorithms/adjust.h > rename to src/ipa/soft/algorithms/adjust.h > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/soft/algorithms/agc.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/agc.cpp > rename to src/ipa/soft/algorithms/agc.cpp > diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/soft/algorithms/agc.h > similarity index 100% > rename from src/ipa/simple/algorithms/agc.h > rename to src/ipa/soft/algorithms/agc.h > diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/soft/algorithms/algorithm.h > similarity index 100% > rename from src/ipa/simple/algorithms/algorithm.h > rename to src/ipa/soft/algorithms/algorithm.h > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/soft/algorithms/awb.cpp > similarity index 98% > rename from src/ipa/simple/algorithms/awb.cpp > rename to src/ipa/soft/algorithms/awb.cpp > index 05155c83d..35b6e28f1 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/soft/algorithms/awb.cpp > @@ -15,7 +15,7 @@ > #include <libcamera/control_ids.h> > > #include "libipa/colours.h" > -#include "simple/ipa_context.h" > +#include "soft/ipa_context.h" I'm surprised this is the only algorithm that has to include ipa_context.h > > namespace libcamera { > > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/soft/algorithms/awb.h > similarity index 100% > rename from src/ipa/simple/algorithms/awb.h > rename to src/ipa/soft/algorithms/awb.h > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/soft/algorithms/blc.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/blc.cpp > rename to src/ipa/soft/algorithms/blc.cpp > diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/soft/algorithms/blc.h > similarity index 100% > rename from src/ipa/simple/algorithms/blc.h > rename to src/ipa/soft/algorithms/blc.h > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/soft/algorithms/ccm.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/ccm.cpp > rename to src/ipa/soft/algorithms/ccm.cpp > diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/soft/algorithms/ccm.h > similarity index 100% > rename from src/ipa/simple/algorithms/ccm.h > rename to src/ipa/soft/algorithms/ccm.h > diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/soft/algorithms/meson.build > similarity index 100% > rename from src/ipa/simple/algorithms/meson.build > rename to src/ipa/soft/algorithms/meson.build > diff --git a/src/ipa/simple/data/meson.build b/src/ipa/soft/data/meson.build > similarity index 80% > rename from src/ipa/simple/data/meson.build > rename to src/ipa/soft/data/meson.build > index 92795ee4c..f4b806083 100644 > --- a/src/ipa/simple/data/meson.build > +++ b/src/ipa/soft/data/meson.build > @@ -6,5 +6,5 @@ conf_files = files([ > > # The install_dir must match the name from the IPAModuleInfo > install_data(conf_files, > - install_dir : ipa_data_dir / 'simple', > + install_dir : ipa_data_dir / 'soft', > install_tag : 'runtime') > diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/soft/data/uncalibrated.yaml > similarity index 100% > rename from src/ipa/simple/data/uncalibrated.yaml > rename to src/ipa/soft/data/uncalibrated.yaml > diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/soft/ipa_context.cpp > similarity index 100% > rename from src/ipa/simple/ipa_context.cpp > rename to src/ipa/soft/ipa_context.cpp > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/soft/ipa_context.h > similarity index 100% > rename from src/ipa/simple/ipa_context.h > rename to src/ipa/soft/ipa_context.h > diff --git a/src/ipa/simple/meson.build b/src/ipa/soft/meson.build > similarity index 100% > rename from src/ipa/simple/meson.build > rename to src/ipa/soft/meson.build > diff --git a/src/ipa/simple/module.h b/src/ipa/soft/module.h > similarity index 100% > rename from src/ipa/simple/module.h > rename to src/ipa/soft/module.h > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/soft/soft_simple.cpp > similarity index 99% > rename from src/ipa/simple/soft_simple.cpp > rename to src/ipa/soft/soft_simple.cpp > index d4ab91e30..887a1586e 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/soft/soft_simple.cpp > @@ -342,7 +342,7 @@ extern "C" { > const struct IPAModuleInfo ipaModuleInfo = { > IPA_MODULE_API_VERSION, > 0, > - "simple", > + "soft", > }; It might also be appropriate to remove all occurences of Soft from the "SoftSimple" name scheme we have everywhere in the IPA (the log tag, the IPA class name, the IPA module file name). It quite some yak shaving and it can safely be done on top as it's only internal changes anyway. > > IPAInterface *ipaCreate() > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 25fc11989..d92cb5347 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -844,8 +844,8 @@ void PipelineHandler::disconnect() > * > * Create an IPA module using \a ipaName as the matching identifier. This > * overload allows pipeline handlers to create an IPA module by specifying its > - * name instead of relying on the fact that the IPA module matches the pipeline > - * handler's one. > + * name instead of relying on the fact that the IPA module matches the IPA > + * proxy's type name. I anticipate the request of marking this function as "deprectated" and discourage pipeline handler from using it. > * > * \return A newly created IPA proxy, or nullptr if no matching IPA module is > * found or if the IPA proxy fails to initialize > @@ -858,7 +858,7 @@ void PipelineHandler::disconnect() > * \param[in] minVersion Minimum acceptable version of IPA module > * \param[in] maxVersion Maximum acceptable version of IPA module > * > - * Create an IPA module using the pipeline handler name as the matching > + * Create an IPA module using the IPA proxy's type name as the matching > * identifier. This overload allows pipeline handler to create an IPA module > * whose name matches the pipeline handler one. > * > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > index d48b90dcf..c18c94c2c 100644 > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; > class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object > { > public: > + static constexpr const char *name() { return "{{module_name}}"; }; > + > using Threaded = {{proxy_name}}Threaded; > using Isolated = {{proxy_name}}Isolated; All minors, and the SoftSimple/Soft rename can be eventually be addressed on top if desired Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > -- > 2.54.0 >
On Fri, Jul 31, 2026 at 10:32:06AM +0200, Jacopo Mondi wrote: > On Tue, Jul 28, 2026 at 07:24:47PM +0200, Hans de Goede wrote: > > Currently createIPA() / IPAManager::module() assume that there is a 1:1 > > relationship between pipeline handlers and IPAs and IPA matching is done > > based on matching the pipe's name to ipaModuleInfo.name[]. > > > > This coupling between pipeline name and which IPA to create is undesirable > > and makes it impossible to for example use the softISP as a fallback in > > other pipeline handlers then the simple pipeline handler. > > > > The actual API contract between the IPA and the pipeline handler is given > > by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated > > IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead > > of using PipelineHandler::name(). > > > > This requires changing the name, as well as the subdirectory of the softISP > > IPA from "simple" to "soft", which is the more sensible name anyway. I'd prefer naming it "softisp" to be more explicit. Any opinion ? > Took me a bit to get why the IPA was named "simple" and found out it's > because the pipeline handler name used to create SoftwareISP (which > calls createIPA<>) is the "simple" one. > > Of course this breaks once you need other pipelines to use the soft > IPA, showing that it's not the pipeline that matters but the IPAProxy, > so I think this really goes in the right direction. > > > Note: this means that softISP IPA config files will now be looked for under > > /usr/share/libcamera/ipa/soft instead of /usr/share/libcamera/ipa/simple! > > Updating libcamera to a new version that contains this patch will also > re-install the tuning file. > > The only possible issue is if a user has manually installed files in > ipa/simple/ as they will be ignored now. > > > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > > --- > > include/libcamera/internal/pipeline_handler.h | 2 +- > > meson_options.txt | 2 +- > > src/ipa/meson.build | 2 +- > > src/ipa/{simple => soft}/algorithms/adjust.cpp | 0 > > src/ipa/{simple => soft}/algorithms/adjust.h | 0 > > src/ipa/{simple => soft}/algorithms/agc.cpp | 0 > > src/ipa/{simple => soft}/algorithms/agc.h | 0 > > src/ipa/{simple => soft}/algorithms/algorithm.h | 0 > > src/ipa/{simple => soft}/algorithms/awb.cpp | 2 +- > > src/ipa/{simple => soft}/algorithms/awb.h | 0 > > src/ipa/{simple => soft}/algorithms/blc.cpp | 0 > > src/ipa/{simple => soft}/algorithms/blc.h | 0 > > src/ipa/{simple => soft}/algorithms/ccm.cpp | 0 > > src/ipa/{simple => soft}/algorithms/ccm.h | 0 > > src/ipa/{simple => soft}/algorithms/meson.build | 0 > > src/ipa/{simple => soft}/data/meson.build | 2 +- > > src/ipa/{simple => soft}/data/uncalibrated.yaml | 0 > > src/ipa/{simple => soft}/ipa_context.cpp | 0 > > src/ipa/{simple => soft}/ipa_context.h | 0 > > src/ipa/{simple => soft}/meson.build | 0 > > src/ipa/{simple => soft}/module.h | 0 > > src/ipa/{simple => soft}/soft_simple.cpp | 2 +- > > src/libcamera/pipeline_handler.cpp | 6 +++--- > > .../generators/libcamera_templates/module_ipa_proxy.h.tmpl | 2 ++ > > 24 files changed, 11 insertions(+), 9 deletions(-) > > rename src/ipa/{simple => soft}/algorithms/adjust.cpp (100%) > > rename src/ipa/{simple => soft}/algorithms/adjust.h (100%) > > rename src/ipa/{simple => soft}/algorithms/agc.cpp (100%) > > rename src/ipa/{simple => soft}/algorithms/agc.h (100%) > > rename src/ipa/{simple => soft}/algorithms/algorithm.h (100%) > > rename src/ipa/{simple => soft}/algorithms/awb.cpp (98%) > > rename src/ipa/{simple => soft}/algorithms/awb.h (100%) > > rename src/ipa/{simple => soft}/algorithms/blc.cpp (100%) > > rename src/ipa/{simple => soft}/algorithms/blc.h (100%) > > rename src/ipa/{simple => soft}/algorithms/ccm.cpp (100%) > > rename src/ipa/{simple => soft}/algorithms/ccm.h (100%) > > rename src/ipa/{simple => soft}/algorithms/meson.build (100%) > > rename src/ipa/{simple => soft}/data/meson.build (80%) > > rename src/ipa/{simple => soft}/data/uncalibrated.yaml (100%) > > rename src/ipa/{simple => soft}/ipa_context.cpp (100%) > > rename src/ipa/{simple => soft}/ipa_context.h (100%) > > rename src/ipa/{simple => soft}/meson.build (100%) > > rename src/ipa/{simple => soft}/module.h (100%) > > rename src/ipa/{simple => soft}/soft_simple.cpp (99%) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index b60c07b13..2a2ba95a4 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -76,7 +76,7 @@ public: > > std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) > > { > > IPAManager *ipaManager = manager_->_d()->ipaManager(); > > - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); > > + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); > > } > > > > template<typename T> > > diff --git a/meson_options.txt b/meson_options.txt > > index 20baacc4f..953b093e3 100644 > > --- a/meson_options.txt > > +++ b/meson_options.txt > > @@ -48,7 +48,7 @@ option('gstreamer', > > > > option('ipas', > > type : 'array', > > - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', > > + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'soft', > > 'vimc'], > > description : 'Select which IPA modules to build') > > > > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > > index c583c7efd..2ec9d7c4b 100644 > > --- a/src/ipa/meson.build > > +++ b/src/ipa/meson.build > > @@ -30,7 +30,7 @@ supported_ipas = { > > 'rkisp1': 'rkisp1', > > 'rpi/pisp': 'rpi/pisp', > > 'rpi/vc4': 'rpi/vc4', > > - 'simple': 'simple', > > + 'simple': 'soft', > > 'vimc': 'vimc' > > } > > These two last hunk will also break pre-configured meson build > directories. I think it's fine, there is no way around it anyway > > > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/soft/algorithms/adjust.cpp > > similarity index 100% > > rename from src/ipa/simple/algorithms/adjust.cpp > > rename to src/ipa/soft/algorithms/adjust.cpp > > diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/soft/algorithms/adjust.h > > similarity index 100% > > rename from src/ipa/simple/algorithms/adjust.h > > rename to src/ipa/soft/algorithms/adjust.h > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/soft/algorithms/agc.cpp > > similarity index 100% > > rename from src/ipa/simple/algorithms/agc.cpp > > rename to src/ipa/soft/algorithms/agc.cpp > > diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/soft/algorithms/agc.h > > similarity index 100% > > rename from src/ipa/simple/algorithms/agc.h > > rename to src/ipa/soft/algorithms/agc.h > > diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/soft/algorithms/algorithm.h > > similarity index 100% > > rename from src/ipa/simple/algorithms/algorithm.h > > rename to src/ipa/soft/algorithms/algorithm.h > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/soft/algorithms/awb.cpp > > similarity index 98% > > rename from src/ipa/simple/algorithms/awb.cpp > > rename to src/ipa/soft/algorithms/awb.cpp > > index 05155c83d..35b6e28f1 100644 > > --- a/src/ipa/simple/algorithms/awb.cpp > > +++ b/src/ipa/soft/algorithms/awb.cpp > > @@ -15,7 +15,7 @@ > > #include <libcamera/control_ids.h> > > > > #include "libipa/colours.h" > > -#include "simple/ipa_context.h" > > +#include "soft/ipa_context.h" > > I'm surprised this is the only algorithm that has to include > ipa_context.h > > > namespace libcamera { > > > > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/soft/algorithms/awb.h > > similarity index 100% > > rename from src/ipa/simple/algorithms/awb.h > > rename to src/ipa/soft/algorithms/awb.h > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/soft/algorithms/blc.cpp > > similarity index 100% > > rename from src/ipa/simple/algorithms/blc.cpp > > rename to src/ipa/soft/algorithms/blc.cpp > > diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/soft/algorithms/blc.h > > similarity index 100% > > rename from src/ipa/simple/algorithms/blc.h > > rename to src/ipa/soft/algorithms/blc.h > > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/soft/algorithms/ccm.cpp > > similarity index 100% > > rename from src/ipa/simple/algorithms/ccm.cpp > > rename to src/ipa/soft/algorithms/ccm.cpp > > diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/soft/algorithms/ccm.h > > similarity index 100% > > rename from src/ipa/simple/algorithms/ccm.h > > rename to src/ipa/soft/algorithms/ccm.h > > diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/soft/algorithms/meson.build > > similarity index 100% > > rename from src/ipa/simple/algorithms/meson.build > > rename to src/ipa/soft/algorithms/meson.build > > diff --git a/src/ipa/simple/data/meson.build b/src/ipa/soft/data/meson.build > > similarity index 80% > > rename from src/ipa/simple/data/meson.build > > rename to src/ipa/soft/data/meson.build > > index 92795ee4c..f4b806083 100644 > > --- a/src/ipa/simple/data/meson.build > > +++ b/src/ipa/soft/data/meson.build > > @@ -6,5 +6,5 @@ conf_files = files([ > > > > # The install_dir must match the name from the IPAModuleInfo > > install_data(conf_files, > > - install_dir : ipa_data_dir / 'simple', > > + install_dir : ipa_data_dir / 'soft', > > install_tag : 'runtime') > > diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/soft/data/uncalibrated.yaml > > similarity index 100% > > rename from src/ipa/simple/data/uncalibrated.yaml > > rename to src/ipa/soft/data/uncalibrated.yaml > > diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/soft/ipa_context.cpp > > similarity index 100% > > rename from src/ipa/simple/ipa_context.cpp > > rename to src/ipa/soft/ipa_context.cpp > > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/soft/ipa_context.h > > similarity index 100% > > rename from src/ipa/simple/ipa_context.h > > rename to src/ipa/soft/ipa_context.h > > diff --git a/src/ipa/simple/meson.build b/src/ipa/soft/meson.build > > similarity index 100% > > rename from src/ipa/simple/meson.build > > rename to src/ipa/soft/meson.build > > diff --git a/src/ipa/simple/module.h b/src/ipa/soft/module.h > > similarity index 100% > > rename from src/ipa/simple/module.h > > rename to src/ipa/soft/module.h > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/soft/soft_simple.cpp > > similarity index 99% > > rename from src/ipa/simple/soft_simple.cpp > > rename to src/ipa/soft/soft_simple.cpp > > index d4ab91e30..887a1586e 100644 > > --- a/src/ipa/simple/soft_simple.cpp > > +++ b/src/ipa/soft/soft_simple.cpp > > @@ -342,7 +342,7 @@ extern "C" { > > const struct IPAModuleInfo ipaModuleInfo = { > > IPA_MODULE_API_VERSION, > > 0, > > - "simple", > > + "soft", > > }; > > It might also be appropriate to remove all occurences of Soft from the > "SoftSimple" name scheme we have everywhere in the IPA (the log tag, > the IPA class name, the IPA module file name). I think it's a good idea. > It quite some yak shaving and it can safely be done on top as it's > only internal changes anyway. > > > IPAInterface *ipaCreate() > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 25fc11989..d92cb5347 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -844,8 +844,8 @@ void PipelineHandler::disconnect() > > * > > * Create an IPA module using \a ipaName as the matching identifier. This > > * overload allows pipeline handlers to create an IPA module by specifying its > > - * name instead of relying on the fact that the IPA module matches the pipeline > > - * handler's one. > > + * name instead of relying on the fact that the IPA module matches the IPA > > + * proxy's type name. > > I anticipate the request of marking this function as "deprectated" and > discourage pipeline handler from using it. Yes please, let's document that no new user will be accepted. > > * > > * \return A newly created IPA proxy, or nullptr if no matching IPA module is > > * found or if the IPA proxy fails to initialize > > @@ -858,7 +858,7 @@ void PipelineHandler::disconnect() > > * \param[in] minVersion Minimum acceptable version of IPA module > > * \param[in] maxVersion Maximum acceptable version of IPA module > > * > > - * Create an IPA module using the pipeline handler name as the matching > > + * Create an IPA module using the IPA proxy's type name as the matching > > * identifier. This overload allows pipeline handler to create an IPA module > > * whose name matches the pipeline handler one. > > * > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > > index d48b90dcf..c18c94c2c 100644 > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > > @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; > > class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object > > { > > public: > > + static constexpr const char *name() { return "{{module_name}}"; }; > > + > > using Threaded = {{proxy_name}}Threaded; > > using Isolated = {{proxy_name}}Isolated; > > All minors, and the SoftSimple/Soft rename can be eventually be addressed on top > if desired > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Hi, On 31-Jul-26 10:32, Jacopo Mondi wrote: > Hi Hans > > On Tue, Jul 28, 2026 at 07:24:47PM +0200, Hans de Goede wrote: >> Currently createIPA() / IPAManager::module() assume that there is a 1:1 >> relationship between pipeline handlers and IPAs and IPA matching is done >> based on matching the pipe's name to ipaModuleInfo.name[]. >> >> This coupling between pipeline name and which IPA to create is undesirable >> and makes it impossible to for example use the softISP as a fallback in >> other pipeline handlers then the simple pipeline handler. >> >> The actual API contract between the IPA and the pipeline handler is given >> by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated >> IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead >> of using PipelineHandler::name(). >> >> This requires changing the name, as well as the subdirectory of the softISP >> IPA from "simple" to "soft", which is the more sensible name anyway. <snip> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/soft/soft_simple.cpp >> similarity index 99% >> rename from src/ipa/simple/soft_simple.cpp >> rename to src/ipa/soft/soft_simple.cpp >> index d4ab91e30..887a1586e 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/soft/soft_simple.cpp >> @@ -342,7 +342,7 @@ extern "C" { >> const struct IPAModuleInfo ipaModuleInfo = { >> IPA_MODULE_API_VERSION, >> 0, >> - "simple", >> + "soft", >> }; > > > It might also be appropriate to remove all occurrences of Soft from the > "SoftSimple" name scheme we have everywhere in the IPA (the log tag, > the IPA class name, the IPA module file name). I assume you mean remove all occurrences of "Simple" not "Soft" :) > It quite some yak shaving and it can safely be done on top as it's > only internal changes anyway. Right, I was not sure whether to make more changes here or not. But since both you and Laurent have requested this now I'll add an extra patch to the series replacing SoftSimple with SoftIsp everywhere. I'm going to do this in an extra patch to keep this patch which also renames all the SoftISP IPA files as minimal as possible, focused on just switching to the matching by proxy tpe name + the required renames for this. I believe things will be easier to review this way. >> IPAInterface *ipaCreate() >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >> index 25fc11989..d92cb5347 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -844,8 +844,8 @@ void PipelineHandler::disconnect() >> * >> * Create an IPA module using \a ipaName as the matching identifier. This >> * overload allows pipeline handlers to create an IPA module by specifying its >> - * name instead of relying on the fact that the IPA module matches the pipeline >> - * handler's one. >> + * name instead of relying on the fact that the IPA module matches the IPA >> + * proxy's type name. > > > I anticipate the request of marking this function as "deprectated" and > discourage pipeline handler from using it. Ack, done for the upcoming v7 of this series. <snip> > All minors, and the SoftSimple/Soft rename can be eventually be addressed on top > if desired > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thank you. Regards, Hans
Hi, On 31-Jul-26 19:53, Laurent Pinchart wrote: > On Fri, Jul 31, 2026 at 10:32:06AM +0200, Jacopo Mondi wrote: >> On Tue, Jul 28, 2026 at 07:24:47PM +0200, Hans de Goede wrote: >>> Currently createIPA() / IPAManager::module() assume that there is a 1:1 >>> relationship between pipeline handlers and IPAs and IPA matching is done >>> based on matching the pipe's name to ipaModuleInfo.name[]. >>> >>> This coupling between pipeline name and which IPA to create is undesirable >>> and makes it impossible to for example use the softISP as a fallback in >>> other pipeline handlers then the simple pipeline handler. >>> >>> The actual API contract between the IPA and the pipeline handler is given >>> by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated >>> IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead >>> of using PipelineHandler::name(). >>> >>> This requires changing the name, as well as the subdirectory of the softISP >>> IPA from "simple" to "soft", which is the more sensible name anyway. > > I'd prefer naming it "softisp" to be more explicit. Any opinion ? Ack, done for the upcoming v7 series. <snip> >> It might also be appropriate to remove all occurences of Soft from the >> "SoftSimple" name scheme we have everywhere in the IPA (the log tag, >> the IPA class name, the IPA module file name). > > I think it's a good idea. Ack, as I wrote in reply to Jacopo's original comment: I'll add an extra patch to the series replacing SoftSimple with SoftIsp everywhere. I'm going to do this in an extra patch to keep this patch which also renames all the SoftISP IPA files as minimal as possible, focused on just switching to the matching by proxy tpe name + the required renames for this. I believe things will be easier to review this way. >> It quite some yak shaving and it can safely be done on top as it's >> only internal changes anyway. >> >>> IPAInterface *ipaCreate() >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >>> index 25fc11989..d92cb5347 100644 >>> --- a/src/libcamera/pipeline_handler.cpp >>> +++ b/src/libcamera/pipeline_handler.cpp >>> @@ -844,8 +844,8 @@ void PipelineHandler::disconnect() >>> * >>> * Create an IPA module using \a ipaName as the matching identifier. This >>> * overload allows pipeline handlers to create an IPA module by specifying its >>> - * name instead of relying on the fact that the IPA module matches the pipeline >>> - * handler's one. >>> + * name instead of relying on the fact that the IPA module matches the IPA >>> + * proxy's type name. >> >> I anticipate the request of marking this function as "deprectated" and >> discourage pipeline handler from using it. > > Yes please, let's document that no new user will be accepted. Ack, done for the upcoming v7 series. Regards, Hans
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index b60c07b13..2a2ba95a4 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -76,7 +76,7 @@ public: std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) { IPAManager *ipaManager = manager_->_d()->ipaManager(); - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); } template<typename T> diff --git a/meson_options.txt b/meson_options.txt index 20baacc4f..953b093e3 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -48,7 +48,7 @@ option('gstreamer', option('ipas', type : 'array', - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'soft', 'vimc'], description : 'Select which IPA modules to build') diff --git a/src/ipa/meson.build b/src/ipa/meson.build index c583c7efd..2ec9d7c4b 100644 --- a/src/ipa/meson.build +++ b/src/ipa/meson.build @@ -30,7 +30,7 @@ supported_ipas = { 'rkisp1': 'rkisp1', 'rpi/pisp': 'rpi/pisp', 'rpi/vc4': 'rpi/vc4', - 'simple': 'simple', + 'simple': 'soft', 'vimc': 'vimc' } diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/soft/algorithms/adjust.cpp similarity index 100% rename from src/ipa/simple/algorithms/adjust.cpp rename to src/ipa/soft/algorithms/adjust.cpp diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/soft/algorithms/adjust.h similarity index 100% rename from src/ipa/simple/algorithms/adjust.h rename to src/ipa/soft/algorithms/adjust.h diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/soft/algorithms/agc.cpp similarity index 100% rename from src/ipa/simple/algorithms/agc.cpp rename to src/ipa/soft/algorithms/agc.cpp diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/soft/algorithms/agc.h similarity index 100% rename from src/ipa/simple/algorithms/agc.h rename to src/ipa/soft/algorithms/agc.h diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/soft/algorithms/algorithm.h similarity index 100% rename from src/ipa/simple/algorithms/algorithm.h rename to src/ipa/soft/algorithms/algorithm.h diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/soft/algorithms/awb.cpp similarity index 98% rename from src/ipa/simple/algorithms/awb.cpp rename to src/ipa/soft/algorithms/awb.cpp index 05155c83d..35b6e28f1 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/soft/algorithms/awb.cpp @@ -15,7 +15,7 @@ #include <libcamera/control_ids.h> #include "libipa/colours.h" -#include "simple/ipa_context.h" +#include "soft/ipa_context.h" namespace libcamera { diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/soft/algorithms/awb.h similarity index 100% rename from src/ipa/simple/algorithms/awb.h rename to src/ipa/soft/algorithms/awb.h diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/soft/algorithms/blc.cpp similarity index 100% rename from src/ipa/simple/algorithms/blc.cpp rename to src/ipa/soft/algorithms/blc.cpp diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/soft/algorithms/blc.h similarity index 100% rename from src/ipa/simple/algorithms/blc.h rename to src/ipa/soft/algorithms/blc.h diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/soft/algorithms/ccm.cpp similarity index 100% rename from src/ipa/simple/algorithms/ccm.cpp rename to src/ipa/soft/algorithms/ccm.cpp diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/soft/algorithms/ccm.h similarity index 100% rename from src/ipa/simple/algorithms/ccm.h rename to src/ipa/soft/algorithms/ccm.h diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/soft/algorithms/meson.build similarity index 100% rename from src/ipa/simple/algorithms/meson.build rename to src/ipa/soft/algorithms/meson.build diff --git a/src/ipa/simple/data/meson.build b/src/ipa/soft/data/meson.build similarity index 80% rename from src/ipa/simple/data/meson.build rename to src/ipa/soft/data/meson.build index 92795ee4c..f4b806083 100644 --- a/src/ipa/simple/data/meson.build +++ b/src/ipa/soft/data/meson.build @@ -6,5 +6,5 @@ conf_files = files([ # The install_dir must match the name from the IPAModuleInfo install_data(conf_files, - install_dir : ipa_data_dir / 'simple', + install_dir : ipa_data_dir / 'soft', install_tag : 'runtime') diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/soft/data/uncalibrated.yaml similarity index 100% rename from src/ipa/simple/data/uncalibrated.yaml rename to src/ipa/soft/data/uncalibrated.yaml diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/soft/ipa_context.cpp similarity index 100% rename from src/ipa/simple/ipa_context.cpp rename to src/ipa/soft/ipa_context.cpp diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/soft/ipa_context.h similarity index 100% rename from src/ipa/simple/ipa_context.h rename to src/ipa/soft/ipa_context.h diff --git a/src/ipa/simple/meson.build b/src/ipa/soft/meson.build similarity index 100% rename from src/ipa/simple/meson.build rename to src/ipa/soft/meson.build diff --git a/src/ipa/simple/module.h b/src/ipa/soft/module.h similarity index 100% rename from src/ipa/simple/module.h rename to src/ipa/soft/module.h diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/soft/soft_simple.cpp similarity index 99% rename from src/ipa/simple/soft_simple.cpp rename to src/ipa/soft/soft_simple.cpp index d4ab91e30..887a1586e 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/soft/soft_simple.cpp @@ -342,7 +342,7 @@ extern "C" { const struct IPAModuleInfo ipaModuleInfo = { IPA_MODULE_API_VERSION, 0, - "simple", + "soft", }; IPAInterface *ipaCreate() diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 25fc11989..d92cb5347 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -844,8 +844,8 @@ void PipelineHandler::disconnect() * * Create an IPA module using \a ipaName as the matching identifier. This * overload allows pipeline handlers to create an IPA module by specifying its - * name instead of relying on the fact that the IPA module matches the pipeline - * handler's one. + * name instead of relying on the fact that the IPA module matches the IPA + * proxy's type name. * * \return A newly created IPA proxy, or nullptr if no matching IPA module is * found or if the IPA proxy fails to initialize @@ -858,7 +858,7 @@ void PipelineHandler::disconnect() * \param[in] minVersion Minimum acceptable version of IPA module * \param[in] maxVersion Maximum acceptable version of IPA module * - * Create an IPA module using the pipeline handler name as the matching + * Create an IPA module using the IPA proxy's type name as the matching * identifier. This overload allows pipeline handler to create an IPA module * whose name matches the pipeline handler one. * diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl index d48b90dcf..c18c94c2c 100644 --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object { public: + static constexpr const char *name() { return "{{module_name}}"; }; + using Threaded = {{proxy_name}}Threaded; using Isolated = {{proxy_name}}Isolated;
Currently createIPA() / IPAManager::module() assume that there is a 1:1 relationship between pipeline handlers and IPAs and IPA matching is done based on matching the pipe's name to ipaModuleInfo.name[]. This coupling between pipeline name and which IPA to create is undesirable and makes it impossible to for example use the softISP as a fallback in other pipeline handlers then the simple pipeline handler. The actual API contract between the IPA and the pipeline handler is given by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead of using PipelineHandler::name(). This requires changing the name, as well as the subdirectory of the softISP IPA from "simple" to "soft", which is the more sensible name anyway. Note: this means that softISP IPA config files will now be looked for under /usr/share/libcamera/ipa/soft instead of /usr/share/libcamera/ipa/simple! Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> --- include/libcamera/internal/pipeline_handler.h | 2 +- meson_options.txt | 2 +- src/ipa/meson.build | 2 +- src/ipa/{simple => soft}/algorithms/adjust.cpp | 0 src/ipa/{simple => soft}/algorithms/adjust.h | 0 src/ipa/{simple => soft}/algorithms/agc.cpp | 0 src/ipa/{simple => soft}/algorithms/agc.h | 0 src/ipa/{simple => soft}/algorithms/algorithm.h | 0 src/ipa/{simple => soft}/algorithms/awb.cpp | 2 +- src/ipa/{simple => soft}/algorithms/awb.h | 0 src/ipa/{simple => soft}/algorithms/blc.cpp | 0 src/ipa/{simple => soft}/algorithms/blc.h | 0 src/ipa/{simple => soft}/algorithms/ccm.cpp | 0 src/ipa/{simple => soft}/algorithms/ccm.h | 0 src/ipa/{simple => soft}/algorithms/meson.build | 0 src/ipa/{simple => soft}/data/meson.build | 2 +- src/ipa/{simple => soft}/data/uncalibrated.yaml | 0 src/ipa/{simple => soft}/ipa_context.cpp | 0 src/ipa/{simple => soft}/ipa_context.h | 0 src/ipa/{simple => soft}/meson.build | 0 src/ipa/{simple => soft}/module.h | 0 src/ipa/{simple => soft}/soft_simple.cpp | 2 +- src/libcamera/pipeline_handler.cpp | 6 +++--- .../generators/libcamera_templates/module_ipa_proxy.h.tmpl | 2 ++ 24 files changed, 11 insertions(+), 9 deletions(-) rename src/ipa/{simple => soft}/algorithms/adjust.cpp (100%) rename src/ipa/{simple => soft}/algorithms/adjust.h (100%) rename src/ipa/{simple => soft}/algorithms/agc.cpp (100%) rename src/ipa/{simple => soft}/algorithms/agc.h (100%) rename src/ipa/{simple => soft}/algorithms/algorithm.h (100%) rename src/ipa/{simple => soft}/algorithms/awb.cpp (98%) rename src/ipa/{simple => soft}/algorithms/awb.h (100%) rename src/ipa/{simple => soft}/algorithms/blc.cpp (100%) rename src/ipa/{simple => soft}/algorithms/blc.h (100%) rename src/ipa/{simple => soft}/algorithms/ccm.cpp (100%) rename src/ipa/{simple => soft}/algorithms/ccm.h (100%) rename src/ipa/{simple => soft}/algorithms/meson.build (100%) rename src/ipa/{simple => soft}/data/meson.build (80%) rename src/ipa/{simple => soft}/data/uncalibrated.yaml (100%) rename src/ipa/{simple => soft}/ipa_context.cpp (100%) rename src/ipa/{simple => soft}/ipa_context.h (100%) rename src/ipa/{simple => soft}/meson.build (100%) rename src/ipa/{simple => soft}/module.h (100%) rename src/ipa/{simple => soft}/soft_simple.cpp (99%)