[libcamera-devel,1/9] libcamera: Introduce option to customize behavior for a camera module

15345 diff mbox series

Introduce option to customize behavior for a camera module, which might
alter the camera characteristic and should be set before inspecting it.

Re-use the Control generation infrastructure to generate options and define
the first 'PipelineConfigFile' option, which allows user to specify
preference for pipeline configuration.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 include/libcamera/ipa/ipa_controls.h |  1 +
 include/libcamera/meson.build        |  3 +-
 include/libcamera/option_ids.h.in    | 36 +++++++++++++++++
 src/libcamera/control_serializer.cpp | 12 ++++++
 src/libcamera/option_ids.cpp.in      | 58 ++++++++++++++++++++++++++++
 src/libcamera/option_ids.yaml        | 16 ++++++++
 6 files changed, 125 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/option_ids.h.in
 create mode 100644 src/libcamera/option_ids.cpp.in
 create mode 100644 src/libcamera/option_ids.yaml

Hi Han-lin,

On Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:
> Introduce option to customize behavior for a camera module, which might
> alter the camera characteristic and should be set before inspecting it.
> 
> Re-use the Control generation infrastructure to generate options and define
> the first 'PipelineConfigFile' option, which allows user to specify
> preference for pipeline configuration.

When thinking about this, I wasn't envisioning passing the configuration
file to libcamera through the C++ API, but looking for it in standard
locations on the file system. Could you explain the reason why you think
this should be dynamic ?

> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  include/libcamera/ipa/ipa_controls.h |  1 +
>  include/libcamera/meson.build        |  3 +-
>  include/libcamera/option_ids.h.in    | 36 +++++++++++++++++
>  src/libcamera/control_serializer.cpp | 12 ++++++
>  src/libcamera/option_ids.cpp.in      | 58 ++++++++++++++++++++++++++++
>  src/libcamera/option_ids.yaml        | 16 ++++++++
>  6 files changed, 125 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/option_ids.h.in
>  create mode 100644 src/libcamera/option_ids.cpp.in
>  create mode 100644 src/libcamera/option_ids.yaml
> 
> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> index da1a7596..8173a2e5 100644
> --- a/include/libcamera/ipa/ipa_controls.h
> +++ b/include/libcamera/ipa/ipa_controls.h
> @@ -18,6 +18,7 @@ extern "C" {
>  enum ipa_controls_id_map_type {
>  	IPA_CONTROL_ID_MAP_CONTROLS,
>  	IPA_CONTROL_ID_MAP_PROPERTIES,
> +	IPA_CONTROL_ID_MAP_OPTIONS,

Do you have use cases for passing a configuration file to the IPA
modules ? As IPA modules are sandboxed, they don't have unrestricted
access to the filesystem. I think this could create issues, especially
if the path to the configuration is under the control of an application.

>  	IPA_CONTROL_ID_MAP_V4L2,
>  };
>  
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 408b7acf..a9b71bc3 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -31,10 +31,11 @@ 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, property_ids.h, and option_ids.h
>  control_source_files = [
>      'control_ids',
>      'property_ids',
> +    'option_ids',
>  ]
>  
>  control_headers = []
> diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in
> new file mode 100644
> index 00000000..53058a4b
> --- /dev/null
> +++ b/include/libcamera/option_ids.h.in
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * option_ids.h - Option ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#pragma once
> +
> +#include <stdint.h>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +namespace options {
> +
> +enum {
> +${ids}
> +};
> +
> +${controls}
> +
> +namespace draft {
> +
> +${draft_controls}
> +
> +} /* namespace draft */
> +
> +extern const ControlIdMap options;
> +
> +} /* namespace options */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index e87d2362..dc8d2906 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -16,6 +16,7 @@
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
> +#include <libcamera/option_ids.h>
>  #include <libcamera/property_ids.h>
>  
>  #include <libcamera/ipa/ipa_controls.h>
> @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  	enum ipa_controls_id_map_type idMapType;
>  	if (idmap == &controls::controls)
>  		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> +	else if (idmap == &options::options)
> +		idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
>  	else if (idmap == &properties::properties)
>  		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
>  	else
> @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,
>  	enum ipa_controls_id_map_type idMapType;
>  	if (idmap == &controls::controls)
>  		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> +	else if (idmap == &options::options)
> +		idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
>  	else if (idmap == &properties::properties)
>  		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
>  	else
> @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  	case IPA_CONTROL_ID_MAP_CONTROLS:
>  		idMap = &controls::controls;
>  		break;
> +	case IPA_CONTROL_ID_MAP_OPTIONS:
> +		idMap = &options::options;
> +		break;
>  	case IPA_CONTROL_ID_MAP_PROPERTIES:
>  		idMap = &properties::properties;
>  		break;
> @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  			idMap = &controls::controls;
>  			break;
>  
> +		case IPA_CONTROL_ID_MAP_OPTIONS:
> +			idMap = &options::options;
> +			break;
> +
>  		case IPA_CONTROL_ID_MAP_PROPERTIES:
>  			idMap = &properties::properties;
>  			break;
> diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in
> new file mode 100644
> index 00000000..adfd0633
> --- /dev/null
> +++ b/src/libcamera/option_ids.cpp.in
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * option_ids.cpp : Option ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#include <libcamera/option_ids.h>
> +
> +/**
> + * \file option_ids.h
> + * \brief Camera option identifiers
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \brief Namespace for libcamera options
> + */
> +namespace options {
> +
> +${controls_doc}
> +
> +/**
> + * \brief Namespace for libcamera draft options
> + */
> +namespace draft {
> +
> +${draft_controls_doc}
> +
> +} /* namespace draft */
> +
> +#ifndef __DOXYGEN__
> +/*
> + * Keep the options definitions hidden from doxygen as it incorrectly parses
> + * them as functions.
> + */
> +${controls_def}
> +
> +namespace draft {
> +
> +${draft_controls_def}
> +
> +} /* namespace draft */
> +#endif
> +
> +/**
> + * \brief List of all supported libcamera options
> + */
> +extern const ControlIdMap options {
> +${controls_map}
> +};
> +
> +} /* namespace options */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml
> new file mode 100644
> index 00000000..3f9304be
> --- /dev/null
> +++ b/src/libcamera/option_ids.yaml
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Copyright (C) 2022, Google Inc.
> +#
> +%YAML 1.2
> +---
> +controls:
> +  - PipelineConfigFile:
> +      type: string
> +      description: |
> +        Some platform supports customizing pipeline configuration preference
> +        for a camera module. The structure and contents of the configuration
> +        file is specific to each platform in YAML format. The PipelineConfigFile
> +        option can be used to specify the path to load the configuration file.

I'm not totally thrilled about using ControlList for this, I feel it may
be a bit over-engineered, but I suppose I'll understand the rationale
better after reviewing the whole series.

> +
> +...

Hi Laurent,
Thanks for the comments.

On Wed, Feb 16, 2022 at 8:57 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Han-lin,
>
> On Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:
> > Introduce option to customize behavior for a camera module, which might
> > alter the camera characteristic and should be set before inspecting it.
> >
> > Re-use the Control generation infrastructure to generate options and define
> > the first 'PipelineConfigFile' option, which allows user to specify
> > preference for pipeline configuration.
>
> When thinking about this, I wasn't envisioning passing the configuration
> file to libcamera through the C++ API, but looking for it in standard
> locations on the file system. Could you explain the reason why you think
> this should be dynamic ?

What I had in mind is to let the application choose the preference per
camera module, and even with the same sensor,
the user might still want different tunings. And we don't need to have
a big configuration file for all parameters.
The option may extend to what we used to set in environment variables,
like LIBCAMERA_RPI_TUNING_FILE for example.

Yes, I think it's up for discussion, and depends on how we think it's
useful and whether it can extend.
If this is the only use case, I do agree it's over-engineered :|

