[PATCH 9/9] libcamera: v4l2_subdevice: Add V4L2Subdevice::Route structure
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Feb 27 17:55:25 CET 2024
Hi Laurent
On Tue, Feb 27, 2024 at 04:09:53PM +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>
> ---
> include/libcamera/internal/v4l2_subdevice.h | 9 ++-
> src/libcamera/pipeline/simple/simple.cpp | 8 +-
> src/libcamera/v4l2_subdevice.cpp | 86 ++++++++++++++++++---
> 3 files changed, 89 insertions(+), 14 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 2f64b3deadfe..38c340236908 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -85,7 +85,13 @@ public:
> unsigned int stream;
> };
>
> - using Routing = std::vector<struct v4l2_subdev_route>;
> + struct Route {
> + Stream sink;
> + Stream source;
> + uint32_t flags;
> + };
> +
> + using Routing = std::vector<Route>;
>
> explicit V4L2Subdevice(const MediaEntity *entity);
> ~V4L2Subdevice();
> @@ -157,6 +163,7 @@ private:
> };
>
> 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 1dbbd7fe91d6..c1eeae643d71 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -851,17 +851,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 1670c31a9656..2e9e05c86059 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -843,6 +843,39 @@ 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_*)
> + */
> +
> +/**
> + * \brief Insert a text representation of a V4L2Subdevice::Route into an
> + * output stream
> + * \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
> @@ -860,10 +893,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 << ", ";
> }
> @@ -1222,6 +1252,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;
> +}
> +
> +} /* namespace */
> +
Why 2 helpers in an anonymous namespace ? I would question if these
are worth to be helpers at all, as they're called from a single
place..
> /**
> * \brief Retrieve the subdevice's internal routing table
> * \param[out] routing The routing table
> @@ -1250,8 +1304,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) {
> @@ -1260,11 +1314,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;
> }
>
> @@ -1285,10 +1344,15 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence)
> if (!caps_.hasStreams())
> 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) {
> @@ -1296,8 +1360,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;
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list