[libcamera-devel] [PATCH] ipa: rkisp1: Do not set controls during configure

[paul.elder at ideasonboard.com]

Hi Laurent,

On Fri, Mar 26, 2021 at 05:00:45AM +0200, Laurent Pinchart wrote:
> Hi Sebastian,
> 
> Thank you for the patch.
> 
> On Thu, Mar 25, 2021 at 06:42:54PM +0100, Sebastian Fricke 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: Sebastian Fricke <sebastian.fricke at posteo.net>
> > ---
> >  src/ipa/rkisp1/rkisp1.cpp | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index d2a10bb9..b0698903 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -32,7 +32,7 @@ class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface
> >  {
> >  public:
> >  	int init(unsigned int hwRevision) override;
> > -	int start() override { return 0; }
> > +	int start() override;
> >  	void stop() override {}
> >  
> >  	int configure(const CameraSensorInfo &info,
> > @@ -80,6 +80,13 @@ int IPARkISP1::init(unsigned int hwRevision)
> >  	return 0;
> >  }
> >  
> > +int IPARkISP1::start()
> > +{
> > +        setControls(0);
> 
> The IPA will send the queueFrameAction event to the pipeline handler
> before the start() function completes. This will cause different
> behaviours in the isolated and non-isolated cases.
> 
> In the isolated case, the message corresponding to queueFrameAction will
> be send to the pipeline handler before the response to start(), and will
> thus be processed first. In the non-isolated case, the deferred call
> related to queueFrameAction will be queued to the pipeline handler
> thread, but will only be processed once control returns to the event
> loop, which will happen after start() completes.
> 
> I'm OK with this patch as a short term fix, as it fixes a real issue and
> we only run the rkisp1 IPA without isolation at this point.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> We however need to figure out a correct fix for the IPC case. Paul,
> what's your opinion on this, how should we handle it ?

As we discussed on irc, I think a good solution would be to disallow
emitting signals from start(). We've allowed custom return values from
start(), which competes with emitting signals from start(). So I think
if somebody wants to emit signals from the IPA during start(), they
should instead return the values directly, and then the pipeline handler
can call the signal handler on the return values (or partially
reimplment the signal handler).

I guess I'll have to add this to the IPA guide and we'll have to add
some more assertions (with a Starting state?).


Thanks,

Paul

> > +
> > +        return 0;
> > +}
> > +
> >  /**
> >   * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
> >   * if the connected sensor does not provide enough information to properly
> > @@ -121,7 +128,6 @@ int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> >  		<< "Exposure: " << minExposure_ << "-" << maxExposure_
> >  		<< " Gain: " << minGain_ << "-" << maxGain_;
> >  
> > -	setControls(0);
> >  	return 0;
> >  }
> >