[libcamera-devel,v5,0/9] Introduce camera properties
mbox series

Message ID 20200206012601.153858-1-jacopo@jmondi.org
Headers show
Series
  • Introduce camera properties
Related show

Message

Jacopo Mondi Feb. 6, 2020, 1:25 a.m. UTC
Hello,
   this series contains the first 9 patches sent as part of the
"Properties and compound controls" series.
I'm mixing up a bit version numbers, I know..

All patches here are tagged, but I have not merged them yet for the following
reasons:

1) what to do with [1/9] ?
   I'm a bit in two minds here, as I see two possible things to happen
   1) we merge the series knowing we'll have to change the header with the
      version that will hit mainline. This should be safe as long as the
      only drivers using this definitions are our downstream devices for
      testing. Although having code in master that we don't want to be used
      even if for a short time makes me feel uncomfortable
   2) we keep the series warm until we don't get kernel support and some users
      in mainline. We would delay this features too long imho.

   Unless you have other ideas, 1 seems to be unavoidable in order to merge
   properties support and start building the definitions of others on top.

2) I have updated the "Rotation" definition with Laurent's suggestions. It
   is now reviewed.

3) I have changed the way enum values are defined in yaml.

   We previously had:

   enum:
     - entry:
       value: x
       description: ".."

   Which is parsed as

   {
        "entry": ,
  	"value" : x,
	"description": "...",
   }

   This required to extract the "entry" value by accessing the first of the
   dictionaries keys. This triggered an error I started noticing when building
   for CrOS, but could have happened earlier, as dictionaries keys are not
   sorted.

   What we actually want is instead

   {
       "entry": {
  	   "value" : x,
	    "description": "...",
       }
   }

   Which is represented in yaml as:

   enum:
     - entry:
         value: x
         description: ".."

   Which is probably also more semantically correct.

Anyway, I have not dropped your tags, but I didn't feel like merging without
pointing out the above.

Thanks
    j

Jacopo Mondi (9):
  [TEMP] include: linux: Update v4l2-controls.h
  libcamera: controls: Parse 'enum' in gen-controls.py
  libcamera: properties: Add location property
  libcamera: properties: Add rotation property
  libcamera: controls: Add default to ControlRange
  libcamera: camera_sensor: Parse camera properties
  libcamera: pipeline_handler: Add Camera properties
  libcamera: camera: Add Camera properties
  android: camera_device: Use Camera properties for static Metadata

 include/libcamera/camera.h               |   1 +
 include/libcamera/controls.h             |   5 +-
 include/libcamera/meson.build            |  26 +-
 include/libcamera/property_ids.h.in      |  33 +++
 include/linux/v4l2-controls.h            |   7 +
 src/android/camera_device.cpp            |  29 +-
 src/libcamera/camera.cpp                 |  16 +-
 src/libcamera/camera_sensor.cpp          |  49 +++-
 src/libcamera/controls.cpp               |  12 +-
 src/libcamera/gen-controls.py            |  50 +++-
 src/libcamera/include/camera_sensor.h    |   7 +-
 src/libcamera/include/pipeline_handler.h |   2 +
 src/libcamera/meson.build                |  21 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
 src/libcamera/pipeline/vimc.cpp          |   4 +
 src/libcamera/pipeline_handler.cpp       |  19 ++
 src/libcamera/property_ids.cpp.in        |  43 +++
 src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
 src/libcamera/v4l2_controls.cpp          |   9 +-
 20 files changed, 662 insertions(+), 34 deletions(-)
 create mode 100644 include/libcamera/property_ids.h.in
 create mode 100644 src/libcamera/property_ids.cpp.in
 create mode 100644 src/libcamera/property_ids.yaml

--
2.24.1

Comments

Kieran Bingham Feb. 6, 2020, 3:57 p.m. UTC | #1
Hi Jacopo,

On 06/02/2020 01:25, Jacopo Mondi wrote:
> Hello,
>    this series contains the first 9 patches sent as part of the
> "Properties and compound controls" series.
> I'm mixing up a bit version numbers, I know..
> 
> All patches here are tagged, but I have not merged them yet for the following
> reasons:
> 
> 1) what to do with [1/9] ?
>    I'm a bit in two minds here, as I see two possible things to happen
>    1) we merge the series knowing we'll have to change the header with the
>       version that will hit mainline. This should be safe as long as the
>       only drivers using this definitions are our downstream devices for
>       testing. Although having code in master that we don't want to be used
>       even if for a short time makes me feel uncomfortable

