[libcamera-devel,7/7] android: soraka: Add camera HAL configuration
diff mbox series

Message ID 20210324112527.63701-8-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • android: Add support for HAL configuration file
Related show

Commit Message

Jacopo Mondi March 24, 2021, 11:25 a.m. UTC 
Add camera HAL configuration file for IPU3 Soraka.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/data/soraka/camera_hal.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 src/android/data/soraka/camera_hal.yaml

Comments

Niklas Söderlund March 25, 2021, 10:39 p.m. UTC | #1 
Hi Jacopo,

Thanks for your patch.

On 2021-03-24 12:25:27 +0100, Jacopo Mondi wrote:
> Add camera HAL configuration file for IPU3 Soraka.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/android/data/soraka/camera_hal.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 src/android/data/soraka/camera_hal.yaml
> 
> diff --git a/src/android/data/soraka/camera_hal.yaml b/src/android/data/soraka/camera_hal.yaml
> new file mode 100644
> index 000000000000..489e601ac5d0
> --- /dev/null
> +++ b/src/android/data/soraka/camera_hal.yaml
> @@ -0,0 +1,14 @@
> +- manufacturer: Google
> +- model: Soraka
> +
> +- camera:
> +  - name: "\\_SB_.PCI0.I2C4.CAM1"
> +  - model: "ov5670"
> +  - location: "front"
> +  - rotation: 0
> +
> +- camera:
> +  - name: "\\_SB_.PCI0.I2C2.CAM0"
> +  - model: "ov13858"
> +  - location: "back"
> +  - rotation: 0
> -- 
> 2.30.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 26, 2021, 3:31 a.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Wed, Mar 24, 2021 at 12:25:27PM +0100, Jacopo Mondi wrote:
> Add camera HAL configuration file for IPU3 Soraka.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/data/soraka/camera_hal.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 src/android/data/soraka/camera_hal.yaml
> 
> diff --git a/src/android/data/soraka/camera_hal.yaml b/src/android/data/soraka/camera_hal.yaml
> new file mode 100644
> index 000000000000..489e601ac5d0
> --- /dev/null
> +++ b/src/android/data/soraka/camera_hal.yaml
> @@ -0,0 +1,14 @@
> +- manufacturer: Google
> +- model: Soraka
> +
> +- camera:
> +  - name: "\\_SB_.PCI0.I2C4.CAM1"
> +  - model: "ov5670"
> +  - location: "front"
> +  - rotation: 0
> +
> +- camera:
> +  - name: "\\_SB_.PCI0.I2C2.CAM0"
> +  - model: "ov13858"
> +  - location: "back"
> +  - rotation: 0

I'm sure we can spend an endless amount of time bikeshedding on the YAML
schema, so I'll only make one comment :-)

At the top level, you have a list, with each list entry being a
dictionary with a single key:value pair. There's no guarantee on unicity
of properties with this type of schema, you could for instance have
multiple "manufacturer" list entries. Same for the properties within
each camera.

This would I believe be a more natural usage of YAML (to the extent that
YAML has anything natural about it):

manufacturer: Google
model: Soraka

cameras:
  - name: "\\_SB_.PCI0.I2C4.CAM1"
    model: "ov5670"
    location: "front"
    rotation: 0

  - name: "\\_SB_.PCI0.I2C2.CAM0"
    model: "ov13858"
    location: "back"
    rotation: 0

Or, possibly even better, to guarantee unicity of camera entries,

manufacturer: Google
model: Soraka

cameras:
  "\\_SB_.PCI0.I2C4.CAM1":
    model: "ov5670"
    location: "front"
    rotation: 0

  "\\_SB_.PCI0.I2C2.CAM0":
    model: "ov13858"
    location: "back"
    rotation: 0

The three options produce the following objects in Python:

