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

[Laurent Pinchart]

Hi Sebastian,

One more comment.

On Fri, Mar 26, 2021 at 05:00:46AM +0200, Laurent Pinchart wrote:
> 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);

Tabs for indentation. Didn't checkstyle.py warn you ?

> 
> 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 ?
> 
> > +
> > +        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;
> >  }
> >  

-- 
Regards,

Laurent Pinchart