[libcamera-devel,v2] meson: Only build pipeline handlers needed in the host architecture
diff mbox series

Message ID 20221224102828.2602512-1-javierm@redhat.com
State Accepted
Commit 0a8ac1ee06a47079e5272656be4653b1b12916ef
Headers show
Series
  • [libcamera-devel,v2] meson: Only build pipeline handlers needed in the host architecture
Related show

Commit Message

Javier Martinez Canillas Dec. 24, 2022, 10:28 a.m. UTC 
By default all pipeline handlers are built, regardless on whether these
are needed in the host architecture or not. It makes more sense to build
only the pipeline handlers that will be used for the given architecture.

Let's do that by default now, but still allow to build the other pipeline
handlers if needed, by using the `pipelines` meson option. For example:

  $ meson build
  ...
    Configuration
    Enabled pipelines        : ipu3
                               simple
                               uvcvideo
    Enabled IPA modules      : ipu3
  ...

  $ meson build -Dpipelines="ipu3,raspberrypi,rkisp1" -Dtest=true
  ...
    Configuration
    Enabled pipelines        : ipu3
                               raspberrypi
                               rkisp1
                               vimc
    Enabled IPA modules      : ipu3
                               raspberrypi
                               rkisp1
                               vimc
  ...

Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---

Changes in v2:
- Add collected reviewed-by tags.
- Don't include ipu3 pipeline handler for aarch64 (pinchartl).
- Only include the simple pipeline handler for arm and aarch64 (pinchartl).
- Remove spaces in pipelines meson option description (pinchartl).
- Wrap pipelines meson option choice list (pinchartl).

 meson.build       | 17 +++++++++++++++++
 meson_options.txt | 14 ++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Dec. 24, 2022, 3:43 p.m. UTC | #1 
Hi Javier,

Thank you for the patch.

On Sat, Dec 24, 2022 at 11:28:28AM +0100, Javier Martinez Canillas wrote:
> By default all pipeline handlers are built, regardless on whether these
> are needed in the host architecture or not. It makes more sense to build
> only the pipeline handlers that will be used for the given architecture.
> 
> Let's do that by default now, but still allow to build the other pipeline
> handlers if needed, by using the `pipelines` meson option. For example:

I'd write "For example, on a x86-64 platform:"

> 
>   $ meson build
>   ...
>     Configuration
>     Enabled pipelines        : ipu3
>                                simple

The simple pipeline handler isn't included on x86 platforms anymore.

>                                uvcvideo
>     Enabled IPA modules      : ipu3
>   ...
> 
>   $ meson build -Dpipelines="ipu3,raspberrypi,rkisp1" -Dtest=true
>   ...
>     Configuration
>     Enabled pipelines        : ipu3
>                                raspberrypi
>                                rkisp1
>                                vimc
>     Enabled IPA modules      : ipu3
>                                raspberrypi
>                                rkisp1
>                                vimc
>   ...
> 
> Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can fix the commit message when applying, no need to resubmit.

> ---
> 
> Changes in v2:
> - Add collected reviewed-by tags.
> - Don't include ipu3 pipeline handler for aarch64 (pinchartl).
> - Only include the simple pipeline handler for arm and aarch64 (pinchartl).
> - Remove spaces in pipelines meson option description (pinchartl).
> - Wrap pipelines meson option choice list (pinchartl).
> 
>  meson.build       | 17 +++++++++++++++++
>  meson_options.txt | 14 ++++++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index d02f9917965c..df9099d0b996 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -164,6 +164,23 @@ liblttng = dependency('lttng-ust', required : get_option('tracing'))
>  # are enabled.
>  pipelines = get_option('pipelines')
>  
> +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']
> +endif
> +
>  if get_option('test') and 'vimc' not in pipelines
>      message('Enabling vimc pipeline handler to support tests')
>      pipelines += ['vimc']
> diff --git a/meson_options.txt b/meson_options.txt
> index 1ba6778ce257..1a68bcd37e88 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -37,8 +37,18 @@ option('lc-compliance',
>  
>  option('pipelines',
>          type : 'array',
> -        choices : ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],
> -        description : 'Select which pipeline handlers to include')
> +        value : ['auto'],
> +        choices : [
> +            'auto',
> +            'imx8-isi',
> +            'ipu3',
> +            'raspberrypi',
> +            'rkisp1',
> +            'simple',
> +            'uvcvideo',
> +            'vimc'
> +        ],
> +        description : 'Select which pipeline handlers to build. If it this set to auto, all the pipelines applicable to the target architecture will be built.')
>  
>  option('qcam',
>          type : 'feature',
Kieran Bingham Dec. 24, 2022, 3:47 p.m. UTC | #2 
Hi Javier

Quoting Javier Martinez Canillas (2022-12-24 10:28:28)
> By default all pipeline handlers are built, regardless on whether these
> are needed in the host architecture or not. It makes more sense to build
> only the pipeline handlers that will be used for the given architecture.
> 
> Let's do that by default now, but still allow to build the other pipeline
> handlers if needed, by using the `pipelines` meson option. For example:
> 
>   $ meson build
>   ...
>     Configuration
>     Enabled pipelines        : ipu3
>                                simple
>                                uvcvideo
>     Enabled IPA modules      : ipu3
>   ...
> 
>   $ meson build -Dpipelines="ipu3,raspberrypi,rkisp1" -Dtest=true
>   ...
>     Configuration
>     Enabled pipelines        : ipu3
>                                raspberrypi
>                                rkisp1
>                                vimc
>     Enabled IPA modules      : ipu3
>                                raspberrypi
>                                rkisp1
>                                vimc
>   ...
> 
> Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>


I like this :-)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
> 
> Changes in v2:
> - Add collected reviewed-by tags.
> - Don't include ipu3 pipeline handler for aarch64 (pinchartl).
> - Only include the simple pipeline handler for arm and aarch64 (pinchartl).
> - Remove spaces in pipelines meson option description (pinchartl).
> - Wrap pipelines meson option choice list (pinchartl).
> 
>  meson.build       | 17 +++++++++++++++++
>  meson_options.txt | 14 ++++++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index d02f9917965c..df9099d0b996 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -164,6 +164,23 @@ liblttng = dependency('lttng-ust', required : get_option('tracing'))
>  # are enabled.
>  pipelines = get_option('pipelines')
>  
> +if pipelines.contains('auto')

