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

16375 diff mbox series

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@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 45 ++++++++++++++++--------
 1 file changed, 31 insertions(+), 14 deletions(-)

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@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);
>