I would be happy to merge this patch to master for the following:

 - You have already posted the corresponding change upstream.

 - If anything changes in the meanwhile, you'll know first and fast.

 - libcamera is not yet 'stable' so if we have to revert or change this
   this further it's not that big a deal?

 - IIUC - Running this on a kernel which does not have those features
   should not 'explode' but the feature just won't work.

>    2) we keep the series warm until we don't get kernel support and some users
>       in mainline. We would delay this features too long imho.

 - Keeping code out of tree is really painful.

So I would vote for adding the patch to get this series merged.

I would however probably put a comment above the addition saying that
this has been added before it hits mainline linux.

And that comment would simply be automatically removed when we update to
a set of headers that has an upstream implementation.


Anyway, that's just my 5 cents.
--
Kieran


>    Unless you have other ideas, 1 seems to be unavoidable in order to merge
>    properties support and start building the definitions of others on top.
> 
> 2) I have updated the "Rotation" definition with Laurent's suggestions. It
>    is now reviewed.
> 
> 3) I have changed the way enum values are defined in yaml.
> 
>    We previously had:
> 
>    enum:
>      - entry:
>        value: x
>        description: ".."
> 
>    Which is parsed as
> 
>    {
>         "entry": ,
>   	"value" : x,
> 	"description": "...",
>    }
> 
>    This required to extract the "entry" value by accessing the first of the
>    dictionaries keys. This triggered an error I started noticing when building
>    for CrOS, but could have happened earlier, as dictionaries keys are not
>    sorted.
> 
>    What we actually want is instead
> 
>    {
>        "entry": {
>   	   "value" : x,
> 	    "description": "...",
>        }
>    }
> 
>    Which is represented in yaml as:
> 
>    enum:
>      - entry:
>          value: x
>          description: ".."
> 
>    Which is probably also more semantically correct.
> 
> Anyway, I have not dropped your tags, but I didn't feel like merging without
> pointing out the above.
> 
> Thanks
>     j
> 
> Jacopo Mondi (9):
>   [TEMP] include: linux: Update v4l2-controls.h
>   libcamera: controls: Parse 'enum' in gen-controls.py
>   libcamera: properties: Add location property
>   libcamera: properties: Add rotation property
>   libcamera: controls: Add default to ControlRange
>   libcamera: camera_sensor: Parse camera properties
>   libcamera: pipeline_handler: Add Camera properties
>   libcamera: camera: Add Camera properties
>   android: camera_device: Use Camera properties for static Metadata
> 
>  include/libcamera/camera.h               |   1 +
>  include/libcamera/controls.h             |   5 +-
>  include/libcamera/meson.build            |  26 +-
>  include/libcamera/property_ids.h.in      |  33 +++
>  include/linux/v4l2-controls.h            |   7 +
>  src/android/camera_device.cpp            |  29 +-
>  src/libcamera/camera.cpp                 |  16 +-
>  src/libcamera/camera_sensor.cpp          |  49 +++-
>  src/libcamera/controls.cpp               |  12 +-
>  src/libcamera/gen-controls.py            |  50 +++-
>  src/libcamera/include/camera_sensor.h    |   7 +-
>  src/libcamera/include/pipeline_handler.h |   2 +
>  src/libcamera/meson.build                |  21 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
>  src/libcamera/pipeline/vimc.cpp          |   4 +
>  src/libcamera/pipeline_handler.cpp       |  19 ++
>  src/libcamera/property_ids.cpp.in        |  43 +++
>  src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
>  src/libcamera/v4l2_controls.cpp          |   9 +-
>  20 files changed, 662 insertions(+), 34 deletions(-)
>  create mode 100644 include/libcamera/property_ids.h.in
>  create mode 100644 src/libcamera/property_ids.cpp.in
>  create mode 100644 src/libcamera/property_ids.yaml
> 
> --
> 2.24.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Feb. 8, 2020, 1:18 a.m. UTC | #2
Hello,

