Message ID | 20200718132324.867815-2-niklas.soderlund@ragnatech.se |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Niklas, On Sat, Jul 18, 2020 at 03:23:16PM +0200, Niklas Söderlund wrote: > Add a method to lookup a V4L2 devices path in sysfs. The device path > describes the bus and device backing the V4L2 device and is guaranteed > to be unique in the system and is persistent between reboots of the > system. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/internal/v4l2_device.h | 1 + > src/libcamera/v4l2_device.cpp | 27 ++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index bf643f2ec966bb33..3b605aab343b3b94 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -30,6 +30,7 @@ public: > int setControls(ControlList *ctrls); > > const std::string &deviceNode() const { return deviceNode_; } > + std::string devicePath() const; > > protected: > V4L2Device(const std::string &deviceNode); > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 56ea1ddda2c1425f..2f95e45261463f34 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -9,6 +9,7 @@ > > #include <fcntl.h> > #include <iomanip> > +#include <limits.h> > #include <string.h> > #include <sys/ioctl.h> > #include <sys/syscall.h> > @@ -350,6 +351,32 @@ int V4L2Device::setControls(ControlList *ctrls) > return ret; > } > > +/** > + * \brief Retrieve the device path unique device path in sysfs > + * > + * The device path describes the bus and device backing the V4L2 device and is > + * guaranteed to be unique in the system and is persistent between reboots of > + * the system. drop the last 'the system' as it is repeated. > + * > + * The path is located by utilising that every V4L2 device have an entry under > + * /sys/class/video4linux/ that contains a symlink the device backing it. I'm not sure if it is required to state this, if that's part of the V4L2 specification. I would however change 'utilising' with 'assuming', but maybe it's me not knowing how that word should be used. > + * > + * \todo When switching to C++17 use std::filesystem:: in the implementation. > + * > + * \return The device path The unique device path in sysfs Do we want to use 'predictable' as systemd does with network interface names ? > + */ > +std::string V4L2Device::devicePath() const > +{ > + std::string vdev = basename(deviceNode_.c_str()); > + std::string sysfs = "/sys/class/video4linux/" + vdev + "/device"; > + > + char path[PATH_MAX]; > + if (!realpath(sysfs.c_str(), path)) > + LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_; I would cache the error and report why we failed (ie path non existing or permission issues). > + > + return path; > +} > + > /** > * \brief Perform an IOCTL system call on the device node > * \param[in] request The IOCTL request code > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Mon, Jul 20, 2020 at 03:12:53PM +0200, Jacopo Mondi wrote: > On Sat, Jul 18, 2020 at 03:23:16PM +0200, Niklas Söderlund wrote: > > Add a method to lookup a V4L2 devices path in sysfs. The device path > > describes the bus and device backing the V4L2 device and is guaranteed > > to be unique in the system and is persistent between reboots of the > > system. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/internal/v4l2_device.h | 1 + > > src/libcamera/v4l2_device.cpp | 27 ++++++++++++++++++++++++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > index bf643f2ec966bb33..3b605aab343b3b94 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -30,6 +30,7 @@ public: > > int setControls(ControlList *ctrls); > > > > const std::string &deviceNode() const { return deviceNode_; } > > + std::string devicePath() const; > > > > protected: > > V4L2Device(const std::string &deviceNode); > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 56ea1ddda2c1425f..2f95e45261463f34 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -9,6 +9,7 @@ > > > > #include <fcntl.h> > > #include <iomanip> > > +#include <limits.h> > > #include <string.h> > > #include <sys/ioctl.h> > > #include <sys/syscall.h> > > @@ -350,6 +351,32 @@ int V4L2Device::setControls(ControlList *ctrls) > > return ret; > > } > > > > +/** > > + * \brief Retrieve the device path > > unique device path in sysfs > > > + * > > + * The device path describes the bus and device backing the V4L2 device and is > > + * guaranteed to be unique in the system and is persistent between reboots of > > + * the system. > > drop the last 'the system' as it is repeated. > > > + * > > + * The path is located by utilising that every V4L2 device have an entry under > > + * /sys/class/video4linux/ that contains a symlink the device backing it. > > I'm not sure if it is required to state this, if that's part of the V4L2 > specification. I would however change 'utilising' with 'assuming', but > maybe it's me not knowing how that word should be used. I would indeed drop this paragraph, or move it to the commit message, or to a normal comment in the function. > > + * > > + * \todo When switching to C++17 use std::filesystem:: in the implementation. > > + * > > + * \return The device path > > The unique device path in sysfs > > Do we want to use 'predictable' as systemd does with network interface > names ? > > > + */ > > +std::string V4L2Device::devicePath() const > > +{ > > + std::string vdev = basename(deviceNode_.c_str()); > > + std::string sysfs = "/sys/class/video4linux/" + vdev + "/device"; I'm not sure this can always be guaranteed to be valid. It's entirely possible to set udev rules that will rename the device node. One option to prepare for that would be to query the device path from the device enumerator. For the udev enumerator that would be formward to udev (which should know about all these details), for the sysfs enumerator the above code would be used, as a fallback. > > + > > + char path[PATH_MAX]; > > + if (!realpath(sysfs.c_str(), path)) > > + LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_; > > I would cache the error and report why we failed (ie path non existing > or permission issues). > > > + > > + return path; > > +} > > + > > /** > > * \brief Perform an IOCTL system call on the device node > > * \param[in] request The IOCTL request code
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index bf643f2ec966bb33..3b605aab343b3b94 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -30,6 +30,7 @@ public: int setControls(ControlList *ctrls); const std::string &deviceNode() const { return deviceNode_; } + std::string devicePath() const; protected: V4L2Device(const std::string &deviceNode); diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 56ea1ddda2c1425f..2f95e45261463f34 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -9,6 +9,7 @@ #include <fcntl.h> #include <iomanip> +#include <limits.h> #include <string.h> #include <sys/ioctl.h> #include <sys/syscall.h> @@ -350,6 +351,32 @@ int V4L2Device::setControls(ControlList *ctrls) return ret; } +/** + * \brief Retrieve the device path + * + * The device path describes the bus and device backing the V4L2 device and is + * guaranteed to be unique in the system and is persistent between reboots of + * the system. + * + * The path is located by utilising that every V4L2 device have an entry under + * /sys/class/video4linux/ that contains a symlink the device backing it. + * + * \todo When switching to C++17 use std::filesystem:: in the implementation. + * + * \return The device path + */ +std::string V4L2Device::devicePath() const +{ + std::string vdev = basename(deviceNode_.c_str()); + std::string sysfs = "/sys/class/video4linux/" + vdev + "/device"; + + char path[PATH_MAX]; + if (!realpath(sysfs.c_str(), path)) + LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_; + + return path; +} + /** * \brief Perform an IOCTL system call on the device node * \param[in] request The IOCTL request code
Add a method to lookup a V4L2 devices path in sysfs. The device path describes the bus and device backing the V4L2 device and is guaranteed to be unique in the system and is persistent between reboots of the system. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/internal/v4l2_device.h | 1 + src/libcamera/v4l2_device.cpp | 27 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)