[libcamera-devel] [PATCH 10/13] libcamera: pipeline: simple: Setup links in the context of sink entities

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 01, 2022 at 05:54:13PM +0200, Jacopo Mondi wrote:
> On Mon, Aug 01, 2022 at 03:05:40AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > To setup links the pipeline handler iterates over all entities in the
> > pipeline and disables all links but the ones we want to enable. Some
> > entities don't allow multiple sink links to be enabled, so the iteration
> > needs to be based on sink entities, disabling all their links first and
> > then enabling the sink link that is part of the pipeline.
> >
> > The loop implementation iterates over all Entity instances, and uses
> > their source link to locate the MediaEntity for the connected sink. The
> > sink entity is then processed in the context of the source's loop
> > iteration. This prevents the code from being able to access extra
> > information about the sink entity, as we only have access to the
> > MediaEntity, not the Entity.
> >
> > To prepare for subdev routing support that will require accessing
> > additional entity information, refactor the loop to process the sink
> > entity in the context of its Entity instance.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> I guess that's easier than storing the sink link along with the source
> one in SimpleCameraData(), just checking if you think there will be
> more use cases where the sink link will be usefull.

I can't rule that out, but I don't have any such use case in mind.

> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 731d355efda6..4bde9caa7254 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -578,15 +578,23 @@ int SimpleCameraData::setupLinks()
> >  	 * multiple sink links to be enabled together, even on different sink
> >  	 * pads. We must thus start by disabling all sink links (but the one we
> >  	 * want to enable) before enabling the pipeline link.
> > +	 *
> > +	 * The entities_ list stores entities along with their source link. We
> > +	 * need to process the link in the context of the sink entity, so
> > +	 * record the source link of the current entity as the sink link of the
> > +	 * next entity, and skip the first entity in the loop.
> >  	 */
> > +	MediaLink *sinkLink = nullptr;
> > +
> >  	for (SimpleCameraData::Entity &e : entities_) {
> > -		if (!e.sourceLink)
> > -			break;
> > +		if (!sinkLink) {
> > +			sinkLink = e.sourceLink;
> > +			continue;
> > +		}
> 
> Do we assume to start from the sensor (which has no sink pads to
> disable links on) ?
> 
> If that's the case, for my understanding, how is entities_ ordered ?
> It is populated at SimpleCameraData construction time, by iterating
> over "parents" which is an unordered map.
> 
> Is there a risk we actually start from a different entity and not the
> sensor here ?

We indeed start from the sensor. The entities_ list is built in
SimpleCameraData::SimpleCameraData(). The first large while () loop
performs a breath-first search starting from the camera sensor, as
passed to the constructor. At each step, how an entity was reached is
recorded in the parents unordered map, whose key is the sink and value
is the source (a.k.a. parent). The loop stops when it reaches a video
capture device.

Then, we create the entities_ list by first adding the video device, and
then walking from sink to source using the parents map and pushing each
entity in turn at the *front* of the list. Note that we don't iterate
over the parents map, we use it to retrieve the parent (a.k.a. source)
for a given sink. The list is thus guaranteed to start with the sensor
and be ordered from source to sink, ending with the video device.

> >
> > -		MediaEntity *remote = e.sourceLink->sink()->entity();
> > -		for (MediaPad *pad : remote->pads()) {
> > +		for (MediaPad *pad : e.entity->pads()) {
> >  			for (MediaLink *link : pad->links()) {
> > -				if (link == e.sourceLink)
> > +				if (link == sinkLink)
> >  					continue;
> >
> >  				if ((link->flags() & MEDIA_LNK_FL_ENABLED) &&
> > @@ -598,11 +606,13 @@ int SimpleCameraData::setupLinks()
> >  			}
> >  		}
> >
> > -		if (!(e.sourceLink->flags() & MEDIA_LNK_FL_ENABLED)) {
> > -			ret = e.sourceLink->setEnabled(true);
> > +		if (!(sinkLink->flags() & MEDIA_LNK_FL_ENABLED)) {
> > +			ret = sinkLink->setEnabled(true);
> >  			if (ret < 0)
> >  				return ret;
> >  		}
> > +
> > +		sinkLink = e.sourceLink;
> >  	}
> >
> >  	return 0;

-- 
Regards,

Laurent Pinchart