>
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > ---
> >  include/libcamera/ipa/ipa_controls.h |  1 +
> >  include/libcamera/meson.build        |  3 +-
> >  include/libcamera/option_ids.h.in    | 36 +++++++++++++++++
> >  src/libcamera/control_serializer.cpp | 12 ++++++
> >  src/libcamera/option_ids.cpp.in      | 58 ++++++++++++++++++++++++++++
> >  src/libcamera/option_ids.yaml        | 16 ++++++++
> >  6 files changed, 125 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/option_ids.h.in
> >  create mode 100644 src/libcamera/option_ids.cpp.in
> >  create mode 100644 src/libcamera/option_ids.yaml
> >
> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> > index da1a7596..8173a2e5 100644
> > --- a/include/libcamera/ipa/ipa_controls.h
> > +++ b/include/libcamera/ipa/ipa_controls.h
> > @@ -18,6 +18,7 @@ extern "C" {
> >  enum ipa_controls_id_map_type {
> >       IPA_CONTROL_ID_MAP_CONTROLS,
> >       IPA_CONTROL_ID_MAP_PROPERTIES,
> > +     IPA_CONTROL_ID_MAP_OPTIONS,
>
> Do you have use cases for passing a configuration file to the IPA
> modules ? As IPA modules are sandboxed, they don't have unrestricted
> access to the filesystem. I think this could create issues, especially
> if the path to the configuration is under the control of an application.

I've seen cases that read the content of a file and pass it to IPA
with a FD to avoid the problem.
In fact, for ChromeOS, we control the access of IPA to a white-listed
locations, so it's something I can configure.
For other platforms, I think we need a definition of "sandboxed" and
who should do the sandboxing, like libcamera or the device vendor?

>
> >       IPA_CONTROL_ID_MAP_V4L2,
> >  };
> >
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 408b7acf..a9b71bc3 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -31,10 +31,11 @@ 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, property_ids.h, and option_ids.h
> >  control_source_files = [
> >      'control_ids',
> >      'property_ids',
> > +    'option_ids',
> >  ]
> >
> >  control_headers = []
> > diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in
> > new file mode 100644
> > index 00000000..53058a4b
> > --- /dev/null
> > +++ b/include/libcamera/option_ids.h.in
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * option_ids.h - Option ID list
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#pragma once
> > +
> > +#include <stdint.h>
> > +
> > +#include <libcamera/controls.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace options {
> > +
> > +enum {
> > +${ids}
> > +};
> > +
> > +${controls}
> > +
> > +namespace draft {
> > +
> > +${draft_controls}
> > +
> > +} /* namespace draft */
> > +
> > +extern const ControlIdMap options;
> > +
> > +} /* namespace options */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index e87d2362..dc8d2906 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -16,6 +16,7 @@
> >
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> > +#include <libcamera/option_ids.h>
> >  #include <libcamera/property_ids.h>
> >
> >  #include <libcamera/ipa/ipa_controls.h>
> > @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> >       enum ipa_controls_id_map_type idMapType;
> >       if (idmap == &controls::controls)
> >               idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> > +     else if (idmap == &options::options)
> > +             idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
> >       else if (idmap == &properties::properties)
> >               idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
> >       else
> > @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,
> >       enum ipa_controls_id_map_type idMapType;
> >       if (idmap == &controls::controls)
> >               idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> > +     else if (idmap == &options::options)
> > +             idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
> >       else if (idmap == &properties::properties)
> >               idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
> >       else
> > @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> >       case IPA_CONTROL_ID_MAP_CONTROLS:
> >               idMap = &controls::controls;
> >               break;
> > +     case IPA_CONTROL_ID_MAP_OPTIONS:
> > +             idMap = &options::options;
> > +             break;
> >       case IPA_CONTROL_ID_MAP_PROPERTIES:
> >               idMap = &properties::properties;
> >               break;
> > @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
> >                       idMap = &controls::controls;
> >                       break;
> >
> > +             case IPA_CONTROL_ID_MAP_OPTIONS:
> > +                     idMap = &options::options;
> > +                     break;
> > +
> >               case IPA_CONTROL_ID_MAP_PROPERTIES:
> >                       idMap = &properties::properties;
> >                       break;
> > diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in
> > new file mode 100644
> > index 00000000..adfd0633
> > --- /dev/null
> > +++ b/src/libcamera/option_ids.cpp.in
> > @@ -0,0 +1,58 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * option_ids.cpp : Option ID list
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#include <libcamera/option_ids.h>
> > +
> > +/**
> > + * \file option_ids.h
> > + * \brief Camera option identifiers
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \brief Namespace for libcamera options
> > + */
> > +namespace options {
> > +
> > +${controls_doc}
> > +
> > +/**
> > + * \brief Namespace for libcamera draft options
> > + */
> > +namespace draft {
> > +
> > +${draft_controls_doc}
> > +
> > +} /* namespace draft */
> > +
> > +#ifndef __DOXYGEN__
> > +/*
> > + * Keep the options definitions hidden from doxygen as it incorrectly parses
> > + * them as functions.
> > + */
> > +${controls_def}
> > +
> > +namespace draft {
> > +
> > +${draft_controls_def}
> > +
> > +} /* namespace draft */
> > +#endif
> > +
> > +/**
> > + * \brief List of all supported libcamera options
> > + */
> > +extern const ControlIdMap options {
> > +${controls_map}
> > +};
> > +
> > +} /* namespace options */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml
> > new file mode 100644
> > index 00000000..3f9304be
> > --- /dev/null
> > +++ b/src/libcamera/option_ids.yaml
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +#
> > +# Copyright (C) 2022, Google Inc.
> > +#
> > +%YAML 1.2
> > +---
> > +controls:
> > +  - PipelineConfigFile:
> > +      type: string
> > +      description: |
> > +        Some platform supports customizing pipeline configuration preference
> > +        for a camera module. The structure and contents of the configuration
> > +        file is specific to each platform in YAML format. The PipelineConfigFile
> > +        option can be used to specify the path to load the configuration file.
>
> I'm not totally thrilled about using ControlList for this, I feel it may
> be a bit over-engineered, but I suppose I'll understand the rationale
> better after reviewing the whole series.

