| Message ID | 20210519101954.77711-1-umang.jain@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
On Wed, May 19, 2021 at 03:49:51PM +0530, Umang Jain wrote: > Generated IPA headers from mojom files need to be installed to > $INCLUDE_PATH in order to be available system-wide. Without this, > out-of-tree IPAs won't be able to link and build themselves. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/meson.build | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > index 40c4e737..eca4e9ee 100644 > --- a/include/libcamera/ipa/meson.build > +++ b/include/libcamera/ipa/meson.build > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: CC0-1.0 > > +libcamera_ipa_include_dir = libcamera_include_dir / 'ipa' > + > libcamera_ipa_headers = files([ > 'ipa_controls.h', > 'ipa_interface.h', > @@ -7,7 +9,7 @@ libcamera_ipa_headers = files([ > ]) > > install_headers(libcamera_ipa_headers, > - subdir: libcamera_include_dir / 'ipa') > + subdir: libcamera_ipa_include_dir) > > libcamera_generated_ipa_headers = [] > > @@ -31,6 +33,8 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_interface_h', > input : ipa_mojom_core, > output : 'core_ipa_interface.h', > depends : mojom_templates, > + install : true, > + install_dir : get_option('includedir') / libcamera_ipa_include_dir, > command : [ > mojom_generator, 'generate', > '-g', 'libcamera', > @@ -93,6 +97,8 @@ foreach file : ipa_mojom_files > input : mojom, > output : name + '_ipa_interface.h', > depends : mojom_templates, > + install : true, > + install_dir : get_option('includedir') / libcamera_ipa_include_dir, > command : [ > mojom_generator, 'generate', > '-g', 'libcamera', > -- > 2.26.2 >
On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: > There can be multiple IPAs per pipeline-handler or platform. > They can live in-tree or externally linked. To support the externally > linked IPA use-case, provide a mechanism to choose whether or not > to build the IPAs in tree, with the help of a meson configuration > option. > > By default, all in-tree IPAs are built. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > meson.build | 1 + > meson_options.txt | 5 +++++ > src/ipa/meson.build | 5 +++-- > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index 46eb1b46..6626fa7e 100644 > --- a/meson.build > +++ b/meson.build > @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) > ## Summarise Configurations > summary({ > 'Enabled pipelines': pipelines, > + 'Enabled IPA modules': ipa_modules, > 'Android support': android_enabled, > 'GStreamer support': gst_enabled, > 'V4L2 emulation support': v4l2_enabled, > diff --git a/meson_options.txt b/meson_options.txt > index 69f11f85..2c80ad8b 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -25,6 +25,11 @@ option('gstreamer', > value : 'auto', > description : 'Compile libcamera GStreamer plugin') > > +option('ipas', > + type : 'array', > + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], > + description : 'Select which IPA modules to build') > + > option('lc-compliance', > type : 'feature', > value : 'auto', > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > index 5b5684a1..49245e5e 100644 > --- a/src/ipa/meson.build > +++ b/src/ipa/meson.build > @@ -19,14 +19,15 @@ subdir('libipa') > > ipa_sign = files('ipa-sign.sh') > > -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] > ipa_names = [] > > +ipa_modules = get_option('ipas') > + > # The ipa-sign-install.sh script which uses the ipa_names variable will itself > # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we > # must not include the prefix string here. > foreach pipeline : pipelines > - if ipas.contains(pipeline) > + if ipa_modules.contains(pipeline) > subdir(pipeline) > ipa_names += ipa_install_dir / ipa_name + '.so' > endif > -- > 2.26.2 >
Hi Umang, On 20/05/2021 04:52, paul.elder@ideasonboard.com wrote: > On Wed, May 19, 2021 at 03:49:51PM +0530, Umang Jain wrote: >> Generated IPA headers from mojom files need to be installed to >> $INCLUDE_PATH in order to be available system-wide. Without this, >> out-of-tree IPAs won't be able to link and build themselves. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- >> include/libcamera/ipa/meson.build | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build >> index 40c4e737..eca4e9ee 100644 >> --- a/include/libcamera/ipa/meson.build >> +++ b/include/libcamera/ipa/meson.build >> @@ -1,5 +1,7 @@ >> # SPDX-License-Identifier: CC0-1.0 >> >> +libcamera_ipa_include_dir = libcamera_include_dir / 'ipa' >> + >> libcamera_ipa_headers = files([ >> 'ipa_controls.h', >> 'ipa_interface.h', >> @@ -7,7 +9,7 @@ libcamera_ipa_headers = files([ >> ]) >> >> install_headers(libcamera_ipa_headers, >> - subdir: libcamera_include_dir / 'ipa') >> + subdir: libcamera_ipa_include_dir) >> >> libcamera_generated_ipa_headers = [] >> >> @@ -31,6 +33,8 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_interface_h', >> input : ipa_mojom_core, >> output : 'core_ipa_interface.h', >> depends : mojom_templates, >> + install : true, >> + install_dir : get_option('includedir') / libcamera_ipa_include_dir, >> command : [ >> mojom_generator, 'generate', >> '-g', 'libcamera', >> @@ -93,6 +97,8 @@ foreach file : ipa_mojom_files >> input : mojom, >> output : name + '_ipa_interface.h', >> depends : mojom_templates, >> + install : true, >> + install_dir : get_option('includedir') / libcamera_ipa_include_dir, >> command : [ >> mojom_generator, 'generate', >> '-g', 'libcamera', >> -- >> 2.26.2 >>
Hi Umang, On 20/05/2021 05:02, paul.elder@ideasonboard.com wrote: > On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: >> There can be multiple IPAs per pipeline-handler or platform. >> They can live in-tree or externally linked. To support the externally >> linked IPA use-case, provide a mechanism to choose whether or not >> to build the IPAs in tree, with the help of a meson configuration >> option. >> >> By default, all in-tree IPAs are built. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- >> meson.build | 1 + >> meson_options.txt | 5 +++++ >> src/ipa/meson.build | 5 +++-- >> 3 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index 46eb1b46..6626fa7e 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) >> ## Summarise Configurations >> summary({ >> 'Enabled pipelines': pipelines, >> + 'Enabled IPA modules': ipa_modules, >> 'Android support': android_enabled, >> 'GStreamer support': gst_enabled, >> 'V4L2 emulation support': v4l2_enabled, >> diff --git a/meson_options.txt b/meson_options.txt >> index 69f11f85..2c80ad8b 100644 >> --- a/meson_options.txt >> +++ b/meson_options.txt >> @@ -25,6 +25,11 @@ option('gstreamer', >> value : 'auto', >> description : 'Compile libcamera GStreamer plugin') >> >> +option('ipas', >> + type : 'array', >> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], >> + description : 'Select which IPA modules to build') >> + >> option('lc-compliance', >> type : 'feature', >> value : 'auto', >> diff --git a/src/ipa/meson.build b/src/ipa/meson.build >> index 5b5684a1..49245e5e 100644 >> --- a/src/ipa/meson.build >> +++ b/src/ipa/meson.build >> @@ -19,14 +19,15 @@ subdir('libipa') >> >> ipa_sign = files('ipa-sign.sh') >> >> -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] >> ipa_names = [] >> >> +ipa_modules = get_option('ipas') >> + >> # The ipa-sign-install.sh script which uses the ipa_names variable will itself >> # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we >> # must not include the prefix string here. >> foreach pipeline : pipelines >> - if ipas.contains(pipeline) >> + if ipa_modules.contains(pipeline) >> subdir(pipeline) >> ipa_names += ipa_install_dir / ipa_name + '.so' >> endif >> -- >> 2.26.2 >>
Hi Umang, On 20/05/2021 05:02, paul.elder@ideasonboard.com wrote: > On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: >> There can be multiple IPAs per pipeline-handler or platform. >> They can live in-tree or externally linked. To support the externally >> linked IPA use-case, provide a mechanism to choose whether or not >> to build the IPAs in tree, with the help of a meson configuration >> option. >> >> By default, all in-tree IPAs are built. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> meson.build | 1 + >> meson_options.txt | 5 +++++ >> src/ipa/meson.build | 5 +++-- >> 3 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index 46eb1b46..6626fa7e 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) >> ## Summarise Configurations >> summary({ >> 'Enabled pipelines': pipelines, >> + 'Enabled IPA modules': ipa_modules, >> 'Android support': android_enabled, >> 'GStreamer support': gst_enabled, >> 'V4L2 emulation support': v4l2_enabled, >> diff --git a/meson_options.txt b/meson_options.txt >> index 69f11f85..2c80ad8b 100644 >> --- a/meson_options.txt >> +++ b/meson_options.txt >> @@ -25,6 +25,11 @@ option('gstreamer', >> value : 'auto', >> description : 'Compile libcamera GStreamer plugin') >> >> +option('ipas', >> + type : 'array', >> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], >> + description : 'Select which IPA modules to build') >> + >> option('lc-compliance', >> type : 'feature', >> value : 'auto', >> diff --git a/src/ipa/meson.build b/src/ipa/meson.build >> index 5b5684a1..49245e5e 100644 >> --- a/src/ipa/meson.build >> +++ b/src/ipa/meson.build >> @@ -19,14 +19,15 @@ subdir('libipa') >> >> ipa_sign = files('ipa-sign.sh') >> >> -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] >> ipa_names = [] >> >> +ipa_modules = get_option('ipas') >> + >> # The ipa-sign-install.sh script which uses the ipa_names variable will itself >> # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we >> # must not include the prefix string here. >> foreach pipeline : pipelines >> - if ipas.contains(pipeline) >> + if ipa_modules.contains(pipeline) >> subdir(pipeline) >> ipa_names += ipa_install_dir / ipa_name + '.so' >> endif >> -- >> 2.26.2 >>
Hi Umang, On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: > There can be multiple IPAs per pipeline-handler or platform. > They can live in-tree or externally linked. To support the externally > linked IPA use-case, provide a mechanism to choose whether or not > to build the IPAs in tree, with the help of a meson configuration > option. > > By default, all in-tree IPAs are built. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > meson.build | 1 + > meson_options.txt | 5 +++++ > src/ipa/meson.build | 5 +++-- > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index 46eb1b46..6626fa7e 100644 > --- a/meson.build > +++ b/meson.build > @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) > ## Summarise Configurations > summary({ > 'Enabled pipelines': pipelines, > + 'Enabled IPA modules': ipa_modules, > 'Android support': android_enabled, > 'GStreamer support': gst_enabled, > 'V4L2 emulation support': v4l2_enabled, > diff --git a/meson_options.txt b/meson_options.txt > index 69f11f85..2c80ad8b 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -25,6 +25,11 @@ option('gstreamer', > value : 'auto', > description : 'Compile libcamera GStreamer plugin') > > +option('ipas', > + type : 'array', > + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], > + description : 'Select which IPA modules to build') > + Mmm, this new options means that by default all the IPAs are built, even if the pipeline handler is not built. This requires a more precise control of the build options, as it's now easier to mis-align pipelines and IPAs. Have we considered the other way around ? Build by default the IPAs for which a pipeline is built (like we do today) unless it is blacklisted ? > option('lc-compliance', > type : 'feature', > value : 'auto', > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > index 5b5684a1..49245e5e 100644 > --- a/src/ipa/meson.build > +++ b/src/ipa/meson.build > @@ -19,14 +19,15 @@ subdir('libipa') > > ipa_sign = files('ipa-sign.sh') > > -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] > ipa_names = [] > > +ipa_modules = get_option('ipas') > + > # The ipa-sign-install.sh script which uses the ipa_names variable will itself > # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we > # must not include the prefix string here. > foreach pipeline : pipelines > - if ipas.contains(pipeline) > + if ipa_modules.contains(pipeline) > subdir(pipeline) > ipa_names += ipa_install_dir / ipa_name + '.so' > endif > -- > 2.26.2 >
Hi Jacopo, On 21/05/2021 10:48, Jacopo Mondi wrote: > Hi Umang, > > On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: >> There can be multiple IPAs per pipeline-handler or platform. >> They can live in-tree or externally linked. To support the externally >> linked IPA use-case, provide a mechanism to choose whether or not >> to build the IPAs in tree, with the help of a meson configuration >> option. >> >> By default, all in-tree IPAs are built. "By default, all in-tree IPAs are built when a matching Pipeline handler is also enabled." >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> meson.build | 1 + >> meson_options.txt | 5 +++++ >> src/ipa/meson.build | 5 +++-- >> 3 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index 46eb1b46..6626fa7e 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) >> ## Summarise Configurations >> summary({ >> 'Enabled pipelines': pipelines, >> + 'Enabled IPA modules': ipa_modules, >> 'Android support': android_enabled, >> 'GStreamer support': gst_enabled, >> 'V4L2 emulation support': v4l2_enabled, >> diff --git a/meson_options.txt b/meson_options.txt >> index 69f11f85..2c80ad8b 100644 >> --- a/meson_options.txt >> +++ b/meson_options.txt >> @@ -25,6 +25,11 @@ option('gstreamer', >> value : 'auto', >> description : 'Compile libcamera GStreamer plugin') >> >> +option('ipas', >> + type : 'array', >> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], >> + description : 'Select which IPA modules to build') >> + > > Mmm, this new options means that by default all the IPAs are built, > even if the pipeline handler is not built. It doesn't because of the implementation below. > This requires a more precise control of the build options, as it's now > easier to mis-align pipelines and IPAs. > > Have we considered the other way around ? Build by default the IPAs > for which a pipeline is built (like we do today) unless it is > blacklisted ? That would be an 'enable' list for PipelineHandlers, and a 'disable' list for IPA's. Would that be confusing? >> option('lc-compliance', >> type : 'feature', >> value : 'auto', >> diff --git a/src/ipa/meson.build b/src/ipa/meson.build >> index 5b5684a1..49245e5e 100644 >> --- a/src/ipa/meson.build >> +++ b/src/ipa/meson.build >> @@ -19,14 +19,15 @@ subdir('libipa') >> >> ipa_sign = files('ipa-sign.sh') >> >> -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] >> ipa_names = [] >> >> +ipa_modules = get_option('ipas') >> + >> # The ipa-sign-install.sh script which uses the ipa_names variable will itself >> # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we >> # must not include the prefix string here. >> foreach pipeline : pipelines This is filtering to only parse the enabled pipelines, so if the pipeline is not enabled, the IPA will not be enabled. However that does lead to a tiny issue around what's reported in the Sumary, as that will now print what the option contains, rather than what was actually enabled. >> - if ipas.contains(pipeline) >> + if ipa_modules.contains(pipeline) >> subdir(pipeline) >> ipa_names += ipa_install_dir / ipa_name + '.so' >> endif >> -- >> 2.26.2 >>
Hi Kieran, On Fri, May 21, 2021 at 11:29:28AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 21/05/2021 10:48, Jacopo Mondi wrote: > > Hi Umang, > > > > On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: > >> There can be multiple IPAs per pipeline-handler or platform. > >> They can live in-tree or externally linked. To support the externally > >> linked IPA use-case, provide a mechanism to choose whether or not > >> to build the IPAs in tree, with the help of a meson configuration > >> option. > >> > >> By default, all in-tree IPAs are built. > > "By default, all in-tree IPAs are built when a matching Pipeline handler > is also enabled." > > > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> meson.build | 1 + > >> meson_options.txt | 5 +++++ > >> src/ipa/meson.build | 5 +++-- > >> 3 files changed, 9 insertions(+), 2 deletions(-) > >> > >> diff --git a/meson.build b/meson.build > >> index 46eb1b46..6626fa7e 100644 > >> --- a/meson.build > >> +++ b/meson.build > >> @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) > >> ## Summarise Configurations > >> summary({ > >> 'Enabled pipelines': pipelines, > >> + 'Enabled IPA modules': ipa_modules, > >> 'Android support': android_enabled, > >> 'GStreamer support': gst_enabled, > >> 'V4L2 emulation support': v4l2_enabled, > >> diff --git a/meson_options.txt b/meson_options.txt > >> index 69f11f85..2c80ad8b 100644 > >> --- a/meson_options.txt > >> +++ b/meson_options.txt > >> @@ -25,6 +25,11 @@ option('gstreamer', > >> value : 'auto', > >> description : 'Compile libcamera GStreamer plugin') > >> > >> +option('ipas', > >> + type : 'array', > >> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], > >> + description : 'Select which IPA modules to build') > >> + > > > > Mmm, this new options means that by default all the IPAs are built, > > even if the pipeline handler is not built. > > It doesn't because of the implementation below. > Correct, sorry, I got fooled by the fact the values of a meson array option correspond to the available choices if not elsewhere specified. But yes, they get filtered below. My bad. > > This requires a more precise control of the build options, as it's now > > easier to mis-align pipelines and IPAs. > > > > Have we considered the other way around ? Build by default the IPAs > > for which a pipeline is built (like we do today) unless it is > > blacklisted ? > > > That would be an 'enable' list for PipelineHandlers, and a 'disable' > list for IPA's. Would that be confusing? Not sure. By default to run a pipeline that has an associated IPA module, the module needs to be built otherwise the pipeline won't work, right ? Looking at ipa_ = IPAManager::createIPA<ipa::ipu3::IPAProxyIPU3>(pipe_, 1, 1); if (!ipa_) return -ENOENT; If you wish to disable an IPA module you do so because you know what you're doing and you want to run a different one, which I assume will anyway require some manual handling, if nothing else just for the correct deploy paths setup. Considering that, isn't it more natural to express "please do not build the IPA as I'm running a different one" instead of mix-matching pipeline handlers and IPA modules ? > > > > >> option('lc-compliance', > >> type : 'feature', > >> value : 'auto', > >> diff --git a/src/ipa/meson.build b/src/ipa/meson.build > >> index 5b5684a1..49245e5e 100644 > >> --- a/src/ipa/meson.build > >> +++ b/src/ipa/meson.build > >> @@ -19,14 +19,15 @@ subdir('libipa') > >> > >> ipa_sign = files('ipa-sign.sh') > >> > >> -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] > >> ipa_names = [] > >> > >> +ipa_modules = get_option('ipas') > >> + > >> # The ipa-sign-install.sh script which uses the ipa_names variable will itself > >> # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we > >> # must not include the prefix string here. > >> foreach pipeline : pipelines > > This is filtering to only parse the enabled pipelines, so if the > pipeline is not enabled, the IPA will not be enabled. > > However that does lead to a tiny issue around what's reported in the > Sumary, as that will now print what the option contains, rather than > what was actually enabled. > > >> - if ipas.contains(pipeline) > >> + if ipa_modules.contains(pipeline) > >> subdir(pipeline) > >> ipa_names += ipa_install_dir / ipa_name + '.so' > >> endif > >> -- > >> 2.26.2 > >> > > -- > Regards > -- > Kieran
Hi Jacopo, On 21/05/2021 12:38, Jacopo Mondi wrote: > Hi Kieran, > > On Fri, May 21, 2021 at 11:29:28AM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 21/05/2021 10:48, Jacopo Mondi wrote: >>> Hi Umang, >>> >>> On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: >>>> There can be multiple IPAs per pipeline-handler or platform. >>>> They can live in-tree or externally linked. To support the externally >>>> linked IPA use-case, provide a mechanism to choose whether or not >>>> to build the IPAs in tree, with the help of a meson configuration >>>> option. >>>> >>>> By default, all in-tree IPAs are built. >> >> "By default, all in-tree IPAs are built when a matching Pipeline handler >> is also enabled." >> >> >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> meson.build | 1 + >>>> meson_options.txt | 5 +++++ >>>> src/ipa/meson.build | 5 +++-- >>>> 3 files changed, 9 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/meson.build b/meson.build >>>> index 46eb1b46..6626fa7e 100644 >>>> --- a/meson.build >>>> +++ b/meson.build >>>> @@ -167,6 +167,7 @@ py_mod.find_installation('python3', modules: py_modules) >>>> ## Summarise Configurations >>>> summary({ >>>> 'Enabled pipelines': pipelines, >>>> + 'Enabled IPA modules': ipa_modules, >>>> 'Android support': android_enabled, >>>> 'GStreamer support': gst_enabled, >>>> 'V4L2 emulation support': v4l2_enabled, >>>> diff --git a/meson_options.txt b/meson_options.txt >>>> index 69f11f85..2c80ad8b 100644 >>>> --- a/meson_options.txt >>>> +++ b/meson_options.txt >>>> @@ -25,6 +25,11 @@ option('gstreamer', >>>> value : 'auto', >>>> description : 'Compile libcamera GStreamer plugin') >>>> >>>> +option('ipas', >>>> + type : 'array', >>>> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], >>>> + description : 'Select which IPA modules to build') >>>> + >>> >>> Mmm, this new options means that by default all the IPAs are built, >>> even if the pipeline handler is not built. >> >> It doesn't because of the implementation below. >> > > Correct, sorry, I got fooled by the fact the values of a meson array option > correspond to the available choices if not elsewhere specified. But > yes, they get filtered below. My bad. > >>> This requires a more precise control of the build options, as it's now >>> easier to mis-align pipelines and IPAs. >>> >>> Have we considered the other way around ? Build by default the IPAs >>> for which a pipeline is built (like we do today) unless it is >>> blacklisted ? >> >> >> That would be an 'enable' list for PipelineHandlers, and a 'disable' >> list for IPA's. Would that be confusing? > > Not sure. By default to run a pipeline that has an associated IPA > module, the module needs to be built otherwise the pipeline won't > work, right ? At least 'one' needs to be found for the pipeline to work yes. > Looking at > ipa_ = IPAManager::createIPA<ipa::ipu3::IPAProxyIPU3>(pipe_, 1, 1); > if (!ipa_) > return -ENOENT; > > If you wish to disable an IPA module you do so because you know what you're > doing and you want to run a different one, which I assume will anyway > require some manual handling, if nothing else just for the correct > deploy paths setup. > > Considering that, isn't it more natural to express "please do not > build the IPA as I'm running a different one" instead of mix-matching > pipeline handlers and IPA modules ? This method makes it possible to disable all IPA's with -Dipas='' Enable all IPA's by not specifying at all, or something in between by specifying exactly what is required. But lets say for example purposes we want to use a closed source IPU3, but an open RKISP1 that means the build line is: # IPU3 IPA is built externally -Dpipelines="ipu3,rkisp1" -Dipas="rkisp1" Your suggestion (which I'm not opposed to, either way is fine, as long as we have something) is: # IPU3 IPA is built externally -Dpipelines="ipu3,rkisp1" -Dno-ipa="ipu3" To me, this actually reads better - but it doesn't allow for disabling all IPA modules. That doesn't matter in the case of specifying pipelines, as you know which pipelines to disable, so the only use case would be if someone wanted to build all pipelines but no IPAs. I 'think' that would count as quite the corner case and not likely, as if someone doesn't want any IPA's they'd at least know what IPA's they are building themselves to replace the internal ones... >>>> option('lc-compliance', >>>> type : 'feature', >>>> value : 'auto', >>>> diff --git a/src/ipa/meson.build b/src/ipa/meson.build >>>> index 5b5684a1..49245e5e 100644 >>>> --- a/src/ipa/meson.build >>>> +++ b/src/ipa/meson.build >>>> @@ -19,14 +19,15 @@ subdir('libipa') >>>> >>>> ipa_sign = files('ipa-sign.sh') >>>> >>>> -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] >>>> ipa_names = [] >>>> >>>> +ipa_modules = get_option('ipas') >>>> + >>>> # The ipa-sign-install.sh script which uses the ipa_names variable will itself >>>> # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we >>>> # must not include the prefix string here. >>>> foreach pipeline : pipelines >> >> This is filtering to only parse the enabled pipelines, so if the >> pipeline is not enabled, the IPA will not be enabled. >> >> However that does lead to a tiny issue around what's reported in the >> Sumary, as that will now print what the option contains, rather than >> what was actually enabled. >> >>>> - if ipas.contains(pipeline) >>>> + if ipa_modules.contains(pipeline) >>>> subdir(pipeline) >>>> ipa_names += ipa_install_dir / ipa_name + '.so' >>>> endif >>>> -- >>>> 2.26.2 >>>> >> >> -- >> Regards >> -- >> Kieran
Hi Kieran, On Fri, May 21, 2021 at 03:46:27PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 21/05/2021 12:38, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Fri, May 21, 2021 at 11:29:28AM +0100, Kieran Bingham wrote: > >> Hi Jacopo, > >> > >> On 21/05/2021 10:48, Jacopo Mondi wrote: > >>> Hi Umang, > >>> > >>> On Wed, May 19, 2021 at 03:49:53PM +0530, Umang Jain wrote: > >>>> +option('ipas', > >>>> + type : 'array', > >>>> + choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'], > >>>> + description : 'Select which IPA modules to build') > >>>> + > >>> > >>> Mmm, this new options means that by default all the IPAs are built, > >>> even if the pipeline handler is not built. > >> > >> It doesn't because of the implementation below. > >> > > > > Correct, sorry, I got fooled by the fact the values of a meson array option > > correspond to the available choices if not elsewhere specified. But > > yes, they get filtered below. My bad. > > > >>> This requires a more precise control of the build options, as it's now > >>> easier to mis-align pipelines and IPAs. > >>> > >>> Have we considered the other way around ? Build by default the IPAs > >>> for which a pipeline is built (like we do today) unless it is > >>> blacklisted ? > >> > >> > >> That would be an 'enable' list for PipelineHandlers, and a 'disable' > >> list for IPA's. Would that be confusing? > > > > Not sure. By default to run a pipeline that has an associated IPA > > module, the module needs to be built otherwise the pipeline won't > > work, right ? > > At least 'one' needs to be found for the pipeline to work yes. > > > > Looking at > > ipa_ = IPAManager::createIPA<ipa::ipu3::IPAProxyIPU3>(pipe_, 1, 1); > > if (!ipa_) > > return -ENOENT; > > > > If you wish to disable an IPA module you do so because you know what you're > > doing and you want to run a different one, which I assume will anyway > > require some manual handling, if nothing else just for the correct > > deploy paths setup. > > > > Considering that, isn't it more natural to express "please do not > > build the IPA as I'm running a different one" instead of mix-matching > > pipeline handlers and IPA modules ? > > This method makes it possible to disable all IPA's with -Dipas='' > Enable all IPA's by not specifying at all, or something in between by > specifying exactly what is required. > > > But lets say for example purposes we want to use a closed source IPU3, > but an open RKISP1 > > that means the build line is: > > # IPU3 IPA is built externally > -Dpipelines="ipu3,rkisp1" -Dipas="rkisp1" > > > Your suggestion (which I'm not opposed to, either way is fine, as long > as we have something) is: > > # IPU3 IPA is built externally > -Dpipelines="ipu3,rkisp1" -Dno-ipa="ipu3" > > To me, this actually reads better - but it doesn't allow for disabling > all IPA modules. > > That doesn't matter in the case of specifying pipelines, as you know > which pipelines to disable, so the only use case would be if someone > wanted to build all pipelines but no IPAs. > > I 'think' that would count as quite the corner case and not likely, as > if someone doesn't want any IPA's they'd at least know what IPA's they > are building themselves to replace the internal ones... Thanks for the example. I won't push in one direction or another, as long as the counter-example is clear as it is now, I'm fine letting the ones who are working on this decide how to move forward. Let me just add one point: if I'm very new to libcamera and I see options to specify which IPAs have to be built I can assume they do not get build by default. If I see a "no-ipas" it's easier to recoginze disabling an IPA is the 'unlikely' choice and I would simply ignore the option. Thanks j > > > > > > >>>> option('lc-compliance', > >>>> type : 'feature', > >>>> value : 'auto', > >>>> diff --git a/src/ipa/meson.build b/src/ipa/meson.build > >>>> index 5b5684a1..49245e5e 100644 > >>>> --- a/src/ipa/meson.build > >>>> +++ b/src/ipa/meson.build > >>>> @@ -19,14 +19,15 @@ subdir('libipa') > >>>> > >>>> ipa_sign = files('ipa-sign.sh') > >>>> > >>>> -ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'] > >>>> ipa_names = [] > >>>> > >>>> +ipa_modules = get_option('ipas') > >>>> + > >>>> # The ipa-sign-install.sh script which uses the ipa_names variable will itself > >>>> # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we > >>>> # must not include the prefix string here. > >>>> foreach pipeline : pipelines > >> > >> This is filtering to only parse the enabled pipelines, so if the > >> pipeline is not enabled, the IPA will not be enabled. > >> > >> However that does lead to a tiny issue around what's reported in the > >> Sumary, as that will now print what the option contains, rather than > >> what was actually enabled. > >> > >>>> - if ipas.contains(pipeline) > >>>> + if ipa_modules.contains(pipeline) > >>>> subdir(pipeline) > >>>> ipa_names += ipa_install_dir / ipa_name + '.so' > >>>> endif > >>>> -- > >>>> 2.26.2 > >>>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran
Changes in v2: - IPA Docs rework patch split (into 3) - Don't try to make a different 'internal' helper library - Drop relevant patch - Under discussion for now AND out of scope for this series. - Drop IPAConfigInfo documentation - Needs to happen during a follow up "doc" patch for entire ipu3.mojom adapted same as [PATCH 2/7] Kieran Bingham (1): libcamera: pipeline: ipu3: Pass request metadata to IPA Umang Jain (6): ipa: Move core IPA interface documentation to a .cpp file ipa: mojom: Move CameraSensorInfo struct exclusively to IPA IPC ipa: ipc: Rename CameraSensorInfo to IPACameraSensorInfo ipa: meson: Install mojom generated headers to include paths ipa: ipu3: Introduce IPAConfigInfo in IPC meson: Add a configuration option to build IPAs Documentation/Doxyfile.in | 8 +- Documentation/guides/ipa.rst | 8 +- Documentation/meson.build | 1 + include/libcamera/internal/camera_sensor.h | 19 +- include/libcamera/ipa/core.mojom | 74 +----- include/libcamera/ipa/ipa_interface.h | 2 - include/libcamera/ipa/ipu3.mojom | 10 +- include/libcamera/ipa/meson.build | 8 +- include/libcamera/ipa/raspberrypi.mojom | 2 +- include/libcamera/ipa/rkisp1.mojom | 2 +- meson.build | 1 + meson_options.txt | 5 + src/ipa/ipu3/ipu3.cpp | 14 +- src/ipa/ipu3/ipu3_agc.cpp | 2 +- src/ipa/meson.build | 5 +- src/ipa/raspberrypi/raspberrypi.cpp | 9 +- src/ipa/rkisp1/rkisp1.cpp | 6 +- src/libcamera/camera_sensor.cpp | 117 +-------- src/libcamera/ipa/core_ipa_interface.cpp | 236 ++++++++++++++++++ src/libcamera/ipa/meson.build | 5 + src/libcamera/meson.build | 1 + src/libcamera/pipeline/ipu3/ipu3.cpp | 19 +- .../pipeline/raspberrypi/raspberrypi.cpp | 4 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- 24 files changed, 315 insertions(+), 245 deletions(-) create mode 100644 src/libcamera/ipa/core_ipa_interface.cpp create mode 100644 src/libcamera/ipa/meson.build