[libcamera-devel,v3,8/8] ipa: rkisp1: Add OV5675 tuning file

18147 diff mbox series

Add the OV5675 tuning file with default values and enabled AF algorithm.

Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
---
 src/ipa/rkisp1/data/meson.build |  1 +
 src/ipa/rkisp1/data/ov5675.yaml | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)
 create mode 100644 src/ipa/rkisp1/data/ov5675.yaml

Hi Daniel,
   thank you for this big chunk of work. I still haven't got to
run-time test it, let me point out a few things I've noticed here
first

On Thu, Jan 19, 2023 at 09:41:12AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> Add the OV5675 tuning file with default values and enabled AF algorithm.
>
> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> ---
>  src/ipa/rkisp1/data/meson.build |  1 +
>  src/ipa/rkisp1/data/ov5675.yaml | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
>  create mode 100644 src/ipa/rkisp1/data/ov5675.yaml
>
> diff --git a/src/ipa/rkisp1/data/meson.build b/src/ipa/rkisp1/data/meson.build
> index c3b4e388..03d71cbf 100644
> --- a/src/ipa/rkisp1/data/meson.build
> +++ b/src/ipa/rkisp1/data/meson.build
> @@ -3,6 +3,7 @@
>  conf_files = files([
>      'imx219.yaml',
>      'ov5640.yaml',
> +    'ov5675.yaml',
>      'uncalibrated.yaml',
>  ])
>
> diff --git a/src/ipa/rkisp1/data/ov5675.yaml b/src/ipa/rkisp1/data/ov5675.yaml
> new file mode 100644
> index 00000000..2c088d18
> --- /dev/null
> +++ b/src/ipa/rkisp1/data/ov5675.yaml
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: CC0-1.0
> +%YAML 1.1
> +---
> +version: 1
> +algorithms:
> +  - Af:
> +      min-vcm-position: 0
> +      max-vcm-position: 1023
> +      coarse-search-step: 30
> +      fine-search-step: 1
> +      fine-scan-range: 0.05
> +      max-variance-change: 0.5
> +      wait-frames-lens: 2 # tuned for 30fps stream
> +      isp-threshold: 128
> +      isp-var-shift: 4

Do these information belong to the -camera sensor- tuning file ?
The same sensor, depending on how the camera module is assembled,
might be paired with different lenses/VCM combo.

My gut feeling is that would be better expressed with something like a
lens helper, similar to what we have in libipa's CameraSensorHelper ?

What do you think ?

> +  - Agc:
> +  - Awb:
> +  - BlackLevelCorrection:
> +  - ColorProcessing:

To be honest, I wonder if it was a good idea to list what algorithms
to enable in the sensor tuning file.

The decision of what algorithms to use, and possibly any tuning data
associated with them, are maybe properties that need to be associated to
a global device tuning file. But that's not a discussion that should
block this series.

Thanks
   j

> +...
> --
> 2.39.0
>

Hi Jacopo,

On Mon, Feb 6, 2023 at 10:12 AM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Daniel,
>    thank you for this big chunk of work. I still haven't got to
> run-time test it, let me point out a few things I've noticed here
> first
>
> On Thu, Jan 19, 2023 at 09:41:12AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > Add the OV5675 tuning file with default values and enabled AF algorithm.
> >
> > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > ---
> >  src/ipa/rkisp1/data/meson.build |  1 +
> >  src/ipa/rkisp1/data/ov5675.yaml | 20 ++++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> >  create mode 100644 src/ipa/rkisp1/data/ov5675.yaml
> >
> > diff --git a/src/ipa/rkisp1/data/meson.build b/src/ipa/rkisp1/data/meson.build
> > index c3b4e388..03d71cbf 100644
> > --- a/src/ipa/rkisp1/data/meson.build
> > +++ b/src/ipa/rkisp1/data/meson.build
> > @@ -3,6 +3,7 @@
> >  conf_files = files([
> >      'imx219.yaml',
> >      'ov5640.yaml',
> > +    'ov5675.yaml',
> >      'uncalibrated.yaml',
> >  ])
> >
> > diff --git a/src/ipa/rkisp1/data/ov5675.yaml b/src/ipa/rkisp1/data/ov5675.yaml
> > new file mode 100644
> > index 00000000..2c088d18
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/data/ov5675.yaml
> > @@ -0,0 +1,20 @@
> > +# SPDX-License-Identifier: CC0-1.0
> > +%YAML 1.1
> > +---
> > +version: 1
> > +algorithms:
> > +  - Af:
> > +      min-vcm-position: 0
> > +      max-vcm-position: 1023
> > +      coarse-search-step: 30
> > +      fine-search-step: 1
> > +      fine-scan-range: 0.05
> > +      max-variance-change: 0.5
> > +      wait-frames-lens: 2 # tuned for 30fps stream
> > +      isp-threshold: 128
> > +      isp-var-shift: 4
>
> Do these information belong to the -camera sensor- tuning file ?
> The same sensor, depending on how the camera module is assembled,
> might be paired with different lenses/VCM combo.
>
> My gut feeling is that would be better expressed with something like a
> lens helper, similar to what we have in libipa's CameraSensorHelper ?
>
> What do you think ?
>

The first two, "min-vcm-position" and "max-vcm-position", indeed are
not a part of the sensor, but describe the lens range limits.
This is a quick workaround for the missing possibility to pass lens
configuration to the algorithm during algorithm init.
It would be good to solve this problem, get rid of the manual config
and read the limits from the v4l subdevice. But we are back to the old
question about the interface. As a simple solution, I can just add the
lens range limits to the IPAContext and read it in the AF algorithm
during initialization. Or do we want to have some common parameter for
that and hide the hardware values [1, 1023] behind some abstract range,
for example [0, 1]?

The rest of the parameters are more user-specific and allow to
configure AF for specific usage (We can have a very fast, but less
precise auto-focus or the other way). This is probably more connected
to your second question, whether we should include algorithms
configuration in the sensor tuning file at all.

> > +  - Agc:
> > +  - Awb:
> > +  - BlackLevelCorrection:
> > +  - ColorProcessing:
>
> To be honest, I wonder if it was a good idea to list what algorithms
> to enable in the sensor tuning file.
>
> The decision of what algorithms to use, and possibly any tuning data
> associated with them, are maybe properties that need to be associated to
> a global device tuning file. But that's not a discussion that should
> block this series.

Currently, all the tuning is done in the IPA algorithms, so it
would be difficult to distinguish which paramters should go to which
file.

Best regards
Daniel

>
> Thanks
>    j
>
> > +...
> > --
> > 2.39.0
> >

Hi Daniel

On Mon, Feb 06, 2023 at 10:59:04AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> Hi Jacopo,
>
> On Mon, Feb 6, 2023 at 10:12 AM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Daniel,
> >    thank you for this big chunk of work. I still haven't got to
> > run-time test it, let me point out a few things I've noticed here
> > first
> >
> > On Thu, Jan 19, 2023 at 09:41:12AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > > Add the OV5675 tuning file with default values and enabled AF algorithm.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > > ---
> > >  src/ipa/rkisp1/data/meson.build |  1 +
> > >  src/ipa/rkisp1/data/ov5675.yaml | 20 ++++++++++++++++++++
> > >  2 files changed, 21 insertions(+)
> > >  create mode 100644 src/ipa/rkisp1/data/ov5675.yaml
> > >
> > > diff --git a/src/ipa/rkisp1/data/meson.build b/src/ipa/rkisp1/data/meson.build
> > > index c3b4e388..03d71cbf 100644
> > > --- a/src/ipa/rkisp1/data/meson.build
> > > +++ b/src/ipa/rkisp1/data/meson.build
> > > @@ -3,6 +3,7 @@
> > >  conf_files = files([
> > >      'imx219.yaml',
> > >      'ov5640.yaml',
> > > +    'ov5675.yaml',
> > >      'uncalibrated.yaml',
> > >  ])
> > >
> > > diff --git a/src/ipa/rkisp1/data/ov5675.yaml b/src/ipa/rkisp1/data/ov5675.yaml
> > > new file mode 100644
> > > index 00000000..2c088d18
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/data/ov5675.yaml
> > > @@ -0,0 +1,20 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +%YAML 1.1
> > > +---
> > > +version: 1
> > > +algorithms:
> > > +  - Af:
> > > +      min-vcm-position: 0
> > > +      max-vcm-position: 1023
> > > +      coarse-search-step: 30
> > > +      fine-search-step: 1
> > > +      fine-scan-range: 0.05
> > > +      max-variance-change: 0.5
> > > +      wait-frames-lens: 2 # tuned for 30fps stream
> > > +      isp-threshold: 128
> > > +      isp-var-shift: 4
> >
> > Do these information belong to the -camera sensor- tuning file ?
> > The same sensor, depending on how the camera module is assembled,
> > might be paired with different lenses/VCM combo.
> >
> > My gut feeling is that would be better expressed with something like a
> > lens helper, similar to what we have in libipa's CameraSensorHelper ?
> >
> > What do you think ?
> >
>
> The first two, "min-vcm-position" and "max-vcm-position", indeed are
> not a part of the sensor, but describe the lens range limits.
> This is a quick workaround for the missing possibility to pass lens
> configuration to the algorithm during algorithm init.
> It would be good to solve this problem, get rid of the manual config
> and read the limits from the v4l subdevice. But we are back to the old
> question about the interface. As a simple solution, I can just add the
> lens range limits to the IPAContext and read it in the AF algorithm
> during initialization. Or do we want to have some common parameter for
> that and hide the hardware values [1, 1023] behind some abstract range,
> for example [0, 1]?
>
> The rest of the parameters are more user-specific and allow to
> configure AF for specific usage (We can have a very fast, but less
> precise auto-focus or the other way). This is probably more connected
> to your second question, whether we should include algorithms
> configuration in the sensor tuning file at all.
>

I had a chat with the rest of the group about this and I was pointed
to two things:
- we already have lens-specific data in the LSC tables
- we'll have to move to a "camera module" configuration file sooner
  than later, so it is acceptable to have these information here

So, for the time being, I think it's fine to have the settings here!

I guess this mean
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks!

> > > +  - Agc:
> > > +  - Awb:
> > > +  - BlackLevelCorrection:
> > > +  - ColorProcessing:
> >
> > To be honest, I wonder if it was a good idea to list what algorithms
> > to enable in the sensor tuning file.
> >
> > The decision of what algorithms to use, and possibly any tuning data
> > associated with them, are maybe properties that need to be associated to
> > a global device tuning file. But that's not a discussion that should
> > block this series.
>
> Currently, all the tuning is done in the IPA algorithms, so it
> would be difficult to distinguish which paramters should go to which
> file.
>
> Best regards
> Daniel
>
> >
> > Thanks
> >    j
> >
> > > +...
> > > --
> > > 2.39.0
> > >