>
> > +
> > +...
>
> --
> Regards,
>
> Laurent Pinchart

Hi Han-Lin,

On Wed, Feb 16, 2022 at 07:43:07PM +0800, Hanlin Chen wrote:
> On Wed, Feb 16, 2022 at 8:57 AM Laurent Pinchart wrote:
> > On Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:
> > > Introduce option to customize behavior for a camera module, which might
> > > alter the camera characteristic and should be set before inspecting it.
> > >
> > > Re-use the Control generation infrastructure to generate options and define
> > > the first 'PipelineConfigFile' option, which allows user to specify
> > > preference for pipeline configuration.
> >
> > When thinking about this, I wasn't envisioning passing the configuration
> > file to libcamera through the C++ API, but looking for it in standard
> > locations on the file system. Could you explain the reason why you think
> > this should be dynamic ?
> 
> What I had in mind is to let the application choose the preference per
> camera module, and even with the same sensor,
> the user might still want different tunings. And we don't need to have
> a big configuration file for all parameters.
> The option may extend to what we used to set in environment variables,
> like LIBCAMERA_RPI_TUNING_FILE for example.

Yes, that's nice indeed. When it comes to the API between the pipeline
handler and the IPA module, I think using just a file name would be good
enough. When it comes to the application-facing API, however, it's a
good question. I think I need to sleep over it. One thing I'd like to
see is how this will work in the default case, when the application
doesn't provide any option. Will we have a system-wide top-level
configuration file that will contain the paths to per-camera IPA
configuration (tuning) files ? If so, would the application be allowrd
to specify a custom top-level configuration file too ? And possible both
a custom top-level configuration file and also IPA configuration files,
or would the custom top-level file need to point to custom IPA files to
override the latter ?

