[PATCH 3/3] libcamera: v4l2_subdevice: Update to the new kernel routing API
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 3 11:18:06 CEST 2024
Hi Kieran,
On Mon, Jun 03, 2024 at 10:05:59AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-01 23:03:20)
> > The subdev embedded data support series includes a change to the
> > VIDIOC_SUBDEV_G_ROUTING and VIDIOC_SUBDEV_S_ROUTING ioctls that impacts
> > the userspace API.
> >
> > Update to the new API, while preserving backward compatibility to ease
> > the transition. Document the backward compatibility to only be supported
> > for two kernel releases. As the routing API isn't enabled in any
> > upstream kernel yet, users of the API need kernel patches, and are
> > expected to be able to upgrade quickly.
>
> That sounds reasonable to me.
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since combined RFC:
> >
> > - Drop -ENOTTY special handling in getRouting()
> > - Preserve backward compatibility for a couple of kernel releases.
> > ---
> > include/libcamera/internal/v4l2_subdevice.h | 3 +
> > src/libcamera/v4l2_subdevice.cpp | 121 +++++++++++++++++++-
> > 2 files changed, 120 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index a1de0fb00ee3..194382f84d97 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -176,6 +176,9 @@ private:
> > std::vector<SizeRange> enumPadSizes(const Stream &stream,
> > unsigned int code);
> >
> > + int getRoutingLegacy(Routing *routing, Whence whence);
> > + int setRoutingLegacy(Routing *routing, Whence whence);
> > +
> > const MediaEntity *entity_;
> >
> > std::string model_;
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 6da77775778f..e7be21d7250e 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -1366,8 +1366,62 @@ void routeToKernel(const V4L2Subdevice::Route &route,
> > kroute.flags = route.flags;
> > }
> >
> > +/*
> > + * Legacy routing support for pre-v6.10-rc1 kernels. Drop when v6.12-rc1 gets
> > + * released.
> > + */
> > +struct v4l2_subdev_routing_legacy {
> > + __u32 which;
> > + __u32 num_routes;
> > + __u64 routes;
> > + __u32 reserved[6];
> > +};
> > +
> > +#define VIDIOC_SUBDEV_G_ROUTING_LEGACY _IOWR('V', 38, struct v4l2_subdev_routing_legacy)
> > +#define VIDIOC_SUBDEV_S_ROUTING_LEGACY _IOWR('V', 39, struct v4l2_subdev_routing_legacy)
> > +
> > } /* namespace */
> >
> > +int V4L2Subdevice::getRoutingLegacy(Routing *routing, Whence whence)
> > +{
> > + struct v4l2_subdev_routing_legacy rt = {};
> > +
> > + rt.which = whence;
> > +
> > + int ret = ioctl(VIDIOC_SUBDEV_G_ROUTING_LEGACY, &rt);
> > + if (ret == 0 || ret == -ENOTTY)
> > + return ret;
> > +
> > + if (ret != -ENOSPC) {
> > + LOG(V4L2, Error)
> > + << "Failed to retrieve number of routes: "
> > + << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + std::vector<struct v4l2_subdev_route> routes{ rt.num_routes };
> > + rt.routes = reinterpret_cast<uintptr_t>(routes.data());
> > +
> > + ret = ioctl(VIDIOC_SUBDEV_G_ROUTING_LEGACY, &rt);
> > + if (ret) {
> > + LOG(V4L2, Error)
> > + << "Failed to retrieve routes: " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + 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;
> > +}
> > +
> > /**
> > * \brief Retrieve the subdevice's internal routing table
> > * \param[out] routing The routing table
> > @@ -1388,19 +1442,25 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)
> > rt.which = whence;
> >
> > int ret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);
> > - if (ret == 0 || ret == -ENOTTY)
> > - return ret;
> > + if (ret == -ENOTTY)
> > + return V4L2Subdevice::getRoutingLegacy(routing, whence);
> >
> > - if (ret != -ENOSPC) {
> > + if (ret) {
> > LOG(V4L2, Error)
> > << "Failed to retrieve number of routes: "
> > << strerror(-ret);
> > return ret;
> > }
> >
> > + if (!rt.num_routes)
> > + return 0;
> > +
> > std::vector<struct v4l2_subdev_route> routes{ rt.num_routes };
> > rt.routes = reinterpret_cast<uintptr_t>(routes.data());
> >
> > + rt.len_routes = rt.num_routes;
> > + rt.num_routes = 0;
> > +
> > ret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);
> > if (ret) {
> > LOG(V4L2, Error)
> > @@ -1421,6 +1481,33 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)
> > return 0;
> > }
> >
> > +int V4L2Subdevice::setRoutingLegacy(Routing *routing, Whence whence)
> > +{
> > + 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_legacy rt = {};
> > + rt.which = whence;
> > + rt.num_routes = routes.size();
> > + rt.routes = reinterpret_cast<uintptr_t>(routes.data());
> > +
> > + int ret = ioctl(VIDIOC_SUBDEV_S_ROUTING_LEGACY, &rt);
> > + if (ret) {
> > + LOG(V4L2, Error) << "Failed to set routes: " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + routes.resize(rt.num_routes);
>
> Can the 'kernel may return a larger routing table than was set' issue
> happen here in the legacy version?
No, only the new API allows this. The old API didn't update num_routes.
> Perhaps it was only implemented later (hence the legacy) but we do still
> have the rt.num_routes to check ... ?
>
> > + routing->resize(rt.num_routes);
> > +
> > + for (const auto &[i, route] : utils::enumerate(routes))
> > + routeFromKernel((*routing)[i], route);
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * \brief Set a routing table on the V4L2 subdevice
> > * \param[inout] routing The routing table
> > @@ -1447,16 +1534,42 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence)
> >
> > struct v4l2_subdev_routing rt = {};
> > rt.which = whence;
> > + rt.len_routes = routes.size();
> > rt.num_routes = routes.size();
> > rt.routes = reinterpret_cast<uintptr_t>(routes.data());
> >
> > int ret = ioctl(VIDIOC_SUBDEV_S_ROUTING, &rt);
> > + if (ret == -ENOTTY)
> > + return setRoutingLegacy(routing, whence);
> > +
> > if (ret) {
> > LOG(V4L2, Error) << "Failed to set routes: " << strerror(-ret);
> > return ret;
> > }
> >
> > - routes.resize(rt.num_routes);
> > + /*
> > + * The kernel wants to return more routes than we have space for. We
> > + * need to issue a VIDIOC_SUBDEV_G_ROUTING call.
> > + */
>
> It's a real nit - but the way this is phrased and being outside of the
> conditional - makes it sound like /the kernel always/ wants to do this.
>
> Phrased like that I'd put the comment /inside/ the conditional.
> Outside I would phrase it as
>
> * The kernel may want to return more routes than we have space
> * for. In that event, we must issue a VIDIOC_SUBDEV_G_ROUTING
> * call to determine the correct size.
Not to determine the correct size (that information is returned by
S_ROUTING), but to retrieve the additional routes. I'll update the
comment to
/*
* The kernel may want to return more routes than we have space for. In
* that event, we must issue a VIDIOC_SUBDEV_G_ROUTING call to retrieve
* the additional routes.
*/
> But it doesn't specifically matter here. It's just the context of the
> position of the comment...
>
> > + if (rt.num_routes > routes.size()) {
> > + routes.resize(rt.num_routes);
> > +
> > + rt.len_routes = rt.num_routes;
> > + rt.num_routes = 0;
> > +
> > + ret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);
> > + if (ret) {
> > + LOG(V4L2, Error)
> > + << "Failed to retrieve routes: " << strerror(-ret);
> > + return ret;
>
> I presume the 'set routing' has succeeded even though not all routes
> were specified, and this is simply returning the 'entire' routing table
> back and the extra get is to know that we have all the context in
> userspace for the next part...
Yes.
> I'm happy here. I won't consider the return routing size on the legacy a
> blocker as that's current version and ... legacy to be removed, so I'll
> leave it to you to determine.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + }
> > + }
> > +
> > + 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))
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list