[libcamera-devel] [PATCH v5 14/19] libcamera: ipu3: Implement camera start/stop

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Mar 26, 2019 at 09:38:57AM +0100, Jacopo Mondi wrote:
> Start and stop video devices in the pipeline.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++++++++++++++++++++--
>  1 file changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d3519bb1d536..5b3c44174566 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -111,6 +111,9 @@ public:
>  	int exportBuffers(enum OutputId id, unsigned int count);
>  	void freeBuffers();
>  
> +	int start();
> +	void stop();
> +
>  	unsigned int index_;
>  	std::string name_;
>  	MediaDevice *media_;
> @@ -154,6 +157,9 @@ public:
>  	BufferPool *exportBuffers();
>  	void freeBuffers();
>  
> +	int start();
> +	void stop();
> +
>  	V4L2Device *output_;
>  	V4L2Subdevice *csi2_;
>  	V4L2Subdevice *sensor_;
> @@ -371,12 +377,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
>  int PipelineHandlerIPU3::start(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	V4L2Device *cio2 = data->cio2_.output_;
> +	CIO2Device *cio2 = &data->cio2_;
> +	ImgUDevice *imgu = data->imgu_;
>  	int ret;
>  
> -	ret = cio2->streamOn();
> +	/*
> +	 * Start the ImgU video devices, buffers will be queued to the
> +	 * ImgU output and viewfinder when requests will be queued.
> +	 */
> +	ret = cio2->start();
> +	if (ret) {
> +		cio2->stop();

Is this needed ?

> +		return ret;
> +	}
> +
> +	ret = imgu->start();
>  	if (ret) {
> -		LOG(IPU3, Info) << "Failed to start camera " << camera->name();

I think this message would be useful to keep. The start() functions log
the detailed error cause (either directly, or in the functions they
call), but an overall message stating that the camera failed to start is
useful. You can use a goto error to avoid duplicating the message in the
cio2 and imgu start error paths.

> +		imgu->stop();
> +		cio2->stop();
>  		return ret;
>  	}
>  
> @@ -386,10 +404,9 @@ int PipelineHandlerIPU3::start(Camera *camera)
>  void PipelineHandlerIPU3::stop(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	V4L2Device *cio2 = data->cio2_.output_;
>  
> -	if (cio2->streamOff())
> -		LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
> +	data->cio2_.stop();
> +	data->imgu_->stop();

Similarly I think an error message would be useful here too (but without
returning early, we should stop the imgu even if the cio2 fails to
stop). As we don't care about the error code the code below should o.

	ret = data->cio2_.stop();
	ret |= data->imgu_->stop();
	if (ret)
		LOG(IPU3, Warning) << "Failed to stop camera " << camera->name();

>  
>  	PipelineHandler::stop(camera);
>  }
> @@ -833,6 +850,46 @@ void ImgUDevice::freeBuffers()
>  		LOG(IPU3, Error) << "Failed to release ImgU input buffers";
>  }
>  
> +int ImgUDevice::start()
> +{
> +	int ret;
> +
> +	/* Start the ImgU video devices. */
> +	ret = output_->streamOn();
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to start ImgU output";
> +		return ret;
> +	}
> +
> +	ret = viewfinder_->streamOn();
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to start ImgU viewfinder";
> +		return ret;
> +	}
> +
> +	ret = stat_->streamOn();
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to start ImgU stat";
> +		return ret;
> +	}
> +
> +	ret = input_->streamOn();
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to start ImgU input";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void ImgUDevice::stop()
> +{
> +	output_->streamOff();
> +	viewfinder_->streamOff();
> +	stat_->streamOff();
> +	input_->streamOff();
> +}
> +
>  /*------------------------------------------------------------------------------
>   * CIO2 Device
>   */
> @@ -1026,6 +1083,30 @@ void CIO2Device::freeBuffers()
>  		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
>  }
>  
> +int CIO2Device::start()
> +{
> +	int ret;
> +
> +	for (Buffer &buffer : pool_.buffers()) {
> +		ret = output_->queueBuffer(&buffer);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = output_->streamOn();
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to start CIO2";

The streamOn() function already logs a message, I think you can remove
this one.

With those small issues fixed,

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

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void CIO2Device::stop()
> +{
> +	output_->streamOff();
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart