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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran