[libcamera-devel] [PATCH v3 21/22] libcamera: pipeline: rkisp1: Use the media link to track if a path is enabled

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Sep 25, 2020 at 03:42:06AM +0200, Niklas Söderlund wrote:
> Instead of manually track if a path is enable or not use the media graph

s/track/tracking/

> link status. This enables RkISP1Path::start() to check the link status
> thus allowing it to always be called from PipelineHandlerRkISP1::start()
> making the code simpler. There is no functional changed.

s/changed/change/

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 82 +++++++++-----------
>  src/libcamera/pipeline/rkisp1/rkisp1path.cpp |  3 +
>  src/libcamera/pipeline/rkisp1/rkisp1path.h   |  1 +
>  3 files changed, 39 insertions(+), 47 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e41a9a51bda576b0..c6c2e3aa3dc6d0dc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -122,8 +122,7 @@ public:
>  	RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
>  			 RkISP1SelfPath *selfPath)
>  		: CameraData(pipe), sensor_(nullptr), frame_(0),
> -		  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath),
> -		  mainPathActive_(false), selfPathActive_(false)
> +		  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath)
>  	{
>  	}
>  
> @@ -145,9 +144,6 @@ public:
>  	RkISP1MainPath *mainPath_;
>  	RkISP1SelfPath *selfPath_;
>  
> -	bool mainPathActive_;
> -	bool selfPathActive_;
> -
>  private:
>  	void queueFrameAction(unsigned int frame,
>  			      const IPAOperationData &action);
> @@ -714,20 +710,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  
>  	LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString();
>  
> -	data->mainPathActive_ = false;
> -	data->selfPathActive_ = false;
>  	for (const StreamConfiguration &cfg : *config) {
> -		if (cfg.stream() == &data->mainPathStream_) {
> +		if (cfg.stream() == &data->mainPathStream_)
>  			ret = mainPath_.configure(cfg, format);
> -			if (ret)
> -				return ret;
> -			data->mainPathActive_ = true;
> -		} else {
> +		else
>  			ret = selfPath_.configure(cfg, format);
> -			if (ret)
> -				return ret;
> -			data->selfPathActive_ = true;
> -		}
> +
> +		if (ret)
> +			return ret;
>  	}
>  
>  	V4L2DeviceFormat paramFormat = {};
> @@ -871,39 +861,23 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  		return ret;
>  	}
>  
> -	std::map<unsigned int, IPAStream> streamConfig;
> -
> -	if (data->mainPathActive_) {
> -		ret = mainPath_.start();
> -		if (ret) {
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> -			return ret;
> -		}
> -
> -		streamConfig[0] = {
> -			.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> -			.size = data->mainPathStream_.configuration().size,
> -		};

I think I'd prefer keeping a mainPath_.isEnabled() check here, as the
code would look confusing for the casual reader. You need such a check
for the streamConfig handling below anyway, so just replacing
data->*PathActive_ with *Path_.isEnabled() should be enough.

> +	ret = mainPath_.start();
> +	if (ret) {
> +		param_->streamOff();
> +		stat_->streamOff();
> +		data->ipa_->stop();
> +		freeBuffers(camera);
> +		return ret;
>  	}
>  
> -	if (data->selfPathActive_) {
> -		ret = selfPath_.start();
> -		if (ret) {
> -			mainPath_.stop();
> -			param_->streamOff();
> -			stat_->streamOff();
> -			data->ipa_->stop();
> -			freeBuffers(camera);
> -			return ret;
> -		}
> -
> -		streamConfig[1] = {
> -			.pixelFormat = data->selfPathStream_.configuration().pixelFormat,
> -			.size = data->selfPathStream_.configuration().size,
> -		};
> +	ret = selfPath_.start();
> +	if (ret) {
> +		mainPath_.stop();
> +		param_->streamOff();
> +		stat_->streamOff();
> +		data->ipa_->stop();
> +		freeBuffers(camera);
> +		return ret;
>  	}
>  
>  	activeCamera_ = camera;
> @@ -918,6 +892,20 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  		ret = 0;
>  	}
>  
> +	std::map<unsigned int, IPAStream> streamConfig;
> +	if (data->mainPath_->isEnabled()) {
> +		streamConfig[0] = {
> +			.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> +			.size = data->mainPathStream_.configuration().size,
> +		};
> +	}
> +	if (data->selfPath_->isEnabled()) {
> +		streamConfig[1] = {
> +			.pixelFormat = data->selfPathStream_.configuration().pixelFormat,
> +			.size = data->selfPathStream_.configuration().size,
> +		};
> +	}
> +
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> index 8010cb92423c8269..623ee91888acd983 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> @@ -163,6 +163,9 @@ int RkISP1Path::start()
>  {
>  	int ret;
>  
> +	if (!isEnabled())
> +		return 0;
> +
>  	if (running_)
>  		return -EBUSY;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> index f57f4b646c3a3164..01f9c751f9e54c64 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> @@ -34,6 +34,7 @@ public:
>  	bool init(MediaDevice *media);
>  
>  	int enable() { return link_->setEnabled(true); }
> +	bool isEnabled() { return link_->flags() & MEDIA_LNK_FL_ENABLED; }

It could look a bit confusing to the reader that there's no disable()
call anywhere. Maybe a comment somewhere would be useful.

>  
>  	StreamConfiguration generateConfiguration(const Size &resolution);
>  	CameraConfiguration::Status validate(StreamConfiguration *cfg);

-- 
Regards,

Laurent Pinchart