| Message ID | 20221223082229.2559907-1-javierm@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Javier, thank you for the patch. On 12/23/22 1:52 PM, Javier Martinez Canillas via libcamera-devel 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: > > $ 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> > --- > > meson.build | 14 ++++++++++++++ > meson_options.txt | 6 ++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index d02f9917965c..fc1023326ed3 100644 > --- a/meson.build > +++ b/meson.build > @@ -164,6 +164,20 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > # are enabled. > pipelines = get_option('pipelines') > > +if pipelines.contains('auto') > + host_cpu = host_machine.cpu_family() > + if host_cpu == 'x86' or host_cpu == 'x86_64' > + pipelines = ['ipu3'] > + elif host_cpu == 'aarch64' > + pipelines = ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1'] > + elif host_cpu == 'arm' > + pipelines = ['raspberrypi'] > + endif > + > + # Always include the simple and uvcvideo pipeline handlers. > + pipelines += ['simple', '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..23505805de41 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -37,8 +37,10 @@ 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 this is set to auto, all > + the pipelines applicable to the target architecture will be built.''') > > option('qcam', > type : 'feature',
On Fri, Dec 23, 2022 at 09:22:29AM +0100, Javier Martinez Canillas via libcamera-devel 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: > > $ 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: Paul Elder <paul.elder@ideasonboard.com> > --- > > meson.build | 14 ++++++++++++++ > meson_options.txt | 6 ++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index d02f9917965c..fc1023326ed3 100644 > --- a/meson.build > +++ b/meson.build > @@ -164,6 +164,20 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > # are enabled. > pipelines = get_option('pipelines') > > +if pipelines.contains('auto') > + host_cpu = host_machine.cpu_family() > + if host_cpu == 'x86' or host_cpu == 'x86_64' > + pipelines = ['ipu3'] > + elif host_cpu == 'aarch64' > + pipelines = ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1'] > + elif host_cpu == 'arm' > + pipelines = ['raspberrypi'] > + endif > + > + # Always include the simple and uvcvideo pipeline handlers. > + pipelines += ['simple', '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..23505805de41 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -37,8 +37,10 @@ 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 this is set to auto, all > + the pipelines applicable to the target architecture will be built.''') > > option('qcam', > type : 'feature', > -- > 2.38.1 >
Hi Javier, Thank you for the patch. On Fri, Dec 23, 2022 at 09:22:29AM +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: As long as CI systems can build all pipeline handlers, that's fine with me. We may start getting somes patches that introduce compilation breakages on pipeline handlers that are not compile-tested by developers, but we can catch that in CI. > $ 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> > --- > > meson.build | 14 ++++++++++++++ > meson_options.txt | 6 ++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index d02f9917965c..fc1023326ed3 100644 > --- a/meson.build > +++ b/meson.build > @@ -164,6 +164,20 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > # are enabled. > pipelines = get_option('pipelines') > > +if pipelines.contains('auto') > + host_cpu = host_machine.cpu_family() > + if host_cpu == 'x86' or host_cpu == 'x86_64' > + pipelines = ['ipu3'] > + elif host_cpu == 'aarch64' > + pipelines = ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1'] ipu3 is only for x86. > + elif host_cpu == 'arm' > + pipelines = ['raspberrypi'] > + endif > + > + # Always include the simple and uvcvideo pipeline handlers. > + pipelines += ['simple', 'uvcvideo'] The simple pipeline handler is only useful on ARM platforms (both 32 and 64 bits) at the moment. > +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..23505805de41 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -37,8 +37,10 @@ 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'], This is getting long, let's wrap it: choices : [ 'auto', 'imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc', ], > + description : '''Select which pipeline handlers to build. If this is set to auto, all > + the pipelines applicable to the target architecture will be built.''') The spaces in the description cause weird-looking output depending on the terminal width: pipelines [imx8-isi, ipu3, raspberrypi, [auto, imx8-isi, ipu3, Select which pipeline handlers to build. If this is set to auto, rkisp1, simple, uvcvideo, vimc] raspberrypi, rkisp1, simple, all the pipelines applicable to the uvcvideo, target architecture will be built. vimc] You can write description : '''Select which pipeline handlers to build. If this is set to auto, all the pipelines applicable to the target architecture will be built.''') or description : 'Select which pipeline handlers to build. If this is set to auto, all the pipelines applicable to the target architecture will be built.') > > option('qcam', > type : 'feature',
Hello Laurent, Thanks a lot for your feedback. On 12/24/22 02:01, Laurent Pinchart wrote: > Hi Javier, > > Thank you for the patch. > > On Fri, Dec 23, 2022 at 09:22:29AM +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: > > As long as CI systems can build all pipeline handlers, that's fine with > me. We may start getting somes patches that introduce compilation > breakages on pipeline handlers that are not compile-tested by > developers, but we can catch that in CI. > Agreed. >> +if pipelines.contains('auto') >> + host_cpu = host_machine.cpu_family() >> + if host_cpu == 'x86' or host_cpu == 'x86_64' >> + pipelines = ['ipu3'] >> + elif host_cpu == 'aarch64' >> + pipelines = ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1'] > > ipu3 is only for x86. > Yes, that's a left over since I didn't meant to include it for aarch64. >> + elif host_cpu == 'arm' >> + pipelines = ['raspberrypi'] >> + endif >> + >> + # Always include the simple and uvcvideo pipeline handlers. >> + pipelines += ['simple', 'uvcvideo'] > > The simple pipeline handler is only useful on ARM platforms (both 32 and > 64 bits) at the moment. > Ah, good to know. I'll adjust that accordingly. >> +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..23505805de41 100644 >> --- a/meson_options.txt >> +++ b/meson_options.txt >> @@ -37,8 +37,10 @@ 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'], > > This is getting long, let's wrap it: > OK. [...] >> + description : '''Select which pipeline handlers to build. If this is set to auto, all >> + the pipelines applicable to the target architecture will be built.''') > > The spaces in the description cause weird-looking output depending on > the terminal width: > > pipelines [imx8-isi, ipu3, raspberrypi, [auto, imx8-isi, ipu3, Select which pipeline handlers to build. If this is set to auto, > rkisp1, simple, uvcvideo, vimc] raspberrypi, rkisp1, simple, all the pipelines applicable to the > uvcvideo, target architecture will be built. > vimc] > > You can write > > description : '''Select which pipeline handlers to build. If this is set > to auto, all the pipelines applicable to the target architecture will be built.''') > Right. I'll do that. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
diff --git a/meson.build b/meson.build index d02f9917965c..fc1023326ed3 100644 --- a/meson.build +++ b/meson.build @@ -164,6 +164,20 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) # are enabled. pipelines = get_option('pipelines') +if pipelines.contains('auto') + host_cpu = host_machine.cpu_family() + if host_cpu == 'x86' or host_cpu == 'x86_64' + pipelines = ['ipu3'] + elif host_cpu == 'aarch64' + pipelines = ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1'] + elif host_cpu == 'arm' + pipelines = ['raspberrypi'] + endif + + # Always include the simple and uvcvideo pipeline handlers. + pipelines += ['simple', '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..23505805de41 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -37,8 +37,10 @@ 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 this is set to auto, all + the pipelines applicable to the target architecture will be built.''') option('qcam', type : 'feature',
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> --- meson.build | 14 ++++++++++++++ meson_options.txt | 6 ++++-- 2 files changed, 18 insertions(+), 2 deletions(-)