>>> pprint.pprint(yaml.safe_load(open('yaml1.yaml', 'rb').read()))
[{'manufacturer': 'Google'},
 {'model': 'Soraka'},
 {'camera': [{'name': '\\_SB_.PCI0.I2C4.CAM1'},
             {'model': 'ov5670'},
             {'location': 'front'},
             {'rotation': 0}]},
 {'camera': [{'name': '\\_SB_.PCI0.I2C2.CAM0'},
             {'model': 'ov13858'},
             {'location': 'back'},
             {'rotation': 0}]}]
>>> pprint.pprint(yaml.safe_load(open('yaml2.yaml', 'rb').read()))
{'cameras': [{'location': 'front',
              'model': 'ov5670',
              'name': '\\_SB_.PCI0.I2C4.CAM1',
              'rotation': 0},
             {'location': 'back',
              'model': 'ov13858',
              'name': '\\_SB_.PCI0.I2C2.CAM0',
              'rotation': 0}],
 'manufacturer': 'Google',
 'model': 'Soraka'}
>>> pprint.pprint(yaml.safe_load(open('yaml3.yaml', 'rb').read()))
{'cameras': {'\\_SB_.PCI0.I2C2.CAM0': {'location': 'back',
                                       'model': 'ov13858',
                                       'rotation': 0},
             '\\_SB_.PCI0.I2C4.CAM1': {'location': 'front',
                                       'model': 'ov5670',
                                       'rotation': 0}},
 'manufacturer': 'Google',
 'model': 'Soraka'}
Hirokazu Honda March 26, 2021, 8:27 a.m. UTC | #3 
Hi Jacopo, thanks for this work,

Can you add a document for the yaml configuration?
Does this direction cause us to need preparing those files for all
supported devices?
I am a bit concerned about SKU differences.
Do we have an issue tracker for this?
+Tomasz Figa

Best Regards,
-Hiro

On Fri, Mar 26, 2021 at 12:32 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 24, 2021 at 12:25:27PM +0100, Jacopo Mondi wrote:
> > Add camera HAL configuration file for IPU3 Soraka.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/data/soraka/camera_hal.yaml | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >  create mode 100644 src/android/data/soraka/camera_hal.yaml
> >
> > diff --git a/src/android/data/soraka/camera_hal.yaml b/src/android/data/soraka/camera_hal.yaml
> > new file mode 100644
> > index 000000000000..489e601ac5d0
> > --- /dev/null
> > +++ b/src/android/data/soraka/camera_hal.yaml
> > @@ -0,0 +1,14 @@
> > +- manufacturer: Google
> > +- model: Soraka
> > +
> > +- camera:
> > +  - name: "\\_SB_.PCI0.I2C4.CAM1"
> > +  - model: "ov5670"
> > +  - location: "front"
> > +  - rotation: 0
> > +
> > +- camera:
> > +  - name: "\\_SB_.PCI0.I2C2.CAM0"
> > +  - model: "ov13858"
> > +  - location: "back"
> > +  - rotation: 0
>
> I'm sure we can spend an endless amount of time bikeshedding on the YAML
> schema, so I'll only make one comment :-)
>
> At the top level, you have a list, with each list entry being a
> dictionary with a single key:value pair. There's no guarantee on unicity
> of properties with this type of schema, you could for instance have
> multiple "manufacturer" list entries. Same for the properties within
> each camera.
>
> This would I believe be a more natural usage of YAML (to the extent that
> YAML has anything natural about it):
>
> manufacturer: Google
> model: Soraka
>
> cameras:
>   - name: "\\_SB_.PCI0.I2C4.CAM1"
>     model: "ov5670"
>     location: "front"
>     rotation: 0
>
>   - name: "\\_SB_.PCI0.I2C2.CAM0"
>     model: "ov13858"
>     location: "back"
>     rotation: 0
>
> Or, possibly even better, to guarantee unicity of camera entries,
>
> manufacturer: Google
> model: Soraka
>
> cameras:
>   "\\_SB_.PCI0.I2C4.CAM1":
>     model: "ov5670"
>     location: "front"
>     rotation: 0
>
>   "\\_SB_.PCI0.I2C2.CAM0":
>     model: "ov13858"
>     location: "back"
>     rotation: 0
>
> The three options produce the following objects in Python:
>
> >>> pprint.pprint(yaml.safe_load(open('yaml1.yaml', 'rb').read()))
> [{'manufacturer': 'Google'},
>  {'model': 'Soraka'},
>  {'camera': [{'name': '\\_SB_.PCI0.I2C4.CAM1'},
>              {'model': 'ov5670'},
>              {'location': 'front'},
>              {'rotation': 0}]},
>  {'camera': [{'name': '\\_SB_.PCI0.I2C2.CAM0'},
>              {'model': 'ov13858'},
>              {'location': 'back'},
>              {'rotation': 0}]}]
> >>> pprint.pprint(yaml.safe_load(open('yaml2.yaml', 'rb').read()))
> {'cameras': [{'location': 'front',
>               'model': 'ov5670',
>               'name': '\\_SB_.PCI0.I2C4.CAM1',
>               'rotation': 0},
>              {'location': 'back',
>               'model': 'ov13858',
>               'name': '\\_SB_.PCI0.I2C2.CAM0',
>               'rotation': 0}],
>  'manufacturer': 'Google',
>  'model': 'Soraka'}
> >>> pprint.pprint(yaml.safe_load(open('yaml3.yaml', 'rb').read()))
> {'cameras': {'\\_SB_.PCI0.I2C2.CAM0': {'location': 'back',
>                                        'model': 'ov13858',
>                                        'rotation': 0},
>              '\\_SB_.PCI0.I2C4.CAM1': {'location': 'front',
>                                        'model': 'ov5670',
>                                        'rotation': 0}},
>  'manufacturer': 'Google',
>  'model': 'Soraka'}
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 26, 2021, 12:24 p.m. UTC | #4 
Hi Hiro,

