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

[Kieran Bingham]

Hi Laurent,

On 24/03/2021 16:40, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wed, Mar 24, 2021 at 04:14:43PM +0000, Kieran Bingham wrote:
>> On 24/03/2021 15:47, Laurent Pinchart wrote:
>>> 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.
> 
> That depends on how fast the first request is queued after start()
> though, as the event from the IPA could race with the application and
> camera manager thread. Even if the controls are set before the first
> request is queued, if the sensor is started before setting controls,
> then the first controls will be subject to delays, while if they're set
> when the sensor is stopped, they will be taken into account immediately.
> 
> We don't have to handle all of this as part of this patch, but I want
> everybody to be aware of this issue as it will become a problem pretty
> quickly.
> 
>> 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.
> 
> We agree on that, yes. One option could be to just drop the
> setControls() call too, until Jean-Michel's patches get merged :-)
> 
>> 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.
> 
> I should have read the cover letter first :-S
> 
> Now that we're all aware that there's an issue that needs to be fixed
> later,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> As an initial idea, fixing the issue correctly could involve returning
> initial controls from the start() function instead of going through a
> separate async call.
> 
> Another thing that needs to be taken into account is that, in the
> isolated case, the event sent by setControls() (with this patch applied)
> will be received by the libcamera process before the IPC reply to the
> start() call. This means that we may want to forbid sending any
> asynchronous events from start() (with appropriate documentation).

If we want to do that, we can change the running_ state after start()
completes, rather than before.

(There's already a patch to change running_ to state_ in my next series,
so lets try to remember to discuss that there).


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