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

[Jean-Michel Hautbois]

Hi Laurent,

On 16/08/2021 11:49, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> 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]
        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;
>