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

[Jacopo Mondi]

Hi Laurent

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.

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

THanks
  j
>
> -		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
>