Hello Laurent and Kieran,
On 1/12/23 10:47, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2023-01-12 09:30:46)
>> 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.
>
> Precisely what I had in mind last night.
>
Agreed with you both. I wasn't aware that the meson language was that
expressive and had support for dictionaries.
> Sketched out that would be:
>
> pipelines = get_option('pipelines')
>
> pipes_support = {
> 'imx8-isi': ['aarch64'],
> 'ipu3': ['x86', 'x86_64'],
> 'raspberrypi': ['arm', 'aarch64'],
> 'rkisp1': ['aarch64'],
> 'simple': ['arm', 'aarch64'],
> 'uvcvideo': ['any'],
> 'vimc': ['test'],
> 'virtual': ['test'],
> }
>
> if pipelines.contains('all')
> pipelines = []
> foreach pipe, archs : pipes_support
> pipelines += pipe
> endforeach
> endif
>
> if pipelines.contains('auto')
> host_cpu = host_machine.cpu_family()
> pipelines = []
> foreach pipe, archs : pipes_support
> if host_cpu in archs or 'any' in archs
> message('Auto: Enabling ' + pipe + ' pipeline handler')
> pipelines += pipe
> endif
> endforeach
> endif
>
> if get_option('test')
> foreach pipe, archs : pipes_support
> if 'test' in archs and pipe not in pipelines
> message('Test Support: Enabling ' + pipe + ' pipeline handler')
> pipelines += pipe
> endif
> endforeach
> endif
>
>
This is indeed a much better approach. If you post a patch, feel free to add
my Reviewed-by. Thanks!
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat