Message ID | 20220801000543.3501-13-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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
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() } 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) + continue; + } + for (MediaLink *link : pad->links()) { if (link == sinkLink) continue;