[libcamera-devel] [PATCH 03/13] libcamera: pipeline: rkisp1: Setup links as part of configuration

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote:
> In preparation of supporting both the main and self path move all link
> setup to be part of the configuration step. The main path is still the
> only possible path.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++-----------
>  1 file changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a0e36a44d8d91260..38382a6894dac22a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -211,7 +211,8 @@ private:
>  	friend RkISP1CameraData;
>  	friend RkISP1Frames;
>  
> -	int initLinks();
> +	int initLinks(const Camera *camera,
> +		      const RkISP1CameraConfiguration &config);
>  	int createCamera(MediaEntity *sensor);
>  	void tryCompleteRequest(Request *request);
>  	void bufferReady(FrameBuffer *buffer);
> @@ -601,28 +602,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>  
> -	/*
> -	 * Configure the sensor links: enable the link corresponding to this
> -	 * camera and disable all the other sensor links.
> -	 */
> -	const MediaPad *pad = isp_->entity()->getPadByIndex(0);
> -
> -	for (MediaLink *link : pad->links()) {
> -		bool enable = link->source()->entity() == sensor->entity();
> -
> -		if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable)
> -			continue;
> -
> -		LOG(RkISP1, Debug)
> -			<< (enable ? "Enabling" : "Disabling")
> -			<< " link from sensor '"
> -			<< link->source()->entity()->name()
> -			<< "' to ISP";
> -
> -		ret = link->setEnabled(enable);
> -		if (ret < 0)
> -			return ret;
> -	}
> +	ret = initLinks(camera, *config);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * Configure the format on the sensor output and propagate it through
> @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>   * Match and Setup
>   */
>  
> -int PipelineHandlerRkISP1::initLinks()
> +int PipelineHandlerRkISP1::initLinks(const Camera *camera,
> +				     const RkISP1CameraConfiguration &config)
>  {
> -	MediaLink *link;
> +	const RkISP1CameraData *data = cameraData(camera);
> +	const CameraSensor *sensor = data->sensor_;
>  	int ret;
>  
>  	ret = media_->disableLinks();
>  	if (ret < 0)
>  		return ret;
>  
> -	link = media_->link("rkisp1_isp", 2, "rkisp1_resizer_mainpath", 0);
> -	if (!link)
> -		return -ENODEV;
> +	/*
> +	 * Configure the sensor links: enable the link corresponding to this
> +	 * camera.
> +	 */
> +	const MediaPad *pad = isp_->entity()->getPadByIndex(0);
> +	for (MediaLink *link : pad->links()) {
> +		if (link->source()->entity() != sensor->entity())
> +			continue;
>  
> -	ret = link->setEnabled(true);
> -	if (ret < 0)
> -		return ret;
> +		LOG(RkISP1, Debug)
> +			<< "Enabling link from sensor '"
> +			<< link->source()->entity()->name()
> +			<< "' to ISP";
> +
> +		ret = link->setEnabled(true);
> +		if (ret < 0)
> +			return ret;
> +	}

This will break switching between cameras, as you would have both links
enabled. You not only need to enable the link to the sensor, but disable
links to other sensors. The link disable step needs to happen first, as
the driver won't (or at least shouldn't) allow two links to be enabled
at the same time.

> +
> +	for (const StreamConfiguration &cfg : config) {
> +		std::string resizer;
> +		if (cfg.stream() == &data->stream_)
> +			resizer = "rkisp1_resizer_mainpath";
> +		else
> +			return -EINVAL;

I'm not sure to get this change, it doesn't match the commit message. I
suspect it belongs to a different patch.

> +
> +		MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0);
> +		if (!link)
> +			return -ENODEV;
> +
> +		ret = link->setEnabled(true);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -1021,12 +1032,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
>  	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>  
> -	/* Configure default links. */
> -	if (initLinks() < 0) {
> -		LOG(RkISP1, Error) << "Failed to setup links";
> -		return false;
> -	}
> -
>  	/*
>  	 * Enumerate all sensors connected to the ISP and create one
>  	 * camera instance for each of them.

-- 
Regards,

Laurent Pinchart