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

[Jacopo Mondi]

Hi Laurent

On Thu, Jul 04, 2024 at 04:38:34PM GMT, Laurent Pinchart wrote:
> 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 ?
>

This indeed is not possible with the current uAPI. However, if to do
we need to expand the number of enablment states

enum rkisp1_ext_params_block_enable {
	RKISP1_EXT_PARAMS_BLOCK_DISABLE,
	RKISP1_EXT_PARAMS_BLOCK_ENABLE,
};

in example by adding an additional entry as

enum rkisp1_ext_params_block_enable {
	RKISP1_EXT_PARAMS_BLOCK_DISABLE,
	RKISP1_EXT_PARAMS_BLOCK_ENABLE,
        RKISP1_EXT_PARAMS_BLOCK_ENABLE_NO_CONFIG,
};


would could do so later without breaking the existing users

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