> Yes, I think it's up for discussion, and depends on how we think it's
> useful and whether it can extend.
> If this is the only use case, I do agree it's over-engineered :|
> 
> > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > ---
> > >  include/libcamera/ipa/ipa_controls.h |  1 +
> > >  include/libcamera/meson.build        |  3 +-
> > >  include/libcamera/option_ids.h.in    | 36 +++++++++++++++++
> > >  src/libcamera/control_serializer.cpp | 12 ++++++
> > >  src/libcamera/option_ids.cpp.in      | 58 ++++++++++++++++++++++++++++
> > >  src/libcamera/option_ids.yaml        | 16 ++++++++
> > >  6 files changed, 125 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/libcamera/option_ids.h.in
> > >  create mode 100644 src/libcamera/option_ids.cpp.in
> > >  create mode 100644 src/libcamera/option_ids.yaml
> > >
> > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> > > index da1a7596..8173a2e5 100644
> > > --- a/include/libcamera/ipa/ipa_controls.h
> > > +++ b/include/libcamera/ipa/ipa_controls.h
> > > @@ -18,6 +18,7 @@ extern "C" {
> > >  enum ipa_controls_id_map_type {
> > >       IPA_CONTROL_ID_MAP_CONTROLS,
> > >       IPA_CONTROL_ID_MAP_PROPERTIES,
> > > +     IPA_CONTROL_ID_MAP_OPTIONS,
> >
> > Do you have use cases for passing a configuration file to the IPA
> > modules ? As IPA modules are sandboxed, they don't have unrestricted
> > access to the filesystem. I think this could create issues, especially
> > if the path to the configuration is under the control of an application.
> 
> I've seen cases that read the content of a file and pass it to IPA
> with a FD to avoid the problem.
> In fact, for ChromeOS, we control the access of IPA to a white-listed
> locations, so it's something I can configure.
> For other platforms, I think we need a definition of "sandboxed" and
> who should do the sandboxing, like libcamera or the device vendor?

Agreed, and that's an interesting discussion. We've always envisioned
the sandboxing to be under the control of the system integrator, with
libcamera providing a default implementation for reference. There is
however no reason to keep the default and Chrome OS sandboxing
implementation separate just for the sake of it, if the Chrome OS
implementation ends up being suitable as a default. We are already using
the mojom IDL internally, and I'd like to modify the serialization
binary format to match mojo too (there's even a GSoC proposal for that
if I recall properly). We however can't depend on the rest of mojo in
the default implementation, but I'm increasingly thinking that an IPA
daemon is likely a better option that spawning a process manually from
libcamera, but we may need to keep spawning as a fallback method, for
unit tests. We would also need to find a way to make testing without
installing easy (libcamera test applications can be run from the build
directory even if a different version of libcamera is available
system-wide, and that's a very very useful feature during the
development that I want to keep). Lots of questions :-)

Anyway, back to the more immediate topic, accessing a tuning file
directly in the IPA module is fine with me for now (we do so already for
RPi). I'm however not sure we need to pass its name through a
ControlList, we have a dedicated function parameter (or structure field,
I don't remember) for RPi, and that seems good enough for now.

> > >       IPA_CONTROL_ID_MAP_V4L2,
> > >  };
> > >
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index 408b7acf..a9b71bc3 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -31,10 +31,11 @@ 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, property_ids.h, and option_ids.h

"option" comes before "property" :-)

> > >  control_source_files = [
> > >      'control_ids',
> > >      'property_ids',
> > > +    'option_ids',

Same here.

> > >  ]
> > >
> > >  control_headers = []
> > > diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in
> > > new file mode 100644
> > > index 00000000..53058a4b
> > > --- /dev/null
> > > +++ b/include/libcamera/option_ids.h.in
> > > @@ -0,0 +1,36 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Google Inc.
> > > + *
> > > + * option_ids.h - Option ID list
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <stdint.h>
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace options {
> > > +
> > > +enum {
> > > +${ids}
> > > +};
> > > +
> > > +${controls}
> > > +
> > > +namespace draft {
> > > +
> > > +${draft_controls}
> > > +
> > > +} /* namespace draft */
> > > +
> > > +extern const ControlIdMap options;
> > > +
> > > +} /* namespace options */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > > index e87d2362..dc8d2906 100644
> > > --- a/src/libcamera/control_serializer.cpp
> > > +++ b/src/libcamera/control_serializer.cpp
> > > @@ -16,6 +16,7 @@
> > >
> > >  #include <libcamera/control_ids.h>
> > >  #include <libcamera/controls.h>
> > > +#include <libcamera/option_ids.h>
> > >  #include <libcamera/property_ids.h>
> > >
> > >  #include <libcamera/ipa/ipa_controls.h>
> > > @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > >       enum ipa_controls_id_map_type idMapType;
> > >       if (idmap == &controls::controls)
> > >               idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> > > +     else if (idmap == &options::options)
> > > +             idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
> > >       else if (idmap == &properties::properties)
> > >               idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
> > >       else
> > > @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,
> > >       enum ipa_controls_id_map_type idMapType;
> > >       if (idmap == &controls::controls)
> > >               idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> > > +     else if (idmap == &options::options)
> > > +             idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
> > >       else if (idmap == &properties::properties)
> > >               idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
> > >       else
> > > @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > >       case IPA_CONTROL_ID_MAP_CONTROLS:
> > >               idMap = &controls::controls;
> > >               break;
> > > +     case IPA_CONTROL_ID_MAP_OPTIONS:
> > > +             idMap = &options::options;
> > > +             break;
> > >       case IPA_CONTROL_ID_MAP_PROPERTIES:
> > >               idMap = &properties::properties;
> > >               break;
> > > @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
> > >                       idMap = &controls::controls;
> > >                       break;
> > >
> > > +             case IPA_CONTROL_ID_MAP_OPTIONS:
> > > +                     idMap = &options::options;
> > > +                     break;
> > > +
> > >               case IPA_CONTROL_ID_MAP_PROPERTIES:
> > >                       idMap = &properties::properties;
> > >                       break;
> > > diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in
> > > new file mode 100644
> > > index 00000000..adfd0633
> > > --- /dev/null
> > > +++ b/src/libcamera/option_ids.cpp.in
> > > @@ -0,0 +1,58 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Google Inc.
> > > + *
> > > + * option_ids.cpp : Option ID list
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#include <libcamera/option_ids.h>
> > > +
> > > +/**
> > > + * \file option_ids.h
> > > + * \brief Camera option identifiers
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \brief Namespace for libcamera options
> > > + */
> > > +namespace options {
> > > +
> > > +${controls_doc}
> > > +
> > > +/**
> > > + * \brief Namespace for libcamera draft options
> > > + */
> > > +namespace draft {
> > > +
> > > +${draft_controls_doc}
> > > +
> > > +} /* namespace draft */
> > > +
> > > +#ifndef __DOXYGEN__
> > > +/*
> > > + * Keep the options definitions hidden from doxygen as it incorrectly parses
> > > + * them as functions.
> > > + */
> > > +${controls_def}
> > > +
> > > +namespace draft {
> > > +
> > > +${draft_controls_def}
> > > +
> > > +} /* namespace draft */
> > > +#endif
> > > +
> > > +/**
> > > + * \brief List of all supported libcamera options
> > > + */
> > > +extern const ControlIdMap options {
> > > +${controls_map}
> > > +};
> > > +
> > > +} /* namespace options */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml
> > > new file mode 100644
> > > index 00000000..3f9304be
> > > --- /dev/null
> > > +++ b/src/libcamera/option_ids.yaml
> > > @@ -0,0 +1,16 @@
> > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > +#
> > > +# Copyright (C) 2022, Google Inc.
> > > +#
> > > +%YAML 1.2
> > > +---
> > > +controls:
> > > +  - PipelineConfigFile:
> > > +      type: string
> > > +      description: |
> > > +        Some platform supports customizing pipeline configuration preference
> > > +        for a camera module. The structure and contents of the configuration
> > > +        file is specific to each platform in YAML format. The PipelineConfigFile
> > > +        option can be used to specify the path to load the configuration file.
> >
> > I'm not totally thrilled about using ControlList for this, I feel it may
> > be a bit over-engineered, but I suppose I'll understand the rationale
> > better after reviewing the whole series.
> >
> > > +
> > > +...