Message ID | 20200206012601.153858-1-jacopo@jmondi.org |
---|---|
Headers | show |
Series |
|
Related | show |
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 >
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
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
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
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