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

[Laurent Pinchart]

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 ?

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