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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Sep 14, 2020 at 04:21:39PM +0200, Niklas Söderlund wrote:
> In preparation of supporting both the main and self path configure all
> the media graph links as a part of the configuration step. Before this
> change the link between ISP and DMA engine was setup at match time as
> the only supported path was the main path and only the link between
> sensor and ISP was updated at part of the configuration step.
> 
> The main path is still the only path between ISP and DMA engine that is
> possible to enable.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Pass sensor directly to initLinks().
> - Remove string variable form link selection.
> - Rewrite commit message.
> ---
>  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 86a3e3c63b17762b..96046ec32c91cff3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -213,7 +213,8 @@ private:
>  	friend RkISP1CameraData;
>  	friend RkISP1Frames;
>  
> -	int initLinks();
> +	int initLinks(const Camera *camera, const CameraSensor *sensor,
> +		      const RkISP1CameraConfiguration &config);
>  	int createCamera(MediaEntity *sensor);
>  	void tryCompleteRequest(Request *request);
>  	void bufferReady(FrameBuffer *buffer);
> @@ -603,28 +604,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, sensor, *config);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * Configure the format on the sensor output and propagate it through
> @@ -927,22 +909,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>   * Match and Setup
>   */
>  
> -int PipelineHandlerRkISP1::initLinks()
> +int PipelineHandlerRkISP1::initLinks(const Camera *camera,
> +				     const CameraSensor *sensor,
> +				     const RkISP1CameraConfiguration &config)
>  {
> -	MediaLink *link;
> +	RkISP1CameraData *data = cameraData(camera);
>  	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;
> +	}
> +
> +	for (const StreamConfiguration &cfg : config) {
> +		MediaLink *link = nullptr;
> +		if (cfg.stream() == &data->stream_)
> +			link = media_->link("rkisp1_isp", 2,
> +					    "rkisp1_resizer_mainpath", 0);
> +		else
> +			return -EINVAL;
> +
> +		if (!link)
> +			return -ENODEV;

		if (cfg.stream() != &data->stream_)
			return -EINVAL;

		MediaLink *link = media_->link("rkisp1_isp", 2,
					       "rkisp1_resizer_mainpath", 0);
		if (!link)
			return -ENODEV;

(It probably affects subsequent patches, but here at least it looks
neater :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +		ret = link->setEnabled(true);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -1024,12 +1035,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