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

[paul.elder at ideasonboard.com]

Hi Sebastian,

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.

The reason why the events shouldn't be sent from the IPA to the pipeline
handler is not because configure is synchronous, it's because start()
hasn't been called yet, so the IPA thread hasn't started running yet.

It just happens that all calls before start() are synchronous, and all
calls after start() are asynchronous, so the rules regarding events also
line up :)

> 
> If information needs to be returned from configure it should be handled
> through the interface directly.

I think s/handled through the interface/returned/ ?

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

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> ---
>  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);
> +
> +        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;
>  }
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel