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

[Laurent Pinchart]

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 ?

> +  '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.

> +}
> +
>  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()

>  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'.

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

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