[PATCH 9/9] libcamera: v4l2_subdevice: Add V4L2Subdevice::Route structure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Feb 28 11:53:43 CET 2024
Hi Jacopo,
On Tue, Feb 27, 2024 at 05:55:25PM +0100, Jacopo Mondi wrote:
> 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..
routeFromKernel() is called in two places, hence the helper. I then
added routeToKernel() as well as I found it more readable to have both
helpers.
> > /**
> > * \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