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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, May 14, 2021 at 11:09:18AM +0100, Kieran Bingham wrote:
> On 14/05/2021 08:58, Umang Jain wrote:
> > There can be multiple IPAs per pipeline-handler or platform.
> > For e.g., the IPU3 platform has a open source IPA in-tree whereas

s/a open/an open/

> > it shall use a closed source one from a standalone separate remote
> > for ChromeOS. In a case like this, there should be configure-time
> > option whether to build the in-tree IPAs or not.
> 
> We don't need to state where external IPAs will be used. That's just
> this current use case, and it may not be true.
> 
> They might use it. Or they might choose to use the open one. That's up
> to them, not us.
> 
> > By default, all in-tree IPAs are built.
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  meson.build                     | 1 +
> >  meson_options.txt               | 5 +++++
> >  src/ipa/ipu3/meson.build        | 4 ++++
> >  src/ipa/meson.build             | 2 ++
> >  src/ipa/raspberrypi/meson.build | 4 ++++
> >  src/ipa/rkisp1/meson.build      | 4 ++++
> >  src/ipa/vimc/meson.build        | 4 ++++
> >  7 files changed, 24 insertions(+)
> > 
> > diff --git a/meson.build b/meson.build
> > index fa2a62cf..b89797f3 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 IPAs': ipas,

I'd write 'Enabled IPA modules', and perhaps rename the ipas variable to
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..1fcbecc1 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -25,6 +25,11 @@ option('gstreamer',
> >          value : 'auto',
> >          description : 'Compile libcamera GStreamer plugin')
> >  
> > +option('ipas',

We could also rename the option to ipa_modules.

> > +        type : 'array',
> > +        choices : ['ipu3', 'raspberrypi', 'rkisp1', 'vimc'],
> > +        description : 'Select which internal IPA to include')

'IPA modules' here too.

> I know this description comes from the pipelines option, but 'include'
> doesn't sound right to me.
> 
> This decides which ones get built. I'd prefer something like
> 	"Select which IPA modules to build"
> 
> > +
> >  option('lc-compliance',
> >          type : 'feature',
> >          value : 'auto',
> > diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> > index 0d843846..a9f5d0aa 100644
> > --- a/src/ipa/ipu3/meson.build
> > +++ b/src/ipa/ipu3/meson.build
> > @@ -2,6 +2,10 @@
> >  
> >  ipa_name = 'ipa_ipu3'
> >  
> > +if 'ipu3' not in ipas
> > +    subdir_done()
> > +endif
> 
> You shouldn't need this.
> 
> If ipu3 isn't specified in get_option('ipas') then it won't go into the
> subdir.
> 
> > +
> >  ipu3_ipa_sources = files([
> >      'ipu3.cpp',
> >      'ipu3_agc.cpp',
> > diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> > index 5b5684a1..fb687497 100644
> > --- a/src/ipa/meson.build
> > +++ b/src/ipa/meson.build
> > @@ -22,6 +22,8 @@ ipa_sign = files('ipa-sign.sh')
> >  ipas = ['ipu3', 'raspberrypi', 'rkisp1', 'vimc']
> >  ipa_names = []
> >  
> > +ipas = get_option('ipas')
> 
> I think you forgot to remove the ipas list 3 lines above it.
> This should replace that line.
> 
> > +
> >  # 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.
> > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> > index d1397a32..02c143b1 100644
> > --- a/src/ipa/raspberrypi/meson.build
> > +++ b/src/ipa/raspberrypi/meson.build
> > @@ -2,6 +2,10 @@
> >  
> >  ipa_name = 'ipa_rpi'
> >  
> > +if 'rasberrypi' not in ipas
> > +    subdir_done()
> > +endif
> 
> This isn't needed.
> 
> > +
> >  rpi_ipa_deps = [
> >      libcamera_dep,
> >      dependency('boost'),
> > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> > index 1a1c7159..eecc5877 100644
> > --- a/src/ipa/rkisp1/meson.build
> > +++ b/src/ipa/rkisp1/meson.build
> > @@ -2,6 +2,10 @@
> >  
> >  ipa_name = 'ipa_rkisp1'
> >  
> > +if 'rkisp1' not in ipas
> > +    subdir_done()
> > +endif
> 
> same.
> 
> > +
> >  mod = shared_module(ipa_name,
> >                      ['rkisp1.cpp', libcamera_generated_ipa_headers],
> >                      name_prefix : '',
> > diff --git a/src/ipa/vimc/meson.build b/src/ipa/vimc/meson.build
> > index a35825ae..a41a0407 100644
> > --- a/src/ipa/vimc/meson.build
> > +++ b/src/ipa/vimc/meson.build
> > @@ -2,6 +2,10 @@
> >  
> >  ipa_name = 'ipa_vimc'
> >  
> > +if 'ipu3' not in ipas
> > +    subdir_done()
> > +endif
> 
> same.
> 
> > +
> >  mod = shared_module(ipa_name,
> >                      ['vimc.cpp', libcamera_generated_ipa_headers],
> >                      name_prefix : '',

-- 
Regards,

Laurent Pinchart