| Message ID | 20230111232221.265057-1-javierm@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi! Just a drive-by comment: On 12.01.23 00:22, Javier Martinez Canillas 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 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@redhat.com> > --- > > meson.build | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/meson.build b/meson.build > index 389c547206fb..f9e3280ec0f2 100644 > --- a/meson.build > +++ b/meson.build > @@ -168,38 +168,41 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > # are enabled. > pipelines = get_option('pipelines') > > +pipelines_aarch64 = ['imx8-isi', 'rkisp1'] > +pipelines_arm = ['raspberrypi', 'simple'] > +pipelines_agnostic = ['uvcvideo'] > +pipelines_test = ['vimc'] > +pipelines_x86 = ['ipu3'] > + > if pipelines.contains('all') > - pipelines = [ > - 'imx8-isi', > - 'ipu3', > - 'raspberrypi', > - 'rkisp1', > - 'simple', > - 'uvcvideo', > - 'vimc', > - ] > + pipelines = [] > + pipelines += pipelines_aarch64 > + pipelines += pipelines_arm > + pipelines += pipelines_agnostic > + pipelines += pipelines_test > + pipelines += pipelines_x86 > endif > > if pipelines.contains('auto') > host_cpu = host_machine.cpu_family() > pipelines = [] > if host_cpu == 'x86' or host_cpu == 'x86_64' > - pipelines += ['ipu3'] > + pipelines += pipelines_x86 > elif host_cpu == 'aarch64' > - pipelines += ['imx8-isi', 'rkisp1'] > + pipelines += pipelines_aarch64 > endif > > if host_cpu == 'arm' or host_cpu == 'aarch64' > - pipelines += ['raspberrypi', 'simple'] > + pipelines += pipeines_arm Typo: pipeines_arm > endif > > # Always include the uvcvideo pipeline handler. > - pipelines += ['uvcvideo'] > + pipelines += pipelines_agnostic > endif > > if get_option('test') and 'vimc' not in pipelines > message('Enabling vimc pipeline handler to support tests') > - pipelines += ['vimc'] > + pipelines += pipelines_test > endif > > # Utilities are parsed first to provide support for other components.
Hello Robert, On 1/12/23 00:29, Robert Mader via libcamera-devel wrote: > Hi! Just a drive-by comment: [...] >> if host_cpu == 'arm' or host_cpu == 'aarch64' >> - pipelines += ['raspberrypi', 'simple'] >> + pipelines += pipeines_arm > Typo: pipeines_arm >> endif Oh, indeed! Thanks a lot for catching this. Kieran though has an idea that these pipelines could be defined using a data driven approach. So I'll wait for his proposal and have an agreement on the way forward before posting a v2.
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@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. > diff --git a/meson.build b/meson.build > index 389c547206fb..f9e3280ec0f2 100644 > --- a/meson.build > +++ b/meson.build > @@ -168,38 +168,41 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > # are enabled. > pipelines = get_option('pipelines') > > +pipelines_aarch64 = ['imx8-isi', 'rkisp1'] > +pipelines_arm = ['raspberrypi', 'simple'] > +pipelines_agnostic = ['uvcvideo'] > +pipelines_test = ['vimc'] > +pipelines_x86 = ['ipu3'] > + > if pipelines.contains('all') > - pipelines = [ > - 'imx8-isi', > - 'ipu3', > - 'raspberrypi', > - 'rkisp1', > - 'simple', > - 'uvcvideo', > - 'vimc', > - ] > + pipelines = [] > + pipelines += pipelines_aarch64 > + pipelines += pipelines_arm > + pipelines += pipelines_agnostic > + pipelines += pipelines_test > + pipelines += pipelines_x86 > endif > > if pipelines.contains('auto') > host_cpu = host_machine.cpu_family() > pipelines = [] > if host_cpu == 'x86' or host_cpu == 'x86_64' > - pipelines += ['ipu3'] > + pipelines += pipelines_x86 > elif host_cpu == 'aarch64' > - pipelines += ['imx8-isi', 'rkisp1'] > + pipelines += pipelines_aarch64 > endif > > if host_cpu == 'arm' or host_cpu == 'aarch64' > - pipelines += ['raspberrypi', 'simple'] > + pipelines += pipeines_arm > endif > > # Always include the uvcvideo pipeline handler. > - pipelines += ['uvcvideo'] > + pipelines += pipelines_agnostic > endif > > if get_option('test') and 'vimc' not in pipelines > message('Enabling vimc pipeline handler to support tests') > - pipelines += ['vimc'] > + pipelines += pipelines_test > endif > > # Utilities are parsed first to provide support for other components.
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@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. 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 > > > diff --git a/meson.build b/meson.build > > index 389c547206fb..f9e3280ec0f2 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -168,38 +168,41 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) > > # are enabled. > > pipelines = get_option('pipelines') > > > > +pipelines_aarch64 = ['imx8-isi', 'rkisp1'] > > +pipelines_arm = ['raspberrypi', 'simple'] > > +pipelines_agnostic = ['uvcvideo'] > > +pipelines_test = ['vimc'] > > +pipelines_x86 = ['ipu3'] > > + > > if pipelines.contains('all') > > - pipelines = [ > > - 'imx8-isi', > > - 'ipu3', > > - 'raspberrypi', > > - 'rkisp1', > > - 'simple', > > - 'uvcvideo', > > - 'vimc', > > - ] > > + pipelines = [] > > + pipelines += pipelines_aarch64 > > + pipelines += pipelines_arm > > + pipelines += pipelines_agnostic > > + pipelines += pipelines_test > > + pipelines += pipelines_x86 > > endif > > > > if pipelines.contains('auto') > > host_cpu = host_machine.cpu_family() > > pipelines = [] > > if host_cpu == 'x86' or host_cpu == 'x86_64' > > - pipelines += ['ipu3'] > > + pipelines += pipelines_x86 > > elif host_cpu == 'aarch64' > > - pipelines += ['imx8-isi', 'rkisp1'] > > + pipelines += pipelines_aarch64 > > endif > > > > if host_cpu == 'arm' or host_cpu == 'aarch64' > > - pipelines += ['raspberrypi', 'simple'] > > + pipelines += pipeines_arm > > endif > > > > # Always include the uvcvideo pipeline handler. > > - pipelines += ['uvcvideo'] > > + pipelines += pipelines_agnostic > > endif > > > > if get_option('test') and 'vimc' not in pipelines > > message('Enabling vimc pipeline handler to support tests') > > - pipelines += ['vimc'] > > + pipelines += pipelines_test > > endif > > > > # Utilities are parsed first to provide support for other components. > > -- > Regards, > > Laurent Pinchart
Quoting Kieran Bingham (2023-01-12 09:47:12) > 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@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. > > Sketched out that would be: > > pipelines = get_option('pipelines') > > pipes_support = { > 'imx8-isi': ['aarch64'], > 'ipu3': ['x86', 'x86_64'], > 'raspberrypi': ['arm', 'aarch64'], > 'rkisp1': ['aarch64'], Note I've intentionally kept rkisp1 and imx8-isi as aarch64 only, as I believe that's how I interpret the current implementation. I expect it should really be both for each of them? We could wrap a x86 = ['x86', 'x86_64'], and an arm = ['arm', 'aarch64'] helper too. > '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
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@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!
diff --git a/meson.build b/meson.build index 389c547206fb..f9e3280ec0f2 100644 --- a/meson.build +++ b/meson.build @@ -168,38 +168,41 @@ liblttng = dependency('lttng-ust', required : get_option('tracing')) # are enabled. pipelines = get_option('pipelines') +pipelines_aarch64 = ['imx8-isi', 'rkisp1'] +pipelines_arm = ['raspberrypi', 'simple'] +pipelines_agnostic = ['uvcvideo'] +pipelines_test = ['vimc'] +pipelines_x86 = ['ipu3'] + if pipelines.contains('all') - pipelines = [ - 'imx8-isi', - 'ipu3', - 'raspberrypi', - 'rkisp1', - 'simple', - 'uvcvideo', - 'vimc', - ] + pipelines = [] + pipelines += pipelines_aarch64 + pipelines += pipelines_arm + pipelines += pipelines_agnostic + pipelines += pipelines_test + pipelines += pipelines_x86 endif if pipelines.contains('auto') host_cpu = host_machine.cpu_family() pipelines = [] if host_cpu == 'x86' or host_cpu == 'x86_64' - pipelines += ['ipu3'] + pipelines += pipelines_x86 elif host_cpu == 'aarch64' - pipelines += ['imx8-isi', 'rkisp1'] + pipelines += pipelines_aarch64 endif if host_cpu == 'arm' or host_cpu == 'aarch64' - pipelines += ['raspberrypi', 'simple'] + pipelines += pipeines_arm endif # Always include the uvcvideo pipeline handler. - pipelines += ['uvcvideo'] + pipelines += pipelines_agnostic endif if get_option('test') and 'vimc' not in pipelines message('Enabling vimc pipeline handler to support tests') - pipelines += ['vimc'] + pipelines += pipelines_test endif # Utilities are parsed first to provide support for other components.
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@redhat.com> --- meson.build | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)