perhaps on top of this patch we can try an "all" target option.

if contains auto or all:

> +    host_cpu = host_machine.cpu_family()
> +    pipelines = []
> +    if host_cpu == 'x86' or host_cpu == 'x86_64'

and then "or all" on these too, would let us easily keep builds as
-Dpipelines=all.

but I can experiment on top of this patch when merged to avoid the yaks.


> +        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']
> +endif
> +
>  if get_option('test') and 'vimc' not in pipelines
>      message('Enabling vimc pipeline handler to support tests')
>      pipelines += ['vimc']
> diff --git a/meson_options.txt b/meson_options.txt
> index 1ba6778ce257..1a68bcd37e88 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -37,8 +37,18 @@ option('lc-compliance',
>  
>  option('pipelines',
>          type : 'array',
> -        choices : ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],
> -        description : 'Select which pipeline handlers to include')
> +        value : ['auto'],
> +        choices : [
> +            'auto',
> +            'imx8-isi',
> +            'ipu3',
> +            'raspberrypi',
> +            'rkisp1',
> +            'simple',
> +            'uvcvideo',
> +            'vimc'
> +        ],
> +        description : 'Select which pipeline handlers to build. If it this set to auto, all the pipelines applicable to the target architecture will be built.')
>  
>  option('qcam',
>          type : 'feature',
> -- 
> 2.38.1
>
Javier Martinez Canillas Dec. 24, 2022, 5:56 p.m. UTC | #3 
Hello Laurent,

On 12/24/22 16:43, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.

[...]

>>   $ meson build
>>   ...
>>     Configuration
>>     Enabled pipelines        : ipu3
>>                                simple
> 
> The simple pipeline handler isn't included on x86 platforms anymore.
>

Indeed. I forgot to amend the commit message after doing the changes
that you suggested.

[..] 

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I can fix the commit message when applying, no need to resubmit.
>

Awesome. Thanks!
Javier Martinez Canillas Dec. 24, 2022, 5:58 p.m. UTC | #4 
Hello Kieran,

On 12/24/22 16:47, Kieran Bingham wrote:

[...]

>> Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> 
> I like this :-)
>

I figured that since you suggested it :)
 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks! I also thought about an "all" choice but thought that
it would be out of the scope for this patch. It would be great
if you can explore that as a follow-up.

Patch
diff mbox series 

diff --git a/meson.build b/meson.build
index d02f9917965c..df9099d0b996 100644
--- a/meson.build
+++ b/meson.build
@@ -164,6 +164,23 @@  liblttng = dependency('lttng-ust', required : get_option('tracing'))
 # are enabled.
 pipelines = get_option('pipelines')
 
+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']
+endif
+
 if get_option('test') and 'vimc' not in pipelines
     message('Enabling vimc pipeline handler to support tests')
     pipelines += ['vimc']
diff --git a/meson_options.txt b/meson_options.txt
index 1ba6778ce257..1a68bcd37e88 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -37,8 +37,18 @@  option('lc-compliance',
 
 option('pipelines',
         type : 'array',
-        choices : ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],
-        description : 'Select which pipeline handlers to include')
+        value : ['auto'],
+        choices : [
+            'auto',
+            'imx8-isi',
+            'ipu3',
+            'raspberrypi',
+            'rkisp1',
+            'simple',
+            'uvcvideo',
+            'vimc'
+        ],
+        description : 'Select which pipeline handlers to build. If it this set to auto, all the pipelines applicable to the target architecture will be built.')
 
 option('qcam',
         type : 'feature',