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

16399 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>

---
Changes in v2:
- simplify matching
- clean up self path availability in validate()
- fix configure(), return -ENODEV if multiple roles but no self path
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 40 ++++++++++++++----------
 1 file changed, 24 insertions(+), 16 deletions(-)

Hi Paul,

Thank you for the patch.

On Tue, Jun 28, 2022 at 05:33:43PM +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>
> 
> ---
> Changes in v2:
> - simplify matching
> - clean up self path availability in validate()
> - fix configure(), return -ENODEV if multiple roles but no self path
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 40 ++++++++++++++----------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a233c961..2c457f0e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -180,6 +180,8 @@ private:
>  	std::unique_ptr<V4L2VideoDevice> stat_;
>  	std::unique_ptr<V4L2Subdevice> csi_;
>  
> +	bool hasSelfPath_;
> +
>  	RkISP1MainPath mainPath_;
>  	RkISP1SelfPath selfPath_;
>  
> @@ -344,7 +346,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)
>  	if (info->mainPathBuffer)
>  		mainPath_->queueBuffer(info->mainPathBuffer);
>  
> -	if (info->selfPathBuffer)
> +	if (info->selfPathBuffer && selfPath_)

I don't think selfPathBuffer can be non-null if selfPath_ is null, but
it doesn't really hurt to check. I probably would have put the selfPath_
check first as that's what you do everywhere else.

>  		selfPath_->queueBuffer(info->selfPathBuffer);
>  }
>  
> @@ -383,7 +385,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;
> @@ -393,6 +395,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_.get();
>  	Status status = Valid;
> +	unsigned int pathCount = data_->selfPath_ ? 2 : 1;

Move it one line up ?

>  
>  	if (config_.empty())
>  		return Invalid;
> @@ -403,8 +406,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  	}
>  
>  	/* Cap the number of entries to the available streams. */
> -	if (config_.size() > 2) {
> -		config_.resize(2);
> +	if (config_.size() > pathCount) {
> +		config_.resize(pathCount);
>  		status = Adjusted;
>  	}
>  
> @@ -421,7 +424,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		std::reverse(order.begin(), order.end());
>  
>  	bool mainPathAvailable = true;
> -	bool selfPathAvailable = true;
> +	bool selfPathAvailable = (pathCount == 2);

No need for parentheses, and I would actually write

	bool selfPathAvailable = data->selfPath_;

as it's more logical (and it's what you do below too). You could then
possibly move the pathCount variable just before the check that uses it,
up to you.

>  	for (unsigned int index : order) {
>  		StreamConfiguration &cfg = config_[index];
>  
> @@ -500,7 +503,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  }
>  
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> -	: PipelineHandler(manager)
> +	: PipelineHandler(manager), hasSelfPath_(true)
>  {
>  }
>  
> @@ -527,7 +530,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	}
>  
>  	bool mainPathAvailable = true;
> -	bool selfPathAvailable = true;
> +	bool selfPathAvailable = data->selfPath_;
>  	for (const StreamRole role : roles) {
>  		bool useMainPath;

This won't be enough. If you have two StillCapture roles, the first one
will use the main path and the second one the self path. If you have two
ViewFinder or VideoRecording roles, both will try to use the main path.
If you have more than two roles it's even worse. The logic is already
broken, you may want to fix it in a separate patch first, and maybe this
change will then be enough.

>  
> @@ -636,10 +639,12 @@ 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);
> +		} else {
> +			return -ENODEV;

Should never happen, but looks safer.

>  		}
>  
>  		if (ret)
> @@ -687,7 +692,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_)
>  		return selfPath_.exportBuffers(count, buffers);
>  
>  	return -EINVAL;
> @@ -816,7 +821,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();
> @@ -843,7 +848,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>  
>  	data->ipa_->stop();
>  
> -	selfPath_.stop();
> +	if (hasSelfPath_)
> +		selfPath_.stop();
>  	mainPath_.stop();
>  
>  	ret = stat_->streamOff();

There's one additional usage of selfPath_ in
PipelineHandlerRkISP1::initLinks():

		else if (cfg.stream() == &data->selfPathStream_)
			ret = data->selfPath_->setEnabled(true);

I don't think it can get triggered, as the configuration has been
validated and the stream check thus can't pass, but as there's another
case above where a hasSelfPath_ check is added while not strictly
needed, I'd add one here too just to be safe.

> @@ -946,7 +952,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,
> @@ -1002,9 +1009,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  
>  	DeviceMatch dm("rkisp1");
>  	dm.add("rkisp1_isp");
> -	dm.add("rkisp1_resizer_selfpath");
>  	dm.add("rkisp1_resizer_mainpath");
> -	dm.add("rkisp1_selfpath");
>  	dm.add("rkisp1_mainpath");
>  	dm.add("rkisp1_stats");
>  	dm.add("rkisp1_params");
> @@ -1019,6 +1024,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  		return false;
>  	}
>  
> +	hasSelfPath_ = !!media_->getEntityByName("rkisp1_selfpath");
> +
>  	/* Create the V4L2 subdevices we will need. */
>  	isp_ = V4L2Subdevice::fromEntityName(media_, "rkisp1_isp");
>  	if (isp_->open() < 0)
> @@ -1049,11 +1056,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);
>