[libcamera-devel] [RFCv2 5/7] libcamera: pipeline: rkisp1: Call IPA start() and stop()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Mar 26, 2020 at 05:08:17PM +0100, Niklas Söderlund wrote:
> Call the IPA start()/stop() functions before/after the camera is
> started. This makes sure the IPA functions properly once thread
> management is moved into the start/stop interface.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index f49b3a7f0945ac92..3287fd3a18204cbc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -113,8 +113,8 @@ class RkISP1CameraData : public CameraData
>  {
>  public:
>  	RkISP1CameraData(PipelineHandler *pipe)
> -		: CameraData(pipe), sensor_(nullptr), frame_(0),
> -		  frameInfo_(pipe)
> +		: CameraData(pipe), running_(false), sensor_(nullptr),
> +		  frame_(0), frameInfo_(pipe)
>  	{
>  	}
>  
> @@ -125,6 +125,7 @@ public:
>  
>  	int loadIPA();
>  
> +	bool running_;
>  	Stream stream_;
>  	CameraSensor *sensor_;
>  	unsigned int frame_;
> @@ -394,6 +395,9 @@ int RkISP1CameraData::loadIPA()
>  void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  					const IPAOperationData &action)
>  {
> +	if (!running_)
> +		return;
> +
>  	switch (action.operation) {
>  	case RKISP1_IPA_ACTION_V4L2_SET: {
>  		const ControlList &controls = action.controls[0];
> @@ -764,10 +768,19 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  	if (ret)
>  		return ret;
>  
> +	ret = data->ipa_->start();
> +	if (ret) {
> +		freeBuffers(camera);
> +		LOG(RkISP1, Error)
> +			<< "Failed to start IPA " << camera->name();
> +		return ret;
> +	}
> +

I wonder if it would simplify error handling if you moved this last (the
stop could go first at stop time then).

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	data->frame_ = 0;
>  
>  	ret = param_->streamOn();
>  	if (ret) {
> +		data->ipa_->stop();
>  		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start parameters " << camera->name();
> @@ -777,6 +790,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  	ret = stat_->streamOn();
>  	if (ret) {
>  		param_->streamOff();
> +		data->ipa_->stop();
>  		freeBuffers(camera);
>  		LOG(RkISP1, Error)
>  			<< "Failed to start statistics " << camera->name();
> @@ -787,12 +801,14 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  	if (ret) {
>  		param_->streamOff();
>  		stat_->streamOff();
> +		data->ipa_->stop();
>  		freeBuffers(camera);
>  
>  		LOG(RkISP1, Error)
>  			<< "Failed to start camera " << camera->name();
>  	}
>  
> +	data->running_ = true;
>  	activeCamera_ = camera;
>  
>  	/* Inform IPA of stream configuration and sensor controls. */
> @@ -830,6 +846,10 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  		LOG(RkISP1, Warning)
>  			<< "Failed to stop parameters " << camera->name();
>  
> +	data->running_ = false;
> +
> +	data->ipa_->stop();
> +
>  	data->timeline_.reset();
>  
>  	freeBuffers(camera);

-- 
Regards,

Laurent Pinchart