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

[Laurent Pinchart]

On Thu, Jul 04, 2024 at 04:14:19PM +0200, Jacopo Mondi wrote:
> 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,
> };

And we may need the ability to configure the block while keeping it
disabled :-/

> would could do so later without breaking the existing users

That, or we could omit the data after the header. It would decouple
enabling (set through the enable flag) and config (set through inclusion
of the data after the header). This should also not break existing
users, although we may want to then drop the structures that wrap the
header and configuration data.

It may also be more troublesome for userspace to use, as code would need
to know whether or not configuration data is needed at the time the
block is allocated. We would need to prototype first.

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