[libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls during configure

[Kieran Bingham]

Hi Laurent,

On 24/03/2021 15:47, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote:
>> The configure operation is synchronous and should not send events back
>> to the pipeline handler.
>>
>> If information needs to be returned from configure it should be handled
>> through the interface directly.
>>
>> Move the initial call to setControls() out of configure() and into the
>> start() method which is called after the IPA running_ state is updated.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/ipa/ipu3/ipu3.cpp | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index a5c5e029f465..34a907f23ef5 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -32,7 +32,7 @@ public:
>>  	{
>>  		return 0;
>>  	}
>> -	int start() override { return 0; }
>> +	int start() override;
>>  	void stop() override {}
>>  
>>  	void configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
>> @@ -63,6 +63,13 @@ private:
>>  	uint32_t maxGain_;
>>  };
>>  
>> +int IPAIPU3::start()
>> +{
>> +	setControls(0);
> 
> This will send an asynchronous event to the pipeline handler, which will
> process it after starting the device. It defeats a little bit the point
> of setting initial controls, doesn't it ?

But it will set the controls before the first request, which was what I
thought was the aim.

I believe setControls() was just an initial skeleton implementation to
demonstrate how to set controls. I don't think it's really been thought
about correctly at all indeed.

But the aim of this patch isn't to 'fix' the design, just stop the call
being made in an invalid state.

The actual design process is separate, and on-going, and I don't think
this is the place to design how the controls should be set.

The actual design of the IPA should determine that.


> The IPU3 IPA protocol seems ill-defined here, as it uses a frame
> counter, and it's not clear if that counter is 0-based or 1-based.
> 
> All this doesn't matter much right now as we never call setControls() at
> runtime, so we could merge the patch as is, but please keep in mind that
> the mechanism is likely broken, and JM would then need to fix it
> (including redesigning part of the IPA protocol) as part of his pending
> IPU3 IPA series. This will involve figuring out how to properly interact
> with delayed controls.

This was detailed in the cover letter:

 Finally patch [6/6] moves an asynchronous callback out of the
 synchronous configure() call, and into start(). This is not expected to
 be a substantial change in functionality currently, but should be
 considered in follow on developments with the IPA.


> 
>> +
>> +	return 0;
>> +}
>> +
>>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
>>  			[[maybe_unused]] const Size &bdsOutputSize)
>>  {
>> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>  	maxGain_ = itGain->second.max().get<int32_t>();
>>  	gain_ = maxGain_;
>> -
>> -	setControls(0);
>>  }
>>  
>>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> 

-- 
Regards
--
Kieran