On Thu, Feb 06, 2020 at 03:57:25PM +0000, Kieran Bingham wrote:
> On 06/02/2020 01:25, Jacopo Mondi wrote:
> > Hello,
> >    this series contains the first 9 patches sent as part of the
> > "Properties and compound controls" series.
> > I'm mixing up a bit version numbers, I know..
> > 
> > All patches here are tagged, but I have not merged them yet for the following
> > reasons:
> > 
> > 1) what to do with [1/9] ?
> >    I'm a bit in two minds here, as I see two possible things to happen
> >    1) we merge the series knowing we'll have to change the header with the
> >       version that will hit mainline. This should be safe as long as the
> >       only drivers using this definitions are our downstream devices for
> >       testing. Although having code in master that we don't want to be used
> >       even if for a short time makes me feel uncomfortable
> 
> I would be happy to merge this patch to master for the following:
> 
>  - You have already posted the corresponding change upstream.
> 
>  - If anything changes in the meanwhile, you'll know first and fast.
> 
>  - libcamera is not yet 'stable' so if we have to revert or change this
>    this further it's not that big a deal?
> 
>  - IIUC - Running this on a kernel which does not have those features
>    should not 'explode' but the feature just won't work.
> 
> >    2) we keep the series warm until we don't get kernel support and some users
> >       in mainline. We would delay this features too long imho.
> 
>  - Keeping code out of tree is really painful.
> 
> So I would vote for adding the patch to get this series merged.
> 
> I would however probably put a comment above the addition saying that
> this has been added before it hits mainline linux.
> 
> And that comment would simply be automatically removed when we update to
> a set of headers that has an upstream implementation.
> 
> 
> Anyway, that's just my 5 cents.

For what it's worth, I agree with the above. It seems to make it 10
cents.

Jacopo, now that we have converged on the definition of controls, could
you resubmit them for the kernel ? I don't think we need to wait until
they get merged, but it's useful to get the ball rolling.

> >    Unless you have other ideas, 1 seems to be unavoidable in order to merge
> >    properties support and start building the definitions of others on top.
> > 
> > 2) I have updated the "Rotation" definition with Laurent's suggestions. It
> >    is now reviewed.
> > 
> > 3) I have changed the way enum values are defined in yaml.
> > 
> >    We previously had:
> > 
> >    enum:
> >      - entry:
> >        value: x
> >        description: ".."
> > 
> >    Which is parsed as
> > 
> >    {
> >         "entry": ,
> >   	"value" : x,
> > 	"description": "...",
> >    }
> > 
> >    This required to extract the "entry" value by accessing the first of the
> >    dictionaries keys. This triggered an error I started noticing when building
> >    for CrOS, but could have happened earlier, as dictionaries keys are not
> >    sorted.
> > 
> >    What we actually want is instead
> > 
> >    {
> >        "entry": {
> >   	   "value" : x,
> > 	    "description": "...",
> >        }
> >    }
> > 
> >    Which is represented in yaml as:
> > 
> >    enum:
> >      - entry:
> >          value: x
> >          description: ".."
> > 
> >    Which is probably also more semantically correct.
> > 
> > Anyway, I have not dropped your tags, but I didn't feel like merging without
> > pointing out the above.
> > 
> > Jacopo Mondi (9):
> >   [TEMP] include: linux: Update v4l2-controls.h
> >   libcamera: controls: Parse 'enum' in gen-controls.py
> >   libcamera: properties: Add location property
> >   libcamera: properties: Add rotation property
> >   libcamera: controls: Add default to ControlRange
> >   libcamera: camera_sensor: Parse camera properties
> >   libcamera: pipeline_handler: Add Camera properties
> >   libcamera: camera: Add Camera properties
> >   android: camera_device: Use Camera properties for static Metadata
> > 
> >  include/libcamera/camera.h               |   1 +
> >  include/libcamera/controls.h             |   5 +-
> >  include/libcamera/meson.build            |  26 +-
> >  include/libcamera/property_ids.h.in      |  33 +++
> >  include/linux/v4l2-controls.h            |   7 +
> >  src/android/camera_device.cpp            |  29 +-
> >  src/libcamera/camera.cpp                 |  16 +-
> >  src/libcamera/camera_sensor.cpp          |  49 +++-
> >  src/libcamera/controls.cpp               |  12 +-
> >  src/libcamera/gen-controls.py            |  50 +++-
> >  src/libcamera/include/camera_sensor.h    |   7 +-
> >  src/libcamera/include/pipeline_handler.h |   2 +
> >  src/libcamera/meson.build                |  21 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
> >  src/libcamera/pipeline/vimc.cpp          |   4 +
> >  src/libcamera/pipeline_handler.cpp       |  19 ++
> >  src/libcamera/property_ids.cpp.in        |  43 +++
> >  src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
> >  src/libcamera/v4l2_controls.cpp          |   9 +-
> >  20 files changed, 662 insertions(+), 34 deletions(-)
> >  create mode 100644 include/libcamera/property_ids.h.in
> >  create mode 100644 src/libcamera/property_ids.cpp.in
> >  create mode 100644 src/libcamera/property_ids.yaml
Laurent Pinchart Feb. 13, 2020, 10:06 p.m. UTC | #3
Hi Jacopo,

