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

[Laurent Pinchart]

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

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

Laurent Pinchart