[PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters abstraction

[Laurent Pinchart]

On Thu, Jul 04, 2024 at 02:33:19PM +0200, Jacopo Mondi wrote:
> Hi Laurent
> 
>   only one comment on dpf which is the only one with a more convoluted
> handling. The rest looks very nice!
> 
> On Thu, Jul 04, 2024 at 01:52:25AM GMT, Laurent Pinchart wrote:
> > Use the new ISP parameters abstraction class RkISP1Params to access the
> > ISP parameters in the IPA algorithms. The class replaces the pointer to
> > the rkisp1_params_cfg structure passed to the algorithms' prepare()
> > function, and is used to access individual parameters blocks.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp    | 46 +++++++++++------------
> >  src/ipa/rkisp1/algorithms/agc.h      |  2 +-
> >  src/ipa/rkisp1/algorithms/awb.cpp    | 56 ++++++++++++----------------
> >  src/ipa/rkisp1/algorithms/awb.h      |  2 +-
> >  src/ipa/rkisp1/algorithms/blc.cpp    | 18 ++++-----
> >  src/ipa/rkisp1/algorithms/blc.h      |  3 +-
> >  src/ipa/rkisp1/algorithms/ccm.cpp    | 14 ++-----
> >  src/ipa/rkisp1/algorithms/ccm.h      |  4 +-
> >  src/ipa/rkisp1/algorithms/cproc.cpp  | 13 +++----
> >  src/ipa/rkisp1/algorithms/cproc.h    |  2 +-
> >  src/ipa/rkisp1/algorithms/dpcc.cpp   |  9 ++---
> >  src/ipa/rkisp1/algorithms/dpcc.h     |  2 +-
> >  src/ipa/rkisp1/algorithms/dpf.cpp    | 28 +++++++-------
> >  src/ipa/rkisp1/algorithms/dpf.h      |  2 +-
> >  src/ipa/rkisp1/algorithms/filter.cpp | 51 ++++++++++++-------------
> >  src/ipa/rkisp1/algorithms/filter.h   |  2 +-
> >  src/ipa/rkisp1/algorithms/goc.cpp    | 15 ++++----
> >  src/ipa/rkisp1/algorithms/goc.h      |  2 +-
> >  src/ipa/rkisp1/algorithms/gsl.cpp    | 20 ++++------
> >  src/ipa/rkisp1/algorithms/gsl.h      |  2 +-
> >  src/ipa/rkisp1/algorithms/lsc.cpp    | 27 ++++++--------
> >  src/ipa/rkisp1/algorithms/lsc.h      |  4 +-
> >  src/ipa/rkisp1/module.h              |  3 +-
> >  src/ipa/rkisp1/rkisp1.cpp            | 12 ++----
> >  24 files changed, 148 insertions(+), 191 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> 
> [snip]
> 
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > index abf957288550..b3f4446631fc 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> > @@ -215,15 +215,26 @@ void Dpf::queueRequest(IPAContext &context,
> >   * \copydoc libcamera::ipa::Algorithm::prepare
> >   */
> >  void Dpf::prepare(IPAContext &context, const uint32_t frame,
> > -		  IPAFrameContext &frameContext, rkisp1_params_cfg *params)
> > +		  IPAFrameContext &frameContext, RkISP1Params *params)
> >  {
> > +	if (!frameContext.dpf.update && frame > 0)
> > +		return;
> > +
> > +	RkISP1Params::State state = frameContext.dpf.denoise
> > +				  ? RkISP1Params::State::Enable
> > +				  : RkISP1Params::State::Disable;
> > +	auto config = params->block<Block::Dpf>(state);
> > +
> >  	if (frame == 0) {
> > -		params->others.dpf_config = config_;
> > -		params->others.dpf_strength_config = strengthConfig_;
> > +		auto strengthConfig = params->block<Block::DpfStrength>(state);
> 
> The dpf strength handler in the kernel ignores the enable state, and
> strengths are configured only when frame == 0, so I would use Enable
> here for clarity (even if it has not practical differences).

OK.

> > +		*strengthConfig = strengthConfig_;
> > +
> > +		*config = config_;
> 
> This, however needs to be done everytime we enable the block. The
> driver sees that state == enable and programs the dpf block with the
> content of the config structure. If frame > 0 &&
> frameContext.dpf.denoise == true, the kernel will program the block
> with 0-initialized data if I read it right.
> 
> I would set *config = config_ for all frames if dpf.update &&
> dpf.denoise ?

Yes there's an issue indeed.

This leads to an annoying question: do we need the ability to
enable/disable blocks without reconfiguring them ?

> >
> >  		const auto &awb = context.configuration.awb;
> >  		const auto &lsc = context.configuration.lsc;
> > -		auto &mode = params->others.dpf_config.gain.mode;
> > +
> > +		auto &mode = config->gain.mode;
> >
> >  		/*
> >  		 * The DPF needs to take into account the total amount of
> > @@ -241,15 +252,6 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,
> >  			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> >  		else
> >  			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> > -
> > -		params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF |
> > -					     RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;
> > -	}
> > -
> > -	if (frameContext.dpf.update) {
> > -		params->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;
> > -		if (frameContext.dpf.denoise)
> > -			params->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;
> >  	}
> >  }
> >

[snip]

-- 
Regards,

Laurent Pinchart