[PATCH/RFC 09/32] libcamera: v4l2_subdevice: Add V4L2Subdevice::Route structure
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Mar 5 09:55:58 CET 2024
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 ?
> +
> +} /* 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
>
More information about the libcamera-devel
mailing list