On Thu, Feb 06, 2020 at 02:25:52AM +0100, Jacopo Mondi wrote:
> Hello,
>    this series contains the first 9 patches sent as part of the
> "Properties and compound controls" series.
> I'm mixing up a bit version numbers, I know..
> 
> All patches here are tagged, but I have not merged them yet for the following
> reasons:
> 
> 1) what to do with [1/9] ?
>    I'm a bit in two minds here, as I see two possible things to happen
>    1) we merge the series knowing we'll have to change the header with the
>       version that will hit mainline. This should be safe as long as the
>       only drivers using this definitions are our downstream devices for
>       testing. Although having code in master that we don't want to be used
>       even if for a short time makes me feel uncomfortable
>    2) we keep the series warm until we don't get kernel support and some users
>       in mainline. We would delay this features too long imho.
> 
>    Unless you have other ideas, 1 seems to be unavoidable in order to merge
>    properties support and start building the definitions of others on top.
> 
> 2) I have updated the "Rotation" definition with Laurent's suggestions. It
>    is now reviewed.
> 
> 3) I have changed the way enum values are defined in yaml.
> 
>    We previously had:
> 
>    enum:
>      - entry:
>        value: x
>        description: ".."
> 
>    Which is parsed as
> 
>    {
>         "entry": ,
>   	"value" : x,
> 	"description": "...",
>    }
> 
>    This required to extract the "entry" value by accessing the first of the
>    dictionaries keys. This triggered an error I started noticing when building
>    for CrOS, but could have happened earlier, as dictionaries keys are not
>    sorted.
> 
>    What we actually want is instead
> 
>    {
>        "entry": {
>   	   "value" : x,
> 	    "description": "...",
>        }
>    }
> 
>    Which is represented in yaml as:
> 
>    enum:
>      - entry:
>          value: x
>          description: ".."
> 
>    Which is probably also more semantically correct.

I wonder if we should instead go fo

    enum:
      - name: entry
        value: x
	description: ".."

The rationale is that the key ('entry' in your example) is of no
significance to the parser, and should thus likely not be a key. Parsing
becomes also slightly easier, as the key doesn't have to be extracted,
but the 'entry' name can be access through element['name'].

It's probably no big deal, both options can work, but it seems to be
more aligned with how yaml is used in the Linux kernel for DT bindings.
I've asked Maxime Ripard (my go to expert in this domain) about his
opinion, and the only rationale he had to offer was the one I already
mentioned, he had no more compeling argument to go one way or the other.

> Anyway, I have not dropped your tags, but I didn't feel like merging without
> pointing out the above.
> 
> Jacopo Mondi (9):
>   [TEMP] include: linux: Update v4l2-controls.h
>   libcamera: controls: Parse 'enum' in gen-controls.py
>   libcamera: properties: Add location property
>   libcamera: properties: Add rotation property
>   libcamera: controls: Add default to ControlRange
>   libcamera: camera_sensor: Parse camera properties
>   libcamera: pipeline_handler: Add Camera properties
>   libcamera: camera: Add Camera properties
>   android: camera_device: Use Camera properties for static Metadata
> 
>  include/libcamera/camera.h               |   1 +
>  include/libcamera/controls.h             |   5 +-
>  include/libcamera/meson.build            |  26 +-
>  include/libcamera/property_ids.h.in      |  33 +++
>  include/linux/v4l2-controls.h            |   7 +
>  src/android/camera_device.cpp            |  29 +-
>  src/libcamera/camera.cpp                 |  16 +-
>  src/libcamera/camera_sensor.cpp          |  49 +++-
>  src/libcamera/controls.cpp               |  12 +-
>  src/libcamera/gen-controls.py            |  50 +++-
>  src/libcamera/include/camera_sensor.h    |   7 +-
>  src/libcamera/include/pipeline_handler.h |   2 +
>  src/libcamera/meson.build                |  21 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
>  src/libcamera/pipeline/vimc.cpp          |   4 +
>  src/libcamera/pipeline_handler.cpp       |  19 ++
>  src/libcamera/property_ids.cpp.in        |  43 +++
>  src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
>  src/libcamera/v4l2_controls.cpp          |   9 +-
>  20 files changed, 662 insertions(+), 34 deletions(-)
>  create mode 100644 include/libcamera/property_ids.h.in
>  create mode 100644 src/libcamera/property_ids.cpp.in
>  create mode 100644 src/libcamera/property_ids.yaml
Jacopo Mondi Feb. 14, 2020, 8:24 a.m. UTC | #4
Hi Laurent,

On Fri, Feb 14, 2020 at 12:06:00AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Feb 06, 2020 at 02:25:52AM +0100, Jacopo Mondi wrote:
> > Hello,
> >    this series contains the first 9 patches sent as part of the
> > "Properties and compound controls" series.
> > I'm mixing up a bit version numbers, I know..
> >
> > All patches here are tagged, but I have not merged them yet for the following
> > reasons:
> >
> > 1) what to do with [1/9] ?
> >    I'm a bit in two minds here, as I see two possible things to happen
> >    1) we merge the series knowing we'll have to change the header with the
> >       version that will hit mainline. This should be safe as long as the
> >       only drivers using this definitions are our downstream devices for
> >       testing. Although having code in master that we don't want to be used
> >       even if for a short time makes me feel uncomfortable
> >    2) we keep the series warm until we don't get kernel support and some users
> >       in mainline. We would delay this features too long imho.
> >
> >    Unless you have other ideas, 1 seems to be unavoidable in order to merge
> >    properties support and start building the definitions of others on top.
> >
> > 2) I have updated the "Rotation" definition with Laurent's suggestions. It
> >    is now reviewed.
> >
> > 3) I have changed the way enum values are defined in yaml.
> >
> >    We previously had:
> >
> >    enum:
> >      - entry:
> >        value: x
> >        description: ".."
> >
> >    Which is parsed as
> >
> >    {
> >         "entry": ,
> >   	"value" : x,
> > 	"description": "...",
> >    }
> >
> >    This required to extract the "entry" value by accessing the first of the
> >    dictionaries keys. This triggered an error I started noticing when building
> >    for CrOS, but could have happened earlier, as dictionaries keys are not
> >    sorted.
> >
> >    What we actually want is instead
> >
> >    {
> >        "entry": {
> >   	   "value" : x,
> > 	    "description": "...",
> >        }
> >    }
> >
> >    Which is represented in yaml as:
> >
> >    enum:
> >      - entry:
> >          value: x
> >          description: ".."
> >
> >    Which is probably also more semantically correct.
>
> I wonder if we should instead go fo
>
>     enum:
>       - name: entry
>         value: x
> 	description: ".."
>
> The rationale is that the key ('entry' in your example) is of no
> significance to the parser, and should thus likely not be a key. Parsing
> becomes also slightly easier, as the key doesn't have to be extracted,
> but the 'entry' name can be access through element['name'].

Parsing it's not an issue, it's easy enough to access the key.

>
> It's probably no big deal, both options can work, but it seems to be
> more aligned with how yaml is used in the Linux kernel for DT bindings.
> I've asked Maxime Ripard (my go to expert in this domain) about his
> opinion, and the only rationale he had to offer was the one I already
> mentioned, he had no more compeling argument to go one way or the other.
>

