[libcamera-devel] [PATCH] meson: Rework automatic pipeline selection

[Kieran Bingham]

Quoting Laurent Pinchart (2023-01-26 11:13:05)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Jan 12, 2023 at 11:07:56AM +0000, Kieran Bingham via libcamera-devel wrote:
> > The supported pipelines are listed in three places: the
> > meson_options.txt file, the defined array when a user selects
> > -Dpipelines="all", and arrays defined when the default
> > -Dpipelines="auto" is selected.
> > 
> > This can be hard to maintain and error prone.
> > 
> > Rework the definition of pipeline selection to a single table within the
> > meson.build to reduce duplication within this file. The new table
> > specifies the architecture(s) that the pipeline handler supports and
> > is iterated to handle the special cases for 'all', 'auto' and 'test'.
> > 
> > The current behaviour such that 'all' takes precedence over 'auto' is
> > maintained, and 'test' is now extended such that additional test
> > pipeline handlers can easily be introduced.
> > 
> > The existing implementation defines the i.MX8-ISI and RKISP1 pipeline
> > handlers as only supported by 'aarch64'. This conversion changes the
> > behaviour such that those pipeline handlers are now supported on both
> > 'arm' and 'aarch64' as each of those platforms could support a 32-bit
> > ARM build.
> > 
> > Suggested-by: Javier Martinez Canillas <javierm at redhat.com>
> > Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  meson.build | 54 +++++++++++++++++++++++++++++------------------------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index e86673dd5c0c..c35376e1e97b 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -164,38 +164,44 @@ liblttng = dependency('lttng-ust', required : get_option('tracing'))
> >  # are enabled.
> >  pipelines = get_option('pipelines')
> >  
> > +arch_arm = ['arm', 'aarch64']
> > +arch_x86 = ['x86', 'x86_64']
> > +pipes_support = {
> 
> pipelines_support to match the pipelines option ?

Ack.

> 
> > +  'imx8-isi':     arch_arm,
> > +  'ipu3':         arch_x86,
> > +  'raspberrypi':  arch_arm,
> > +  'rkisp1':       arch_arm,
> > +  'simple':       arch_arm,
> > +  'uvcvideo':     ['any'],
> > +  'vimc':         ['test'],
> 
> 4 spaces for indentation please.

Ack.

> > +}
> > +
> >  if pipelines.contains('all')
> > -    pipelines = [
> > -        'imx8-isi',
> > -        'ipu3',
> > -        'raspberrypi',
> > -        'rkisp1',
> > -        'simple',
> > -        'uvcvideo',
> > -        'vimc',
> > -    ]
> > +    pipelines = []
> > +    foreach pipe, archs : pipes_support
> 
> Same here, s/pipe/pipeline/ ?
> 
> > +        pipelines += pipe
> > +    endforeach
> 
> I think this could be simplified to
> 
>     pipelines = pipes_support.keys()

Aha, I didn't realise we could do that.

> 
> >  endif
> >  
> >  if pipelines.contains('auto')
> 
> elif ? The above case has added all pipeline handlers already. It will
> also making it clearer that 'all' takes precedence over 'auto'.

Ack

> 
> >      host_cpu = host_machine.cpu_family()
> >      pipelines = []
> > -    if host_cpu == 'x86' or host_cpu == 'x86_64'
> > -        pipelines += ['ipu3']
> > -    elif host_cpu == 'aarch64'
> > -        pipelines += ['imx8-isi', 'rkisp1']
> > -    endif
> > -
> > -    if host_cpu == 'arm' or host_cpu == 'aarch64'
> > -        pipelines += ['raspberrypi', 'simple']
> > -    endif
> > -
> > -    # Always include the uvcvideo pipeline handler.
> > -    pipelines += ['uvcvideo']
> > +    foreach pipe, archs : pipes_support
> > +        if host_cpu in archs or 'any' in archs
> > +            message('Auto-enabling ' + pipe + ' pipeline handler')
> 
> Do we need a message ? The selected pipeline handlers are printed in the
> summary, I'm not sure this message adds much value.

I can drop it - it was mostly about the fact that it's happening
'automatically' rather than explicitly.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > +            pipelines += pipe
> > +        endif
> > +    endforeach
> >  endif
> >  
> > -if get_option('test') and 'vimc' not in pipelines
> > -    message('Enabling vimc pipeline handler to support tests')
> > -    pipelines += ['vimc']
> > +if get_option('test')
> > +    foreach pipe, archs : pipes_support
> > +        if 'test' in archs and pipe not in pipelines
> > +            message('Enabling ' + pipe + ' pipeline handler for tests')
> > +            pipelines += pipe
> > +        endif
> > +    endforeach
> >  endif
> >  
> >  # Utilities are parsed first to provide support for other components.
> 
> -- 
> Regards,
> 
> Laurent Pinchart