| Message ID | 20241217151513.133599-1-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Tue, Dec 17, 2024 at 03:15:16PM +0000, Barnabás Pőcze wrote: > Consider `pipelines=auto,virtual`. Previously that would select > everything that `auto` would, but not actually enable the `virtual` > pipeline handler because the `pipelines` list was reset. Use the imperative mood in the commit message, and the present tense to describe the current behaviour.: Consider the `pipelines=auto,virtual` meson option. One could expect it to auto-select pipeline handlers and to enable the virtual pipeline handler in addition. libcamera however ignores the `virtual` pipeline handler because the `pipelines` list is reset by `auto`. As enabling additional pipeline handlers beside auto-selection, fix this by considering all pipeline handlers in the list. > Bug: https://bugs.libcamera.org/show_bug.cgi?id=247 > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > meson.build | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/meson.build b/meson.build > index 33afbb741..5e5e4756d 100644 > --- a/meson.build > +++ b/meson.build > @@ -204,7 +204,7 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > > # Pipeline handlers > # > -pipelines = get_option('pipelines') > +wanted_pipelines = get_option('pipelines') > > arch_arm = ['arm', 'aarch64'] > arch_x86 = ['x86', 'x86_64'] > @@ -220,16 +220,18 @@ pipelines_support = { > 'virtual': ['test'], > } > > -if pipelines.contains('all') > +if wanted_pipelines.contains('all') > pipelines = pipelines_support.keys() > -elif pipelines.contains('auto') > +elif wanted_pipelines.contains('auto') > host_cpu = host_machine.cpu_family() > pipelines = [] > foreach pipeline, archs : pipelines_support > - if host_cpu in archs or 'any' in archs > + if pipeline in wanted_pipelines or host_cpu in archs or 'any' in archs > pipelines += pipeline > endif > endforeach > +else > + pipelines = wanted_pipelines > endif Would the following be clearer ? pipelines = {} foreach pipeline : get_option('pipelines') if pipeline == 'all' pipelines += pipelines_support elif pipeline == 'auto' host_cpu = host_machine.cpu_family() foreach pipeline, archs : pipelines_support if host_cpu in archs or 'any' in archs pipelines += {pipeline: true} endif endforeach else pipelines += {pipeline: true} endif endforeach pipelines = pipelines.keys() I don't have a strong preference. Either way, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > # Tests require the vimc pipeline handler, include it automatically when tests
Quoting Laurent Pinchart (2024-12-17 16:22:12) > Hi Barnabás, > > Thank you for the patch. > > On Tue, Dec 17, 2024 at 03:15:16PM +0000, Barnabás Pőcze wrote: > > Consider `pipelines=auto,virtual`. Previously that would select > > everything that `auto` would, but not actually enable the `virtual` > > pipeline handler because the `pipelines` list was reset. > > Use the imperative mood in the commit message, and the present tense to > describe the current behaviour.: > > Consider the `pipelines=auto,virtual` meson option. One could expect it > to auto-select pipeline handlers and to enable the virtual pipeline > handler in addition. libcamera however ignores the `virtual` pipeline > handler because the `pipelines` list is reset by `auto`. As enabling > additional pipeline handlers beside auto-selection, fix this by > considering all pipeline handlers in the list. > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=247 > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > meson.build | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/meson.build b/meson.build > > index 33afbb741..5e5e4756d 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -204,7 +204,7 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > > > > # Pipeline handlers > > # > > -pipelines = get_option('pipelines') > > +wanted_pipelines = get_option('pipelines') > > > > arch_arm = ['arm', 'aarch64'] > > arch_x86 = ['x86', 'x86_64'] > > @@ -220,16 +220,18 @@ pipelines_support = { > > 'virtual': ['test'], > > } > > > > -if pipelines.contains('all') > > +if wanted_pipelines.contains('all') > > pipelines = pipelines_support.keys() > > -elif pipelines.contains('auto') > > +elif wanted_pipelines.contains('auto') > > host_cpu = host_machine.cpu_family() > > pipelines = [] > > foreach pipeline, archs : pipelines_support > > - if host_cpu in archs or 'any' in archs > > + if pipeline in wanted_pipelines or host_cpu in archs or 'any' in archs > > pipelines += pipeline > > endif > > endforeach > > +else > > + pipelines = wanted_pipelines > > endif > > Would the following be clearer ? > > pipelines = {} > > foreach pipeline : get_option('pipelines') > if pipeline == 'all' > pipelines += pipelines_support > elif pipeline == 'auto' > host_cpu = host_machine.cpu_family() > foreach pipeline, archs : pipelines_support > if host_cpu in archs or 'any' in archs > pipelines += {pipeline: true} > endif > endforeach > else > pipelines += {pipeline: true} > endif > endforeach > > pipelines = pipelines.keys() > > I don't have a strong preference. Either way, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Likewise, I like the abiltiy to have auto+specific when desired. Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > # Tests require the vimc pipeline handler, include it automatically when tests > > -- > Regards, > > Laurent Pinchart
2024. december 17., kedd 17:22 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Tue, Dec 17, 2024 at 03:15:16PM +0000, Barnabás Pőcze wrote: > > Consider `pipelines=auto,virtual`. Previously that would select > > everything that `auto` would, but not actually enable the `virtual` > > pipeline handler because the `pipelines` list was reset. > > Use the imperative mood in the commit message, and the present tense to > describe the current behaviour.: > > Consider the `pipelines=auto,virtual` meson option. One could expect it > to auto-select pipeline handlers and to enable the virtual pipeline > handler in addition. libcamera however ignores the `virtual` pipeline > handler because the `pipelines` list is reset by `auto`. As enabling > additional pipeline handlers beside auto-selection, fix this by > considering all pipeline handlers in the list. > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=247 > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > meson.build | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/meson.build b/meson.build > > index 33afbb741..5e5e4756d 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -204,7 +204,7 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > > > > # Pipeline handlers > > # > > -pipelines = get_option('pipelines') > > +wanted_pipelines = get_option('pipelines') > > > > arch_arm = ['arm', 'aarch64'] > > arch_x86 = ['x86', 'x86_64'] > > @@ -220,16 +220,18 @@ pipelines_support = { > > 'virtual': ['test'], > > } > > > > -if pipelines.contains('all') > > +if wanted_pipelines.contains('all') > > pipelines = pipelines_support.keys() > > -elif pipelines.contains('auto') > > +elif wanted_pipelines.contains('auto') > > host_cpu = host_machine.cpu_family() > > pipelines = [] > > foreach pipeline, archs : pipelines_support > > - if host_cpu in archs or 'any' in archs > > + if pipeline in wanted_pipelines or host_cpu in archs or 'any' in archs > > pipelines += pipeline > > endif > > endforeach > > +else > > + pipelines = wanted_pipelines > > endif > > Would the following be clearer ? Well, after some thinking, unfortunately I have to report that it does not seem clearer to me personally. But if there is a consensus, I will change it. Regards, Barnabás Pőcze > > pipelines = {} > > foreach pipeline : get_option('pipelines') > if pipeline == 'all' > pipelines += pipelines_support > elif pipeline == 'auto' > host_cpu = host_machine.cpu_family() > foreach pipeline, archs : pipelines_support > if host_cpu in archs or 'any' in archs > pipelines += {pipeline: true} > endif > endforeach > else > pipelines += {pipeline: true} > endif > endforeach > > pipelines = pipelines.keys() > > I don't have a strong preference. Either way, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > # Tests require the vimc pipeline handler, include it automatically when tests > > -- > Regards, > > Laurent Pinchart >
On Tue, Dec 17, 2024 at 06:18:28PM +0000, Barnabás Pőcze wrote: > 2024. december 17., kedd 17:22 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > > > Hi Barnabás, > > > > Thank you for the patch. > > > > On Tue, Dec 17, 2024 at 03:15:16PM +0000, Barnabás Pőcze wrote: > > > Consider `pipelines=auto,virtual`. Previously that would select > > > everything that `auto` would, but not actually enable the `virtual` > > > pipeline handler because the `pipelines` list was reset. > > > > Use the imperative mood in the commit message, and the present tense to > > describe the current behaviour.: > > > > Consider the `pipelines=auto,virtual` meson option. One could expect it > > to auto-select pipeline handlers and to enable the virtual pipeline > > handler in addition. libcamera however ignores the `virtual` pipeline > > handler because the `pipelines` list is reset by `auto`. As enabling > > additional pipeline handlers beside auto-selection, fix this by > > considering all pipeline handlers in the list. > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=247 > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > meson.build | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/meson.build b/meson.build > > > index 33afbb741..5e5e4756d 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -204,7 +204,7 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > > > > > > # Pipeline handlers > > > # > > > -pipelines = get_option('pipelines') > > > +wanted_pipelines = get_option('pipelines') > > > > > > arch_arm = ['arm', 'aarch64'] > > > arch_x86 = ['x86', 'x86_64'] > > > @@ -220,16 +220,18 @@ pipelines_support = { > > > 'virtual': ['test'], > > > } > > > > > > -if pipelines.contains('all') > > > +if wanted_pipelines.contains('all') > > > pipelines = pipelines_support.keys() > > > -elif pipelines.contains('auto') > > > +elif wanted_pipelines.contains('auto') > > > host_cpu = host_machine.cpu_family() > > > pipelines = [] > > > foreach pipeline, archs : pipelines_support > > > - if host_cpu in archs or 'any' in archs > > > + if pipeline in wanted_pipelines or host_cpu in archs or 'any' in archs > > > pipelines += pipeline > > > endif > > > endforeach > > > +else > > > + pipelines = wanted_pipelines > > > endif > > > > Would the following be clearer ? > > Well, after some thinking, unfortunately I have to report that > it does not seem clearer to me personally. But if there is a > consensus, I will change it. I have a slight preference for the version below but both are OK with me. > > pipelines = {} > > > > foreach pipeline : get_option('pipelines') > > if pipeline == 'all' > > pipelines += pipelines_support > > elif pipeline == 'auto' > > host_cpu = host_machine.cpu_family() > > foreach pipeline, archs : pipelines_support > > if host_cpu in archs or 'any' in archs > > pipelines += {pipeline: true} > > endif > > endforeach > > else > > pipelines += {pipeline: true} > > endif > > endforeach > > > > pipelines = pipelines.keys() > > > > I don't have a strong preference. Either way, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > # Tests require the vimc pipeline handler, include it automatically when tests
diff --git a/meson.build b/meson.build index 33afbb741..5e5e4756d 100644 --- a/meson.build +++ b/meson.build @@ -204,7 +204,7 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) # Pipeline handlers # -pipelines = get_option('pipelines') +wanted_pipelines = get_option('pipelines') arch_arm = ['arm', 'aarch64'] arch_x86 = ['x86', 'x86_64'] @@ -220,16 +220,18 @@ pipelines_support = { 'virtual': ['test'], } -if pipelines.contains('all') +if wanted_pipelines.contains('all') pipelines = pipelines_support.keys() -elif pipelines.contains('auto') +elif wanted_pipelines.contains('auto') host_cpu = host_machine.cpu_family() pipelines = [] foreach pipeline, archs : pipelines_support - if host_cpu in archs or 'any' in archs + if pipeline in wanted_pipelines or host_cpu in archs or 'any' in archs pipelines += pipeline endif endforeach +else + pipelines = wanted_pipelines endif # Tests require the vimc pipeline handler, include it automatically when tests
Consider `pipelines=auto,virtual`. Previously that would select everything that `auto` would, but not actually enable the `virtual` pipeline handler because the `pipelines` list was reset. Bug: https://bugs.libcamera.org/show_bug.cgi?id=247 Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- meson.build | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)