[libcamera-devel] [PATCH v1 2/5] controls: build: Allow separate vendor control YAML files
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 20 17:03:22 CET 2023
Hi Naush,
On Mon, Nov 20, 2023 at 03:00:52PM +0000, Naushir Patuck wrote:
> On Mon, 20 Nov 2023 at 04:09, Laurent Pinchart wrote:
> > On Fri, Nov 10, 2023 at 10:59:59AM +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.
> > >
> > > The command line argument handling for the gen-py-controls.py and
> > > gen-controls.py have now changed to allow multiple input files to be
> > > provided. This means that all positional arguments have been replaced
> > > by non-positional equivalents.
> >
> > This could have been done in a separate patch to ease review.
>
> I'll split this to a separate patch as best I can in the next version.
>
> > > 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 at raspberrypi.com>
> > > ---
> > > include/libcamera/meson.build | 48 ++++++++++++++++++++++++++---
> > > meson.build | 2 ++
> > > src/libcamera/control_ids_rpi.yaml | 17 ++++++++++
> > > src/libcamera/meson.build | 19 ++++++++++--
> > > src/py/libcamera/gen-py-controls.py | 12 +++++---
> > > src/py/libcamera/meson.build | 26 ++++++++++------
> > > utils/gen-controls.py | 15 +++++----
> > > 7 files changed, 111 insertions(+), 28 deletions(-)
> > > create mode 100644 src/libcamera/control_ids_rpi.yaml
> > >
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index f736cca07228..f8af51174f08 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -32,21 +32,61 @@ install_headers(libcamera_public_headers,
> > >
> > > libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir
> > >
> > > -# control_ids.h and property_ids.h
> > > +# control_ids.h and property_ids.h and associated modes
> >
> > I think this belongs to the previous patch.
>
> Ack.
>
> > > control_source_files = {
> > > 'control_ids': 'controls',
> > > 'property_ids': 'properties',
> > > }
> > >
> > > +vendor_mappings = {
> >
> > The commit message names this vendor_controls_mapping, which I think is
> > a better name as it's related to controls.
>
> Ack
>
> > > + # 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', header + '.h.in')
> > > + # Start by populating the vendor specific mappings into vendor_ctrl_files
> > > + # vendor_prop_files. These will be cached for later use.
> >
> > s/vendor_prop_files/and vendor_prop_files/
>
> Ack
>
> > > + vendor_files = []
> > > + foreach pipeline, file : vendor_mappings[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')
> >
> > While at it, add the missing space before the '.yaml'.
>
> Ack
>
> > > +
> > > + 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,
> > > output : header + '.h',
> > > - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',
> > > - '--mode', mode],
> > > + command : [gen_controls, '-o', '@OUTPUT@', '-i', '@INPUT@',
> > > + '--mode', mode, '-t', template_file],
> > > install : true,
> > > install_dir : libcamera_headers_install_dir)
> > > endforeach
> > > 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..239904a008a8
> > > --- /dev/null
> > > +++ b/src/libcamera/control_ids_rpi.yaml
> > > @@ -0,0 +1,17 @@
> > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > +#
> > > +# Copyright (C) 2023, Raspberry Pi Ltd
> > > +#
> > > +%YAML 1.1
> > > +---
> > > +controls:
> > > +
> > > + - PispConfigDumpFile:
> > > + type: string
> > > + vendor: rpi
> > > + 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 e49bf850b355..8891052a3316 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -128,12 +128,25 @@ endif
> > > control_sources = []
> > >
> > > foreach source, mode : control_source_files
> > > - input_files = files(source +'.yaml', source + '.cpp.in')
> > > + 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,
> > > output : source + '.cpp',
> > > - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',
> > > - '--mode', mode])
> > > + command : [gen_controls, '-o', '@OUTPUT@', '-i', '@INPUT@',
> >
> > meson will not expand this to multiple -i arguments, I'm actually
> > surprised it works as expected. Could we keep the input files as
> > positional arguments ?
>
> The above construct will add "-i <file1> <file2>..." which is what the
> script expects (i.e. not multiple -i arguments). I'll change things
> around so input files are treated as the only positional argument.
That's what I've seen, and I was surprised that
parser.add_argument('-i', dest='inputs', type=str, nargs='+', required=True,
help='Input file name(s).')
actually supports that fine. I thought it would only work with multiple
instances of the -i argument. I don't know if it's a guaranteed
behaviour or an undefined behaviour, so that's why I'm thinking
positional parameters would be safer (beside being more standard).
> > > + '--mode', mode, '-t', template_file])
> > > endforeach
> > >
> > > libcamera_sources += control_sources
> > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py
> > > index e89de674966a..a32ef09c2f48 100755
> > > --- a/src/py/libcamera/gen-py-controls.py
> > > +++ b/src/py/libcamera/gen-py-controls.py
> > > @@ -91,9 +91,9 @@ 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,
> > > - help='Input file name.')
> > > - parser.add_argument('template', type=str,
> > > + parser.add_argument('-i', dest='inputs', type=str, nargs='+', required=True,
> > > + help='Input file name(s).')
> > > + parser.add_argument('-t', dest='template', type=str, required=True,
> > > help='Template file name.')
> > > parser.add_argument('--mode', type=str, required=True,
> > > help='Mode is either "controls" or "properties"')
> > > @@ -103,8 +103,10 @@ def main(argv):
> > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr)
> > > return -1
> > >
> > > - data = open(args.input, 'rb').read()
> > > - controls = yaml.safe_load(data)['controls']
> > > + controls = []
> > > + for input in args.inputs:
> > > + data = open(input, 'rb').read()
> > > + controls += 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 f58c7198ee9e..e4bc208b4ac1 100644
> > > --- a/src/py/libcamera/meson.build
> > > +++ b/src/py/libcamera/meson.build
> > > @@ -28,29 +28,35 @@ pycamera_sources = files([
> > >
> > > # Generate controls
> > >
> > > -gen_py_controls_input_files = files([
> > > - '../../libcamera/control_ids.yaml',
> > > - 'py_controls_generated.cpp.in',
> > > -])
> > > +gen_py_controls_input_files = files('../../libcamera/control_ids.yaml')
> > > +
> > > +foreach file : vendor_ctrl_files
> > > + gen_py_controls_input_files += files('../../libcamera/' + file)
> > > +endforeach
> > >
> > > gen_py_controls = files('gen-py-controls.py')
> > > +gen_py_controls_template = files('py_controls_generated.cpp.in')
> > >
> > > pycamera_sources += custom_target('py_gen_controls',
> > > input : gen_py_controls_input_files,
> > > output : ['py_controls_generated.cpp'],
> > > - command : [gen_py_controls, '--mode', 'controls', '-o', '@OUTPUT@', '@INPUT@'])
> > > + command : [gen_py_controls, '--mode', 'controls', '-o', '@OUTPUT@', '-i', '@INPUT@',
> > > + '-t', gen_py_controls_template])
> > >
> > > # Generate properties
> > >
> > > -gen_py_property_enums_input_files = files([
> > > - '../../libcamera/property_ids.yaml',
> > > - 'py_properties_generated.cpp.in',
> > > -])
> > > +gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml')
> > > +gen_py_property_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'],
> > > - command : [gen_py_controls, '--mode', 'properties', '-o', '@OUTPUT@', '@INPUT@'])
> > > + command : [gen_py_controls, '--mode', 'properties', '-o', '@OUTPUT@', '-i', '@INPUT@',
> > > + '-t', gen_py_property_template])
> > >
> > > # Generate formats
> > >
> > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > > index dd55753e792c..3c490a562676 100755
> > > --- a/utils/gen-controls.py
> > > +++ b/utils/gen-controls.py
> > > @@ -362,18 +362,21 @@ def main(argv):
> > >
> > > # Parse command line arguments
> > > parser = argparse.ArgumentParser()
> > > - parser.add_argument('-o', dest='output', metavar='file', type=str,
> > > + parser.add_argument('-o', dest='output', metavar='file', type=str, required=True,
> > > help='Output file name. Defaults to standard output if not specified.')
> > > - parser.add_argument('input', type=str,
> > > - help='Input file name.')
> > > - parser.add_argument('template', type=str,
> > > + parser.add_argument('-i', dest='inputs', type=str, nargs='+', required=True,
> > > + help='Input file name(s).')
> > > + parser.add_argument('-t', dest='template', type=str, required=True,
> > > help='Template file name.')
> > > parser.add_argument('--mode', type=str, required=True, choices=['controls', 'properties'],
> > > help='Mode of operation')
> > > args = parser.parse_args(argv[1:])
> > >
> > > - data = open(args.input, 'rb').read()
> > > - controls = yaml.safe_load(data)['controls']
> > > + controls = []
> > > + for input in args.inputs:
> > > + data = open(input, 'rb').read()
> > > + controls = controls + yaml.safe_load(data)['controls']
> > > +
> > > controls = [Control(*ctrl.popitem()) for ctrl in controls]
> > >
> > > if args.template.endswith('.cpp.in'):
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list