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

[Jacopo Mondi]

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 at 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 at 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 at 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
> > >