On Fri, Mar 26, 2021 at 05:27:34PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thanks for this work,
> 
> Can you add a document for the yaml configuration?
> Does this direction cause us to need preparing those files for all
> supported devices?
> I am a bit concerned about SKU differences.

The configuration needs to be different only to the extent that the
different SKUs exhibit differences that need to be taken into account on
the camera side. You can think about this file as being, in the worst
case, as SKU-dependent as the vendor camera tuning file. In particular,
we will store lens-related information in the configuration file, so if
two different SKUs have, for instance, different lens focal distances,
then different configuration files will be needed.

> Do we have an issue tracker for this?
> +Tomasz Figa
> 
> Best Regards,
> -Hiro
> 
> On Fri, Mar 26, 2021 at 12:32 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Mar 24, 2021 at 12:25:27PM +0100, Jacopo Mondi wrote:
> > > Add camera HAL configuration file for IPU3 Soraka.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/android/data/soraka/camera_hal.yaml | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >  create mode 100644 src/android/data/soraka/camera_hal.yaml
> > >
> > > diff --git a/src/android/data/soraka/camera_hal.yaml b/src/android/data/soraka/camera_hal.yaml
> > > new file mode 100644
> > > index 000000000000..489e601ac5d0
> > > --- /dev/null
> > > +++ b/src/android/data/soraka/camera_hal.yaml
> > > @@ -0,0 +1,14 @@
> > > +- manufacturer: Google
> > > +- model: Soraka
> > > +
> > > +- camera:
> > > +  - name: "\\_SB_.PCI0.I2C4.CAM1"
> > > +  - model: "ov5670"
> > > +  - location: "front"
> > > +  - rotation: 0
> > > +
> > > +- camera:
> > > +  - name: "\\_SB_.PCI0.I2C2.CAM0"
> > > +  - model: "ov13858"
> > > +  - location: "back"
> > > +  - rotation: 0
> >
> > I'm sure we can spend an endless amount of time bikeshedding on the YAML
> > schema, so I'll only make one comment :-)
> >
> > At the top level, you have a list, with each list entry being a
> > dictionary with a single key:value pair. There's no guarantee on unicity
> > of properties with this type of schema, you could for instance have
> > multiple "manufacturer" list entries. Same for the properties within
> > each camera.
> >
> > This would I believe be a more natural usage of YAML (to the extent that
> > YAML has anything natural about it):
> >
> > manufacturer: Google
> > model: Soraka
> >
> > cameras:
> >   - name: "\\_SB_.PCI0.I2C4.CAM1"
> >     model: "ov5670"
> >     location: "front"
> >     rotation: 0
> >
> >   - name: "\\_SB_.PCI0.I2C2.CAM0"
> >     model: "ov13858"
> >     location: "back"
> >     rotation: 0
> >
> > Or, possibly even better, to guarantee unicity of camera entries,
> >
> > manufacturer: Google
> > model: Soraka
> >
> > cameras:
> >   "\\_SB_.PCI0.I2C4.CAM1":
> >     model: "ov5670"
> >     location: "front"
> >     rotation: 0
> >
> >   "\\_SB_.PCI0.I2C2.CAM0":
> >     model: "ov13858"
> >     location: "back"
> >     rotation: 0
> >
> > The three options produce the following objects in Python:
> >
> > >>> pprint.pprint(yaml.safe_load(open('yaml1.yaml', 'rb').read()))
> > [{'manufacturer': 'Google'},
> >  {'model': 'Soraka'},
> >  {'camera': [{'name': '\\_SB_.PCI0.I2C4.CAM1'},
> >              {'model': 'ov5670'},
> >              {'location': 'front'},
> >              {'rotation': 0}]},
> >  {'camera': [{'name': '\\_SB_.PCI0.I2C2.CAM0'},
> >              {'model': 'ov13858'},
> >              {'location': 'back'},
> >              {'rotation': 0}]}]
> > >>> pprint.pprint(yaml.safe_load(open('yaml2.yaml', 'rb').read()))
> > {'cameras': [{'location': 'front',
> >               'model': 'ov5670',
> >               'name': '\\_SB_.PCI0.I2C4.CAM1',
> >               'rotation': 0},
> >              {'location': 'back',
> >               'model': 'ov13858',
> >               'name': '\\_SB_.PCI0.I2C2.CAM0',
> >               'rotation': 0}],
> >  'manufacturer': 'Google',
> >  'model': 'Soraka'}
> > >>> pprint.pprint(yaml.safe_load(open('yaml3.yaml', 'rb').read()))
> > {'cameras': {'\\_SB_.PCI0.I2C2.CAM0': {'location': 'back',
> >                                        'model': 'ov13858',
> >                                        'rotation': 0},
> >              '\\_SB_.PCI0.I2C4.CAM1': {'location': 'front',
> >                                        'model': 'ov5670',
> >                                        'rotation': 0}},
> >  'manufacturer': 'Google',
> >  'model': 'Soraka'}
> >
Jacopo Mondi March 29, 2021, 12:28 p.m. UTC | #5 
Hi Laurent,

