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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-08-20 17:55:59 +0300, Laurent Pinchart wrote:
> 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.

It's not a change of behavior, but it's accomplished in a different way.  
With this change all links are configured in initLinks() so a first step 
is to disable all links with a call to 'media_->disableLinks()' all that 
is left after that is to enable the desired links.

> 
> > +
> > +	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.

I will update the commit message as this is intended to be a part of 
this change. It may however be reworked to remove the intermediate 
resizer variable and save one LoC ;-)

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

-- 
Regards,
Niklas Söderlund