[libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a prepare() call to Algorithm

[Laurent Pinchart]

Hi Jean-Michel,

On Wed, Aug 18, 2021 at 08:47:28AM +0200, Jean-Michel Hautbois wrote:
> On 16/08/2021 11:49, Laurent Pinchart wrote:
> > On Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote:
> >> On 16/08/2021 11:41, Laurent Pinchart wrote:
> >>> On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote:
> >>>> On 15/08/2021 20:02, Laurent Pinchart wrote:
> >>>>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:
> >>>>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a
> >>>>>> EventFillParams. When this is called, the algorithms should fill every
> >>>>>> field in the parameter structure they need to update.
> >>>>>>
> >>>>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let
> >>>>>> the Grid algorithm update the grid field.
> >>>>>> This leads to removing this same update from the Awb algorithm.
> >>>>>>
> >>>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >>>>>> ---
> >>>>>>  src/ipa/ipu3/algorithms/algorithm.h | 2 ++
> >>>>>>  src/ipa/ipu3/algorithms/grid.cpp    | 6 ++++++
> >>>>>>  src/ipa/ipu3/algorithms/grid.h      | 1 +
> >>>>>>  src/ipa/ipu3/ipu3.cpp               | 3 +++
> >>>>>>  src/ipa/ipu3/ipu3_awb.cpp           | 1 -
> >>>>>>  5 files changed, 12 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> >>>>>> index c1b37276..b571f55a 100644
> >>>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h
> >>>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> >>>>>> @@ -26,6 +26,8 @@ public:
> >>>>>>  	{
> >>>>>>  		return 0;
> >>>>>>  	}
> >>>>>> +
> >>>>>> +	virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}
> >>>>>
> >>>>> Line wrap.
> >>>>>
> >>>>> Could you add a patch after 02/10 that adds all functions (configure(),
> >>>>> prepare() and process()) to the Algorithm class, and call them in the
> >>>>> IPAIPU3 class as appropriate ? That patch should include the
> >>>>> documentation of the functions, to explain how they're used. After that
> >>>>> the rest of the series can begin moving the algorithms to the new class.
> >>>>
> >>>> I did that in the first place, and it was a bit of a heavy patch. So I
> >>>> decided to split it to introduce each of configure/prepare/process calls
> >>>> step by step...
> >>>
> >>> The issue with this approach is that it's harder to have an overview of
> >>> the proposed API. Is it *that* heavy to define a class with three
> >>> abstract functions and their documentation ?
> >>
> >> No, but as we already have a process() call in AGC it must be rewritten
> >> accordingly.
> > 
> > The existing process() function takes different parameters, so the
> > compiler shouldn't complain. Worst case it could be renamed in a small
> > patch at the beginning of the series to move it out of the way. I agree
> > that adapting the algorithm is best done in a patch separate from the
> > introduction of the new Algorithm structure.
> 
> The compiler does not agree with you :-).
> 
> ../src/ipa/ipu3/ipu3_agc.h:33:7: error:
> 'libcamera::ipa::ipu3::IPU3Agc::process' hides overloaded virtual
> function [-Werror,-Woverloaded-virtual]

It's a warning that we treat as an error, as all warnings. You could
disable the warning temporarily and then restore it as part of the
series to keep patches from getting large.

>         void process(const ipu3_uapi_stats_3a *stats, uint32_t
> &exposure, double &gain);
>              ^
> ../src/ipa/ipu3/algorithms/algorithm.h:26:15: note: hidden overloaded
> virtual function 'libcamera::ipa::ipu3::Algorithm::process' declared
> here: different number of parameters (2 vs 3)
>         virtual void process(IPAContext &context, ipu3_uapi_stats_3a
> &stats);
> 
> >>>>>>  };
> >>>>>>  
> >>>>>>  } /* namespace ipa::ipu3 */
> >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp
> >>>>>> index 3578f41b..e42a760d 100644
> >>>>>> --- a/src/ipa/ipu3/algorithms/grid.cpp
> >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp
> >>>>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>  
> >>>>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params &params)
> >>>>>> +{
> >>>>>> +	/* Update the IPU3 parameters with new calculated grid */
> >>>>>> +	params.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;
> >>>>>
> >>>>> And this is where the spaghetti dish begins :-( It should be the
> >>>>> responsibility of the AWB algorithm to set params.acc_param.awb.
> >>>>
> >>>> Yes, I moved it at least 5 times while writing the patches as I could
> >>>> not decide where it should be :-p.
> >>>
> >>> As commented in patch 03/10, I don't think "grid" should be an
> >>> algorithm.
> >>>
> >>>>>> +}
> >>>>>> +
> >>>>>>  } /* namespace ipa::ipu3::algorithms */
> >>>>>>  
> >>>>>>  } /* namespace libcamera */
> >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h
> >>>>>> index b4a51b42..89399bd2 100644
> >>>>>> --- a/src/ipa/ipu3/algorithms/grid.h
> >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.h
> >>>>>> @@ -19,6 +19,7 @@ public:
> >>>>>>  	~Grid() = default;
> >>>>>>  
> >>>>>>  	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> >>>>>> +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
> >>>>>>  };
> >>>>>>  
> >>>>>>  } /* namespace ipa::ipu3::algorithms */
> >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >>>>>> index ef7fec86..394a7a45 100644
> >>>>>> --- a/src/ipa/ipu3/ipu3.cpp
> >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp
> >>>>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> >>>>>>  
> >>>>>>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >>>>>>  {
> >>>>>> +	for (auto const &algo : algorithms_)
> >>>>>> +		algo->prepare(context_, params_);
> >>>>>> +
> >>>>>>  	if (agcAlgo_->updateControls())
> >>>>>>  		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> >>>>>>  
> >>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> >>>>>> index 4ee5ee6f..911760b3 100644
> >>>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp
> >>>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> >>>>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
> >>>>>>  	params.acc_param.awb.config.rgbs_thr_r = 8191;
> >>>>>>  	params.acc_param.awb.config.rgbs_thr_gb = 8191;
> >>>>>>  	params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;
> >>>>>> -	params.acc_param.awb.config.grid = bdsGrid;
> >>>>>>  	awbGrid_ = bdsGrid;
> >>>>>>  
> >>>>>>  	params.use.acc_bnr = 1;

-- 
Regards,

Laurent Pinchart