On Fri, Mar 26, 2021 at 05:31:36AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 24, 2021 at 12:25:27PM +0100, Jacopo Mondi wrote:
> > Add camera HAL configuration file for IPU3 Soraka.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/data/soraka/camera_hal.yaml | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >  create mode 100644 src/android/data/soraka/camera_hal.yaml
> >
> > diff --git a/src/android/data/soraka/camera_hal.yaml b/src/android/data/soraka/camera_hal.yaml
> > new file mode 100644
> > index 000000000000..489e601ac5d0
> > --- /dev/null
> > +++ b/src/android/data/soraka/camera_hal.yaml
> > @@ -0,0 +1,14 @@
> > +- manufacturer: Google
> > +- model: Soraka
> > +
> > +- camera:
> > +  - name: "\\_SB_.PCI0.I2C4.CAM1"
> > +  - model: "ov5670"
> > +  - location: "front"
> > +  - rotation: 0
> > +
> > +- camera:
> > +  - name: "\\_SB_.PCI0.I2C2.CAM0"
> > +  - model: "ov13858"
> > +  - location: "back"
> > +  - rotation: 0
>
> I'm sure we can spend an endless amount of time bikeshedding on the YAML
> schema, so I'll only make one comment :-)
>
> At the top level, you have a list, with each list entry being a
> dictionary with a single key:value pair. There's no guarantee on unicity
> of properties with this type of schema, you could for instance have
> multiple "manufacturer" list entries. Same for the properties within
> each camera.
>
> This would I believe be a more natural usage of YAML (to the extent that
> YAML has anything natural about it):
>
> manufacturer: Google
> model: Soraka
>
> cameras:
>   - name: "\\_SB_.PCI0.I2C4.CAM1"
>     model: "ov5670"
>     location: "front"
>     rotation: 0
>
>   - name: "\\_SB_.PCI0.I2C2.CAM0"
>     model: "ov13858"
>     location: "back"
>     rotation: 0
>
> Or, possibly even better, to guarantee unicity of camera entries,
>
> manufacturer: Google
> model: Soraka
>
> cameras:
>   "\\_SB_.PCI0.I2C4.CAM1":
>     model: "ov5670"
>     location: "front"
>     rotation: 0
>
>   "\\_SB_.PCI0.I2C2.CAM0":
>     model: "ov13858"
>     location: "back"
>     rotation: 0
>

