[libcamera-devel] [PATCH v2 12/12] libcamera: pipeline: raspberrypi: Start IPA when starting camera

[Jacopo Mondi]

On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:
> The IPA is meant to be started when starting the camera, and stopped
> when stopping it. It was so far start early in order to handle the

s/start/started ?

> IPAInterface::processEvent() call related to lens shading table
> allocation before IPAInterface::configure() to pass the table to the
> IPA. Now that the lens shading table is passed through configure(),
> starting the IPA early isn't needed anymore.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9babf9f4a19c..c877820a6e1e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -303,10 +303,6 @@ public:
>  			vcsm_.free(lsTable_);
>  			lsTable_ = nullptr;
>  		}
> -
> -		/* Stop the IPA proxy thread. */
> -		if (ipa_)
> -			ipa_->stop();
>  	}
>
>  	void frameStarted(uint32_t sequence);
> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
>  	ret = queueAllBuffers(camera);
>  	if (ret) {
>  		LOG(RPI, Error) << "Failed to queue buffers";
> +		stop(camera);
> +		return ret;
> +	}
> +
> +	/* Start the IPA. */
> +	ret = data->ipa_->start();
> +	if (ret) {
> +		LOG(RPI, Error)
> +			<< "Failed to start IPA for " << camera->name();
> +		stop(camera);
>  		return ret;
>  	}
>
> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
>  	V4L2DeviceFormat sensorFormat;
>  	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
>  	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> -	if (ret)
> +	if (ret) {
> +		stop(camera);

Do we want to stop the whole camera or just the IPA here ?
I think you meant data->ipa_->stop(), right ?

With this fixed
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  		return ret;
> +	}
>
>  	/* Enable SOF event generation. */
>  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
>  	data->bayerQueue_ = std::queue<FrameBuffer *>{};
>  	data->embeddedQueue_ = std::queue<FrameBuffer *>{};
>
> +	/* Stop the IPA. */
> +	data->ipa_->stop();
> +
>  	freeBuffers(camera);
>  }
>
> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
>  		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
>  	};
>
> -	ipa_->init(settings);
> -
> -	/*
> -	 * Startup the IPA thread now. Without this call, none of the IPA API
> -	 * functions will run.
> -	 *
> -	 * It only gets stopped in the class destructor.
> -	 */
> -	return ipa_->start();
> +	return ipa_->init(settings);
>  }
>
>  int RPiCameraData::configureIPA()
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel