[PATCH v3 20/23] tuning: rkisp1: Add some static modules

[Paul Elder]

On Fri, Jul 05, 2024 at 09:34:23AM +0200, Stefan Klug wrote:
> Hi Paul,
> 
> Thanks for the review.
> 
> On Thu, Jul 04, 2024 at 07:07:32PM +0900, Paul Elder wrote:
> > On Wed, Jul 03, 2024 at 04:17:09PM +0200, Stefan Klug wrote:
> > > Add awb, blc, cproc, filter, and gamma by default to the tuning file. These
> > > don't need any configuration.
> > > 
> > > While at it, sort the modules alphabetically.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  utils/tuning/rkisp1.py | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> > > index fae35783c656..0d279a39ab1b 100755
> > > --- a/utils/tuning/rkisp1.py
> > > +++ b/utils/tuning/rkisp1.py
> > > @@ -15,11 +15,24 @@ from libtuning.generators import YamlOutput
> > >  from libtuning.modules.lsc import LSCRkISP1
> > >  from libtuning.modules.agc import AGCRkISP1
> > >  from libtuning.modules.ccm import CCMRkISP1
> > > -
> > > +from libtuning.modules.static import StaticModule
> > >  
> > >  coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')
> > >  
> > > +awb = StaticModule('Awb')
> > > +blc = StaticModule('BlackLevelCorrection')
> > > +color_processing = StaticModule('ColorProcessing')
> > > +filter = StaticModule('Filter')
> > > +gamma_out = StaticModule('GammaOutCorrection', {'gamma': 2.2})
> > > +
> > >  tuner = lt.Tuner('RkISP1')
> > > +tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
> > > +tuner.add(awb)
> > > +tuner.add(blc)
> > > +tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
> > > +tuner.add(color_processing)
> > > +tuner.add(filter)
> > > +tuner.add(gamma_out)
> > >  tuner.add(LSCRkISP1(
> > >            debug=[lt.Debug.Plot],
> > >            # This is for the actual LSC tuning, and is part of the base LSC
> > > @@ -39,11 +52,11 @@ tuner.add(LSCRkISP1(
> > >            # values.  This can also be a custom function.
> > >            smoothing_function=lt.smoothing.MedianBlur(3),
> > >            ))
> > > -tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
> > > -tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
> > 
> > This changes order of executing the tuning. I thought you want lsc to
> > tune before ccm and agc?
> 
> This was discussed briefly here: https://patchwork.libcamera.org/patch/20479/#30202
> Right now we don't have inner module dependencies. But I'm fine with
> keeping the old order if you prefer.

Ah ok I hadn' seen that. If there's no dependency *now* then yeah we can
come up with a better dependency mechanism later when you do add it
later.

> 
> > 
> > > +
> > >  tuner.set_input_parser(YamlParser())
> > >  tuner.set_output_formatter(YamlOutput())
> > > -tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])
> > > +tuner.set_output_order([AGCRkISP1, awb, blc, CCMRkISP1, color_processing,
> > > +                        filter, gamma_out, LSCRkISP1])
> > 
> > This determines the order of output and thus the order of executing the
> > algorithms.
> 
> Ah ok. I wasn't aware that this also has an influence on the order of
> processing inside the IPA. But now that you say it, it seems logical.
>
> From a user point of view I didn't expect that. Should the order of
> processing be defined/fixed inside the IPA?
> 

I'm not sure. I thought it was fine for users to be able to modify it
via the tuning file (that's how algos are enabled/disabled anyway).

> To get this patch in: Would it be ok for you if I order the modules here
> (and possibly above) in the order of processing inside the actual
> hardware and we postpone the discussion until we get there?

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> > 
> > >  
> > >  if __name__ == '__main__':
> > >      sys.exit(tuner.run(sys.argv))
> > > -- 
> > > 2.43.0
> > >