[libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple: Walk pipeline using subdev internal routing
Jacopo Mondi
jacopo at jmondi.org
Tue Aug 2 09:48:30 CEST 2022
Hi Laurent
On Mon, Aug 01, 2022 at 11:46:23PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 01, 2022 at 11:36:19PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > On Mon, Aug 01, 2022 at 06:16:03PM +0200, Jacopo Mondi wrote:
> > > On Mon, Aug 01, 2022 at 03:05:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > From: Phi-Bang Nguyen <pnguyen at baylibre.com>
> > > >
> > > > When traversing the media graph to discover a pipeline from the camera
> > > > sensor to a video node, all sink-to-source paths inside subdevs are
> > > > considered. This can lead to invalid paths being followed, when a subdev
> > > > has restrictions on its internal routing.
> > > >
> > > > The V4L2 API supports exposing subdev internal routing to userspace.
> > > > Make use if this feature, when implemented by a subdev, to restrict the
> > >
> > > Make use "of"
> > >
> > > > internal paths to the currently active routes. If a subdev doesn't
> > > > implement the internal routing operations, all source pads are
> > > > considered, as done today.
> > > >
> > > > This change is needed to properly support multiple sensors with devices
> > > > such as the NXP i.MX8 ISI or the MediaTek i350 and i500 SENINF. Support
> > > > for changing routes dynamically will be added later when required.
> > > >
> > > > Signed-off-by: Phi-Bang Nguyen <pnguyen at baylibre.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++++++++--
> > > > 1 file changed, 67 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > > index 4bde9caa7254..2a8811183907 100644
> > > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > > @@ -100,8 +100,14 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
> > > > *
> > > > * During the breadth-first search, the pipeline is traversed from entity to
> > > > * entity, by following media graph links from source to sink, starting at the
> > > > - * camera sensor. When reaching an entity (on its sink side), all its source
> > > > - * pads are considered to continue the graph traversal.
> > > > + * camera sensor.
> > > > + *
> > > > + * When reaching an entity (on its sink side), if the entity is a V4L2 subdev
> > > > + * that supports the streams API, the subdev internal routes are followed to
> > > > + * find the connected source pads. Otherwise all of the entity's source pads
> > > > + * are considered to continue the graph traversal. The pipeline handler
> > > > + * currently considers the default internal routes only and doesn't attempt to
> > > > + * setup custom routes. This can be extended if needed.
> > > > *
> > > > * The shortest path between the camera sensor and a video node is stored in
> > > > * SimpleCameraData::entities_ as a list of SimpleCameraData::Entity structures,
> > > > @@ -261,6 +267,7 @@ public:
> > > >
> > > > private:
> > > > void tryPipeline(unsigned int code, const Size &size);
> > > > + static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> > > >
> > > > void converterInputDone(FrameBuffer *buffer);
> > > > void converterOutputDone(FrameBuffer *buffer);
> > > > @@ -387,12 +394,29 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > > > break;
> > > > }
> > > >
> > > > - /* The actual breadth-first search algorithm. */
> > > > visited.insert(entity);
> > > > - for (MediaPad *pad : entity->pads()) {
> > > > - if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))
> > > > - continue;
> > > >
> > > > + /*
> > > > + * Add direct downstream entities to the search queue. If the
> > > > + * current entity supports the subdev internal routing API,
> > > > + * restrict the search to downstream entities reachable through
> > > > + * active routes.
> > > > + */
> > > > +
> > >
> > > Is the emtpy line necessary ?
> >
> > The code seems to still run fine without it ;-) I'll drop it.
> >
> > > > + std::vector<const MediaPad *> pads;
> > > > +
> > > > + if (sinkPad)
> > > > + pads = routedSourcePads(sinkPad);
> > > > +
> > > > + if (pads.empty()) {
> > >
> > > pads is empty also in case of errors, like failures to open the
> > > subdev.
> >
> > That's right. That's actually the only failure unrelated to routing (the
> > other failure would come from subdev->getRouting(), which would just
> > indicate that the API isn't supported). We could handle the open failure
> > here, but the subdevs are all opened in SimplePipelineHandler::match()
> > just after constructing the SimpleCameraData instances, so we'll also
> > catch errors there. Failures to open subdevs are really not supposed to
> > happen, so as long as we handle them correctly somewhere, that's good
> > enough for me I think.
> >
> > > Isn't it better to consider pads.emtpy and error condition, and handle
> > > the !sinkPad cases in the "routedSourcePads()" function ?
> >
> > These are two different issues. The sinkPad check is only meant to skip
> > the routedSourcePads() call for the camera sensor entity (that's the
> > only one pushed to the queue with a nullptr for the MediaPad pointer).
> > Then, pads can be empty in error cases indeed, but also if the entity
> > doesn't support the routing API (not all entities do, and certainly not
> > on kernels that don't have the stream series applied :-)). In that case
> > the code defaults to exploring all the source pads of the entity, like
> > it did before this patch.
> >
> > And now that I've written that, I see it matches the code you wrote
> > below :-)
> >
> > >
> > >
> > > std::vector<> pads = connectedSources(entity, sinkPad);
> > > if (pads.empty()) {
> > > LOG(Error) ...
> > > return -EINVAL;
> > > }
> > >
> > > }
> > >
> > > std::vector<> SimpleCameraData::nextSources(entity)
> > > {
> > > vector<> pads;
> > >
> > > for (pad : entity->pads()) {
> > > if (pad->flags & SINK)
> > > continue;
> > >
> > > pads.push_back(pad);
> > > }
> > >
> > > return pads;
> > > }
> > >
> > > std::vector<> SimpleCameraData::connectedSources(entity, sinkPad)
> > > {
> > > if (!sinkPad)
> > > return nextSources(entity);
> > >
> > > ret = subdev->open();
> > > if (ret)
> > > return {};
> > >
> > > ret = subdev->getRouting();
> > > if (!routing)
> > > return nextSources(entity);
> > >
> > > ....
> > > }
> >
> > I was going to push back a bit because I'm lazy, but that looks cleaner
> > :-) I'll give it a try.
>
> That's actually not going to work nicely. If for any reason there's no
> connected source pad, we shouldn't fail, but keep walking the graph
> through all the other links to find a suitable path.
>
connected or routed ?
Anyway, I see your point, it would add yet another case to handle and
the function would grow a bit too much ?
> Would you be OK keeping this patch as-is ?
>
OK
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > + for (const MediaPad *pad : entity->pads()) {
> > > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))
> > > > + continue;
> > > > + pads.push_back(pad);
> > > > + }
> > > > + }
> > > > +
> > > > + for (const MediaPad *pad : pads) {
> > > > for (MediaLink *link : pad->links()) {
> > > > MediaEntity *next = link->sink()->entity();
> > > > if (visited.find(next) == visited.end()) {
> > > > @@ -782,6 +806,43 @@ void SimpleCameraData::converterOutputDone(FrameBuffer *buffer)
> > > > pipe->completeRequest(request);
> > > > }
> > > >
> > > > +/* Retrieve all source pads connected to a sink pad through active routes. */
> > > > +std::vector<const MediaPad *> SimpleCameraData::routedSourcePads(MediaPad *sink)
> > > > +{
> > > > + MediaEntity *entity = sink->entity();
> > > > + std::unique_ptr<V4L2Subdevice> subdev =
> > > > + std::make_unique<V4L2Subdevice>(entity);
> > > > +
> > > > + int ret = subdev->open();
> > > > + if (ret < 0)
> > > > + return {};
> > > > +
> > > > + V4L2Subdevice::Routing routing = {};
> > > > + ret = subdev->getRouting(&routing, V4L2Subdevice::ActiveFormat);
> > > > + if (ret < 0)
> > > > + return {};
> > > > +
> > > > + std::vector<const MediaPad *> pads;
> > > > +
> > > > + for (const struct v4l2_subdev_route &route : routing) {
> > > > + if (sink->index() != route.sink_pad ||
> > > > + !(route.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > > > + continue;
> > > > +
> > > > + const MediaPad *pad = entity->getPadByIndex(route.source_pad);
> > > > + if (!pad) {
> > > > + LOG(SimplePipeline, Warning)
> > > > + << "Entity " << entity->name()
> > > > + << " has invalid route source pad "
> > > > + << route.source_pad;
> > > > + }
> > > > +
> > > > + pads.push_back(pad);
> > > > + }
> > > > +
> > > > + return pads;
> > > > +}
> > > > +
> > > > /* -----------------------------------------------------------------------------
> > > > * Camera Configuration
> > > > */
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list