[libcamera-devel] [PATCH v2 3/6] controls: build: Allow separate vendor control YAML files
Naushir Patuck
naush at raspberrypi.com
Thu Nov 23 09:02:28 CET 2023
Hi Jacopo,
On Wed, 22 Nov 2023 at 16:05, Jacopo Mondi
<jacopo.mondi at 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 at 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 at 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
> > > >
More information about the libcamera-devel
mailing list