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

[Naushir Patuck]

Hi Laurent,

On Thu, 4 May 2023 at 17:06, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?

Yes, not sure how/why I missed that up.

>
> With these small issues fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I can handle the fixes locally.

Thanks for that!

Regards,
Naush

>
> --
> Regards,
>
> Laurent Pinchart