[PATCH/RFC 09/32] libcamera: v4l2_subdevice: Add V4L2Subdevice::Route structure

[Laurent Pinchart]

On Tue, Mar 05, 2024 at 09:55:58AM +0100, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Fri, Mar 01, 2024 at 11:20:58PM +0200, Laurent Pinchart wrote:
> > The V4L2Subdevice class deals with streams in two places:
> >
> > - In routing tables, streams as expressed as a pad number and a stream
> >   number in a v4l2_subdev_route instance.
> > - In the format and selection get and set functions, streams as
> >   expressed using the Stream structure, which binds the pad number and
> >   stream number.
> >
> > Expressing streams in different ways requires pipeline handlers and
> > other helpers to convert between the two representations. This isn't
> > much of an issue yet as libcamera has little stream-aware code, but it
> > is expected to increasingly become a burden.
> >
> > To simplify the API, introduce a V4L2Subdevice::Route structure that
> > mimicks the kernel v4l2_subdev_route structure but represents streams as
> > V4L2Subdevice::Stream instances. This will improve seamless integration
> > of routes, formats and selection rectangles.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Clear routing at the beginning of getRouting() and setRouting()
> > ---
> >  include/libcamera/internal/v4l2_subdevice.h |  19 +++-
> >  src/libcamera/pipeline/simple/simple.cpp    |   8 +-
> >  src/libcamera/v4l2_subdevice.cpp            | 106 ++++++++++++++++++--
> >  3 files changed, 118 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index 2939dc2411c6..01ed4c2fc397 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -95,7 +95,23 @@ public:
> >  		unsigned int stream;
> >  	};
> >
> > -	using Routing = std::vector<struct v4l2_subdev_route>;
> > +	struct Route {
> > +		Route()
> > +			: flags(0)
> > +		{
> > +		}
> > +
> > +		Route(const Stream &snk, const Stream &src, uint32_t f)
> > +			: sink(snk), source(src), flags(f)
> > +		{
> > +		}
> > +
> > +		Stream sink;
> > +		Stream source;
> > +		uint32_t flags;
> > +	};
> > +
> > +	using Routing = std::vector<Route>;
> >
> >  	explicit V4L2Subdevice(const MediaEntity *entity);
> >  	~V4L2Subdevice();
> > @@ -174,6 +190,7 @@ static inline bool operator!=(const V4L2Subdevice::Stream &lhs,
> >  }
> >
> >  std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);
> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Route &route);
> >  std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing);
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index feea26fd124f..01f2a97798ba 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -852,17 +852,17 @@ std::vector<const MediaPad *> SimpleCameraData::routedSourcePads(MediaPad *sink)
> >
> >  	std::vector<const MediaPad *> pads;
> >
> > -	for (const struct v4l2_subdev_route &route : routing) {
> > -		if (sink->index() != route.sink_pad ||
> > +	for (const V4L2Subdevice::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);
> > +		const MediaPad *pad = entity->getPadByIndex(route.source.pad);
> >  		if (!pad) {
> >  			LOG(SimplePipeline, Warning)
> >  				<< "Entity " << entity->name()
> >  				<< " has invalid route source pad "
> > -				<< route.source_pad;
> > +				<< route.source.pad;
> >  		}
> >
> >  		pads.push_back(pad);
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 5dedfde4107f..3653f57a7d10 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -870,6 +870,53 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> >  	return out;
> >  }
> >
> > +/**
> > + * \class V4L2Subdevice::Route
> > + * \brief V4L2 subdevice routing table entry
> > + *
> > + * This class models a route in the subdevice routing table. It is similar to
> > + * the v4l2_subdev_route structure, but uses the V4L2Subdevice::Stream class
> > + * for easier usage with the V4L2Subdevice stream-aware functions.
> > + *
> > + * \var V4L2Subdevice::Route::sink
> > + * \brief The sink stream of the route
> > + *
> > + * \var V4L2Subdevice::Route::source
> > + * \brief The source stream of the route
> > + *
> > + * \var V4L2Subdevice::Route::flags
> > + * \brief The route flags (V4L2_SUBDEV_ROUTE_FL_*)
> > + */
> > +
> > +/**
> > + * \fn V4L2Subdevice::Route::Route()
> > + * \brief Construct a Route with default streams
> > + */
> > +
> > +/**
> > + * \fn V4L2Subdevice::Route::Route(const Stream &sink, const Stream &source,
> > + * uint32_t flags)
> > + * \brief Construct a Route from \a sink to \a source
> > + * \param[in] sink The sink stream
> > + * \param[in] source The source stream
> > + * \param[in] flags The route flags
> > + */
> > +
> > +/**
> > + * \brief Insert a text representation of a V4L2Subdevice::Route into an
> > + *	output stream
> 
> Still not sure if this is intentional.
> 
> > + * \param[in] out The output stream
> > + * \param[in] route The V4L2Subdevice::Route
> > + * \return The output stream \a out
> > + */
> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Route &route)
> > +{
> > +	out << route.sink << " -> " << route.source
> > +	    << " (" << utils::hex(route.flags) << ")";
> > +
> > +	return out;
> > +}
> > +
> >  /**
> >   * \typedef V4L2Subdevice::Routing
> >   * \brief V4L2 subdevice routing table
> > @@ -887,10 +934,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> >  std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing)
> >  {
> >  	for (const auto &[i, route] : utils::enumerate(routing)) {
> > -		out << "[" << i << "] "
> > -		    << route.sink_pad << "/" << route.sink_stream << " -> "
> > -		    << route.source_pad << "/" << route.source_stream
> > -		    << " (" << utils::hex(route.flags) << ")";
> > +		out << "[" << i << "] " << route;
> >  		if (i != routing.size() - 1)
> >  			out << ", ";
> >  	}
> > @@ -1244,6 +1288,30 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> >   * \return 0 on success or a negative error code otherwise
> >   */
> >
> > +namespace {
> > +
> > +void routeFromKernel(V4L2Subdevice::Route &route,
> > +		     const struct v4l2_subdev_route &kroute)
> > +{
> > +	route.sink.pad = kroute.sink_pad;
> > +	route.sink.stream = kroute.sink_stream;
> > +	route.source.pad = kroute.source_pad;
> > +	route.source.stream = kroute.source_stream;
> > +	route.flags = kroute.flags;
> > +}
> > +
> > +void routeToKernel(const V4L2Subdevice::Route &route,
> > +		   struct v4l2_subdev_route &kroute)
> > +{
> > +	kroute.sink_pad = route.sink.pad;
> > +	kroute.sink_stream = route.sink.stream;
> > +	kroute.source_pad = route.source.pad;
> > +	kroute.source_stream = route.source.stream;
> > +	kroute.flags = route.flags;
> > +}
> 
> Static helpers always feel a bit meh to me.. have you considered
> 
>         Route::fromKernelRoute(const struct v4l2_subdev_route &kroute);
>         Route::toKernelRoute(struct v4l2_subdev_route *kroute);
> 
> and decided static helpers are better ?

The idea is to make those internal, I don't think they need to be
exposed in the API. The whole point of the Route structure is to avoid
exposing v4l2_subdev_route :-)

> > +
> > +} /* namespace */
> > +
> >  /**
> >   * \brief Retrieve the subdevice's internal routing table
> >   * \param[out] routing The routing table
> > @@ -1254,6 +1322,8 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> >   */
> >  int V4L2Subdevice::getRouting(Routing *routing, Whence whence)
> >  {
> > +	routing->clear();
> > +
> >  	if (!caps_.hasStreams())
> >  		return 0;
> >
> > @@ -1272,8 +1342,8 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)
> >  		return ret;
> >  	}
> >
> > -	routing->resize(rt.num_routes);
> > -	rt.routes = reinterpret_cast<uintptr_t>(routing->data());
> > +	std::vector<struct v4l2_subdev_route> routes{ rt.num_routes };
> > +	rt.routes = reinterpret_cast<uintptr_t>(routes.data());
> >
> >  	ret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);
> >  	if (ret) {
> > @@ -1282,11 +1352,16 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)
> >  		return ret;
> >  	}
> >
> > -	if (rt.num_routes != routing->size()) {
> > +	if (rt.num_routes != routes.size()) {
> >  		LOG(V4L2, Error) << "Invalid number of routes";
> >  		return -EINVAL;
> >  	}
> >
> > +	routing->resize(rt.num_routes);
> > +
> > +	for (const auto &[i, route] : utils::enumerate(routes))
> > +		routeFromKernel((*routing)[i], route);
> > +
> >  	return 0;
> >  }
> >
> > @@ -1304,13 +1379,20 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)
> >   */
> >  int V4L2Subdevice::setRouting(Routing *routing, Whence whence)
> >  {
> > -	if (!caps_.hasStreams())
> > +	if (!caps_.hasStreams()) {
> > +		routing->clear();
> >  		return 0;
> > +	}
> > +
> > +	std::vector<struct v4l2_subdev_route> routes{ routing->size() };
> > +
> > +	for (const auto &[i, route] : utils::enumerate(*routing))
> > +		routeToKernel(route, routes[i]);
> >
> >  	struct v4l2_subdev_routing rt = {};
> >  	rt.which = whence;
> > -	rt.num_routes = routing->size();
> > -	rt.routes = reinterpret_cast<uintptr_t>(routing->data());
> > +	rt.num_routes = routes.size();
> > +	rt.routes = reinterpret_cast<uintptr_t>(routes.data());
> >
> >  	int ret = ioctl(VIDIOC_SUBDEV_S_ROUTING, &rt);
> >  	if (ret) {
> > @@ -1318,8 +1400,12 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence)
> >  		return ret;
> >  	}
> >
> > +	routes.resize(rt.num_routes);
> >  	routing->resize(rt.num_routes);
> >
> > +	for (const auto &[i, route] : utils::enumerate(routes))
> > +		routeFromKernel((*routing)[i], route);
> > +
> >  	return 0;
> 
> With or without the static helpers
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> >  }
> >

-- 
Regards,

Laurent Pinchart