[libcamera-devel] [PATCH] pipeline: rkisp1: Support devices without self path

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Jun 27, 2022 at 08:08:59PM +0900, Paul Elder via libcamera-devel wrote:
> Some hardware supported by the rkisp1 driver, such as the ISP in the
> i.MX8MP, don't have a self path. Although at the moment the driver still
> exposes the self path, prepare the rkisp1 pipeline handler for when the
> self path will be removed for devices that don't support it.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 45 ++++++++++++++++--------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7cf36524..83977fbc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -179,6 +179,8 @@ private:
>  	std::unique_ptr<V4L2VideoDevice> param_;
>  	std::unique_ptr<V4L2VideoDevice> stat_;
>  
> +	bool hasSelfPath_;
> +
>  	RkISP1MainPath mainPath_;
>  	RkISP1SelfPath selfPath_;
>  
> @@ -343,7 +345,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)
>  	if (info->mainPathBuffer)
>  		mainPath_->queueBuffer(info->mainPathBuffer);
>  
> -	if (info->selfPathBuffer)
> +	if (info->selfPathBuffer && selfPath_)
>  		selfPath_->queueBuffer(info->selfPathBuffer);
>  }
>  
> @@ -382,7 +384,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
>  		return false;
>  
>  	config = cfg;
> -	if (data_->selfPath_->validate(&config) != Valid)
> +	if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)
>  		return false;
>  
>  	return true;
> @@ -420,7 +422,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		std::reverse(order.begin(), order.end());
>  
>  	bool mainPathAvailable = true;
> -	bool selfPathAvailable = true;
> +	bool selfPathAvailable = data_->selfPath_;

A bit above in the same function, I think you should resize the config_
vector based on the number of available paths.

>  	for (unsigned int index : order) {
>  		StreamConfiguration &cfg = config_[index];
>  
> @@ -499,7 +501,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  }
>  
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> -	: PipelineHandler(manager)
> +	: PipelineHandler(manager), hasSelfPath_(true)
>  {
>  }
>  
> @@ -516,7 +518,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  		return config;
>  
>  	bool mainPathAvailable = true;
> -	bool selfPathAvailable = true;
> +	bool selfPathAvailable = data->selfPath_;
>  	for (const StreamRole role : roles) {
>  		bool useMainPath;
>  
> @@ -619,7 +621,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  			ret = mainPath_.configure(cfg, format);
>  			streamConfig[0] = IPAStream(cfg.pixelFormat,
>  						    cfg.size);
> -		} else {
> +		} else if (hasSelfPath_) {
>  			ret = selfPath_.configure(cfg, format);
>  			streamConfig[1] = IPAStream(cfg.pixelFormat,
>  						    cfg.size);

This won't work correctly if two roles are specified.

> @@ -670,7 +672,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
>  
>  	if (stream == &data->mainPathStream_)
>  		return mainPath_.exportBuffers(count, buffers);
> -	else if (stream == &data->selfPathStream_)
> +	else if (hasSelfPath_ && stream == &data->selfPathStream_)

I think this change could be skipped as the function should never get
called for the self path if not available, but it doesn't hurt to keep
it.

>  		return selfPath_.exportBuffers(count, buffers);
>  
>  	return -EINVAL;
> @@ -799,7 +801,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  		}
>  	}
>  
> -	if (data->selfPath_->isEnabled()) {
> +	if (hasSelfPath_ && data->selfPath_->isEnabled()) {
>  		ret = selfPath_.start();
>  		if (ret) {
>  			mainPath_.stop();
> @@ -826,7 +828,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>  
>  	data->ipa_->stop();
>  
> -	selfPath_.stop();
> +	if (hasSelfPath_)
> +		selfPath_.stop();
>  	mainPath_.stop();
>  
>  	ret = stat_->streamOff();
> @@ -917,7 +920,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	int ret;
>  
>  	std::unique_ptr<RkISP1CameraData> data =
> -		std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);
> +		std::make_unique<RkISP1CameraData>(this, &mainPath_,
> +						   hasSelfPath_ ? &selfPath_ : nullptr);
>  
>  	ControlInfoMap::Map ctrls;
>  	ctrls.emplace(std::piecewise_construct,
> @@ -980,9 +984,21 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	dm.add("rkisp1_stats");
>  	dm.add("rkisp1_params");
>  
> +	DeviceMatch dmNoSelf("rkisp1");
> +	dmNoSelf.add("rkisp1_isp");
> +	dmNoSelf.add("rkisp1_resizer_mainpath");
> +	dmNoSelf.add("rkisp1_mainpath");
> +	dmNoSelf.add("rkisp1_stats");
> +	dmNoSelf.add("rkisp1_params");
> +
>  	media_ = acquireMediaDevice(enumerator, dm);
> -	if (!media_)
> -		return false;
> +	if (!media_) {
> +		media_ = acquireMediaDevice(enumerator, dmNoSelf);
> +		if (!media_)
> +			return false;
> +
> +		hasSelfPath_ = false;
> +	}

You can make this simpler, use a single DeviceMatch that has no
selfpath, and add

	hasSelfPath_ = !!media_->getEntityByName("rkisp1_selfpath");

>  
>  	if (!media_->hwRevision()) {
>  		LOG(RkISP1, Error)
> @@ -1008,11 +1024,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	if (!mainPath_.init(media_))
>  		return false;
>  
> -	if (!selfPath_.init(media_))
> +	if (hasSelfPath_ && !selfPath_.init(media_))
>  		return false;
>  
>  	mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> -	selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> +	if (hasSelfPath_)
> +		selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>  	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
>  	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>  

-- 
Regards,

Laurent Pinchart