[libcamera-devel] [PATCH 03/13] ipa: meson: Allow nested IPA directory structures

[Laurent Pinchart]

Hi Naush,

On Thu, May 04, 2023 at 08:55:24AM +0100, Naushir Patuck via libcamera-devel wrote:
> On Wed, 3 May 2023 at 13:20, Naushir Patuck wrote:
> >
> > The current IPA build files require a flat directory struture for the

s/struture/structure/

> > IPAs. Modify the build files to remove this restriction and allow a
> > directory structure such as:
> >
> > src/ipa
> >   |- raspberrypi
> >       |- common
> >       |- cam_helpers
> >       |- controller
> >       |- vc4
> >   |- rkisp1
> >   |- ipu3
> >
> > where each subdir (e.g. raspberrypi/common, raspberrypi/cam_helper) has
> > its own meson.build file. Such a directory structure will be introduced
> > for the Raspberry Pi IPA in a future commit.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  meson.build                     |  2 +-
> >  src/ipa/ipu3/meson.build        |  2 ++
> >  src/ipa/meson.build             | 26 +++++++++++++++++++++-----
> >  src/ipa/raspberrypi/meson.build |  2 ++
> >  src/ipa/rkisp1/meson.build      |  2 ++
> >  5 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index d3289181b7b9..2d99029bf5b7 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -262,7 +262,7 @@ py_mod.find_installation('python3', modules: py_modules)
> >  ## Summarise Configurations
> >  summary({
> >              'Enabled pipelines': pipelines,
> > -            'Enabled IPA modules': enabled_ipa_modules,
> > +            'Enabled IPA modules': enabled_ipa_names,
> >              'Tracing support': tracing_enabled,
> >              'Android support': android_enabled,
> >              'GStreamer support': gst_enabled,
> > diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> > index 658e7c9bc366..66c398432d43 100644
> > --- a/src/ipa/ipu3/meson.build
> > +++ b/src/ipa/ipu3/meson.build
> > @@ -29,3 +29,5 @@ if ipa_sign_module
> >                    install : false,
> >                    build_by_default : true)
> >  endif
> > +
> > +ipa_names += ipa_name
> > diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> > index 76ad5b445601..fac92f32fdb9 100644
> > --- a/src/ipa/meson.build
> > +++ b/src/ipa/meson.build
> > @@ -36,16 +36,32 @@ if get_option('test') and 'vimc' not in ipa_modules
> >  endif
> >
> >  enabled_ipa_modules = []
> > +enabled_ipa_names = []
> > +ipa_names = []
> >
> >  # The ipa-sign-install.sh script which uses the ipa_names variable will itself

ipa_names should be renamed to enabled_ipa_modules.

This whole comment should actually move after this foreach loop.

> >  # prepend MESON_INSTALL_DESTDIR_PREFIX to each ipa module name, therefore we
> >  # must not include the prefix string here.
> > +
> > +subdirs = []
> >  foreach pipeline : pipelines
> > -    if ipa_modules.contains(pipeline)
> > -        subdir(pipeline)
> > -        ipa_names += ipa_install_dir / ipa_name + '.so'
> > -        enabled_ipa_modules += pipeline
> > +    if not ipa_modules.contains(pipeline)
> > +        continue
> > +    endif
> > +    enabled_ipa_names += pipeline
> > +
> > +    # Allow multi-level directory structuring for the IPAs if needed.
> > +    pipeline = pipeline.split('/')[0]
> > +    if pipeline in subdirs
> > +        continue
> >      endif
> > +
> > +    subdir(pipeline)
> > +    subdirs += [pipeline]
> 
> I've just spotted a small bug in this code.  The above line should read:
> 
>     subdirs += pipeline
> 
> to work correctly.  Should I send an in-reply-to update to this patch, or can
> that be fixed when applying (of course assuming there are no other changes
> required).

I'll fix it locally.

> > +endforeach
> > +
> > +foreach ipa_name : ipa_names
> > +    enabled_ipa_modules += ipa_install_dir / ipa_name + '.so'
> >  endforeach
> >
> >  if ipa_sign_module
> > @@ -54,5 +70,5 @@ if ipa_sign_module
> >      # install time, which invalidates the signatures.
> >      meson.add_install_script('ipa-sign-install.sh',
> >                               ipa_priv_key.full_path(),
> > -                             ipa_names)
> > +                             enabled_ipa_modules)
> >  endif
> > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> > index de78cbd80f9c..95437cbcc962 100644
> > --- a/src/ipa/raspberrypi/meson.build
> > +++ b/src/ipa/raspberrypi/meson.build
> > @@ -64,3 +64,5 @@ if ipa_sign_module
> >  endif
> >
> >  subdir('data')
> > +
> > +ipa_names += ipa_name
> > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> > index ccb84b27525b..e813da53ae9b 100644
> > --- a/src/ipa/rkisp1/meson.build
> > +++ b/src/ipa/rkisp1/meson.build
> > @@ -29,3 +29,5 @@ if ipa_sign_module
> >                    install : false,
> >                    build_by_default : true)
> >  endif
> > +
> > +ipa_names += ipa_name

Shouldn't vimc also do this ?

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

I can handle the fixes locally.

-- 
Regards,

Laurent Pinchart