The only counter-argument I could have is that we don't define controls
and properties as:

  - name: AeEnable:
    type: bool
    description: |
      Enable or disable the AE.

but rather as

  - AeEnable:
      type: bool
      description: |
        Enable or disable the AE.

If the control name is the key there, the enumeration entry should as
well be the key, as it has a set of associated properties, in the same
way a control has.


>> Anyway, I have not dropped your tags, but I didn't feel like merging without
> > pointing out the above.
> >
> > Jacopo Mondi (9):
> >   [TEMP] include: linux: Update v4l2-controls.h
> >   libcamera: controls: Parse 'enum' in gen-controls.py
> >   libcamera: properties: Add location property
> >   libcamera: properties: Add rotation property
> >   libcamera: controls: Add default to ControlRange
> >   libcamera: camera_sensor: Parse camera properties
> >   libcamera: pipeline_handler: Add Camera properties
> >   libcamera: camera: Add Camera properties
> >   android: camera_device: Use Camera properties for static Metadata
> >
> >  include/libcamera/camera.h               |   1 +
> >  include/libcamera/controls.h             |   5 +-
> >  include/libcamera/meson.build            |  26 +-
> >  include/libcamera/property_ids.h.in      |  33 +++
> >  include/linux/v4l2-controls.h            |   7 +
> >  src/android/camera_device.cpp            |  29 +-
> >  src/libcamera/camera.cpp                 |  16 +-
> >  src/libcamera/camera_sensor.cpp          |  49 +++-
> >  src/libcamera/controls.cpp               |  12 +-
> >  src/libcamera/gen-controls.py            |  50 +++-
> >  src/libcamera/include/camera_sensor.h    |   7 +-
> >  src/libcamera/include/pipeline_handler.h |   2 +
> >  src/libcamera/meson.build                |  21 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
> >  src/libcamera/pipeline/vimc.cpp          |   4 +
> >  src/libcamera/pipeline_handler.cpp       |  19 ++
> >  src/libcamera/property_ids.cpp.in        |  43 +++
> >  src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
> >  src/libcamera/v4l2_controls.cpp          |   9 +-
> >  20 files changed, 662 insertions(+), 34 deletions(-)
> >  create mode 100644 include/libcamera/property_ids.h.in
> >  create mode 100644 src/libcamera/property_ids.cpp.in
> >  create mode 100644 src/libcamera/property_ids.yaml
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Feb. 17, 2020, 12:53 a.m. UTC | #5
Hi Jacopo,

On Fri, Feb 14, 2020 at 09:24:29AM +0100, Jacopo Mondi wrote:
> On Fri, Feb 14, 2020 at 12:06:00AM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 06, 2020 at 02:25:52AM +0100, Jacopo Mondi wrote:
> > > Hello,
> > >    this series contains the first 9 patches sent as part of the
> > > "Properties and compound controls" series.
> > > I'm mixing up a bit version numbers, I know..
> > >
> > > All patches here are tagged, but I have not merged them yet for the following
> > > reasons:
> > >
> > > 1) what to do with [1/9] ?
> > >    I'm a bit in two minds here, as I see two possible things to happen
> > >    1) we merge the series knowing we'll have to change the header with the
> > >       version that will hit mainline. This should be safe as long as the
> > >       only drivers using this definitions are our downstream devices for
> > >       testing. Although having code in master that we don't want to be used
> > >       even if for a short time makes me feel uncomfortable
> > >    2) we keep the series warm until we don't get kernel support and some users
> > >       in mainline. We would delay this features too long imho.
> > >
> > >    Unless you have other ideas, 1 seems to be unavoidable in order to merge
> > >    properties support and start building the definitions of others on top.
> > >
> > > 2) I have updated the "Rotation" definition with Laurent's suggestions. It
> > >    is now reviewed.
> > >
> > > 3) I have changed the way enum values are defined in yaml.
> > >
> > >    We previously had:
> > >
> > >    enum:
> > >      - entry:
> > >        value: x
> > >        description: ".."
> > >
> > >    Which is parsed as
> > >
> > >    {
> > >         "entry": ,
> > >   	"value" : x,
> > > 	"description": "...",
> > >    }
> > >
> > >    This required to extract the "entry" value by accessing the first of the
> > >    dictionaries keys. This triggered an error I started noticing when building
> > >    for CrOS, but could have happened earlier, as dictionaries keys are not
> > >    sorted.
> > >
> > >    What we actually want is instead
> > >
> > >    {
> > >        "entry": {
> > >   	   "value" : x,
> > > 	    "description": "...",
> > >        }
> > >    }
> > >
> > >    Which is represented in yaml as:
> > >
> > >    enum:
> > >      - entry:
> > >          value: x
> > >          description: ".."
> > >
> > >    Which is probably also more semantically correct.
> >
> > I wonder if we should instead go fo
> >
> >     enum:
> >       - name: entry
> >         value: x
> > 	description: ".."
> >
> > The rationale is that the key ('entry' in your example) is of no
> > significance to the parser, and should thus likely not be a key. Parsing
> > becomes also slightly easier, as the key doesn't have to be extracted,
> > but the 'entry' name can be access through element['name'].
> 
> Parsing it's not an issue, it's easy enough to access the key.

