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

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

Hi Jacopo,

Thanks for the review.

On Fri, Jun 20, 2025 at 10:14:58AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> 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.

> 
> (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.
:-)

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

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

-- 
Regards,

Sakari Ailus