[libcamera-devel,v2,10/13] libcamera: pipeline: rkisp1: Configure self path

9602 diff mbox series

Allow for both the main and self path streams to be configured. This
change adds the self path as an internal stream to the pipeline handler.
It is not exposed as a Camera stream so it can not yet be used.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 200 ++++++++++++++++-------
 1 file changed, 141 insertions(+), 59 deletions(-)

Hi Niklas,

Thank you for the patch.

On Mon, Sep 14, 2020 at 04:21:46PM +0200, Niklas Söderlund wrote:
> Allow for both the main and self path streams to be configured. This
> change adds the self path as an internal stream to the pipeline handler.
> It is not exposed as a Camera stream so it can not yet be used.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 200 ++++++++++++++++-------
>  1 file changed, 141 insertions(+), 59 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 5aa01f87afd71183..01cd461427e3a6dc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -135,7 +135,8 @@ public:
>  			 V4L2VideoDevice *selfPathVideo)
>  		: CameraData(pipe), sensor_(nullptr), frame_(0),
>  		  frameInfo_(pipe), mainPathVideo_(mainPathVideo),
> -		  selfPathVideo_(selfPathVideo)
> +		  selfPathVideo_(selfPathVideo), mainPathActive_(false),
> +		  selfPathActive_(false)
>  	{
>  	}
>  
> @@ -147,6 +148,7 @@ public:
>  	int loadIPA();
>  
>  	Stream mainPathStream_;
> +	Stream selfPathStream_;
>  	CameraSensor *sensor_;
>  	unsigned int frame_;
>  	std::vector<IPABuffer> ipaBuffers_;
> @@ -156,6 +158,9 @@ public:
>  	V4L2VideoDevice *mainPathVideo_;
>  	V4L2VideoDevice *selfPathVideo_;
>  
> +	bool mainPathActive_;
> +	bool selfPathActive_;
> +
>  private:
>  	void queueFrameAction(unsigned int frame,
>  			      const IPAOperationData &action);
> @@ -617,7 +622,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	RkISP1CameraConfiguration *config =
>  		static_cast<RkISP1CameraConfiguration *>(c);
>  	RkISP1CameraData *data = cameraData(camera);
> -	StreamConfiguration &cfg = config->at(0);
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>  
> @@ -659,37 +663,55 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  
>  	LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString();
>  
> -	ret = mainPathResizer_->setFormat(0, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString();
> -
> -	format.size = cfg.size;
> -
> -	LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString();
> -
> -	ret = mainPathResizer_->setFormat(1, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString();
> -
> -	const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> -	V4L2DeviceFormat outputFormat = {};
> -	outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> -	outputFormat.size = cfg.size;
> -	outputFormat.planesCount = info.numPlanes();
> -
> -	ret = mainPathVideo_->setFormat(&outputFormat);
> -	if (ret)
> -		return ret;
> -
> -	if (outputFormat.size != cfg.size ||
> -	    outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {
> -		LOG(RkISP1, Error)
> -			<< "Unable to configure capture in " << cfg.toString();
> -		return -EINVAL;
> +	data->mainPathActive_ = false;
> +	data->selfPathActive_ = false;
> +	for (const StreamConfiguration &cfg : *config) {
> +		V4L2SubdeviceFormat ispFormat = format;
> +		V4L2Subdevice *resizer;
> +		V4L2VideoDevice *video;
> +
> +		if (cfg.stream() == &data->mainPathStream_) {
> +			resizer = mainPathResizer_;
> +			video = mainPathVideo_;
> +			data->mainPathActive_ = true;
> +		} else {
> +			resizer = selfPathResizer_;
> +			video = selfPathVideo_;
> +			data->selfPathActive_ = true;
> +		}

Pretty please, could you address the request from 08/13 on top ? This is
getting messy. I wouldn't mind if it was addressed as part of this
series, but going right on top would be acceptable.

> +
> +		ret = resizer->setFormat(0, &ispFormat);
> +		if (ret < 0)
> +			return ret;
> +
> +		LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString();

		LOG(RkISP1, Debug)
			<< "Resizer input pad configured with "
			<< ispFormat.toString();

and should this print which resizer is configured ?

> +
> +		ispFormat.size = cfg.size;
> +
> +		LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString();

Ditto.

> +
> +		ret = resizer->setFormat(1, &ispFormat);
> +		if (ret < 0)
> +			return ret;
> +
> +		LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString();

And here too.

> +
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		V4L2DeviceFormat outputFormat = {};
> +		outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);
> +		outputFormat.size = cfg.size;
> +		outputFormat.planesCount = info.numPlanes();
> +
> +		ret = video->setFormat(&outputFormat);
> +		if (ret)
> +			return ret;
> +
> +		if (outputFormat.size != cfg.size ||
> +		    outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {
> +			LOG(RkISP1, Error)
> +				<< "Unable to configure capture in " << cfg.toString();
> +			return -EINVAL;
> +		}
>  	}
>  
>  	V4L2DeviceFormat paramFormat = {};
> @@ -704,28 +726,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	if (ret)
>  		return ret;
>  
> -	cfg.setStream(&data->mainPathStream_);
> -
>  	return 0;
>  }
>  
>  int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
>  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> +	RkISP1CameraData *data = cameraData(camera);
>  	unsigned int count = stream->configuration().bufferCount;
> -	return mainPathVideo_->exportBuffers(count, buffers);
> +
> +	if (stream == &data->mainPathStream_)
> +		return mainPathVideo_->exportBuffers(count, buffers);
> +
> +	if (stream == &data->selfPathStream_)

else if ?

> +		return selfPathVideo_->exportBuffers(count, buffers);
> +
> +	return -EINVAL;
>  }
>  
>  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	unsigned int count = data->mainPathStream_.configuration().bufferCount;
>  	unsigned int ipaBufferId = 1;
>  	int ret;
>  
> -	ret = mainPathVideo_->importBuffers(count);
> -	if (ret < 0)
> -		goto error;
> +	unsigned int count = std::max({
> +		data->mainPathStream_.configuration().bufferCount,
> +		data->selfPathStream_.configuration().bufferCount,
> +	});

Why max, instead of using the respective values below ?

> +
> +	if (data->mainPathActive_) {
> +		ret = mainPathVideo_->importBuffers(count);
> +		if (ret < 0)
> +			goto error;
> +	}
> +
> +	if (data->selfPathActive_) {
> +		ret = selfPathVideo_->importBuffers(count);
> +		if (ret < 0)
> +			goto error;
> +	}
>  
>  	ret = param_->allocateBuffers(count, &paramBuffers_);
>  	if (ret < 0)
> @@ -757,6 +797,7 @@ error:
>  	paramBuffers_.clear();
>  	statBuffers_.clear();
>  	mainPathVideo_->releaseBuffers();
> +	selfPathVideo_->releaseBuffers();
>  
>  	return ret;
>  }
> @@ -790,6 +831,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>  	if (mainPathVideo_->releaseBuffers())
>  		LOG(RkISP1, Error) << "Failed to release main path buffers";
>  
> +	if (selfPathVideo_->releaseBuffers())
> +		LOG(RkISP1, Error) << "Failed to release self path buffers";
> +
>  	return 0;
>  }
>  
> @@ -832,15 +876,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  		return ret;
>  	}
>  
> -	ret = mainPathVideo_->streamOn();
> -	if (ret) {
> -		param_->streamOff();
> -		stat_->streamOff();
> -		data->ipa_->stop();
> -		freeBuffers(camera);
> -
> -		LOG(RkISP1, Error)
> -			<< "Failed to start camera " << camera->id();
> +	std::map<unsigned int, IPAStream> streamConfig;
> +
> +	if (data->mainPathActive_) {
> +		ret = mainPathVideo_->streamOn();
> +		if (ret) {
> +			param_->streamOff();
> +			stat_->streamOff();
> +			data->ipa_->stop();
> +			freeBuffers(camera);
> +
> +			LOG(RkISP1, Error)
> +				<< "Failed to start main path " << camera->id();
> +			return ret;
> +		}
> +
> +		streamConfig[0] = {
> +			.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> +			.size = data->mainPathStream_.configuration().size,
> +		};
> +	}
> +
> +	if (data->selfPathActive_) {
> +		ret = selfPathVideo_->streamOn();
> +		if (ret) {
> +			if (data->mainPathActive_)
> +				mainPathVideo_->streamOff();
> +

This should stop mainPathVideo_ if it has been activated. The error
handling is really becoming nasty :-S Any chance it could be improved on
top ? Goto's are probably not a great option though as they would jump
over variable declarations. One optio would be to add an error handler
at the beginning of the function:

	class ErrorHandler {
	public:
		ErrorHandler()
			: disabled_(false)
		{
		}

		~ErrorHandler()
		{
			if (disabled_)
				return;

			param_->streamOff();
			stat_->streamOff();
			data->ipa_->stop();
			freeBuffers();
		}

		void disable() { disabled_ = true; }

	private:
		bool disabled_;
	};

	class ErrorHandler eh;

and call

	eh.disable();

at the very end, before returning (names to be bikeshedded). We could
even make it a generic class that takes a lambda function as argument,
and calls it in the destructor. Usage here would become

	ErrorHandler eh([&]() {
		param_->streamOff();
		stat_->streamOff();
		data->ipa_->stop();
		freeBuffers();
	});

which doesn't look too bad I think.

This would require making the V4L2VideoDevice::streamOff(),
IPAProxy::stop() and PipelineHandlerRkISP1::freeBuffer() functions safe
to be called when already off, stopped of freed.

> +			param_->streamOff();
> +			stat_->streamOff();
> +			data->ipa_->stop();
> +			freeBuffers(camera);
> +
> +			LOG(RkISP1, Error)
> +				<< "Failed to start self path " << camera->id();
> +			return ret;
> +		}
> +
> +		streamConfig[1] = {
> +			.pixelFormat = data->selfPathStream_.configuration().pixelFormat,
> +			.size = data->selfPathStream_.configuration().size,
> +		};
>  	}
>  
>  	activeCamera_ = camera;
> @@ -855,12 +931,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  		ret = 0;
>  	}
>  
> -	std::map<unsigned int, IPAStream> streamConfig;
> -	streamConfig[0] = {
> -		.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> -		.size = data->mainPathStream_.configuration().size,
> -	};
> -
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>  
> @@ -876,10 +946,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  	RkISP1CameraData *data = cameraData(camera);
>  	int ret;
>  
> -	ret = mainPathVideo_->streamOff();
> -	if (ret)
> -		LOG(RkISP1, Warning)
> -			<< "Failed to stop camera " << camera->id();
> +	if (data->selfPathActive_) {
> +		ret = selfPathVideo_->streamOff();
> +		if (ret)
> +			LOG(RkISP1, Warning)
> +				<< "Failed to stop self path " << camera->id();

Maybe s/path/path for/ ? Or "Failed to stop self path (camera ...)" ?

> +	}
> +
> +	if (data->mainPathActive_) {
> +		ret = mainPathVideo_->streamOff();
> +		if (ret)
> +			LOG(RkISP1, Warning)
> +				<< "Failed to stop main path " << camera->id();
> +	}
>  
>  	ret = stat_->streamOff();
>  	if (ret)
> @@ -964,6 +1043,9 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,
>  		if (cfg.stream() == &data->mainPathStream_)
>  			link = media_->link("rkisp1_isp", 2,
>  					    "rkisp1_resizer_mainpath", 0);
> +		else if (cfg.stream() == &data->selfPathStream_)
> +			link = media_->link("rkisp1_isp", 2,
> +					    "rkisp1_resizer_selfpath", 0);

This renders my alternative proposal in a previous patch moot, but you
can then skip the nullptr initialization of link.

>  		else
>  			return -EINVAL;
>