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

[Jacopo Mondi]

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