[libcamera-devel] [PATCH v2 6/7] meson: Add a configuration option to build IPAs

[Jacopo Mondi]

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 at 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