[libcamera-devel] [PATCH v3 04/22] libcamera: pipeline: rkisp1: Setup links as part of configuration

[Jacopo Mondi]

Hi Niklas,

On Fri, Sep 25, 2020 at 06:16:07PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-09-25 16:14:53 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Fri, Sep 25, 2020 at 03:41:49AM +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>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > * Changes since v2
> > > - Simplify link selection per Laurent's suggestion. This is reworked
> > >   once more in a later patch but makes thins one looks nicer for now.
> > >
> > > * Changes since v1
> > > - Pass sensor directly to initLinks().
> > > - Remove string variable form link selection.
> > > - Rewrite commit message.
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------
> > >  1 file changed, 40 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index a5b6bb07e4f8dee2..8bb4582eeb688a4c 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);
> > > @@ -598,28 +599,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
> > > @@ -922,22 +904,49 @@ 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);
> >
> > The previous version disables links that are not used, while this one
> > only enables them. Is this intended ?
>
> Yes, the call to disableLinks() above disables all links so there is no
> point in disabling an already disabled link.
>

Ah right, sorry, missed that!

> >
> > Thanks
> >   j
> >
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +
> > > +	for (const StreamConfiguration &cfg : config) {
> > > +		if (cfg.stream() != &data->stream_)
> > > +			return -EINVAL;
> > > +
> > > +		MediaLink *link = media_->link("rkisp1_isp", 2,
> > > +					       "rkisp1_resizer_mainpath", 0);
> > > +		if (!link)
> > > +			return -ENODEV;
> > > +
> > > +		ret = link->setEnabled(true);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > >
> > >  	return 0;
> > >  }
> > > @@ -1019,12 +1028,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