[libcamera-devel,12/13] libcamera: pipeline: simple: Don't disable links carrying other streams

16889 diff mbox series

From: Phi-Bang Nguyen <pnguyen@baylibre.com>

If a subdev supports the internal routing API, pads unrelated to the
pipeline for a given camera sensor may carry streams for other cameras.
The link setup logic is updated to take this into account, by avoiding
disabling links to unrelated pads.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

Hi Laurent,

On Mon, Aug 01, 2022 at 03:05:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Phi-Bang Nguyen <pnguyen@baylibre.com>
>
> If a subdev supports the internal routing API, pads unrelated to the
> pipeline for a given camera sensor may carry streams for other cameras.
> The link setup logic is updated to take this into account, by avoiding
> disabling links to unrelated pads.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 2a8811183907..c80e462bc449 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -221,6 +221,11 @@ public:
>  	struct Entity {
>  		/* The media entity, always valid. */
>  		MediaEntity *entity;
> +		/*
> +		 * Whether or not the entity is a subdev that supports the
> +		 * routing API.
> +		 */
> +		bool supportsRouting;
>  		/*
>  		 * The local sink pad connected to the upstream entity, null for
>  		 * the camera sensor at the beginning of the pipeline.
> @@ -404,9 +409,13 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
>  		 */
>
>  		std::vector<const MediaPad *> pads;
> +		bool supportsRouting = false;
>
> -		if (sinkPad)
> +		if (sinkPad) {
>  			pads = routedSourcePads(sinkPad);
> +			if (!pads.empty())
> +				supportsRouting = true;
> +		}
>
>  		if (pads.empty()) {
>  			for (const MediaPad *pad : entity->pads()) {
> @@ -421,7 +430,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
>  				MediaEntity *next = link->sink()->entity();
>  				if (visited.find(next) == visited.end()) {
>  					queue.push({ next, link->sink() });
> -					parents.insert({ next, { entity, sinkPad, pad, link } });
> +
> +					Entity e{ entity, supportsRouting, sinkPad, pad, link };
> +					parents.insert({ next, e });
>  				}
>  			}
>  		}
> @@ -435,7 +446,7 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
>  	 * to the sensor. Store all the entities in the pipeline, from the
>  	 * camera sensor to the video node, in entities_.
>  	 */
> -	entities_.push_front({ entity, sinkPad, nullptr, nullptr });
> +	entities_.push_front({ entity, false, sinkPad, nullptr, nullptr });
>
>  	for (auto it = parents.find(entity); it != parents.end();
>  	     it = parents.find(entity)) {
> @@ -617,6 +628,17 @@ int SimpleCameraData::setupLinks()

I would also update the comment at function begin that reports

	/*
	 * Configure all links along the pipeline. Some entities may not allow
	 * 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.
	 */

To mention that this doesn't apply to routed subdevs

>  		}
>
>  		for (MediaPad *pad : e.entity->pads()) {
> +			/*
> +			 * If the entity supports the V4L2 internal routing API,
> +			 * assume that it may carry multiple independent streams
> +			 * concurrently, and only disable links on the sink and
> +			 * source pads used by the pipeline.
> +			 */
> +			if (e.supportsRouting) {
> +				if (pad != e.sink && pad != e.source)

Or
                        if (e.supportsRouting &&
                            (pad != e.sink && pad != e.source))

> +					continue;
> +			}
> +
>  			for (MediaLink *link : pad->links()) {
>  				if (link == sinkLink)
>  					continue;
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Mon, Aug 01, 2022 at 06:25:23PM +0200, Jacopo Mondi wrote:
> On Mon, Aug 01, 2022 at 03:05:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Phi-Bang Nguyen <pnguyen@baylibre.com>
> >
> > If a subdev supports the internal routing API, pads unrelated to the
> > pipeline for a given camera sensor may carry streams for other cameras.
> > The link setup logic is updated to take this into account, by avoiding
> > disabling links to unrelated pads.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++++---
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 2a8811183907..c80e462bc449 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -221,6 +221,11 @@ public:
> >  	struct Entity {
> >  		/* The media entity, always valid. */
> >  		MediaEntity *entity;
> > +		/*
> > +		 * Whether or not the entity is a subdev that supports the
> > +		 * routing API.
> > +		 */
> > +		bool supportsRouting;
> >  		/*
> >  		 * The local sink pad connected to the upstream entity, null for
> >  		 * the camera sensor at the beginning of the pipeline.
> > @@ -404,9 +409,13 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >  		 */
> >
> >  		std::vector<const MediaPad *> pads;
> > +		bool supportsRouting = false;
> >
> > -		if (sinkPad)
> > +		if (sinkPad) {
> >  			pads = routedSourcePads(sinkPad);
> > +			if (!pads.empty())
> > +				supportsRouting = true;
> > +		}
> >
> >  		if (pads.empty()) {
> >  			for (const MediaPad *pad : entity->pads()) {
> > @@ -421,7 +430,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >  				MediaEntity *next = link->sink()->entity();
> >  				if (visited.find(next) == visited.end()) {
> >  					queue.push({ next, link->sink() });
> > -					parents.insert({ next, { entity, sinkPad, pad, link } });
> > +
> > +					Entity e{ entity, supportsRouting, sinkPad, pad, link };
> > +					parents.insert({ next, e });
> >  				}
> >  			}
> >  		}
> > @@ -435,7 +446,7 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> >  	 * to the sensor. Store all the entities in the pipeline, from the
> >  	 * camera sensor to the video node, in entities_.
> >  	 */
> > -	entities_.push_front({ entity, sinkPad, nullptr, nullptr });
> > +	entities_.push_front({ entity, false, sinkPad, nullptr, nullptr });
> >
> >  	for (auto it = parents.find(entity); it != parents.end();
> >  	     it = parents.find(entity)) {
> > @@ -617,6 +628,17 @@ int SimpleCameraData::setupLinks()
> 
> I would also update the comment at function begin that reports
> 
> 	/*
> 	 * Configure all links along the pipeline. Some entities may not allow
> 	 * 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.
> 	 */
> 
> To mention that this doesn't apply to routed subdevs

I had a look but it would really duplicate the comment below. As it
describes the internal implementation and not a public API, and the two
comments are 9 lines of code apart, would you mind if I left it as-is ?

> >  		}
> >
> >  		for (MediaPad *pad : e.entity->pads()) {
> > +			/*
> > +			 * If the entity supports the V4L2 internal routing API,
> > +			 * assume that it may carry multiple independent streams
> > +			 * concurrently, and only disable links on the sink and
> > +			 * source pads used by the pipeline.
> > +			 */
> > +			if (e.supportsRouting) {
> > +				if (pad != e.sink && pad != e.source)
> 
> Or
>                         if (e.supportsRouting &&
>                             (pad != e.sink && pad != e.source))

I'll drop the inner parentheses too.

> > +					continue;
> > +			}
> > +
> >  			for (MediaLink *link : pad->links()) {
> >  				if (link == sinkLink)
> >  					continue;

Hi Laurent

On Mon, Aug 01, 2022 at 11:59:33PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 01, 2022 at 06:25:23PM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 01, 2022 at 03:05:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > From: Phi-Bang Nguyen <pnguyen@baylibre.com>
> > >
> > > If a subdev supports the internal routing API, pads unrelated to the
> > > pipeline for a given camera sensor may carry streams for other cameras.
> > > The link setup logic is updated to take this into account, by avoiding
> > > disabling links to unrelated pads.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++++---
> > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 2a8811183907..c80e462bc449 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -221,6 +221,11 @@ public:
> > >  	struct Entity {
> > >  		/* The media entity, always valid. */
> > >  		MediaEntity *entity;
> > > +		/*
> > > +		 * Whether or not the entity is a subdev that supports the
> > > +		 * routing API.
> > > +		 */
> > > +		bool supportsRouting;
> > >  		/*
> > >  		 * The local sink pad connected to the upstream entity, null for
> > >  		 * the camera sensor at the beginning of the pipeline.
> > > @@ -404,9 +409,13 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > >  		 */
> > >
> > >  		std::vector<const MediaPad *> pads;
> > > +		bool supportsRouting = false;
> > >
> > > -		if (sinkPad)
> > > +		if (sinkPad) {
> > >  			pads = routedSourcePads(sinkPad);
> > > +			if (!pads.empty())
> > > +				supportsRouting = true;
> > > +		}
> > >
> > >  		if (pads.empty()) {
> > >  			for (const MediaPad *pad : entity->pads()) {
> > > @@ -421,7 +430,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > >  				MediaEntity *next = link->sink()->entity();
> > >  				if (visited.find(next) == visited.end()) {
> > >  					queue.push({ next, link->sink() });
> > > -					parents.insert({ next, { entity, sinkPad, pad, link } });
> > > +
> > > +					Entity e{ entity, supportsRouting, sinkPad, pad, link };
> > > +					parents.insert({ next, e });
> > >  				}
> > >  			}
> > >  		}
> > > @@ -435,7 +446,7 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > >  	 * to the sensor. Store all the entities in the pipeline, from the
> > >  	 * camera sensor to the video node, in entities_.
> > >  	 */
> > > -	entities_.push_front({ entity, sinkPad, nullptr, nullptr });
> > > +	entities_.push_front({ entity, false, sinkPad, nullptr, nullptr });
> > >
> > >  	for (auto it = parents.find(entity); it != parents.end();
> > >  	     it = parents.find(entity)) {
> > > @@ -617,6 +628,17 @@ int SimpleCameraData::setupLinks()
> >
> > I would also update the comment at function begin that reports
> >
> > 	/*
> > 	 * Configure all links along the pipeline. Some entities may not allow
> > 	 * 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.
> > 	 */
> >
> > To mention that this doesn't apply to routed subdevs
>
> I had a look but it would really duplicate the comment below. As it
> describes the internal implementation and not a public API, and the two
> comments are 9 lines of code apart, would you mind if I left it as-is ?
>

fine

> > >  		}
> > >
> > >  		for (MediaPad *pad : e.entity->pads()) {
> > > +			/*
> > > +			 * If the entity supports the V4L2 internal routing API,
> > > +			 * assume that it may carry multiple independent streams
> > > +			 * concurrently, and only disable links on the sink and
> > > +			 * source pads used by the pipeline.
> > > +			 */
> > > +			if (e.supportsRouting) {
> > > +				if (pad != e.sink && pad != e.source)
> >
> > Or
> >                         if (e.supportsRouting &&
> >                             (pad != e.sink && pad != e.source))
>
> I'll drop the inner parentheses too.

Better thanks

>
> > > +					continue;
> > > +			}
> > > +
> > >  			for (MediaLink *link : pad->links()) {
> > >  				if (link == sinkLink)
> > >  					continue;
>
> --
> Regards,
>
> Laurent Pinchart

Hi Laurent

 forgot to leave a tag here

 Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


On Tue, Aug 02, 2022 at 09:51:10AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Laurent
>
> On Mon, Aug 01, 2022 at 11:59:33PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Mon, Aug 01, 2022 at 06:25:23PM +0200, Jacopo Mondi wrote:
> > > On Mon, Aug 01, 2022 at 03:05:42AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > From: Phi-Bang Nguyen <pnguyen@baylibre.com>
> > > >
> > > > If a subdev supports the internal routing API, pads unrelated to the
> > > > pipeline for a given camera sensor may carry streams for other cameras.
> > > > The link setup logic is updated to take this into account, by avoiding
> > > > disabling links to unrelated pads.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/simple/simple.cpp | 28 +++++++++++++++++++++---
> > > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > > index 2a8811183907..c80e462bc449 100644
> > > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > > @@ -221,6 +221,11 @@ public:
> > > >  	struct Entity {
> > > >  		/* The media entity, always valid. */
> > > >  		MediaEntity *entity;
> > > > +		/*
> > > > +		 * Whether or not the entity is a subdev that supports the
> > > > +		 * routing API.
> > > > +		 */
> > > > +		bool supportsRouting;
> > > >  		/*
> > > >  		 * The local sink pad connected to the upstream entity, null for
> > > >  		 * the camera sensor at the beginning of the pipeline.
> > > > @@ -404,9 +409,13 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > > >  		 */
> > > >
> > > >  		std::vector<const MediaPad *> pads;
> > > > +		bool supportsRouting = false;
> > > >
> > > > -		if (sinkPad)
> > > > +		if (sinkPad) {
> > > >  			pads = routedSourcePads(sinkPad);
> > > > +			if (!pads.empty())
> > > > +				supportsRouting = true;
> > > > +		}
> > > >
> > > >  		if (pads.empty()) {
> > > >  			for (const MediaPad *pad : entity->pads()) {
> > > > @@ -421,7 +430,9 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > > >  				MediaEntity *next = link->sink()->entity();
> > > >  				if (visited.find(next) == visited.end()) {
> > > >  					queue.push({ next, link->sink() });
> > > > -					parents.insert({ next, { entity, sinkPad, pad, link } });
> > > > +
> > > > +					Entity e{ entity, supportsRouting, sinkPad, pad, link };
> > > > +					parents.insert({ next, e });
> > > >  				}
> > > >  			}
> > > >  		}
> > > > @@ -435,7 +446,7 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > > >  	 * to the sensor. Store all the entities in the pipeline, from the
> > > >  	 * camera sensor to the video node, in entities_.
> > > >  	 */
> > > > -	entities_.push_front({ entity, sinkPad, nullptr, nullptr });
> > > > +	entities_.push_front({ entity, false, sinkPad, nullptr, nullptr });
> > > >
> > > >  	for (auto it = parents.find(entity); it != parents.end();
> > > >  	     it = parents.find(entity)) {
> > > > @@ -617,6 +628,17 @@ int SimpleCameraData::setupLinks()
> > >
> > > I would also update the comment at function begin that reports
> > >
> > > 	/*
> > > 	 * Configure all links along the pipeline. Some entities may not allow
> > > 	 * 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.
> > > 	 */
> > >
> > > To mention that this doesn't apply to routed subdevs
> >
> > I had a look but it would really duplicate the comment below. As it
> > describes the internal implementation and not a public API, and the two
> > comments are 9 lines of code apart, would you mind if I left it as-is ?
> >
>
> fine
>
> > > >  		}
> > > >
> > > >  		for (MediaPad *pad : e.entity->pads()) {
> > > > +			/*
> > > > +			 * If the entity supports the V4L2 internal routing API,
> > > > +			 * assume that it may carry multiple independent streams
> > > > +			 * concurrently, and only disable links on the sink and
> > > > +			 * source pads used by the pipeline.
> > > > +			 */
> > > > +			if (e.supportsRouting) {
> > > > +				if (pad != e.sink && pad != e.source)
> > >
> > > Or
> > >                         if (e.supportsRouting &&
> > >                             (pad != e.sink && pad != e.source))
> >
> > I'll drop the inner parentheses too.
>
> Better thanks
>
> >
> > > > +					continue;
> > > > +			}
> > > > +
> > > >  			for (MediaLink *link : pad->links()) {
> > > >  				if (link == sinkLink)
> > > >  					continue;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart