[libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple: Walk pipeline using subdev internal routing

[Laurent Pinchart]

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.

Would you be OK keeping this patch as-is ?

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