[libcamera-devel] [PATCH v3 4/7] libcamera: control: Add vendor control id range reservation
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Nov 28 11:03:30 CET 2023
Hi
On Tue, Nov 28, 2023 at 09:58:39AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck via libcamera-devel (2023-11-28 09:28:08)
> > On Tue, 28 Nov 2023 at 09:24, Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Tue, Nov 28, 2023 at 09:21:05AM +0000, Naushir Patuck wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Mon, 27 Nov 2023 at 16:54, Jacopo Mondi
> > > > <jacopo.mondi at ideasonboard.com> wrote:
> > > > >
> > > > > I wonder if it's worth passing this as param, as I suspect the
> > > > > enumeration of controls will be something that user won't change..
> > > >
> > > > I could do that, but that involves setting the vendor ranges in the
> > > > meson.build file somewhere and fetching it when calling the script.
> > > > Personally I prefer this living in a YAML file as this patch does, but
> > > > if folks want, I can move it into the build scripts.
> > > >
> > >
> > > I was not questioning the usage of the yaml file, that's fine.
> > > I was wondering if it's worth passing the ranges file name and
> > > location as parameter, or gen-controls.py can assume it knows where it
> > > lives..
> >
> > Ah, sorry I misunderstood! I did it as a parameter as all the inputs
> > seem to go through parameters, but again, I am happy to keep the
> > ranges file hardcoded in the script if folks think that's better.
>
> I likely wouldn't have asked to change it to be a parameter specified
> externally, but as it's already there I think it's ok as is.
>
> This script is specifically custom anyway, it doesn't have to be
> generic.
>
> Having it specified in the meson build means we can likely handle things
> easier if we move the files around too.
>
Yeah, mine was defintely more a question more than a requirement, I
think it's fine too
> I anticipate once this lands we'll want to move and collate all
> controls into a subdir to group the likely expansions here, but we don't
> need to do that as part of this series.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
And seems I forgot my tag
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
> >
> > Naush
> >
> >
> > >
> > > > Regards,
> > > > Naush
> > > >
> > > > >
> > > > > On Fri, Nov 24, 2023 at 12:37:10PM +0000, Naushir Patuck via libcamera-devel wrote:
> > > > > > Add a new control_ranges.yaml file that is used to reserve control id
> > > > > > ranges/offsets for libcamera and vendor specific controls. This file is
> > > > > > used by the gen-controls.py script to generate control id values for
> > > > > > each control.
> > > > > >
> > > > > > Draft controls now have a separate range from core libcamera controls,
> > > > > > breaking the existing numbering behaviour.
> > > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > ---
> > > > > > include/libcamera/meson.build | 3 ++-
> > > > > > src/libcamera/control_ranges.yaml | 18 ++++++++++++++++++
> > > > > > src/libcamera/meson.build | 4 +++-
> > > > > > utils/gen-controls.py | 14 ++++++++++----
> > > > > > 4 files changed, 33 insertions(+), 6 deletions(-)
> > > > > > create mode 100644 src/libcamera/control_ranges.yaml
> > > > > >
> > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > > > > index a763c8ff4344..79187d3fdfc9 100644
> > > > > > --- a/include/libcamera/meson.build
> > > > > > +++ b/include/libcamera/meson.build
> > > > > > @@ -76,12 +76,13 @@ foreach mode, entry : controls_map
> > > > > > endif
> > > > > >
> > > > > > template_file = files(outfile + '.in')
> > > > > > + ranges_file = files('../../src/libcamera/control_ranges.yaml')
> > > > > > control_headers += custom_target(header + '_h',
> > > > > > input : input_files,
> > > > > > output : outfile,
> > > > > > command : [gen_controls, '-o', '@OUTPUT@',
> > > > > > '--mode', mode, '-t', template_file,
> > > > > > - '@INPUT@'],
> > > > > > + '-r', ranges_file, '@INPUT@'],
> > > > > > install : true,
> > > > > > install_dir : libcamera_headers_install_dir)
> > > > > > endforeach
> > > > > > diff --git a/src/libcamera/control_ranges.yaml b/src/libcamera/control_ranges.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..d42447d04647
> > > > > > --- /dev/null
> > > > > > +++ b/src/libcamera/control_ranges.yaml
> > > > > > @@ -0,0 +1,18 @@
> > > > > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > > > > +#
> > > > > > +# Copyright (C) 2023, Raspberry Pi Ltd
> > > > > > +#
> > > > > > +%YAML 1.1
> > > > > > +---
> > > > > > +# Specifies the control id ranges/offsets for core/draft libcamera and vendor
> > > > > > +# controls and properties.
> > > > > > +ranges:
> > > > > > + # Core libcamera controls
> > > > > > + libcamera: 0
> > > > > > + # Draft designated libcamera controls
> > > > > > + draft: 10000
> > > > > > + # Raspberry Pi vendor controls
> > > > > > + rpi: 20000
> > > > > > + # Next range starts at 30000
> > > > > > +
> > > > > > +...
> > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > > index 6d9902e6ffd1..45f63e932e4f 100644
> > > > > > --- a/src/libcamera/meson.build
> > > > > > +++ b/src/libcamera/meson.build
> > > > > > @@ -141,11 +141,13 @@ foreach mode, input_files : controls_mode_files
> > > > > > template_file = files('property_ids.cpp.in')
> > > > > > endif
> > > > > >
> > > > > > + ranges_file = files('control_ranges.yaml')
> > > > > > control_sources += custom_target(mode + '_cpp',
> > > > > > input : input_files,
> > > > > > output : mode + '_ids.cpp',
> > > > > > command : [gen_controls, '-o', '@OUTPUT@',
> > > > > > - '--mode', mode, '-t', template_file, '@INPUT@'])
> > > > > > + '--mode', mode, '-t', template_file,
> > > > > > + '-r', ranges_file, '@INPUT@'])
> > > > > > endforeach
> > > > > >
> > > > > > libcamera_sources += control_sources
> > > > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > > > > > index 30c58f35473c..04c63098b19b 100755
> > > > > > --- a/utils/gen-controls.py
> > > > > > +++ b/utils/gen-controls.py
> > > > > > @@ -236,7 +236,7 @@ ${vendor_controls_str}
> > > > > > }
> > > > > >
> > > > > >
> > > > > > -def generate_h(controls, mode):
> > > > > > +def generate_h(controls, mode, ranges):
> > > > > > enum_template_start = string.Template('''enum ${name}Enum {''')
> > > > > > enum_value_template = string.Template('''\t${name} = ${value},''')
> > > > > > enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''')
> > > > > > @@ -251,8 +251,10 @@ def generate_h(controls, mode):
> > > > > >
> > > > > > vendor = 'draft' if ctrl.is_draft else ctrl.vendor
> > > > > > if vendor not in ctrls:
> > > > > > + if vendor not in ranges.keys():
> > > > > > + raise RuntimeError(f'Control id range is not defined for vendor {vendor}')
> > > > > > + id_value[vendor] = ranges[vendor] + 1
> > > > > > ids[vendor] = []
> > > > > > - id_value[vendor] = 1
> > > > > > ctrls[vendor] = []
> > > > > >
> > > > > > # Core and draft controls use the same ID value
> > > > > > @@ -338,13 +340,17 @@ def main(argv):
> > > > > > help='Mode of operation')
> > > > > > parser.add_argument('--output', '-o', metavar='file', type=str,
> > > > > > help='Output file name. Defaults to standard output if not specified.')
> > > > > > + parser.add_argument('--ranges', '-r', type=str, required=True,
> > > > > > + help='Control id range reservation file.')
> > > > > > parser.add_argument('--template', '-t', dest='template', type=str, required=True,
> > > > > > help='Template file name.')
> > > > > > parser.add_argument('input', type=str, nargs='+',
> > > > > > help='Input file name.')
> > > > > > -
> > > > > > args = parser.parse_args(argv[1:])
> > > > > >
> > > > > > + data = open(args.ranges, 'rb').read()
> > > > > > + ranges = yaml.safe_load(data)['ranges']
> > > > > > +
> > > > > > controls = []
> > > > > > for input in args.input:
> > > > > > data = open(input, 'rb').read()
> > > > > > @@ -355,7 +361,7 @@ def main(argv):
> > > > > > if args.template.endswith('.cpp.in'):
> > > > > > data = generate_cpp(controls)
> > > > > > elif args.template.endswith('.h.in'):
> > > > > > - data = generate_h(controls, args.mode)
> > > > > > + data = generate_h(controls, args.mode, ranges)
> > > > > > else:
> > > > > > raise RuntimeError('Unknown template type')
> > > > > >
> > > > > > --
> > > > > > 2.34.1
> > > > > >
More information about the libcamera-devel
mailing list