| Message ID | 20231121145350.5956-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > Add support for using separate YAML files for vendor specific controls. > This simplifies management of vendor control definitions and avoids > possible merge conflicts when changing the control_ids.yaml file for > core and draft controls. The mapping of vendor/pipeline handler to > control file is done through the vendor_controls_mapping variable in > include/libcamera/meson.build. > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > vendor controls. This contains a single control PispConfigDumpFile that > will be used in the Pi 5 pipeline handler as a trigger to dump the > Backend configuration as a JSON file. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > meson.build | 2 ++ > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > src/libcamera/meson.build | 12 +++++++++ > src/py/libcamera/gen-py-controls.py | 10 +++---- > src/py/libcamera/meson.build | 8 ++++++ > utils/gen-controls.py | 12 +++++---- > 7 files changed, 91 insertions(+), 11 deletions(-) > create mode 100644 src/libcamera/control_ids_rpi.yaml > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > index 5fb772e6dd14..54123c5b858d 100644 > --- a/include/libcamera/meson.build > +++ b/include/libcamera/meson.build > @@ -38,10 +38,50 @@ control_source_files = { > 'property_ids': 'properties', > } I really like the series, but this patch is the piece I like less. > > +vendor_controls_mapping = { > + # Mapping of vendor (pipeline handler) specific controls files > + 'controls': > + { Isn't this usually written as 'controls' : { } in meson ? > + 'rpi/pisp': 'control_ids_rpi.yaml', > + 'rpi/vc4': 'control_ids_rpi.yaml' > + }, > + # Mapping of vendor (pipeline handler) specific properties files > + 'properties': > + { > + > + } > +} > + > control_headers = [] > +vendor_ctrl_files = [] > +vendor_prop_files = [] > > foreach header, mode : control_source_files > - input_files = files('../../src/libcamera/' + header +'.yaml') > + > + # Start by populating the vendor specific mappings into vendor_ctrl_files > + # and vendor_prop_files. These will be cached for later use. > + vendor_files = [] > + foreach pipeline, file : vendor_controls_mapping[mode] > + if pipeline not in pipelines > + continue > + endif > + if file not in vendor_files > + vendor_files += file > + endif > + endforeach > + > + if mode == 'controls' > + vendor_ctrl_files = vendor_files > + else > + vendor_prop_files = vendor_files > + endif > + > + input_files = files('../../src/libcamera/' + header + '.yaml') > + > + foreach file : vendor_files > + input_files += files('../../src/libcamera/' + file) > + endforeach > + Wouldn't it be simpler if we try to handle libcamera, draft and vendor controls as first class citizens in the same manner, instead of maintaing the "libcamera" ones on a different level ? I guess if we could do something like: ------------------------------------------------------------------------------- controls_map { 'controls' : { 'libcamera' : controls_ids.yaml, 'drafts' : controls_ids_draft.yaml 'rpi/pisp': 'control_ids_rpi.yaml', 'rpi/vc4': 'control_ids_rpi.yaml' } 'properties' = { 'libcamera' : controls_ids.yaml, 'drafts' : controls_ids_draft.yaml } } control_headers = [] ranges_file = files('../../src/libcamera/control_ranges.yaml') foreach mode, entry : controls_map files_list = [] input_files=[] foreach vendor, header : entry if vendor != 'libcamera' or vendor != 'draft' if vendor not in pipelines continue endif endif if header in files_list continue endif files_list += header input_files += files('../../src/libcamera/' + header) endforeach outfile='' if mode == 'controls' outfile = 'control_ids.h' else outfile = 'property_ids.h' endif template = outfile + '.in' control_headers += custom_target(header + '_h', input : input_files, output : outfile, command : [gen_controls, '-o', '@OUTPUT@', '--mode', mode, '-t', template, '-r', ranges_file, '@INPUT@'], install : true, install_dir : libcamera_headers_install_dir) endforeach libcamera_public_headers += control_headers ------------------------------------------------------------------------------- isn't it nicer ? The above code is valid in meson, but the meson.build in src/libcamera/ should be updated accordingly. In general, I think the relevant part here is controls_map { 'controls' : { 'libcamera' : controls_ids.yaml, 'drafts' : controls_ids_draft.yaml 'rpi/pisp': 'control_ids_rpi.yaml', 'rpi/vc4': 'control_ids_rpi.yaml' } 'properties' = { 'libcamera' : controls_ids.yaml, 'drafts' : controls_ids_draft.yaml } } As seeing all the supported controls and properties listed together might be easier to maintain and parse ? What do you think ? > template_file = files(header + '.h.in') > control_headers += custom_target(header + '_h', > input : input_files, > diff --git a/meson.build b/meson.build > index e9a1c7e360ce..1423abf16c77 100644 > --- a/meson.build > +++ b/meson.build > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > summary({ > 'Enabled pipelines': pipelines, > 'Enabled IPA modules': enabled_ipa_names, > + 'Vendor controls': vendor_ctrl_files, > + 'Vendor properties': vendor_prop_files, > 'Hotplug support': libudev.found(), > 'Tracing support': tracing_enabled, > 'Android support': android_enabled, > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > new file mode 100644 > index 000000000000..a6f8b43a5f62 > --- /dev/null > +++ b/src/libcamera/control_ids_rpi.yaml > @@ -0,0 +1,16 @@ > +# SPDX-License-Identifier: LGPL-2.1-or-later > +# > +# Copyright (C) 2023, Raspberry Pi Ltd > +# > +%YAML 1.1 > +--- > +vendor: rpi > +controls: > + - PispConfigDumpFile: > + type: string > + description: | > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > + formatted dump of the Backend configuration to the filename given by the > + value of the control. > + > +... > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 05ee38daf22b..7eb26ccbb7f5 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -129,6 +129,18 @@ control_sources = [] > > foreach source, mode : control_source_files > input_files = files(source +'.yaml') > + > + # Add the vendor specific files to the input. > + if mode == 'controls' > + vendor_files = vendor_ctrl_files > + else > + vendor_files = vendor_prop_files > + endif > + > + foreach file : vendor_files > + input_files += files(file) > + endforeach > + > template_file = files(source + '.cpp.in') > control_sources += custom_target(source + '_cpp', > input : input_files, > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > index ea28f0139f23..e2ddad8581fd 100755 > --- a/src/py/libcamera/gen-py-controls.py > +++ b/src/py/libcamera/gen-py-controls.py > @@ -91,7 +91,7 @@ def main(argv): > parser = argparse.ArgumentParser() > parser.add_argument('-o', dest='output', metavar='file', type=str, > help='Output file name. Defaults to standard output if not specified.') > - parser.add_argument('input', type=str, > + parser.add_argument('input', type=str, nargs='+', > help='Input file name.') > parser.add_argument('-t', dest='template', type=str, required=True, > help='Template file name.') > @@ -103,11 +103,11 @@ def main(argv): > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > return -1 > > - data = open(args.input, 'rb').read() > - > controls = {} > - vendor = yaml.safe_load(data)['vendor'] > - controls[vendor] = yaml.safe_load(data)['controls'] > + for input in args.input: > + data = open(input, 'rb').read() > + vendor = yaml.safe_load(data)['vendor'] > + controls[vendor] = yaml.safe_load(data)['controls'] > > data = generate_py(controls, args.mode) > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > index 1c3ea1843ac0..ea38ad6ca829 100644 > --- a/src/py/libcamera/meson.build > +++ b/src/py/libcamera/meson.build > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > gen_py_controls = files('gen-py-controls.py') > > +foreach file : vendor_ctrl_files > + gen_py_controls_input_files += files('../../libcamera/' + file) > +endforeach > + > pycamera_sources += custom_target('py_gen_controls', > input : gen_py_controls_input_files, > output : ['py_controls_generated.cpp'], > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > gen_py_properties_template = files('py_properties_generated.cpp.in') > > +foreach file : vendor_prop_files > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > +endforeach > + > pycamera_sources += custom_target('py_gen_properties', > input : gen_py_property_enums_input_files, > output : ['py_properties_generated.cpp'], > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > index 8af33d29cd07..d7862142b8c1 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -359,7 +359,7 @@ def main(argv): > parser = argparse.ArgumentParser() > parser.add_argument('-o', dest='output', metavar='file', type=str, > help='Output file name. Defaults to standard output if not specified.') > - parser.add_argument('input', type=str, > + parser.add_argument('input', type=str, nargs='+', > help='Input file name.') > parser.add_argument('-t', dest='template', type=str, required=True, > help='Template file name.') > @@ -367,10 +367,12 @@ def main(argv): > help='Mode of operation') > args = parser.parse_args(argv[1:]) > > - data = open(args.input, 'rb').read() > - vendor = yaml.safe_load(data)['vendor'] > - controls = yaml.safe_load(data)['controls'] > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > + controls = [] > + for input in args.input: > + data = open(input, 'rb').read() > + vendor = yaml.safe_load(data)['vendor'] > + ctrls = yaml.safe_load(data)['controls'] > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > if args.template.endswith('.cpp.in'): > data = generate_cpp(controls) > -- > 2.34.1 >
Hi Jacopo, On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > Add support for using separate YAML files for vendor specific controls. > > This simplifies management of vendor control definitions and avoids > > possible merge conflicts when changing the control_ids.yaml file for > > core and draft controls. The mapping of vendor/pipeline handler to > > control file is done through the vendor_controls_mapping variable in > > include/libcamera/meson.build. > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > vendor controls. This contains a single control PispConfigDumpFile that > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > Backend configuration as a JSON file. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > meson.build | 2 ++ > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > src/libcamera/meson.build | 12 +++++++++ > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > src/py/libcamera/meson.build | 8 ++++++ > > utils/gen-controls.py | 12 +++++---- > > 7 files changed, 91 insertions(+), 11 deletions(-) > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > index 5fb772e6dd14..54123c5b858d 100644 > > --- a/include/libcamera/meson.build > > +++ b/include/libcamera/meson.build > > @@ -38,10 +38,50 @@ control_source_files = { > > 'property_ids': 'properties', > > } > > I really like the series, but this patch is the piece I like less. > > > > > +vendor_controls_mapping = { > > + # Mapping of vendor (pipeline handler) specific controls files > > + 'controls': > > + { > > Isn't this usually written as > > 'controls' : { > > } > > in meson ? > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > + }, > > + # Mapping of vendor (pipeline handler) specific properties files > > + 'properties': > > + { > > + > > + } > > +} > > + > > control_headers = [] > > +vendor_ctrl_files = [] > > +vendor_prop_files = [] > > > > foreach header, mode : control_source_files > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > + > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > + # and vendor_prop_files. These will be cached for later use. > > + vendor_files = [] > > + foreach pipeline, file : vendor_controls_mapping[mode] > > + if pipeline not in pipelines > > + continue > > + endif > > + if file not in vendor_files > > + vendor_files += file > > + endif > > + endforeach > > + > > + if mode == 'controls' > > + vendor_ctrl_files = vendor_files > > + else > > + vendor_prop_files = vendor_files > > + endif > > + > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > + > > + foreach file : vendor_files > > + input_files += files('../../src/libcamera/' + file) > > + endforeach > > + > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > controls as first class citizens in the same manner, instead of > maintaing the "libcamera" ones on a different level ? > > I guess if we could do something like: > > ------------------------------------------------------------------------------- > > controls_map { > 'controls' : { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > 'rpi/pisp': 'control_ids_rpi.yaml', > 'rpi/vc4': 'control_ids_rpi.yaml' > } > > 'properties' = { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > } > } > > control_headers = [] > ranges_file = files('../../src/libcamera/control_ranges.yaml') > foreach mode, entry : controls_map > files_list = [] > input_files=[] > foreach vendor, header : entry > if vendor != 'libcamera' or vendor != 'draft' > if vendor not in pipelines > continue > endif > endif > > if header in files_list > continue > endif > files_list += header > > input_files += files('../../src/libcamera/' + header) > endforeach > > outfile='' > if mode == 'controls' > outfile = 'control_ids.h' > else > outfile = 'property_ids.h' > endif > > template = outfile + '.in' > control_headers += custom_target(header + '_h', > input : input_files, > output : outfile, > command : [gen_controls, '-o', '@OUTPUT@', > '--mode', mode, '-t', template, > '-r', ranges_file, '@INPUT@'], > install : true, > install_dir : libcamera_headers_install_dir) > endforeach > > libcamera_public_headers += control_headers > > ------------------------------------------------------------------------------- > > isn't it nicer ? > > The above code is valid in meson, but the meson.build in > src/libcamera/ should be updated accordingly. > > In general, I think the relevant part here is > > controls_map { > 'controls' : { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > 'rpi/pisp': 'control_ids_rpi.yaml', > 'rpi/vc4': 'control_ids_rpi.yaml' > } > > 'properties' = { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > } > } > > As seeing all the supported controls and properties listed together > might be easier to maintain and parse ? > > What do you think ? That looks reasonable. I guess I wanted to keep things seperate for vendor controls, but no reason not to combine the libcamera/draft and vendor cases into one list. I can make this change for v3. Regards, Naush > > > > template_file = files(header + '.h.in') > > control_headers += custom_target(header + '_h', > > input : input_files, > > diff --git a/meson.build b/meson.build > > index e9a1c7e360ce..1423abf16c77 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > summary({ > > 'Enabled pipelines': pipelines, > > 'Enabled IPA modules': enabled_ipa_names, > > + 'Vendor controls': vendor_ctrl_files, > > + 'Vendor properties': vendor_prop_files, > > 'Hotplug support': libudev.found(), > > 'Tracing support': tracing_enabled, > > 'Android support': android_enabled, > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > new file mode 100644 > > index 000000000000..a6f8b43a5f62 > > --- /dev/null > > +++ b/src/libcamera/control_ids_rpi.yaml > > @@ -0,0 +1,16 @@ > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > +# > > +# Copyright (C) 2023, Raspberry Pi Ltd > > +# > > +%YAML 1.1 > > +--- > > +vendor: rpi > > +controls: > > + - PispConfigDumpFile: > > + type: string > > + description: | > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > + formatted dump of the Backend configuration to the filename given by the > > + value of the control. > > + > > +... > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -129,6 +129,18 @@ control_sources = [] > > > > foreach source, mode : control_source_files > > input_files = files(source +'.yaml') > > + > > + # Add the vendor specific files to the input. > > + if mode == 'controls' > > + vendor_files = vendor_ctrl_files > > + else > > + vendor_files = vendor_prop_files > > + endif > > + > > + foreach file : vendor_files > > + input_files += files(file) > > + endforeach > > + > > template_file = files(source + '.cpp.in') > > control_sources += custom_target(source + '_cpp', > > input : input_files, > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > index ea28f0139f23..e2ddad8581fd 100755 > > --- a/src/py/libcamera/gen-py-controls.py > > +++ b/src/py/libcamera/gen-py-controls.py > > @@ -91,7 +91,7 @@ def main(argv): > > parser = argparse.ArgumentParser() > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > help='Output file name. Defaults to standard output if not specified.') > > - parser.add_argument('input', type=str, > > + parser.add_argument('input', type=str, nargs='+', > > help='Input file name.') > > parser.add_argument('-t', dest='template', type=str, required=True, > > help='Template file name.') > > @@ -103,11 +103,11 @@ def main(argv): > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > return -1 > > > > - data = open(args.input, 'rb').read() > > - > > controls = {} > > - vendor = yaml.safe_load(data)['vendor'] > > - controls[vendor] = yaml.safe_load(data)['controls'] > > + for input in args.input: > > + data = open(input, 'rb').read() > > + vendor = yaml.safe_load(data)['vendor'] > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > data = generate_py(controls, args.mode) > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > --- a/src/py/libcamera/meson.build > > +++ b/src/py/libcamera/meson.build > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > gen_py_controls = files('gen-py-controls.py') > > > > +foreach file : vendor_ctrl_files > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > +endforeach > > + > > pycamera_sources += custom_target('py_gen_controls', > > input : gen_py_controls_input_files, > > output : ['py_controls_generated.cpp'], > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > +foreach file : vendor_prop_files > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > +endforeach > > + > > pycamera_sources += custom_target('py_gen_properties', > > input : gen_py_property_enums_input_files, > > output : ['py_properties_generated.cpp'], > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > index 8af33d29cd07..d7862142b8c1 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -359,7 +359,7 @@ def main(argv): > > parser = argparse.ArgumentParser() > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > help='Output file name. Defaults to standard output if not specified.') > > - parser.add_argument('input', type=str, > > + parser.add_argument('input', type=str, nargs='+', > > help='Input file name.') > > parser.add_argument('-t', dest='template', type=str, required=True, > > help='Template file name.') > > @@ -367,10 +367,12 @@ def main(argv): > > help='Mode of operation') > > args = parser.parse_args(argv[1:]) > > > > - data = open(args.input, 'rb').read() > > - vendor = yaml.safe_load(data)['vendor'] > > - controls = yaml.safe_load(data)['controls'] > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > + controls = [] > > + for input in args.input: > > + data = open(input, 'rb').read() > > + vendor = yaml.safe_load(data)['vendor'] > > + ctrls = yaml.safe_load(data)['controls'] > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > if args.template.endswith('.cpp.in'): > > data = generate_cpp(controls) > > -- > > 2.34.1 > >
Hi Naush On Wed, Nov 22, 2023 at 03:53:03PM +0000, Naushir Patuck wrote: > Hi Jacopo, > > On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > > Add support for using separate YAML files for vendor specific controls. > > > This simplifies management of vendor control definitions and avoids > > > possible merge conflicts when changing the control_ids.yaml file for > > > core and draft controls. The mapping of vendor/pipeline handler to > > > control file is done through the vendor_controls_mapping variable in > > > include/libcamera/meson.build. > > > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > > vendor controls. This contains a single control PispConfigDumpFile that > > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > > Backend configuration as a JSON file. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > > meson.build | 2 ++ > > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > > src/libcamera/meson.build | 12 +++++++++ > > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > > src/py/libcamera/meson.build | 8 ++++++ > > > utils/gen-controls.py | 12 +++++---- > > > 7 files changed, 91 insertions(+), 11 deletions(-) > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > index 5fb772e6dd14..54123c5b858d 100644 > > > --- a/include/libcamera/meson.build > > > +++ b/include/libcamera/meson.build > > > @@ -38,10 +38,50 @@ control_source_files = { > > > 'property_ids': 'properties', > > > } > > > > I really like the series, but this patch is the piece I like less. > > > > > > > > +vendor_controls_mapping = { > > > + # Mapping of vendor (pipeline handler) specific controls files > > > + 'controls': > > > + { > > > > Isn't this usually written as > > > > 'controls' : { > > > > } > > > > in meson ? > > > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > > + }, > > > + # Mapping of vendor (pipeline handler) specific properties files > > > + 'properties': > > > + { > > > + > > > + } > > > +} > > > + > > > control_headers = [] > > > +vendor_ctrl_files = [] > > > +vendor_prop_files = [] > > > > > > foreach header, mode : control_source_files > > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > > + > > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > > + # and vendor_prop_files. These will be cached for later use. > > > + vendor_files = [] > > > + foreach pipeline, file : vendor_controls_mapping[mode] > > > + if pipeline not in pipelines > > > + continue > > > + endif > > > + if file not in vendor_files > > > + vendor_files += file > > > + endif > > > + endforeach > > > + > > > + if mode == 'controls' > > > + vendor_ctrl_files = vendor_files > > > + else > > > + vendor_prop_files = vendor_files > > > + endif > > > + > > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > > + > > > + foreach file : vendor_files > > > + input_files += files('../../src/libcamera/' + file) > > > + endforeach > > > + > > > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > > controls as first class citizens in the same manner, instead of > > maintaing the "libcamera" ones on a different level ? > > > > I guess if we could do something like: > > > > ------------------------------------------------------------------------------- > > > > controls_map { > > 'controls' : { > > 'libcamera' : controls_ids.yaml, > > 'drafts' : controls_ids_draft.yaml > > 'rpi/pisp': 'control_ids_rpi.yaml', > > 'rpi/vc4': 'control_ids_rpi.yaml' > > } > > > > 'properties' = { > > 'libcamera' : controls_ids.yaml, > > 'drafts' : controls_ids_draft.yaml > > } > > } > > > > control_headers = [] > > ranges_file = files('../../src/libcamera/control_ranges.yaml') > > foreach mode, entry : controls_map > > files_list = [] > > input_files=[] > > foreach vendor, header : entry > > if vendor != 'libcamera' or vendor != 'draft' > > if vendor not in pipelines > > continue > > endif > > endif > > > > if header in files_list > > continue > > endif > > files_list += header > > > > input_files += files('../../src/libcamera/' + header) > > endforeach > > > > outfile='' > > if mode == 'controls' > > outfile = 'control_ids.h' > > else > > outfile = 'property_ids.h' > > endif > > > > template = outfile + '.in' > > control_headers += custom_target(header + '_h', > > input : input_files, > > output : outfile, > > command : [gen_controls, '-o', '@OUTPUT@', > > '--mode', mode, '-t', template, > > '-r', ranges_file, '@INPUT@'], > > install : true, > > install_dir : libcamera_headers_install_dir) > > endforeach > > > > libcamera_public_headers += control_headers > > > > ------------------------------------------------------------------------------- > > > > isn't it nicer ? > > > > The above code is valid in meson, but the meson.build in > > src/libcamera/ should be updated accordingly. > > > > In general, I think the relevant part here is > > > > controls_map { > > 'controls' : { > > 'libcamera' : controls_ids.yaml, > > 'drafts' : controls_ids_draft.yaml > > 'rpi/pisp': 'control_ids_rpi.yaml', > > 'rpi/vc4': 'control_ids_rpi.yaml' > > } > > > > 'properties' = { > > 'libcamera' : controls_ids.yaml, > > 'drafts' : controls_ids_draft.yaml > > } > > } > > > > As seeing all the supported controls and properties listed together > > might be easier to maintain and parse ? > > > > What do you think ? > > That looks reasonable. I guess I wanted to keep things seperate for > vendor controls, but no reason not to combine the libcamera/draft and > vendor cases into one list. I can make this change for v3. Let me know if and how I can help. The change is quite invasive in the whole patch series and I don't want to make it more painful than necessary for you > > Regards, > Naush > > > > > > > > > template_file = files(header + '.h.in') > > > control_headers += custom_target(header + '_h', > > > input : input_files, > > > diff --git a/meson.build b/meson.build > > > index e9a1c7e360ce..1423abf16c77 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > > summary({ > > > 'Enabled pipelines': pipelines, > > > 'Enabled IPA modules': enabled_ipa_names, > > > + 'Vendor controls': vendor_ctrl_files, > > > + 'Vendor properties': vendor_prop_files, > > > 'Hotplug support': libudev.found(), > > > 'Tracing support': tracing_enabled, > > > 'Android support': android_enabled, > > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > > new file mode 100644 > > > index 000000000000..a6f8b43a5f62 > > > --- /dev/null > > > +++ b/src/libcamera/control_ids_rpi.yaml > > > @@ -0,0 +1,16 @@ > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > +# > > > +# Copyright (C) 2023, Raspberry Pi Ltd > > > +# > > > +%YAML 1.1 > > > +--- > > > +vendor: rpi > > > +controls: > > > + - PispConfigDumpFile: > > > + type: string > > > + description: | > > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > > + formatted dump of the Backend configuration to the filename given by the > > > + value of the control. > > > + > > > +... > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -129,6 +129,18 @@ control_sources = [] > > > > > > foreach source, mode : control_source_files > > > input_files = files(source +'.yaml') > > > + > > > + # Add the vendor specific files to the input. > > > + if mode == 'controls' > > > + vendor_files = vendor_ctrl_files > > > + else > > > + vendor_files = vendor_prop_files > > > + endif > > > + > > > + foreach file : vendor_files > > > + input_files += files(file) > > > + endforeach > > > + > > > template_file = files(source + '.cpp.in') > > > control_sources += custom_target(source + '_cpp', > > > input : input_files, > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > > index ea28f0139f23..e2ddad8581fd 100755 > > > --- a/src/py/libcamera/gen-py-controls.py > > > +++ b/src/py/libcamera/gen-py-controls.py > > > @@ -91,7 +91,7 @@ def main(argv): > > > parser = argparse.ArgumentParser() > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > help='Output file name. Defaults to standard output if not specified.') > > > - parser.add_argument('input', type=str, > > > + parser.add_argument('input', type=str, nargs='+', > > > help='Input file name.') > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > help='Template file name.') > > > @@ -103,11 +103,11 @@ def main(argv): > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > > return -1 > > > > > > - data = open(args.input, 'rb').read() > > > - > > > controls = {} > > > - vendor = yaml.safe_load(data)['vendor'] > > > - controls[vendor] = yaml.safe_load(data)['controls'] > > > + for input in args.input: > > > + data = open(input, 'rb').read() > > > + vendor = yaml.safe_load(data)['vendor'] > > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > data = generate_py(controls, args.mode) > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > > --- a/src/py/libcamera/meson.build > > > +++ b/src/py/libcamera/meson.build > > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > > > gen_py_controls = files('gen-py-controls.py') > > > > > > +foreach file : vendor_ctrl_files > > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > > +endforeach > > > + > > > pycamera_sources += custom_target('py_gen_controls', > > > input : gen_py_controls_input_files, > > > output : ['py_controls_generated.cpp'], > > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > > > +foreach file : vendor_prop_files > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > > +endforeach > > > + > > > pycamera_sources += custom_target('py_gen_properties', > > > input : gen_py_property_enums_input_files, > > > output : ['py_properties_generated.cpp'], > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > > index 8af33d29cd07..d7862142b8c1 100755 > > > --- a/utils/gen-controls.py > > > +++ b/utils/gen-controls.py > > > @@ -359,7 +359,7 @@ def main(argv): > > > parser = argparse.ArgumentParser() > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > help='Output file name. Defaults to standard output if not specified.') > > > - parser.add_argument('input', type=str, > > > + parser.add_argument('input', type=str, nargs='+', > > > help='Input file name.') > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > help='Template file name.') > > > @@ -367,10 +367,12 @@ def main(argv): > > > help='Mode of operation') > > > args = parser.parse_args(argv[1:]) > > > > > > - data = open(args.input, 'rb').read() > > > - vendor = yaml.safe_load(data)['vendor'] > > > - controls = yaml.safe_load(data)['controls'] > > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > + controls = [] > > > + for input in args.input: > > > + data = open(input, 'rb').read() > > > + vendor = yaml.safe_load(data)['vendor'] > > > + ctrls = yaml.safe_load(data)['controls'] > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > > > if args.template.endswith('.cpp.in'): > > > data = generate_cpp(controls) > > > -- > > > 2.34.1 > > >
One quick note On Wed, Nov 22, 2023 at 04:24:40PM +0100, Jacopo Mondi via libcamera-devel wrote: > Hi Naush > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > Add support for using separate YAML files for vendor specific controls. > > This simplifies management of vendor control definitions and avoids > > possible merge conflicts when changing the control_ids.yaml file for > > core and draft controls. The mapping of vendor/pipeline handler to > > control file is done through the vendor_controls_mapping variable in > > include/libcamera/meson.build. > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > vendor controls. This contains a single control PispConfigDumpFile that > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > Backend configuration as a JSON file. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > meson.build | 2 ++ > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > src/libcamera/meson.build | 12 +++++++++ > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > src/py/libcamera/meson.build | 8 ++++++ > > utils/gen-controls.py | 12 +++++---- > > 7 files changed, 91 insertions(+), 11 deletions(-) > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > index 5fb772e6dd14..54123c5b858d 100644 > > --- a/include/libcamera/meson.build > > +++ b/include/libcamera/meson.build > > @@ -38,10 +38,50 @@ control_source_files = { > > 'property_ids': 'properties', > > } > > I really like the series, but this patch is the piece I like less. > > > > > +vendor_controls_mapping = { > > + # Mapping of vendor (pipeline handler) specific controls files > > + 'controls': > > + { > > Isn't this usually written as > > 'controls' : { > > } > > in meson ? > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > + }, > > + # Mapping of vendor (pipeline handler) specific properties files > > + 'properties': > > + { > > + > > + } > > +} > > + > > control_headers = [] > > +vendor_ctrl_files = [] > > +vendor_prop_files = [] > > > > foreach header, mode : control_source_files > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > + > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > + # and vendor_prop_files. These will be cached for later use. > > + vendor_files = [] > > + foreach pipeline, file : vendor_controls_mapping[mode] > > + if pipeline not in pipelines > > + continue > > + endif > > + if file not in vendor_files > > + vendor_files += file > > + endif > > + endforeach > > + > > + if mode == 'controls' > > + vendor_ctrl_files = vendor_files > > + else > > + vendor_prop_files = vendor_files > > + endif > > + > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > + > > + foreach file : vendor_files > > + input_files += files('../../src/libcamera/' + file) > > + endforeach > > + > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > controls as first class citizens in the same manner, instead of > maintaing the "libcamera" ones on a different level ? > > I guess if we could do something like: > > ------------------------------------------------------------------------------- > > controls_map { > 'controls' : { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > 'rpi/pisp': 'control_ids_rpi.yaml', > 'rpi/vc4': 'control_ids_rpi.yaml' > } > > 'properties' = { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > } > } > > control_headers = [] > ranges_file = files('../../src/libcamera/control_ranges.yaml') > foreach mode, entry : controls_map > files_list = [] > input_files=[] > foreach vendor, header : entry > if vendor != 'libcamera' or vendor != 'draft' This should probably be an 'and' and not an 'or > if vendor not in pipelines > continue > endif > endif > > if header in files_list > continue > endif > files_list += header > > input_files += files('../../src/libcamera/' + header) > endforeach > > outfile='' > if mode == 'controls' > outfile = 'control_ids.h' > else > outfile = 'property_ids.h' > endif > > template = outfile + '.in' > control_headers += custom_target(header + '_h', > input : input_files, > output : outfile, > command : [gen_controls, '-o', '@OUTPUT@', > '--mode', mode, '-t', template, > '-r', ranges_file, '@INPUT@'], > install : true, > install_dir : libcamera_headers_install_dir) > endforeach > > libcamera_public_headers += control_headers > > ------------------------------------------------------------------------------- > > isn't it nicer ? > > The above code is valid in meson, but the meson.build in > src/libcamera/ should be updated accordingly. > > In general, I think the relevant part here is > > controls_map { > 'controls' : { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > 'rpi/pisp': 'control_ids_rpi.yaml', > 'rpi/vc4': 'control_ids_rpi.yaml' > } > > 'properties' = { > 'libcamera' : controls_ids.yaml, > 'drafts' : controls_ids_draft.yaml > } > } > > As seeing all the supported controls and properties listed together > might be easier to maintain and parse ? > > What do you think ? > > > > template_file = files(header + '.h.in') > > control_headers += custom_target(header + '_h', > > input : input_files, > > diff --git a/meson.build b/meson.build > > index e9a1c7e360ce..1423abf16c77 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > summary({ > > 'Enabled pipelines': pipelines, > > 'Enabled IPA modules': enabled_ipa_names, > > + 'Vendor controls': vendor_ctrl_files, > > + 'Vendor properties': vendor_prop_files, > > 'Hotplug support': libudev.found(), > > 'Tracing support': tracing_enabled, > > 'Android support': android_enabled, > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > new file mode 100644 > > index 000000000000..a6f8b43a5f62 > > --- /dev/null > > +++ b/src/libcamera/control_ids_rpi.yaml > > @@ -0,0 +1,16 @@ > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > +# > > +# Copyright (C) 2023, Raspberry Pi Ltd > > +# > > +%YAML 1.1 > > +--- > > +vendor: rpi > > +controls: > > + - PispConfigDumpFile: > > + type: string > > + description: | > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > + formatted dump of the Backend configuration to the filename given by the > > + value of the control. > > + > > +... > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -129,6 +129,18 @@ control_sources = [] > > > > foreach source, mode : control_source_files > > input_files = files(source +'.yaml') > > + > > + # Add the vendor specific files to the input. > > + if mode == 'controls' > > + vendor_files = vendor_ctrl_files > > + else > > + vendor_files = vendor_prop_files > > + endif > > + > > + foreach file : vendor_files > > + input_files += files(file) > > + endforeach > > + > > template_file = files(source + '.cpp.in') > > control_sources += custom_target(source + '_cpp', > > input : input_files, > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > index ea28f0139f23..e2ddad8581fd 100755 > > --- a/src/py/libcamera/gen-py-controls.py > > +++ b/src/py/libcamera/gen-py-controls.py > > @@ -91,7 +91,7 @@ def main(argv): > > parser = argparse.ArgumentParser() > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > help='Output file name. Defaults to standard output if not specified.') > > - parser.add_argument('input', type=str, > > + parser.add_argument('input', type=str, nargs='+', > > help='Input file name.') > > parser.add_argument('-t', dest='template', type=str, required=True, > > help='Template file name.') > > @@ -103,11 +103,11 @@ def main(argv): > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > return -1 > > > > - data = open(args.input, 'rb').read() > > - > > controls = {} > > - vendor = yaml.safe_load(data)['vendor'] > > - controls[vendor] = yaml.safe_load(data)['controls'] > > + for input in args.input: > > + data = open(input, 'rb').read() > > + vendor = yaml.safe_load(data)['vendor'] > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > data = generate_py(controls, args.mode) > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > --- a/src/py/libcamera/meson.build > > +++ b/src/py/libcamera/meson.build > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > gen_py_controls = files('gen-py-controls.py') > > > > +foreach file : vendor_ctrl_files > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > +endforeach > > + > > pycamera_sources += custom_target('py_gen_controls', > > input : gen_py_controls_input_files, > > output : ['py_controls_generated.cpp'], > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > +foreach file : vendor_prop_files > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > +endforeach > > + > > pycamera_sources += custom_target('py_gen_properties', > > input : gen_py_property_enums_input_files, > > output : ['py_properties_generated.cpp'], > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > index 8af33d29cd07..d7862142b8c1 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -359,7 +359,7 @@ def main(argv): > > parser = argparse.ArgumentParser() > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > help='Output file name. Defaults to standard output if not specified.') > > - parser.add_argument('input', type=str, > > + parser.add_argument('input', type=str, nargs='+', > > help='Input file name.') > > parser.add_argument('-t', dest='template', type=str, required=True, > > help='Template file name.') > > @@ -367,10 +367,12 @@ def main(argv): > > help='Mode of operation') > > args = parser.parse_args(argv[1:]) > > > > - data = open(args.input, 'rb').read() > > - vendor = yaml.safe_load(data)['vendor'] > > - controls = yaml.safe_load(data)['controls'] > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > + controls = [] > > + for input in args.input: > > + data = open(input, 'rb').read() > > + vendor = yaml.safe_load(data)['vendor'] > > + ctrls = yaml.safe_load(data)['controls'] > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > if args.template.endswith('.cpp.in'): > > data = generate_cpp(controls) > > -- > > 2.34.1 > >
Hi Jacopo, On Wed, 22 Nov 2023 at 16:05, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Wed, Nov 22, 2023 at 03:53:03PM +0000, Naushir Patuck wrote: > > Hi Jacopo, > > > > On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > > > Add support for using separate YAML files for vendor specific controls. > > > > This simplifies management of vendor control definitions and avoids > > > > possible merge conflicts when changing the control_ids.yaml file for > > > > core and draft controls. The mapping of vendor/pipeline handler to > > > > control file is done through the vendor_controls_mapping variable in > > > > include/libcamera/meson.build. > > > > > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > > > vendor controls. This contains a single control PispConfigDumpFile that > > > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > > > Backend configuration as a JSON file. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > > > meson.build | 2 ++ > > > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > > > src/libcamera/meson.build | 12 +++++++++ > > > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > > > src/py/libcamera/meson.build | 8 ++++++ > > > > utils/gen-controls.py | 12 +++++---- > > > > 7 files changed, 91 insertions(+), 11 deletions(-) > > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > > index 5fb772e6dd14..54123c5b858d 100644 > > > > --- a/include/libcamera/meson.build > > > > +++ b/include/libcamera/meson.build > > > > @@ -38,10 +38,50 @@ control_source_files = { > > > > 'property_ids': 'properties', > > > > } > > > > > > I really like the series, but this patch is the piece I like less. > > > > > > > > > > > +vendor_controls_mapping = { > > > > + # Mapping of vendor (pipeline handler) specific controls files > > > > + 'controls': > > > > + { > > > > > > Isn't this usually written as > > > > > > 'controls' : { > > > > > > } > > > > > > in meson ? > > > > > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > > > + }, > > > > + # Mapping of vendor (pipeline handler) specific properties files > > > > + 'properties': > > > > + { > > > > + > > > > + } > > > > +} > > > > + > > > > control_headers = [] > > > > +vendor_ctrl_files = [] > > > > +vendor_prop_files = [] > > > > > > > > foreach header, mode : control_source_files > > > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > > > + > > > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > > > + # and vendor_prop_files. These will be cached for later use. > > > > + vendor_files = [] > > > > + foreach pipeline, file : vendor_controls_mapping[mode] > > > > + if pipeline not in pipelines > > > > + continue > > > > + endif > > > > + if file not in vendor_files > > > > + vendor_files += file > > > > + endif > > > > + endforeach > > > > + > > > > + if mode == 'controls' > > > > + vendor_ctrl_files = vendor_files > > > > + else > > > > + vendor_prop_files = vendor_files > > > > + endif > > > > + > > > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > > > + > > > > + foreach file : vendor_files > > > > + input_files += files('../../src/libcamera/' + file) > > > > + endforeach > > > > + > > > > > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > > > controls as first class citizens in the same manner, instead of > > > maintaing the "libcamera" ones on a different level ? > > > > > > I guess if we could do something like: > > > > > > ------------------------------------------------------------------------------- > > > > > > controls_map { > > > 'controls' : { > > > 'libcamera' : controls_ids.yaml, > > > 'drafts' : controls_ids_draft.yaml > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > } > > > > > > 'properties' = { > > > 'libcamera' : controls_ids.yaml, > > > 'drafts' : controls_ids_draft.yaml > > > } > > > } > > > > > > control_headers = [] > > > ranges_file = files('../../src/libcamera/control_ranges.yaml') > > > foreach mode, entry : controls_map > > > files_list = [] > > > input_files=[] > > > foreach vendor, header : entry > > > if vendor != 'libcamera' or vendor != 'draft' > > > if vendor not in pipelines > > > continue > > > endif > > > endif > > > > > > if header in files_list > > > continue > > > endif > > > files_list += header > > > > > > input_files += files('../../src/libcamera/' + header) > > > endforeach > > > > > > outfile='' > > > if mode == 'controls' > > > outfile = 'control_ids.h' > > > else > > > outfile = 'property_ids.h' > > > endif > > > > > > template = outfile + '.in' > > > control_headers += custom_target(header + '_h', > > > input : input_files, > > > output : outfile, > > > command : [gen_controls, '-o', '@OUTPUT@', > > > '--mode', mode, '-t', template, > > > '-r', ranges_file, '@INPUT@'], > > > install : true, > > > install_dir : libcamera_headers_install_dir) > > > endforeach > > > > > > libcamera_public_headers += control_headers > > > > > > ------------------------------------------------------------------------------- > > > > > > isn't it nicer ? > > > > > > The above code is valid in meson, but the meson.build in > > > src/libcamera/ should be updated accordingly. > > > > > > In general, I think the relevant part here is > > > > > > controls_map { > > > 'controls' : { > > > 'libcamera' : controls_ids.yaml, > > > 'drafts' : controls_ids_draft.yaml > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > } > > > > > > 'properties' = { > > > 'libcamera' : controls_ids.yaml, > > > 'drafts' : controls_ids_draft.yaml > > > } > > > } > > > > > > As seeing all the supported controls and properties listed together > > > might be easier to maintain and parse ? > > > > > > What do you think ? > > > > That looks reasonable. I guess I wanted to keep things seperate for > > vendor controls, but no reason not to combine the libcamera/draft and > > vendor cases into one list. I can make this change for v3. > > Let me know if and how I can help. The change is quite invasive in the > whole patch series and I don't want to make it more painful than > necessary for you No worries, I've already made the changes. Should I wait for more feedback or go ahead and push a v3? Regards, Naush > > > > > > Regards, > > Naush > > > > > > > > > > > > > > template_file = files(header + '.h.in') > > > > control_headers += custom_target(header + '_h', > > > > input : input_files, > > > > diff --git a/meson.build b/meson.build > > > > index e9a1c7e360ce..1423abf16c77 100644 > > > > --- a/meson.build > > > > +++ b/meson.build > > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > > > summary({ > > > > 'Enabled pipelines': pipelines, > > > > 'Enabled IPA modules': enabled_ipa_names, > > > > + 'Vendor controls': vendor_ctrl_files, > > > > + 'Vendor properties': vendor_prop_files, > > > > 'Hotplug support': libudev.found(), > > > > 'Tracing support': tracing_enabled, > > > > 'Android support': android_enabled, > > > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > > > new file mode 100644 > > > > index 000000000000..a6f8b43a5f62 > > > > --- /dev/null > > > > +++ b/src/libcamera/control_ids_rpi.yaml > > > > @@ -0,0 +1,16 @@ > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > > +# > > > > +# Copyright (C) 2023, Raspberry Pi Ltd > > > > +# > > > > +%YAML 1.1 > > > > +--- > > > > +vendor: rpi > > > > +controls: > > > > + - PispConfigDumpFile: > > > > + type: string > > > > + description: | > > > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > > > + formatted dump of the Backend configuration to the filename given by the > > > > + value of the control. > > > > + > > > > +... > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > > > --- a/src/libcamera/meson.build > > > > +++ b/src/libcamera/meson.build > > > > @@ -129,6 +129,18 @@ control_sources = [] > > > > > > > > foreach source, mode : control_source_files > > > > input_files = files(source +'.yaml') > > > > + > > > > + # Add the vendor specific files to the input. > > > > + if mode == 'controls' > > > > + vendor_files = vendor_ctrl_files > > > > + else > > > > + vendor_files = vendor_prop_files > > > > + endif > > > > + > > > > + foreach file : vendor_files > > > > + input_files += files(file) > > > > + endforeach > > > > + > > > > template_file = files(source + '.cpp.in') > > > > control_sources += custom_target(source + '_cpp', > > > > input : input_files, > > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > > > index ea28f0139f23..e2ddad8581fd 100755 > > > > --- a/src/py/libcamera/gen-py-controls.py > > > > +++ b/src/py/libcamera/gen-py-controls.py > > > > @@ -91,7 +91,7 @@ def main(argv): > > > > parser = argparse.ArgumentParser() > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > help='Output file name. Defaults to standard output if not specified.') > > > > - parser.add_argument('input', type=str, > > > > + parser.add_argument('input', type=str, nargs='+', > > > > help='Input file name.') > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > help='Template file name.') > > > > @@ -103,11 +103,11 @@ def main(argv): > > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > > > return -1 > > > > > > > > - data = open(args.input, 'rb').read() > > > > - > > > > controls = {} > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > - controls[vendor] = yaml.safe_load(data)['controls'] > > > > + for input in args.input: > > > > + data = open(input, 'rb').read() > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > > data = generate_py(controls, args.mode) > > > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > > > --- a/src/py/libcamera/meson.build > > > > +++ b/src/py/libcamera/meson.build > > > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > > > > > gen_py_controls = files('gen-py-controls.py') > > > > > > > > +foreach file : vendor_ctrl_files > > > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > > > +endforeach > > > > + > > > > pycamera_sources += custom_target('py_gen_controls', > > > > input : gen_py_controls_input_files, > > > > output : ['py_controls_generated.cpp'], > > > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > > > > > +foreach file : vendor_prop_files > > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > > > +endforeach > > > > + > > > > pycamera_sources += custom_target('py_gen_properties', > > > > input : gen_py_property_enums_input_files, > > > > output : ['py_properties_generated.cpp'], > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > > > index 8af33d29cd07..d7862142b8c1 100755 > > > > --- a/utils/gen-controls.py > > > > +++ b/utils/gen-controls.py > > > > @@ -359,7 +359,7 @@ def main(argv): > > > > parser = argparse.ArgumentParser() > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > help='Output file name. Defaults to standard output if not specified.') > > > > - parser.add_argument('input', type=str, > > > > + parser.add_argument('input', type=str, nargs='+', > > > > help='Input file name.') > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > help='Template file name.') > > > > @@ -367,10 +367,12 @@ def main(argv): > > > > help='Mode of operation') > > > > args = parser.parse_args(argv[1:]) > > > > > > > > - data = open(args.input, 'rb').read() > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > - controls = yaml.safe_load(data)['controls'] > > > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > > + controls = [] > > > > + for input in args.input: > > > > + data = open(input, 'rb').read() > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > + ctrls = yaml.safe_load(data)['controls'] > > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > > > > > if args.template.endswith('.cpp.in'): > > > > data = generate_cpp(controls) > > > > -- > > > > 2.34.1 > > > >
Hello, On Thu, Nov 23, 2023 at 08:02:28AM +0000, Naushir Patuck via libcamera-devel wrote: > On Wed, 22 Nov 2023 at 16:05, Jacopo Mondi wrote: > > On Wed, Nov 22, 2023 at 03:53:03PM +0000, Naushir Patuck wrote: > > > On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi wrote: > > > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > > > > Add support for using separate YAML files for vendor specific controls. s/vendor specific/vendor-specific/ Same below where applicable. > > > > > This simplifies management of vendor control definitions and avoids > > > > > possible merge conflicts when changing the control_ids.yaml file for > > > > > core and draft controls. The mapping of vendor/pipeline handler to > > > > > control file is done through the vendor_controls_mapping variable in > > > > > include/libcamera/meson.build. > > > > > > > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > > > > vendor controls. This contains a single control PispConfigDumpFile that > > > > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > > > > Backend configuration as a JSON file. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > > > > meson.build | 2 ++ > > > > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > > > > src/libcamera/meson.build | 12 +++++++++ > > > > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > > > > src/py/libcamera/meson.build | 8 ++++++ > > > > > utils/gen-controls.py | 12 +++++---- > > > > > 7 files changed, 91 insertions(+), 11 deletions(-) > > > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > > > index 5fb772e6dd14..54123c5b858d 100644 > > > > > --- a/include/libcamera/meson.build > > > > > +++ b/include/libcamera/meson.build > > > > > @@ -38,10 +38,50 @@ control_source_files = { > > > > > 'property_ids': 'properties', > > > > > } > > > > > > > > I really like the series, but this patch is the piece I like less. > > > > > > > > > > > > > > +vendor_controls_mapping = { > > > > > + # Mapping of vendor (pipeline handler) specific controls files > > > > > + 'controls': > > > > > + { > > > > > > > > Isn't this usually written as > > > > > > > > 'controls' : { > > > > > > > > } > > > > > > > > in meson ? We seem to do that, yes. > > > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > + }, > > > > > + # Mapping of vendor (pipeline handler) specific properties files > > > > > + 'properties': > > > > > + { > > > > > + > > > > > + } > > > > > +} > > > > > + > > > > > control_headers = [] > > > > > +vendor_ctrl_files = [] > > > > > +vendor_prop_files = [] > > > > > > > > > > foreach header, mode : control_source_files > > > > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > > > > + > > > > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > > > > + # and vendor_prop_files. These will be cached for later use. > > > > > + vendor_files = [] > > > > > + foreach pipeline, file : vendor_controls_mapping[mode] > > > > > + if pipeline not in pipelines > > > > > + continue > > > > > + endif > > > > > + if file not in vendor_files > > > > > + vendor_files += file > > > > > + endif > > > > > + endforeach > > > > > + > > > > > + if mode == 'controls' > > > > > + vendor_ctrl_files = vendor_files > > > > > + else > > > > > + vendor_prop_files = vendor_files > > > > > + endif > > > > > + > > > > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > > > > + > > > > > + foreach file : vendor_files > > > > > + input_files += files('../../src/libcamera/' + file) > > > > > + endforeach > > > > > + > > > > > > > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > > > > controls as first class citizens in the same manner, instead of > > > > maintaing the "libcamera" ones on a different level ? > > > > > > > > I guess if we could do something like: > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > controls_map { > > > > 'controls' : { > > > > 'libcamera' : controls_ids.yaml, > > > > 'drafts' : controls_ids_draft.yaml > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > } > > > > > > > > 'properties' = { > > > > 'libcamera' : controls_ids.yaml, This should be property_ids.yaml. > > > > 'drafts' : controls_ids_draft.yaml > > > > } > > > > } > > > > > > > > control_headers = [] > > > > ranges_file = files('../../src/libcamera/control_ranges.yaml') > > > > foreach mode, entry : controls_map > > > > files_list = [] > > > > input_files=[] > > > > foreach vendor, header : entry > > > > if vendor != 'libcamera' or vendor != 'draft' > > > > if vendor not in pipelines > > > > continue > > > > endif > > > > endif > > > > > > > > if header in files_list > > > > continue > > > > endif > > > > files_list += header > > > > > > > > input_files += files('../../src/libcamera/' + header) > > > > endforeach > > > > > > > > outfile='' > > > > if mode == 'controls' > > > > outfile = 'control_ids.h' > > > > else > > > > outfile = 'property_ids.h' > > > > endif > > > > > > > > template = outfile + '.in' > > > > control_headers += custom_target(header + '_h', > > > > input : input_files, > > > > output : outfile, > > > > command : [gen_controls, '-o', '@OUTPUT@', > > > > '--mode', mode, '-t', template, > > > > '-r', ranges_file, '@INPUT@'], > > > > install : true, > > > > install_dir : libcamera_headers_install_dir) > > > > endforeach > > > > > > > > libcamera_public_headers += control_headers > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > isn't it nicer ? > > > > > > > > The above code is valid in meson, but the meson.build in > > > > src/libcamera/ should be updated accordingly. > > > > > > > > In general, I think the relevant part here is > > > > > > > > controls_map { > > > > 'controls' : { > > > > 'libcamera' : controls_ids.yaml, > > > > 'drafts' : controls_ids_draft.yaml > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > } > > > > > > > > 'properties' = { > > > > 'libcamera' : controls_ids.yaml, > > > > 'drafts' : controls_ids_draft.yaml > > > > } > > > > } > > > > > > > > As seeing all the supported controls and properties listed together > > > > might be easier to maintain and parse ? > > > > > > > > What do you think ? > > > > > > That looks reasonable. I guess I wanted to keep things seperate for > > > vendor controls, but no reason not to combine the libcamera/draft and > > > vendor cases into one list. I can make this change for v3. That seems fine to me too. If we go in that direction, should we replace 'libcamera' with 'core', and name the file controls_ids_core.yaml ? All controls are libcamera controls. > > Let me know if and how I can help. The change is quite invasive in the > > whole patch series and I don't want to make it more painful than > > necessary for you > > No worries, I've already made the changes. Should I wait for more > feedback or go ahead and push a v3? > > > > > > template_file = files(header + '.h.in') > > > > > control_headers += custom_target(header + '_h', > > > > > input : input_files, > > > > > diff --git a/meson.build b/meson.build > > > > > index e9a1c7e360ce..1423abf16c77 100644 > > > > > --- a/meson.build > > > > > +++ b/meson.build > > > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > > > > summary({ > > > > > 'Enabled pipelines': pipelines, > > > > > 'Enabled IPA modules': enabled_ipa_names, > > > > > + 'Vendor controls': vendor_ctrl_files, > > > > > + 'Vendor properties': vendor_prop_files, > > > > > 'Hotplug support': libudev.found(), > > > > > 'Tracing support': tracing_enabled, > > > > > 'Android support': android_enabled, > > > > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > > > > new file mode 100644 > > > > > index 000000000000..a6f8b43a5f62 > > > > > --- /dev/null > > > > > +++ b/src/libcamera/control_ids_rpi.yaml > > > > > @@ -0,0 +1,16 @@ > > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > > > +# > > > > > +# Copyright (C) 2023, Raspberry Pi Ltd > > > > > +# > > > > > +%YAML 1.1 > > > > > +--- > > > > > +vendor: rpi > > > > > +controls: > > > > > + - PispConfigDumpFile: > > > > > + type: string > > > > > + description: | > > > > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > > > > + formatted dump of the Backend configuration to the filename given by the > > > > > + value of the control. As the series doesn't contain an implementation of this control I can't really review the description properly, but that's fine, I understand it will come later. > > > > > + > > > > > +... > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > > > > --- a/src/libcamera/meson.build > > > > > +++ b/src/libcamera/meson.build > > > > > @@ -129,6 +129,18 @@ control_sources = [] > > > > > > > > > > foreach source, mode : control_source_files > > > > > input_files = files(source +'.yaml') > > > > > + > > > > > + # Add the vendor specific files to the input. > > > > > + if mode == 'controls' > > > > > + vendor_files = vendor_ctrl_files > > > > > + else > > > > > + vendor_files = vendor_prop_files > > > > > + endif > > > > > + > > > > > + foreach file : vendor_files > > > > > + input_files += files(file) > > > > > + endforeach > > > > > + > > > > > template_file = files(source + '.cpp.in') > > > > > control_sources += custom_target(source + '_cpp', > > > > > input : input_files, > > > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > > > > index ea28f0139f23..e2ddad8581fd 100755 > > > > > --- a/src/py/libcamera/gen-py-controls.py > > > > > +++ b/src/py/libcamera/gen-py-controls.py > > > > > @@ -91,7 +91,7 @@ def main(argv): > > > > > parser = argparse.ArgumentParser() > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > - parser.add_argument('input', type=str, > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > help='Input file name.') > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > help='Template file name.') > > > > > @@ -103,11 +103,11 @@ def main(argv): > > > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > > > > return -1 > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > - > > > > > controls = {} > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > - controls[vendor] = yaml.safe_load(data)['controls'] > > > > > + for input in args.input: > > > > > + data = open(input, 'rb').read() > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > > > > data = generate_py(controls, args.mode) > > > > > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > > > > --- a/src/py/libcamera/meson.build > > > > > +++ b/src/py/libcamera/meson.build > > > > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > > > > > > > gen_py_controls = files('gen-py-controls.py') > > > > > > > > > > +foreach file : vendor_ctrl_files > > > > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > > > > +endforeach > > > > > + > > > > > pycamera_sources += custom_target('py_gen_controls', > > > > > input : gen_py_controls_input_files, > > > > > output : ['py_controls_generated.cpp'], > > > > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > > > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > > > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > > > > > > > +foreach file : vendor_prop_files > > > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > > > > +endforeach > > > > > + > > > > > pycamera_sources += custom_target('py_gen_properties', > > > > > input : gen_py_property_enums_input_files, > > > > > output : ['py_properties_generated.cpp'], > > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > > > > index 8af33d29cd07..d7862142b8c1 100755 > > > > > --- a/utils/gen-controls.py > > > > > +++ b/utils/gen-controls.py > > > > > @@ -359,7 +359,7 @@ def main(argv): > > > > > parser = argparse.ArgumentParser() > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > - parser.add_argument('input', type=str, > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > help='Input file name.') > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > help='Template file name.') > > > > > @@ -367,10 +367,12 @@ def main(argv): > > > > > help='Mode of operation') > > > > > args = parser.parse_args(argv[1:]) > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > - controls = yaml.safe_load(data)['controls'] > > > > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > > > + controls = [] > > > > > + for input in args.input: > > > > > + data = open(input, 'rb').read() > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > + ctrls = yaml.safe_load(data)['controls'] > > > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > > > > > > > if args.template.endswith('.cpp.in'): > > > > > data = generate_cpp(controls)
Hi Laurent, Thank you for your feedback! On Thu, 23 Nov 2023 at 12:13, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Thu, Nov 23, 2023 at 08:02:28AM +0000, Naushir Patuck via libcamera-devel wrote: > > On Wed, 22 Nov 2023 at 16:05, Jacopo Mondi wrote: > > > On Wed, Nov 22, 2023 at 03:53:03PM +0000, Naushir Patuck wrote: > > > > On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi wrote: > > > > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > > > > > Add support for using separate YAML files for vendor specific controls. > > s/vendor specific/vendor-specific/ > > Same below where applicable. > > > > > > > This simplifies management of vendor control definitions and avoids > > > > > > possible merge conflicts when changing the control_ids.yaml file for > > > > > > core and draft controls. The mapping of vendor/pipeline handler to > > > > > > control file is done through the vendor_controls_mapping variable in > > > > > > include/libcamera/meson.build. > > > > > > > > > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > > > > > vendor controls. This contains a single control PispConfigDumpFile that > > > > > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > > > > > Backend configuration as a JSON file. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > > > > > meson.build | 2 ++ > > > > > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > > > > > src/libcamera/meson.build | 12 +++++++++ > > > > > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > > > > > src/py/libcamera/meson.build | 8 ++++++ > > > > > > utils/gen-controls.py | 12 +++++---- > > > > > > 7 files changed, 91 insertions(+), 11 deletions(-) > > > > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > > > > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > > > > index 5fb772e6dd14..54123c5b858d 100644 > > > > > > --- a/include/libcamera/meson.build > > > > > > +++ b/include/libcamera/meson.build > > > > > > @@ -38,10 +38,50 @@ control_source_files = { > > > > > > 'property_ids': 'properties', > > > > > > } > > > > > > > > > > I really like the series, but this patch is the piece I like less. > > > > > > > > > > > > > > > > > +vendor_controls_mapping = { > > > > > > + # Mapping of vendor (pipeline handler) specific controls files > > > > > > + 'controls': > > > > > > + { > > > > > > > > > > Isn't this usually written as > > > > > > > > > > 'controls' : { > > > > > > > > > > } > > > > > > > > > > in meson ? > > We seem to do that, yes. I'll change that to the above convention. > > > > > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > + }, > > > > > > + # Mapping of vendor (pipeline handler) specific properties files > > > > > > + 'properties': > > > > > > + { > > > > > > + > > > > > > + } > > > > > > +} > > > > > > + > > > > > > control_headers = [] > > > > > > +vendor_ctrl_files = [] > > > > > > +vendor_prop_files = [] > > > > > > > > > > > > foreach header, mode : control_source_files > > > > > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > > > > > + > > > > > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > > > > > + # and vendor_prop_files. These will be cached for later use. > > > > > > + vendor_files = [] > > > > > > + foreach pipeline, file : vendor_controls_mapping[mode] > > > > > > + if pipeline not in pipelines > > > > > > + continue > > > > > > + endif > > > > > > + if file not in vendor_files > > > > > > + vendor_files += file > > > > > > + endif > > > > > > + endforeach > > > > > > + > > > > > > + if mode == 'controls' > > > > > > + vendor_ctrl_files = vendor_files > > > > > > + else > > > > > > + vendor_prop_files = vendor_files > > > > > > + endif > > > > > > + > > > > > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > > > > > + > > > > > > + foreach file : vendor_files > > > > > > + input_files += files('../../src/libcamera/' + file) > > > > > > + endforeach > > > > > > + > > > > > > > > > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > > > > > controls as first class citizens in the same manner, instead of > > > > > maintaing the "libcamera" ones on a different level ? > > > > > > > > > > I guess if we could do something like: > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > controls_map { > > > > > 'controls' : { > > > > > 'libcamera' : controls_ids.yaml, > > > > > 'drafts' : controls_ids_draft.yaml > > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > } > > > > > > > > > > 'properties' = { > > > > > 'libcamera' : controls_ids.yaml, > > This should be property_ids.yaml. > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > } > > > > > } > > > > > > > > > > control_headers = [] > > > > > ranges_file = files('../../src/libcamera/control_ranges.yaml') > > > > > foreach mode, entry : controls_map > > > > > files_list = [] > > > > > input_files=[] > > > > > foreach vendor, header : entry > > > > > if vendor != 'libcamera' or vendor != 'draft' > > > > > if vendor not in pipelines > > > > > continue > > > > > endif > > > > > endif > > > > > > > > > > if header in files_list > > > > > continue > > > > > endif > > > > > files_list += header > > > > > > > > > > input_files += files('../../src/libcamera/' + header) > > > > > endforeach > > > > > > > > > > outfile='' > > > > > if mode == 'controls' > > > > > outfile = 'control_ids.h' > > > > > else > > > > > outfile = 'property_ids.h' > > > > > endif > > > > > > > > > > template = outfile + '.in' > > > > > control_headers += custom_target(header + '_h', > > > > > input : input_files, > > > > > output : outfile, > > > > > command : [gen_controls, '-o', '@OUTPUT@', > > > > > '--mode', mode, '-t', template, > > > > > '-r', ranges_file, '@INPUT@'], > > > > > install : true, > > > > > install_dir : libcamera_headers_install_dir) > > > > > endforeach > > > > > > > > > > libcamera_public_headers += control_headers > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > isn't it nicer ? > > > > > > > > > > The above code is valid in meson, but the meson.build in > > > > > src/libcamera/ should be updated accordingly. > > > > > > > > > > In general, I think the relevant part here is > > > > > > > > > > controls_map { > > > > > 'controls' : { > > > > > 'libcamera' : controls_ids.yaml, > > > > > 'drafts' : controls_ids_draft.yaml > > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > } > > > > > > > > > > 'properties' = { > > > > > 'libcamera' : controls_ids.yaml, > > > > > 'drafts' : controls_ids_draft.yaml > > > > > } > > > > > } > > > > > > > > > > As seeing all the supported controls and properties listed together > > > > > might be easier to maintain and parse ? > > > > > > > > > > What do you think ? > > > > > > > > That looks reasonable. I guess I wanted to keep things seperate for > > > > vendor controls, but no reason not to combine the libcamera/draft and > > > > vendor cases into one list. I can make this change for v3. > > That seems fine to me too. If we go in that direction, should we replace > 'libcamera' with 'core', and name the file controls_ids_core.yaml ? All > controls are libcamera controls. Yes, that sounds like a good change to introduce. > > > > Let me know if and how I can help. The change is quite invasive in the > > > whole patch series and I don't want to make it more painful than > > > necessary for you > > > > No worries, I've already made the changes. Should I wait for more > > feedback or go ahead and push a v3? > > > > > > > > template_file = files(header + '.h.in') > > > > > > control_headers += custom_target(header + '_h', > > > > > > input : input_files, > > > > > > diff --git a/meson.build b/meson.build > > > > > > index e9a1c7e360ce..1423abf16c77 100644 > > > > > > --- a/meson.build > > > > > > +++ b/meson.build > > > > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > > > > > summary({ > > > > > > 'Enabled pipelines': pipelines, > > > > > > 'Enabled IPA modules': enabled_ipa_names, > > > > > > + 'Vendor controls': vendor_ctrl_files, > > > > > > + 'Vendor properties': vendor_prop_files, > > > > > > 'Hotplug support': libudev.found(), > > > > > > 'Tracing support': tracing_enabled, > > > > > > 'Android support': android_enabled, > > > > > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > > > > > new file mode 100644 > > > > > > index 000000000000..a6f8b43a5f62 > > > > > > --- /dev/null > > > > > > +++ b/src/libcamera/control_ids_rpi.yaml > > > > > > @@ -0,0 +1,16 @@ > > > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > > > > +# > > > > > > +# Copyright (C) 2023, Raspberry Pi Ltd > > > > > > +# > > > > > > +%YAML 1.1 > > > > > > +--- > > > > > > +vendor: rpi > > > > > > +controls: > > > > > > + - PispConfigDumpFile: > > > > > > + type: string > > > > > > + description: | > > > > > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > > > > > + formatted dump of the Backend configuration to the filename given by the > > > > > > + value of the control. > > As the series doesn't contain an implementation of this control I can't > really review the description properly, but that's fine, I understand it > will come later. In v3, I'll remove the addition of the rpi specific vendor control and move into a separate commit. We can make a case for delaying merging of that commit until the Pi 5 pipeline handler gets merged. Regards, Naush > > > > > > > + > > > > > > +... > > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > > > > > --- a/src/libcamera/meson.build > > > > > > +++ b/src/libcamera/meson.build > > > > > > @@ -129,6 +129,18 @@ control_sources = [] > > > > > > > > > > > > foreach source, mode : control_source_files > > > > > > input_files = files(source +'.yaml') > > > > > > + > > > > > > + # Add the vendor specific files to the input. > > > > > > + if mode == 'controls' > > > > > > + vendor_files = vendor_ctrl_files > > > > > > + else > > > > > > + vendor_files = vendor_prop_files > > > > > > + endif > > > > > > + > > > > > > + foreach file : vendor_files > > > > > > + input_files += files(file) > > > > > > + endforeach > > > > > > + > > > > > > template_file = files(source + '.cpp.in') > > > > > > control_sources += custom_target(source + '_cpp', > > > > > > input : input_files, > > > > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > > > > > index ea28f0139f23..e2ddad8581fd 100755 > > > > > > --- a/src/py/libcamera/gen-py-controls.py > > > > > > +++ b/src/py/libcamera/gen-py-controls.py > > > > > > @@ -91,7 +91,7 @@ def main(argv): > > > > > > parser = argparse.ArgumentParser() > > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > > - parser.add_argument('input', type=str, > > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > > help='Input file name.') > > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > > help='Template file name.') > > > > > > @@ -103,11 +103,11 @@ def main(argv): > > > > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > > > > > return -1 > > > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > > - > > > > > > controls = {} > > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > > - controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > + for input in args.input: > > > > > > + data = open(input, 'rb').read() > > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > > > > > > data = generate_py(controls, args.mode) > > > > > > > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > > > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > > > > > --- a/src/py/libcamera/meson.build > > > > > > +++ b/src/py/libcamera/meson.build > > > > > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > > > > > > > > > gen_py_controls = files('gen-py-controls.py') > > > > > > > > > > > > +foreach file : vendor_ctrl_files > > > > > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > > > > > +endforeach > > > > > > + > > > > > > pycamera_sources += custom_target('py_gen_controls', > > > > > > input : gen_py_controls_input_files, > > > > > > output : ['py_controls_generated.cpp'], > > > > > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > > > > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > > > > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > > > > > > > > > +foreach file : vendor_prop_files > > > > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > > > > > +endforeach > > > > > > + > > > > > > pycamera_sources += custom_target('py_gen_properties', > > > > > > input : gen_py_property_enums_input_files, > > > > > > output : ['py_properties_generated.cpp'], > > > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > > > > > index 8af33d29cd07..d7862142b8c1 100755 > > > > > > --- a/utils/gen-controls.py > > > > > > +++ b/utils/gen-controls.py > > > > > > @@ -359,7 +359,7 @@ def main(argv): > > > > > > parser = argparse.ArgumentParser() > > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > > - parser.add_argument('input', type=str, > > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > > help='Input file name.') > > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > > help='Template file name.') > > > > > > @@ -367,10 +367,12 @@ def main(argv): > > > > > > help='Mode of operation') > > > > > > args = parser.parse_args(argv[1:]) > > > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > > - controls = yaml.safe_load(data)['controls'] > > > > > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > > > > + controls = [] > > > > > > + for input in args.input: > > > > > > + data = open(input, 'rb').read() > > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > > + ctrls = yaml.safe_load(data)['controls'] > > > > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > > > > > > > > > if args.template.endswith('.cpp.in'): > > > > > > data = generate_cpp(controls) > > -- > Regards, > > Laurent Pinchart
On Thu, 23 Nov 2023 at 12:57, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Laurent, > > Thank you for your feedback! > > On Thu, 23 Nov 2023 at 12:13, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hello, > > > > On Thu, Nov 23, 2023 at 08:02:28AM +0000, Naushir Patuck via libcamera-devel wrote: > > > On Wed, 22 Nov 2023 at 16:05, Jacopo Mondi wrote: > > > > On Wed, Nov 22, 2023 at 03:53:03PM +0000, Naushir Patuck wrote: > > > > > On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi wrote: > > > > > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > > > > > > Add support for using separate YAML files for vendor specific controls. > > > > s/vendor specific/vendor-specific/ > > > > Same below where applicable. > > > > > > > > > This simplifies management of vendor control definitions and avoids > > > > > > > possible merge conflicts when changing the control_ids.yaml file for > > > > > > > core and draft controls. The mapping of vendor/pipeline handler to > > > > > > > control file is done through the vendor_controls_mapping variable in > > > > > > > include/libcamera/meson.build. > > > > > > > > > > > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > > > > > > vendor controls. This contains a single control PispConfigDumpFile that > > > > > > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > > > > > > Backend configuration as a JSON file. > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > --- > > > > > > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > > > > > > meson.build | 2 ++ > > > > > > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > > > > > > src/libcamera/meson.build | 12 +++++++++ > > > > > > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > > > > > > src/py/libcamera/meson.build | 8 ++++++ > > > > > > > utils/gen-controls.py | 12 +++++---- > > > > > > > 7 files changed, 91 insertions(+), 11 deletions(-) > > > > > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > > > > > > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > > > > > index 5fb772e6dd14..54123c5b858d 100644 > > > > > > > --- a/include/libcamera/meson.build > > > > > > > +++ b/include/libcamera/meson.build > > > > > > > @@ -38,10 +38,50 @@ control_source_files = { > > > > > > > 'property_ids': 'properties', > > > > > > > } > > > > > > > > > > > > I really like the series, but this patch is the piece I like less. > > > > > > > > > > > > > > > > > > > > +vendor_controls_mapping = { > > > > > > > + # Mapping of vendor (pipeline handler) specific controls files > > > > > > > + 'controls': > > > > > > > + { > > > > > > > > > > > > Isn't this usually written as > > > > > > > > > > > > 'controls' : { > > > > > > > > > > > > } > > > > > > > > > > > > in meson ? > > > > We seem to do that, yes. > > I'll change that to the above convention. > > > > > > > > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > > + }, > > > > > > > + # Mapping of vendor (pipeline handler) specific properties files > > > > > > > + 'properties': > > > > > > > + { > > > > > > > + > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > control_headers = [] > > > > > > > +vendor_ctrl_files = [] > > > > > > > +vendor_prop_files = [] > > > > > > > > > > > > > > foreach header, mode : control_source_files > > > > > > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > > > > > > + > > > > > > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > > > > > > + # and vendor_prop_files. These will be cached for later use. > > > > > > > + vendor_files = [] > > > > > > > + foreach pipeline, file : vendor_controls_mapping[mode] > > > > > > > + if pipeline not in pipelines > > > > > > > + continue > > > > > > > + endif > > > > > > > + if file not in vendor_files > > > > > > > + vendor_files += file > > > > > > > + endif > > > > > > > + endforeach > > > > > > > + > > > > > > > + if mode == 'controls' > > > > > > > + vendor_ctrl_files = vendor_files > > > > > > > + else > > > > > > > + vendor_prop_files = vendor_files > > > > > > > + endif > > > > > > > + > > > > > > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > > > > > > + > > > > > > > + foreach file : vendor_files > > > > > > > + input_files += files('../../src/libcamera/' + file) > > > > > > > + endforeach > > > > > > > + > > > > > > > > > > > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > > > > > > controls as first class citizens in the same manner, instead of > > > > > > maintaing the "libcamera" ones on a different level ? > > > > > > > > > > > > I guess if we could do something like: > > > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > > > controls_map { > > > > > > 'controls' : { > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > } > > > > > > > > > > > > 'properties' = { > > > > > > 'libcamera' : controls_ids.yaml, > > > > This should be property_ids.yaml. > > > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > } > > > > > > } > > > > > > > > > > > > control_headers = [] > > > > > > ranges_file = files('../../src/libcamera/control_ranges.yaml') > > > > > > foreach mode, entry : controls_map > > > > > > files_list = [] > > > > > > input_files=[] > > > > > > foreach vendor, header : entry > > > > > > if vendor != 'libcamera' or vendor != 'draft' > > > > > > if vendor not in pipelines > > > > > > continue > > > > > > endif > > > > > > endif > > > > > > > > > > > > if header in files_list > > > > > > continue > > > > > > endif > > > > > > files_list += header > > > > > > > > > > > > input_files += files('../../src/libcamera/' + header) > > > > > > endforeach > > > > > > > > > > > > outfile='' > > > > > > if mode == 'controls' > > > > > > outfile = 'control_ids.h' > > > > > > else > > > > > > outfile = 'property_ids.h' > > > > > > endif > > > > > > > > > > > > template = outfile + '.in' > > > > > > control_headers += custom_target(header + '_h', > > > > > > input : input_files, > > > > > > output : outfile, > > > > > > command : [gen_controls, '-o', '@OUTPUT@', > > > > > > '--mode', mode, '-t', template, > > > > > > '-r', ranges_file, '@INPUT@'], > > > > > > install : true, > > > > > > install_dir : libcamera_headers_install_dir) > > > > > > endforeach > > > > > > > > > > > > libcamera_public_headers += control_headers > > > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > > > isn't it nicer ? > > > > > > > > > > > > The above code is valid in meson, but the meson.build in > > > > > > src/libcamera/ should be updated accordingly. > > > > > > > > > > > > In general, I think the relevant part here is > > > > > > > > > > > > controls_map { > > > > > > 'controls' : { > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > } > > > > > > > > > > > > 'properties' = { > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > } > > > > > > } > > > > > > > > > > > > As seeing all the supported controls and properties listed together > > > > > > might be easier to maintain and parse ? > > > > > > > > > > > > What do you think ? > > > > > > > > > > That looks reasonable. I guess I wanted to keep things seperate for > > > > > vendor controls, but no reason not to combine the libcamera/draft and > > > > > vendor cases into one list. I can make this change for v3. > > > > That seems fine to me too. If we go in that direction, should we replace > > 'libcamera' with 'core', and name the file controls_ids_core.yaml ? All > > controls are libcamera controls. > > Yes, that sounds like a good change to introduce. On this change, if I rename 'libcamera' to 'core' above, I should really do the same in the gen-controls.py scripts as well for consistency (even though the 2 labels are unrelated). I'lll do that change unless folks disagree with this. > > > > > > > Let me know if and how I can help. The change is quite invasive in the > > > > whole patch series and I don't want to make it more painful than > > > > necessary for you > > > > > > No worries, I've already made the changes. Should I wait for more > > > feedback or go ahead and push a v3? > > > > > > > > > > template_file = files(header + '.h.in') > > > > > > > control_headers += custom_target(header + '_h', > > > > > > > input : input_files, > > > > > > > diff --git a/meson.build b/meson.build > > > > > > > index e9a1c7e360ce..1423abf16c77 100644 > > > > > > > --- a/meson.build > > > > > > > +++ b/meson.build > > > > > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > > > > > > summary({ > > > > > > > 'Enabled pipelines': pipelines, > > > > > > > 'Enabled IPA modules': enabled_ipa_names, > > > > > > > + 'Vendor controls': vendor_ctrl_files, > > > > > > > + 'Vendor properties': vendor_prop_files, > > > > > > > 'Hotplug support': libudev.found(), > > > > > > > 'Tracing support': tracing_enabled, > > > > > > > 'Android support': android_enabled, > > > > > > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > > > > > > new file mode 100644 > > > > > > > index 000000000000..a6f8b43a5f62 > > > > > > > --- /dev/null > > > > > > > +++ b/src/libcamera/control_ids_rpi.yaml > > > > > > > @@ -0,0 +1,16 @@ > > > > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > > > > > +# > > > > > > > +# Copyright (C) 2023, Raspberry Pi Ltd > > > > > > > +# > > > > > > > +%YAML 1.1 > > > > > > > +--- > > > > > > > +vendor: rpi > > > > > > > +controls: > > > > > > > + - PispConfigDumpFile: > > > > > > > + type: string > > > > > > > + description: | > > > > > > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > > > > > > + formatted dump of the Backend configuration to the filename given by the > > > > > > > + value of the control. > > > > As the series doesn't contain an implementation of this control I can't > > really review the description properly, but that's fine, I understand it > > will come later. > > In v3, I'll remove the addition of the rpi specific vendor control and > move into a separate commit. We can make a case for delaying merging > of that commit until the Pi 5 pipeline handler gets merged. > > Regards, > Naush > > > > > > > > > > > + > > > > > > > +... > > > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > > > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > > > > > > --- a/src/libcamera/meson.build > > > > > > > +++ b/src/libcamera/meson.build > > > > > > > @@ -129,6 +129,18 @@ control_sources = [] > > > > > > > > > > > > > > foreach source, mode : control_source_files > > > > > > > input_files = files(source +'.yaml') > > > > > > > + > > > > > > > + # Add the vendor specific files to the input. > > > > > > > + if mode == 'controls' > > > > > > > + vendor_files = vendor_ctrl_files > > > > > > > + else > > > > > > > + vendor_files = vendor_prop_files > > > > > > > + endif > > > > > > > + > > > > > > > + foreach file : vendor_files > > > > > > > + input_files += files(file) > > > > > > > + endforeach > > > > > > > + > > > > > > > template_file = files(source + '.cpp.in') > > > > > > > control_sources += custom_target(source + '_cpp', > > > > > > > input : input_files, > > > > > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > > > > > > index ea28f0139f23..e2ddad8581fd 100755 > > > > > > > --- a/src/py/libcamera/gen-py-controls.py > > > > > > > +++ b/src/py/libcamera/gen-py-controls.py > > > > > > > @@ -91,7 +91,7 @@ def main(argv): > > > > > > > parser = argparse.ArgumentParser() > > > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > > > - parser.add_argument('input', type=str, > > > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > > > help='Input file name.') > > > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > > > help='Template file name.') > > > > > > > @@ -103,11 +103,11 @@ def main(argv): > > > > > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > > > > > > return -1 > > > > > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > > > - > > > > > > > controls = {} > > > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > > > - controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > + for input in args.input: > > > > > > > + data = open(input, 'rb').read() > > > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > > > > > > > > data = generate_py(controls, args.mode) > > > > > > > > > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > > > > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > > > > > > --- a/src/py/libcamera/meson.build > > > > > > > +++ b/src/py/libcamera/meson.build > > > > > > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > > > > > > > > > > > gen_py_controls = files('gen-py-controls.py') > > > > > > > > > > > > > > +foreach file : vendor_ctrl_files > > > > > > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > > > > > > +endforeach > > > > > > > + > > > > > > > pycamera_sources += custom_target('py_gen_controls', > > > > > > > input : gen_py_controls_input_files, > > > > > > > output : ['py_controls_generated.cpp'], > > > > > > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > > > > > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > > > > > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > > > > > > > > > > > +foreach file : vendor_prop_files > > > > > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > > > > > > +endforeach > > > > > > > + > > > > > > > pycamera_sources += custom_target('py_gen_properties', > > > > > > > input : gen_py_property_enums_input_files, > > > > > > > output : ['py_properties_generated.cpp'], > > > > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > > > > > > index 8af33d29cd07..d7862142b8c1 100755 > > > > > > > --- a/utils/gen-controls.py > > > > > > > +++ b/utils/gen-controls.py > > > > > > > @@ -359,7 +359,7 @@ def main(argv): > > > > > > > parser = argparse.ArgumentParser() > > > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > > > - parser.add_argument('input', type=str, > > > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > > > help='Input file name.') > > > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > > > help='Template file name.') > > > > > > > @@ -367,10 +367,12 @@ def main(argv): > > > > > > > help='Mode of operation') > > > > > > > args = parser.parse_args(argv[1:]) > > > > > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > > > - controls = yaml.safe_load(data)['controls'] > > > > > > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > > > > > + controls = [] > > > > > > > + for input in args.input: > > > > > > > + data = open(input, 'rb').read() > > > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > > > + ctrls = yaml.safe_load(data)['controls'] > > > > > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > > > > > > > > > > > if args.template.endswith('.cpp.in'): > > > > > > > data = generate_cpp(controls) > > > > -- > > Regards, > > > > Laurent Pinchart
On Thu, 23 Nov 2023 at 13:59, Naushir Patuck <naush@raspberrypi.com> wrote: > > On Thu, 23 Nov 2023 at 12:57, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Hi Laurent, > > > > Thank you for your feedback! > > > > On Thu, 23 Nov 2023 at 12:13, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hello, > > > > > > On Thu, Nov 23, 2023 at 08:02:28AM +0000, Naushir Patuck via libcamera-devel wrote: > > > > On Wed, 22 Nov 2023 at 16:05, Jacopo Mondi wrote: > > > > > On Wed, Nov 22, 2023 at 03:53:03PM +0000, Naushir Patuck wrote: > > > > > > On Wed, 22 Nov 2023 at 15:24, Jacopo Mondi wrote: > > > > > > > On Tue, Nov 21, 2023 at 02:53:47PM +0000, Naushir Patuck via libcamera-devel wrote: > > > > > > > > Add support for using separate YAML files for vendor specific controls. > > > > > > s/vendor specific/vendor-specific/ > > > > > > Same below where applicable. > > > > > > > > > > > This simplifies management of vendor control definitions and avoids > > > > > > > > possible merge conflicts when changing the control_ids.yaml file for > > > > > > > > core and draft controls. The mapping of vendor/pipeline handler to > > > > > > > > control file is done through the vendor_controls_mapping variable in > > > > > > > > include/libcamera/meson.build. > > > > > > > > > > > > > > > > Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific > > > > > > > > vendor controls. This contains a single control PispConfigDumpFile that > > > > > > > > will be used in the Pi 5 pipeline handler as a trigger to dump the > > > > > > > > Backend configuration as a JSON file. > > > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > --- > > > > > > > > include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- > > > > > > > > meson.build | 2 ++ > > > > > > > > src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ > > > > > > > > src/libcamera/meson.build | 12 +++++++++ > > > > > > > > src/py/libcamera/gen-py-controls.py | 10 +++---- > > > > > > > > src/py/libcamera/meson.build | 8 ++++++ > > > > > > > > utils/gen-controls.py | 12 +++++---- > > > > > > > > 7 files changed, 91 insertions(+), 11 deletions(-) > > > > > > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > > > > > > > > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > > > > > > index 5fb772e6dd14..54123c5b858d 100644 > > > > > > > > --- a/include/libcamera/meson.build > > > > > > > > +++ b/include/libcamera/meson.build > > > > > > > > @@ -38,10 +38,50 @@ control_source_files = { > > > > > > > > 'property_ids': 'properties', > > > > > > > > } > > > > > > > > > > > > > > I really like the series, but this patch is the piece I like less. > > > > > > > > > > > > > > > > > > > > > > > +vendor_controls_mapping = { > > > > > > > > + # Mapping of vendor (pipeline handler) specific controls files > > > > > > > > + 'controls': > > > > > > > > + { > > > > > > > > > > > > > > Isn't this usually written as > > > > > > > > > > > > > > 'controls' : { > > > > > > > > > > > > > > } > > > > > > > > > > > > > > in meson ? > > > > > > We seem to do that, yes. > > > > I'll change that to the above convention. > > > > > > > > > > > > > + 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > > > + 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > > > + }, > > > > > > > > + # Mapping of vendor (pipeline handler) specific properties files > > > > > > > > + 'properties': > > > > > > > > + { > > > > > > > > + > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > control_headers = [] > > > > > > > > +vendor_ctrl_files = [] > > > > > > > > +vendor_prop_files = [] > > > > > > > > > > > > > > > > foreach header, mode : control_source_files > > > > > > > > - input_files = files('../../src/libcamera/' + header +'.yaml') > > > > > > > > + > > > > > > > > + # Start by populating the vendor specific mappings into vendor_ctrl_files > > > > > > > > + # and vendor_prop_files. These will be cached for later use. > > > > > > > > + vendor_files = [] > > > > > > > > + foreach pipeline, file : vendor_controls_mapping[mode] > > > > > > > > + if pipeline not in pipelines > > > > > > > > + continue > > > > > > > > + endif > > > > > > > > + if file not in vendor_files > > > > > > > > + vendor_files += file > > > > > > > > + endif > > > > > > > > + endforeach > > > > > > > > + > > > > > > > > + if mode == 'controls' > > > > > > > > + vendor_ctrl_files = vendor_files > > > > > > > > + else > > > > > > > > + vendor_prop_files = vendor_files > > > > > > > > + endif > > > > > > > > + > > > > > > > > + input_files = files('../../src/libcamera/' + header + '.yaml') > > > > > > > > + > > > > > > > > + foreach file : vendor_files > > > > > > > > + input_files += files('../../src/libcamera/' + file) > > > > > > > > + endforeach > > > > > > > > + > > > > > > > > > > > > > > Wouldn't it be simpler if we try to handle libcamera, draft and vendor > > > > > > > controls as first class citizens in the same manner, instead of > > > > > > > maintaing the "libcamera" ones on a different level ? > > > > > > > > > > > > > > I guess if we could do something like: > > > > > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > > > > > controls_map { > > > > > > > 'controls' : { > > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > > } > > > > > > > > > > > > > > 'properties' = { > > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > This should be property_ids.yaml. > > > > > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > control_headers = [] > > > > > > > ranges_file = files('../../src/libcamera/control_ranges.yaml') > > > > > > > foreach mode, entry : controls_map > > > > > > > files_list = [] > > > > > > > input_files=[] > > > > > > > foreach vendor, header : entry > > > > > > > if vendor != 'libcamera' or vendor != 'draft' > > > > > > > if vendor not in pipelines > > > > > > > continue > > > > > > > endif > > > > > > > endif > > > > > > > > > > > > > > if header in files_list > > > > > > > continue > > > > > > > endif > > > > > > > files_list += header > > > > > > > > > > > > > > input_files += files('../../src/libcamera/' + header) > > > > > > > endforeach > > > > > > > > > > > > > > outfile='' > > > > > > > if mode == 'controls' > > > > > > > outfile = 'control_ids.h' > > > > > > > else > > > > > > > outfile = 'property_ids.h' > > > > > > > endif > > > > > > > > > > > > > > template = outfile + '.in' > > > > > > > control_headers += custom_target(header + '_h', > > > > > > > input : input_files, > > > > > > > output : outfile, > > > > > > > command : [gen_controls, '-o', '@OUTPUT@', > > > > > > > '--mode', mode, '-t', template, > > > > > > > '-r', ranges_file, '@INPUT@'], > > > > > > > install : true, > > > > > > > install_dir : libcamera_headers_install_dir) > > > > > > > endforeach > > > > > > > > > > > > > > libcamera_public_headers += control_headers > > > > > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > > > > > isn't it nicer ? > > > > > > > > > > > > > > The above code is valid in meson, but the meson.build in > > > > > > > src/libcamera/ should be updated accordingly. > > > > > > > > > > > > > > In general, I think the relevant part here is > > > > > > > > > > > > > > controls_map { > > > > > > > 'controls' : { > > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > > 'rpi/pisp': 'control_ids_rpi.yaml', > > > > > > > 'rpi/vc4': 'control_ids_rpi.yaml' > > > > > > > } > > > > > > > > > > > > > > 'properties' = { > > > > > > > 'libcamera' : controls_ids.yaml, > > > > > > > 'drafts' : controls_ids_draft.yaml > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > As seeing all the supported controls and properties listed together > > > > > > > might be easier to maintain and parse ? > > > > > > > > > > > > > > What do you think ? > > > > > > > > > > > > That looks reasonable. I guess I wanted to keep things seperate for > > > > > > vendor controls, but no reason not to combine the libcamera/draft and > > > > > > vendor cases into one list. I can make this change for v3. > > > > > > That seems fine to me too. If we go in that direction, should we replace > > > 'libcamera' with 'core', and name the file controls_ids_core.yaml ? All > > > controls are libcamera controls. > > > > Yes, that sounds like a good change to introduce. > > On this change, if I rename 'libcamera' to 'core' above, I should > really do the same in the gen-controls.py scripts as well for > consistency (even though the 2 labels are unrelated). I'lll do that > change unless folks disagree with this. Please disregard that. I think the 'libcamera' designation in the gen scripts is actually correct (identifying 'libcamera' as a vendor for 'core' controls). Apologies for the noise! > > > > > > > > > > > Let me know if and how I can help. The change is quite invasive in the > > > > > whole patch series and I don't want to make it more painful than > > > > > necessary for you > > > > > > > > No worries, I've already made the changes. Should I wait for more > > > > feedback or go ahead and push a v3? > > > > > > > > > > > > template_file = files(header + '.h.in') > > > > > > > > control_headers += custom_target(header + '_h', > > > > > > > > input : input_files, > > > > > > > > diff --git a/meson.build b/meson.build > > > > > > > > index e9a1c7e360ce..1423abf16c77 100644 > > > > > > > > --- a/meson.build > > > > > > > > +++ b/meson.build > > > > > > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) > > > > > > > > summary({ > > > > > > > > 'Enabled pipelines': pipelines, > > > > > > > > 'Enabled IPA modules': enabled_ipa_names, > > > > > > > > + 'Vendor controls': vendor_ctrl_files, > > > > > > > > + 'Vendor properties': vendor_prop_files, > > > > > > > > 'Hotplug support': libudev.found(), > > > > > > > > 'Tracing support': tracing_enabled, > > > > > > > > 'Android support': android_enabled, > > > > > > > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml > > > > > > > > new file mode 100644 > > > > > > > > index 000000000000..a6f8b43a5f62 > > > > > > > > --- /dev/null > > > > > > > > +++ b/src/libcamera/control_ids_rpi.yaml > > > > > > > > @@ -0,0 +1,16 @@ > > > > > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > > > > > > > +# > > > > > > > > +# Copyright (C) 2023, Raspberry Pi Ltd > > > > > > > > +# > > > > > > > > +%YAML 1.1 > > > > > > > > +--- > > > > > > > > +vendor: rpi > > > > > > > > +controls: > > > > > > > > + - PispConfigDumpFile: > > > > > > > > + type: string > > > > > > > > + description: | > > > > > > > > + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON > > > > > > > > + formatted dump of the Backend configuration to the filename given by the > > > > > > > > + value of the control. > > > > > > As the series doesn't contain an implementation of this control I can't > > > really review the description properly, but that's fine, I understand it > > > will come later. > > > > In v3, I'll remove the addition of the rpi specific vendor control and > > move into a separate commit. We can make a case for delaying merging > > of that commit until the Pi 5 pipeline handler gets merged. > > > > Regards, > > Naush > > > > > > > > > > > > > > > + > > > > > > > > +... > > > > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > > > > > index 05ee38daf22b..7eb26ccbb7f5 100644 > > > > > > > > --- a/src/libcamera/meson.build > > > > > > > > +++ b/src/libcamera/meson.build > > > > > > > > @@ -129,6 +129,18 @@ control_sources = [] > > > > > > > > > > > > > > > > foreach source, mode : control_source_files > > > > > > > > input_files = files(source +'.yaml') > > > > > > > > + > > > > > > > > + # Add the vendor specific files to the input. > > > > > > > > + if mode == 'controls' > > > > > > > > + vendor_files = vendor_ctrl_files > > > > > > > > + else > > > > > > > > + vendor_files = vendor_prop_files > > > > > > > > + endif > > > > > > > > + > > > > > > > > + foreach file : vendor_files > > > > > > > > + input_files += files(file) > > > > > > > > + endforeach > > > > > > > > + > > > > > > > > template_file = files(source + '.cpp.in') > > > > > > > > control_sources += custom_target(source + '_cpp', > > > > > > > > input : input_files, > > > > > > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > > > > > > > index ea28f0139f23..e2ddad8581fd 100755 > > > > > > > > --- a/src/py/libcamera/gen-py-controls.py > > > > > > > > +++ b/src/py/libcamera/gen-py-controls.py > > > > > > > > @@ -91,7 +91,7 @@ def main(argv): > > > > > > > > parser = argparse.ArgumentParser() > > > > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > > > > - parser.add_argument('input', type=str, > > > > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > > > > help='Input file name.') > > > > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > > > > help='Template file name.') > > > > > > > > @@ -103,11 +103,11 @@ def main(argv): > > > > > > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr) > > > > > > > > return -1 > > > > > > > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > > > > - > > > > > > > > controls = {} > > > > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > > > > - controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > > + for input in args.input: > > > > > > > > + data = open(input, 'rb').read() > > > > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > > > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > > > > > > > > > > > > > data = generate_py(controls, args.mode) > > > > > > > > > > > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > > > > > > index 1c3ea1843ac0..ea38ad6ca829 100644 > > > > > > > > --- a/src/py/libcamera/meson.build > > > > > > > > +++ b/src/py/libcamera/meson.build > > > > > > > > @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') > > > > > > > > > > > > > > > > gen_py_controls = files('gen-py-controls.py') > > > > > > > > > > > > > > > > +foreach file : vendor_ctrl_files > > > > > > > > + gen_py_controls_input_files += files('../../libcamera/' + file) > > > > > > > > +endforeach > > > > > > > > + > > > > > > > > pycamera_sources += custom_target('py_gen_controls', > > > > > > > > input : gen_py_controls_input_files, > > > > > > > > output : ['py_controls_generated.cpp'], > > > > > > > > @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', > > > > > > > > gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') > > > > > > > > gen_py_properties_template = files('py_properties_generated.cpp.in') > > > > > > > > > > > > > > > > +foreach file : vendor_prop_files > > > > > > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file) > > > > > > > > +endforeach > > > > > > > > + > > > > > > > > pycamera_sources += custom_target('py_gen_properties', > > > > > > > > input : gen_py_property_enums_input_files, > > > > > > > > output : ['py_properties_generated.cpp'], > > > > > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > > > > > > > index 8af33d29cd07..d7862142b8c1 100755 > > > > > > > > --- a/utils/gen-controls.py > > > > > > > > +++ b/utils/gen-controls.py > > > > > > > > @@ -359,7 +359,7 @@ def main(argv): > > > > > > > > parser = argparse.ArgumentParser() > > > > > > > > parser.add_argument('-o', dest='output', metavar='file', type=str, > > > > > > > > help='Output file name. Defaults to standard output if not specified.') > > > > > > > > - parser.add_argument('input', type=str, > > > > > > > > + parser.add_argument('input', type=str, nargs='+', > > > > > > > > help='Input file name.') > > > > > > > > parser.add_argument('-t', dest='template', type=str, required=True, > > > > > > > > help='Template file name.') > > > > > > > > @@ -367,10 +367,12 @@ def main(argv): > > > > > > > > help='Mode of operation') > > > > > > > > args = parser.parse_args(argv[1:]) > > > > > > > > > > > > > > > > - data = open(args.input, 'rb').read() > > > > > > > > - vendor = yaml.safe_load(data)['vendor'] > > > > > > > > - controls = yaml.safe_load(data)['controls'] > > > > > > > > - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > > > > > > + controls = [] > > > > > > > > + for input in args.input: > > > > > > > > + data = open(input, 'rb').read() > > > > > > > > + vendor = yaml.safe_load(data)['vendor'] > > > > > > > > + ctrls = yaml.safe_load(data)['controls'] > > > > > > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] > > > > > > > > > > > > > > > > if args.template.endswith('.cpp.in'): > > > > > > > > data = generate_cpp(controls) > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build index 5fb772e6dd14..54123c5b858d 100644 --- a/include/libcamera/meson.build +++ b/include/libcamera/meson.build @@ -38,10 +38,50 @@ control_source_files = { 'property_ids': 'properties', } +vendor_controls_mapping = { + # Mapping of vendor (pipeline handler) specific controls files + 'controls': + { + 'rpi/pisp': 'control_ids_rpi.yaml', + 'rpi/vc4': 'control_ids_rpi.yaml' + }, + # Mapping of vendor (pipeline handler) specific properties files + 'properties': + { + + } +} + control_headers = [] +vendor_ctrl_files = [] +vendor_prop_files = [] foreach header, mode : control_source_files - input_files = files('../../src/libcamera/' + header +'.yaml') + + # Start by populating the vendor specific mappings into vendor_ctrl_files + # and vendor_prop_files. These will be cached for later use. + vendor_files = [] + foreach pipeline, file : vendor_controls_mapping[mode] + if pipeline not in pipelines + continue + endif + if file not in vendor_files + vendor_files += file + endif + endforeach + + if mode == 'controls' + vendor_ctrl_files = vendor_files + else + vendor_prop_files = vendor_files + endif + + input_files = files('../../src/libcamera/' + header + '.yaml') + + foreach file : vendor_files + input_files += files('../../src/libcamera/' + file) + endforeach + template_file = files(header + '.h.in') control_headers += custom_target(header + '_h', input : input_files, diff --git a/meson.build b/meson.build index e9a1c7e360ce..1423abf16c77 100644 --- a/meson.build +++ b/meson.build @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules) summary({ 'Enabled pipelines': pipelines, 'Enabled IPA modules': enabled_ipa_names, + 'Vendor controls': vendor_ctrl_files, + 'Vendor properties': vendor_prop_files, 'Hotplug support': libudev.found(), 'Tracing support': tracing_enabled, 'Android support': android_enabled, diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml new file mode 100644 index 000000000000..a6f8b43a5f62 --- /dev/null +++ b/src/libcamera/control_ids_rpi.yaml @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# +# Copyright (C) 2023, Raspberry Pi Ltd +# +%YAML 1.1 +--- +vendor: rpi +controls: + - PispConfigDumpFile: + type: string + description: | + Triggers the Raspberry Pi PiSP pipeline handler to generate a JSON + formatted dump of the Backend configuration to the filename given by the + value of the control. + +... diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 05ee38daf22b..7eb26ccbb7f5 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -129,6 +129,18 @@ control_sources = [] foreach source, mode : control_source_files input_files = files(source +'.yaml') + + # Add the vendor specific files to the input. + if mode == 'controls' + vendor_files = vendor_ctrl_files + else + vendor_files = vendor_prop_files + endif + + foreach file : vendor_files + input_files += files(file) + endforeach + template_file = files(source + '.cpp.in') control_sources += custom_target(source + '_cpp', input : input_files, diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py index ea28f0139f23..e2ddad8581fd 100755 --- a/src/py/libcamera/gen-py-controls.py +++ b/src/py/libcamera/gen-py-controls.py @@ -91,7 +91,7 @@ def main(argv): parser = argparse.ArgumentParser() parser.add_argument('-o', dest='output', metavar='file', type=str, help='Output file name. Defaults to standard output if not specified.') - parser.add_argument('input', type=str, + parser.add_argument('input', type=str, nargs='+', help='Input file name.') parser.add_argument('-t', dest='template', type=str, required=True, help='Template file name.') @@ -103,11 +103,11 @@ def main(argv): print(f'Invalid mode option "{args.mode}"', file=sys.stderr) return -1 - data = open(args.input, 'rb').read() - controls = {} - vendor = yaml.safe_load(data)['vendor'] - controls[vendor] = yaml.safe_load(data)['controls'] + for input in args.input: + data = open(input, 'rb').read() + vendor = yaml.safe_load(data)['vendor'] + controls[vendor] = yaml.safe_load(data)['controls'] data = generate_py(controls, args.mode) diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build index 1c3ea1843ac0..ea38ad6ca829 100644 --- a/src/py/libcamera/meson.build +++ b/src/py/libcamera/meson.build @@ -33,6 +33,10 @@ gen_py_controls_template = files('py_controls_generated.cpp.in') gen_py_controls = files('gen-py-controls.py') +foreach file : vendor_ctrl_files + gen_py_controls_input_files += files('../../libcamera/' + file) +endforeach + pycamera_sources += custom_target('py_gen_controls', input : gen_py_controls_input_files, output : ['py_controls_generated.cpp'], @@ -44,6 +48,10 @@ pycamera_sources += custom_target('py_gen_controls', gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml') gen_py_properties_template = files('py_properties_generated.cpp.in') +foreach file : vendor_prop_files + gen_py_property_enums_input_files += files('../../libcamera/' + file) +endforeach + pycamera_sources += custom_target('py_gen_properties', input : gen_py_property_enums_input_files, output : ['py_properties_generated.cpp'], diff --git a/utils/gen-controls.py b/utils/gen-controls.py index 8af33d29cd07..d7862142b8c1 100755 --- a/utils/gen-controls.py +++ b/utils/gen-controls.py @@ -359,7 +359,7 @@ def main(argv): parser = argparse.ArgumentParser() parser.add_argument('-o', dest='output', metavar='file', type=str, help='Output file name. Defaults to standard output if not specified.') - parser.add_argument('input', type=str, + parser.add_argument('input', type=str, nargs='+', help='Input file name.') parser.add_argument('-t', dest='template', type=str, required=True, help='Template file name.') @@ -367,10 +367,12 @@ def main(argv): help='Mode of operation') args = parser.parse_args(argv[1:]) - data = open(args.input, 'rb').read() - vendor = yaml.safe_load(data)['vendor'] - controls = yaml.safe_load(data)['controls'] - controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] + controls = [] + for input in args.input: + data = open(input, 'rb').read() + vendor = yaml.safe_load(data)['vendor'] + ctrls = yaml.safe_load(data)['controls'] + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls] if args.template.endswith('.cpp.in'): data = generate_cpp(controls)
Add support for using separate YAML files for vendor specific controls. This simplifies management of vendor control definitions and avoids possible merge conflicts when changing the control_ids.yaml file for core and draft controls. The mapping of vendor/pipeline handler to control file is done through the vendor_controls_mapping variable in include/libcamera/meson.build. Add a new control_ids_rpi.yaml file to hold the Raspberry Pi specific vendor controls. This contains a single control PispConfigDumpFile that will be used in the Pi 5 pipeline handler as a trigger to dump the Backend configuration as a JSON file. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/meson.build | 42 ++++++++++++++++++++++++++++- meson.build | 2 ++ src/libcamera/control_ids_rpi.yaml | 16 +++++++++++ src/libcamera/meson.build | 12 +++++++++ src/py/libcamera/gen-py-controls.py | 10 +++---- src/py/libcamera/meson.build | 8 ++++++ utils/gen-controls.py | 12 +++++---- 7 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 src/libcamera/control_ids_rpi.yaml