Re: [PATCH 11/13] media: v4l2-subdev: Introduce v4l2_subdev_find_route()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Fri, Jun 27, 2025 at 01:20:47AM +0300, Laurent Pinchart wrote:
> On Wed, Jun 25, 2025 at 04:53:54PM +0000, Sakari Ailus wrote:
> > On Fri, Jun 20, 2025 at 10:14:58AM +0200, Jacopo Mondi wrote:
> > > On Thu, Jun 19, 2025 at 11:15:44AM +0300, Sakari Ailus wrote:
> > > > v4l2_subdev_find_route() is like v4l2_subdev_routing_find_opposite_end(),
> > > > with the difference that it's more flexible: it can look up only active
> > > > routes and can find multiple routes, too.
> > > >
> > > > v4l2_subdev_find_route() is intended to replace
> > > > v4l2_subdev_routing_find_opposite_end().
> > > 
> > > To me this feels like v4l2_subdev_find_route() could be used to
> > > implement more helpers like v4l2_subdev_routing_find_opposite_end()
> > > for drivers instead of going the other way around.
> > > 
> > > let's see what the use cases are
> > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-subdev.c | 56 ++++++++++++++++++---------
> > > >  include/media/v4l2-subdev.h           | 19 +++++++++
> > > >  2 files changed, 56 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > index c549a462dac7..13d6e96daf3a 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > @@ -1996,34 +1996,52 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing_with_fmt);
> > > >
> > > > -int v4l2_subdev_routing_find_opposite_end(const struct v4l2_subdev_krouting *routing,
> > > > -					  u32 pad, u32 stream, u32 *other_pad,
> > > > -					  u32 *other_stream)
> > > > +struct v4l2_subdev_route *
> > > > +v4l2_subdev_find_route(const struct v4l2_subdev_krouting *routing,
> > > > +		       u32 pad, u32 stream, bool active, unsigned int index)
> > > >  {
> > > >  	unsigned int i;
> > > >
> > > >  	for (i = 0; i < routing->num_routes; ++i) {
> > > >  		struct v4l2_subdev_route *route = &routing->routes[i];
> > > >
> > > > -		if (route->source_pad == pad &&
> > > > -		    route->source_stream == stream) {
> > > > -			if (other_pad)
> > > > -				*other_pad = route->sink_pad;
> > > > -			if (other_stream)
> > > > -				*other_stream = route->sink_stream;
> > > > -			return 0;
> > > > -		}
> > > > +		if (active && !(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > > > +			continue;
> > > 
> > > I know currently v4l2_subdev_routing_find_opposite_end() does return
> > > any route that matches the provided 'pad' and 'stream' included
> > > non-active ones, but I wonder if this is desirable. What is the use
> > > case for enumerating a non-active route between two pads ?
> > 
> > Good question. v4l2_subdev_routing_find_opposite_end() nevertheless returns
> > them. And the caller won't get the route for checking the state either.
> 
> I'm tempted to check the callers of the function and change the
> behaviour to only return active routes.

I can prepend a patch to do the change there so it won't happen here.

> 
> > > (it is also my impression that all drivers that use
> > > v4l2_subdev_routing_find_opposite_end() assume the route is active)
> > > 
> > > Also I wonder if the usage of V4L2_SUBDEV_ROUTE_FL_ACTIVE is clearly
> > > defined, or, in other words, what is the use case for userspace to
> > > create non-active routes, given that any new VIDIOC_SUBDEV_S_ROUTING
> > > will anyway re-create the routing table (that's a different question,
> > > on the ioctl definition and not on this change though)
> > 
> > I think it is. Please review the UAPI documentation in the metadata series.
> > :-)
> 
> Section "Device types and routing setup" in
> Documentation/userspace-api/media/v4l/dev-subdev.rst is the best
> documentation we have at the moment.
> 
> We essentially have two models for internal routing. In the first model,
> which I'll nickname the crossbar switch model, a large number of routes
> are possible (up to any input to any output types of scenarios). In this
> case, routes are created by userspace, and all routes in the routing
> table are expected to be active. Any inactive route provided by
> userspace would be dropped by the driver and not be included in the
> routing table.
> 
> The second model covers devices such as camera sensors, where a small
> fixed set of routes are hardcoded. The routing table is fixed, an some
> routes (the ones not marked with the IMMUTABLE flag) can be
> enabled/disabled by userspace using the V4L2_SUBDEV_ROUTE_FL_ACTIVE.
> This allows userspace to enumerate the available routes.
> 
> Documentation/userspace-api/media/v4l/dev-subdev.rst should document
> more clearly that we do not allow any hybrid behaviour at the moment.

I agree.

> 
> > > >
> > > > -		if (route->sink_pad == pad && route->sink_stream == stream) {
> > > > -			if (other_pad)
> > > > -				*other_pad = route->source_pad;
> > > > -			if (other_stream)
> > > > -				*other_stream = route->source_stream;
> > > > -			return 0;
> > > > -		}
> > > > +		if ((route->source_pad != pad ||
> > > > +		     route->source_stream != stream) &&
> > > > +		    (route->sink_pad != pad || route->sink_stream != stream))
> > > > +			continue;
> > > > +
> > > > +		if (index--)
> > > > +			continue;
> > > > +
> > > > +		return route;
> > > >  	}
> > > >
> > > > -	return -EINVAL;
> > > > +	return ERR_PTR(-ENOENT);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(v4l2_subdev_find_route);
> > > > +
> > > > +int v4l2_subdev_routing_find_opposite_end(const struct v4l2_subdev_krouting *routing,
> > > > +					  u32 pad, u32 stream, u32 *other_pad,
> > > > +					  u32 *other_stream)
> > > > +{
> > > > +	struct v4l2_subdev_route *route;
> > > > +
> > > > +	route = v4l2_subdev_find_route(routing, pad, stream, false, 0);
> > > > +	if (IS_ERR(route))
> > > > +		return PTR_ERR(route);
> > > > +
> > > > +	bool is_source = route->source_pad == pad;
> > > > +
> > > > +	if (other_pad)
> > > > +		*other_pad = is_source ? route->sink_pad : route->source_pad;
> > > > +	if (other_stream)
> > > > +		*other_stream = is_source ?
> > > > +			route->sink_stream : route->source_stream;
> 
> Having to do this is_source dance makes v4l2_subdev_find_route()
> annoying to use. It may be fine when using the function to implement
> other helpers, but I wouldn't like to see this being done in drivers.
> Maybe we can avoid exporting v4l2_subdev_find_route() for now to ensure
> that, the only user in this series is in v4l2-mc.c.

Sounds reasonable.

> 
> > > > +
> > > > +	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(v4l2_subdev_routing_find_opposite_end);
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index deab128a4779..9ed8600ba3d4 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -1547,6 +1547,23 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
> > > >  				     const struct v4l2_subdev_krouting *routing,
> > > >  				     const struct v4l2_mbus_framefmt *fmt);
> > > >
> > > > +/**
> > > > + * v4l2_subdev_find_route() - Find routes from a (pad, stream) pair
> > > 
> > > from or for ?
> > 
> > From or to. I'll fix this in the next version.
> > 
> > > 
> > > > + * @routing: routing used to find the opposite side
> > > 
> > > I would not say "opposite side" but rather
> > > 
> > >       @routing: routing table used to enumerate routes
> > 
> > How about simply "the routing table"?
> > 
> > > 
> > > > + * @pad: pad id
> > > > + * @stream: stream id
> > > > + * @active: set to true for looking up only active routes
> > > > + * @index: for accessing more than one route from the pad
> > > 
> > > I understand this but maybe
> > > 
> > >      @index: route index for enumerating multiple routes
> > > ?
> > 
> > Sounds good.
> 
> I'm curious to know how this parameter will be used. In the only user
> (in patch 12/13), it is hardcoded to 0. I'm not sure indexing routes
> will be very useful for drivers.

Looks like it'll be needed by v4l2_subdev_set_streams_enabled(), as a
result on discussion related to it.

> 
> > > > + *
> > > > + * Find a route from the routing table where one end has (pad, stream) pair
> > > > + * matching @pad and @stream.
> > > 
> > >     * If multiple routes in @routing match @pad and @stream, return
> > >     * the @index one.
> > >     *
> > >     * Set @active to true to only enumerate active routes.
> > > 
> > > > + *
> > > > + * Returns the route on success or -ENOENT if no matching route is found.
> > > 
> > > I see other functions documentation using
> > > 
> > >     * Return:
> > > 
> > > is this a kernel-doc thing ?
> > 
> > Yes, makes sense.
> > 
> > > > + */
> > > > +struct v4l2_subdev_route *
> > > > +v4l2_subdev_find_route(const struct v4l2_subdev_krouting *routing,
> > > > +		       u32 pad, u32 stream, bool active, unsigned int index);
> > > > +
> > > >  /**
> > > >   * v4l2_subdev_routing_find_opposite_end() - Find the opposite stream
> > > >   * @routing: routing used to find the opposite side
> > > > @@ -1555,6 +1572,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
> > > >   * @other_pad: pointer used to return the opposite pad
> > > >   * @other_stream: pointer used to return the opposite stream
> > > >   *
> > > > + * Prefer v4l2_subdev_find_route() over v4l2_subdev_routing_find_opposite_end().
> > > > + *
> > > 
> > > As said, I'm not sure if that's preferred or we should rather create
> > > more helpers using v4l2_subdev_find_route() internally. Time will tell
> > > I guess ?
> > 
> > I agree.
> > 
> > The benefit of the older function was that it returns information that
> > doesn't need a lock for accessing it.
> 
> I'm not sure to follow you here. v4l2_subdev_find_route() has the same
> locking requirements as v4l2_subdev_routing_find_opposite_end().

The function itself does, but a lock is not required for accessing
information returned by v4l2_subdev_routing_find_opposite_end() (unlike
v4l2_subdev_find_route()).

> 
> Given that v4l2_subdev_find_route() has a single user, and that the only
> difference in that user compared to
> v4l2_subdev_routing_find_opposite_end() is that only active routes are
> considered, I would prefer modifying
> v4l2_subdev_routing_find_opposite_end() to ignore inactive routes after
> checking that no caller would break.

-- 
Regards,

Sakari Ailus