[libcamera-devel] meson: Rework automatic pipeline selection
diff mbox series

Message ID 20230112110756.1944607-1-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] meson: Rework automatic pipeline selection
Related show

Commit Message

Kieran Bingham Jan. 12, 2023, 11:07 a.m. UTC 
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@redhat.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 meson.build | 54 +++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

Comments

Paul Elder Jan. 26, 2023, 5:05 a.m. UTC | #1 
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@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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 = {
> +  'imx8-isi':     arch_arm,
> +  'ipu3':         arch_x86,
> +  'raspberrypi':  arch_arm,
> +  'rkisp1':       arch_arm,
> +  'simple':       arch_arm,
> +  'uvcvideo':     ['any'],
> +  'vimc':         ['test'],
> +}
> +
>  if pipelines.contains('all')
> -    pipelines = [
> -        'imx8-isi',
> -        'ipu3',
> -        'raspberrypi',
> -        'rkisp1',
> -        'simple',
> -        'uvcvideo',
> -        'vimc',
> -    ]
> +    pipelines = []
> +    foreach pipe, archs : pipes_support
> +        pipelines += pipe
> +    endforeach
>  endif
>  
>  if pipelines.contains('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')
> +            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.
> -- 
> 2.34.1
>
Laurent Pinchart Jan. 26, 2023, 11:13 a.m. UTC | #2 
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@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@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@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.
Kieran Bingham Jan. 26, 2023, 11:46 a.m. UTC | #3 
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@redhat.com>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@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@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

Patch
diff mbox series 

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 = {
+  'imx8-isi':     arch_arm,
+  'ipu3':         arch_x86,
+  'raspberrypi':  arch_arm,
+  'rkisp1':       arch_arm,
+  'simple':       arch_arm,
+  'uvcvideo':     ['any'],
+  'vimc':         ['test'],
+}
+
 if pipelines.contains('all')
-    pipelines = [
-        'imx8-isi',
-        'ipu3',
-        'raspberrypi',
-        'rkisp1',
-        'simple',
-        'uvcvideo',
-        'vimc',
-    ]
+    pipelines = []
+    foreach pipe, archs : pipes_support
+        pipelines += pipe
+    endforeach
 endif
 
 if pipelines.contains('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')
+            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.
-- 
2.34.1