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

[Niklas Söderlund]

Hi Jacopo,

On 2020-08-20 10:20:04 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> 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
> all links
> 
> > 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_;
> 
> You could actually pass sensor directly, don't you ?

Grate idea.

> 
> >  	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) {
> > +		std::string resizer;
> > +		if (cfg.stream() == &data->stream_)
> > +			resizer = "rkisp1_resizer_mainpath";
> > +		else
> > +			return -EINVAL;
> > +
> > +		MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0);
> > +		if (!link)
> > +			return -ENODEV;
> > +
> > +		ret = link->setEnabled(true);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> I also see a significant behaviour change, the code is not only moved.
> To be able to actually spot what has changed, could this be broken up
> in two patches ? One that moves the existing code to a separate
> function, the other that changes it on top ?

I really can't as the idea is to change the behavior. I will rewrite the 
commit message to better describe this is not simply a movement of code 
but change of behavior. Instead of setting up the link between DMA and 
ISP at match() and sensor and ISP at configure() all links are now setup 
at the configure() time.

> 
> >
> >  	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.
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund