| Message ID | 20240301212121.9072-12-laurent.pinchart@ideasonboard.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, one question On Fri, Mar 01, 2024 at 11:21:00PM +0200, Laurent Pinchart wrote: > 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. Backward compatibility can't be easily preserved > as the ioctl structure size has changed. This is not a major issue, as > the routing API isn't enabled in any upstream kernel. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/v4l2_subdevice.cpp | 43 ++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 3653f57a7d10..d5d400cdfd58 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -1332,19 +1332,23 @@ 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 != -ENOSPC) { > - LOG(V4L2, Error) > - << "Failed to retrieve number of routes: " > - << strerror(-ret); > + if (ret) { > + if (ret != -ENOTTY) If we get here, it means caps_.hasStreams() If a subdevice claims stream support, shouldn't the routing kAPI be enabled, or is it still legit to get a ENOTTY back ? Thanks j > + 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) > @@ -1391,6 +1395,7 @@ 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()); > > @@ -1400,7 +1405,29 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence) > 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. > + */ > + 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; > + } > + } > + > + 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 >
Hi Jacopo, On Tue, Mar 05, 2024 at 09:57:51AM +0100, Jacopo Mondi wrote: > Hi Laurent, > one question > > On Fri, Mar 01, 2024 at 11:21:00PM +0200, Laurent Pinchart wrote: > > 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. Backward compatibility can't be easily preserved > > as the ioctl structure size has changed. This is not a major issue, as > > the routing API isn't enabled in any upstream kernel. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/v4l2_subdevice.cpp | 43 ++++++++++++++++++++++++++------ > > 1 file changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > index 3653f57a7d10..d5d400cdfd58 100644 > > --- a/src/libcamera/v4l2_subdevice.cpp > > +++ b/src/libcamera/v4l2_subdevice.cpp > > @@ -1332,19 +1332,23 @@ 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 != -ENOSPC) { > > - LOG(V4L2, Error) > > - << "Failed to retrieve number of routes: " > > - << strerror(-ret); > > + if (ret) { > > + if (ret != -ENOTTY) > > If we get here, it means > > caps_.hasStreams() > > If a subdevice claims stream support, shouldn't the routing kAPI be > enabled, or is it still legit to get a ENOTTY back ? Good question. For setRouting() I think it may be considered legit, but for getRouting(), likely not. I'll drop the check and log an error unconditionally. > > + 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) > > @@ -1391,6 +1395,7 @@ 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()); > > > > @@ -1400,7 +1405,29 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence) > > 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. > > + */ > > + 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; > > + } > > + } > > + > > + 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))
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index 3653f57a7d10..d5d400cdfd58 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -1332,19 +1332,23 @@ 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 != -ENOSPC) { - LOG(V4L2, Error) - << "Failed to retrieve number of routes: " - << strerror(-ret); + if (ret) { + if (ret != -ENOTTY) + 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) @@ -1391,6 +1395,7 @@ 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()); @@ -1400,7 +1405,29 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence) 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. + */ + 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; + } + } + + 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))
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. Backward compatibility can't be easily preserved as the ioctl structure size has changed. This is not a major issue, as the routing API isn't enabled in any upstream kernel. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/v4l2_subdevice.cpp | 43 ++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 8 deletions(-)