Hi Javier,
Thank you for the patch.
On Thu, Jan 12, 2023 at 12:22:21AM +0100, Javier Martinez Canillas 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 is hard to maintain and error prone, let's at least in the meson file
> have a single place where these pipelines lists are defined.
>
> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> ---
>
> meson.build | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
Looking at the diffstat, I'm not sure if it's worth it. It feels a bit
overengineered, especially given that the pipeline handlers also have to
be added to meson_options.txt too. If we had thousands of pipeline
handlers we would need to do better, but at the moment, I'm not entirely
convinced.
If you want to continue in this direction, I would at least turn the
pipelines_* variables into a single dictionary indexed by architecture,
or do it the other way around, indexing by pipeline handler name and
listing the supported architectures as the value. This could help making
the x86 vs. x86_64 and arm vs. aarch64 a bit less special cases.
> diff --git a/meson.build b/meson.build
> index 389c547206fb..f9e3280ec0f2 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -168,38 +168,41 @@ liblttng = dependency('lttng-ust', required : get_option('tracing'))
> # are enabled.
> pipelines = get_option('pipelines')
>
> +pipelines_aarch64 = ['imx8-isi', 'rkisp1']
> +pipelines_arm = ['raspberrypi', 'simple']
> +pipelines_agnostic = ['uvcvideo']
> +pipelines_test = ['vimc']
> +pipelines_x86 = ['ipu3']
> +
> if pipelines.contains('all')
> - pipelines = [
> - 'imx8-isi',
> - 'ipu3',
> - 'raspberrypi',
> - 'rkisp1',
> - 'simple',
> - 'uvcvideo',
> - 'vimc',
> - ]
> + pipelines = []
> + pipelines += pipelines_aarch64
> + pipelines += pipelines_arm
> + pipelines += pipelines_agnostic
> + pipelines += pipelines_test
> + pipelines += pipelines_x86
> endif
>
> if pipelines.contains('auto')
> host_cpu = host_machine.cpu_family()
> pipelines = []
> if host_cpu == 'x86' or host_cpu == 'x86_64'
> - pipelines += ['ipu3']
> + pipelines += pipelines_x86
> elif host_cpu == 'aarch64'
> - pipelines += ['imx8-isi', 'rkisp1']
> + pipelines += pipelines_aarch64
> endif
>
> if host_cpu == 'arm' or host_cpu == 'aarch64'
> - pipelines += ['raspberrypi', 'simple']
> + pipelines += pipeines_arm
> endif
>
> # Always include the uvcvideo pipeline handler.
> - pipelines += ['uvcvideo']
> + pipelines += pipelines_agnostic
> endif
>
> if get_option('test') and 'vimc' not in pipelines
> message('Enabling vimc pipeline handler to support tests')
> - pipelines += ['vimc']
> + pipelines += pipelines_test
> endif
>
> # Utilities are parsed first to provide support for other components.
--
Regards,
Laurent Pinchart