This clearly shows how poor my understanding of yaml is.
I'll go with the last proposed option as it guarantees unicity of the
cameras.

Thanks
  j


> The three options produce the following objects in Python:
>
> >>> pprint.pprint(yaml.safe_load(open('yaml1.yaml', 'rb').read()))
> [{'manufacturer': 'Google'},
>  {'model': 'Soraka'},
>  {'camera': [{'name': '\\_SB_.PCI0.I2C4.CAM1'},
>              {'model': 'ov5670'},
>              {'location': 'front'},
>              {'rotation': 0}]},
>  {'camera': [{'name': '\\_SB_.PCI0.I2C2.CAM0'},
>              {'model': 'ov13858'},
>              {'location': 'back'},
>              {'rotation': 0}]}]
> >>> pprint.pprint(yaml.safe_load(open('yaml2.yaml', 'rb').read()))
> {'cameras': [{'location': 'front',
>               'model': 'ov5670',
>               'name': '\\_SB_.PCI0.I2C4.CAM1',
>               'rotation': 0},
>              {'location': 'back',
>               'model': 'ov13858',
>               'name': '\\_SB_.PCI0.I2C2.CAM0',
>               'rotation': 0}],
>  'manufacturer': 'Google',
>  'model': 'Soraka'}
> >>> pprint.pprint(yaml.safe_load(open('yaml3.yaml', 'rb').read()))
> {'cameras': {'\\_SB_.PCI0.I2C2.CAM0': {'location': 'back',
>                                        'model': 'ov13858',
>                                        'rotation': 0},
>              '\\_SB_.PCI0.I2C4.CAM1': {'location': 'front',
>                                        'model': 'ov5670',
>                                        'rotation': 0}},
>  'manufacturer': 'Google',
>  'model': 'Soraka'}
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/data/soraka/camera_hal.yaml b/src/android/data/soraka/camera_hal.yaml
new file mode 100644
index 000000000000..489e601ac5d0
--- /dev/null
+++ b/src/android/data/soraka/camera_hal.yaml
@@ -0,0 +1,14 @@ 
+- manufacturer: Google
+- model: Soraka
+
+- camera:
+  - name: "\\_SB_.PCI0.I2C4.CAM1"
+  - model: "ov5670"
+  - location: "front"
+  - rotation: 0
+
+- camera:
+  - name: "\\_SB_.PCI0.I2C2.CAM0"
+  - model: "ov13858"
+  - location: "back"
+  - rotation: 0