Hence the "slightly easier", I didn't say difficult :-)

> > It's probably no big deal, both options can work, but it seems to be
> > more aligned with how yaml is used in the Linux kernel for DT bindings.
> > I've asked Maxime Ripard (my go to expert in this domain) about his
> > opinion, and the only rationale he had to offer was the one I already
> > mentioned, he had no more compeling argument to go one way or the other.
> 
> The only counter-argument I could have is that we don't define controls
> and properties as:
> 
>   - name: AeEnable:
>     type: bool
>     description: |
>       Enable or disable the AE.
> 
> but rather as
> 
>   - AeEnable:
>       type: bool
>       description: |
>         Enable or disable the AE.
> 
> If the control name is the key there, the enumeration entry should as
> well be the key, as it has a set of associated properties, in the same
> way a control has.

I agree with you, I think we should apply the same logic for both, but
I'm tempted to think that

   - name: AeEnable
     type: bool
     description: |
       Enable or disable the AE.

is the right option.

> >> Anyway, I have not dropped your tags, but I didn't feel like merging without
> > > pointing out the above.
> > >
> > > Jacopo Mondi (9):
> > >   [TEMP] include: linux: Update v4l2-controls.h
> > >   libcamera: controls: Parse 'enum' in gen-controls.py
> > >   libcamera: properties: Add location property
> > >   libcamera: properties: Add rotation property
> > >   libcamera: controls: Add default to ControlRange
> > >   libcamera: camera_sensor: Parse camera properties
> > >   libcamera: pipeline_handler: Add Camera properties
> > >   libcamera: camera: Add Camera properties
> > >   android: camera_device: Use Camera properties for static Metadata
> > >
> > >  include/libcamera/camera.h               |   1 +
> > >  include/libcamera/controls.h             |   5 +-
> > >  include/libcamera/meson.build            |  26 +-
> > >  include/libcamera/property_ids.h.in      |  33 +++
> > >  include/linux/v4l2-controls.h            |   7 +
> > >  src/android/camera_device.cpp            |  29 +-
> > >  src/libcamera/camera.cpp                 |  16 +-
> > >  src/libcamera/camera_sensor.cpp          |  49 +++-
> > >  src/libcamera/controls.cpp               |  12 +-
> > >  src/libcamera/gen-controls.py            |  50 +++-
> > >  src/libcamera/include/camera_sensor.h    |   7 +-
> > >  src/libcamera/include/pipeline_handler.h |   2 +
> > >  src/libcamera/meson.build                |  21 +-
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp     |   3 +
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |   3 +
> > >  src/libcamera/pipeline/vimc.cpp          |   4 +
> > >  src/libcamera/pipeline_handler.cpp       |  19 ++
> > >  src/libcamera/property_ids.cpp.in        |  43 +++
> > >  src/libcamera/property_ids.yaml          | 357 +++++++++++++++++++++++
> > >  src/libcamera/v4l2_controls.cpp          |   9 +-
> > >  20 files changed, 662 insertions(+), 34 deletions(-)
> > >  create mode 100644 include/libcamera/property_ids.h.in
> > >  create mode 100644 src/libcamera/property_ids.cpp.in
> > >  create mode 100644 src/libcamera/property_ids.yaml