[libcamera-devel,v1,2/5] controls: build: Allow separate vendor control YAML files

19195 diff mbox series

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.

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       | 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

Hi Naush,

Thank you for the patch.

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.

> 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       | 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.

>  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.

> +    # 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/

> +    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'.

> +
> +    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 ?

> +                                                '--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'):

Hi Laurent,

Thank you for your feedback.

On Mon, 20 Nov 2023 at 04:09, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> 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@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.

Regards,
Naush

>
> > +                                                '--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

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@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'):