[libcamera-devel] [PATCH v2 2/2] v4l2: v4l2_compat: Add V4L2 compatibility layer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 16 13:07:28 CET 2019
Hi Paul,
On Mon, Dec 16, 2019 at 12:18:53AM -0600, Paul Elder wrote:
> On Mon, Dec 16, 2019 at 02:18:44AM +0200, Laurent Pinchart wrote:
> > On Mon, Dec 09, 2019 at 11:30:13AM +0100, Jacopo Mondi wrote:
> > > On Sun, Dec 08, 2019 at 11:56:03PM -0500, Paul Elder wrote:
> > > > Add libcamera V4L2 compatibility layer.
> > > >
> > > > This initial implementation supports the minimal set of V4L2 operations,
> > > > which allows getting, setting, and enumerating formats, and streaming
> > > > frames from a video device. Some data about the wrapped V4L2 video
> > > > device are hardcoded.
> > > >
> > > > Add a build option named 'v4l2' and adjust the build system to
> > > > selectively compile the V4L2 compatibility layer.
> > > >
> > > > Note that until we have a way of mapping V4L2 device nodes to libcamera
> > > > cameras, the V4L2 compatibility layer will always selects and use the
> >
> > s/selects/select/
>
> This paragraph got removed after I replaced it with the short paragraph
> describing the temporary (?) mapping using sysfs names.
>
> > > > first enumerated libcamera camera.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > >
> > > > ---
> > > > Changes in v2:
> > > > - move all errno acrobatics to V4L2CameraProxy
> > > > - remove all mentions of dmabuf
> > > > - make V4L2CompatManager::getCamera return pointer rather than
> > > > shared_ptr
> > > > - match V4L2 device nodes to libcamera cameras using Camera::name()
> > > > compared to /sys/dev/char/maj:min/name (only works for UVC cameras)
> > > > - in V4L2CompatManager::getCameraIndex()
> > > > - add -fvisibility=hidden to v4l2 compat
> > > > - cache the results of V4L2CompatManager::imageSize() within V4L2Camera
> > > > (where V4L2Camera is interested)
> > > > - remove V4L2CompatManager::valid[fd|mmap], and where those methods were
> > > > used, check V4L2CompatManager::getCamera() != nullptr instead
> > > > - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888
> > > > and DRM_FORMAT_RGB888
> > > > - other cosmetic changes
> > > > ---
> > > > meson_options.txt | 5 +
> > > > src/meson.build | 4 +
> > > > src/v4l2/meson.build | 30 ++
> > > > src/v4l2/v4l2_camera.cpp | 299 ++++++++++++++++++++
> > > > src/v4l2/v4l2_camera.h | 67 +++++
> > > > src/v4l2/v4l2_camera_proxy.cpp | 452 +++++++++++++++++++++++++++++++
> > > > src/v4l2/v4l2_camera_proxy.h | 63 +++++
> > > > src/v4l2/v4l2_compat.cpp | 81 ++++++
> > > > src/v4l2/v4l2_compat_manager.cpp | 382 ++++++++++++++++++++++++++
> > > > src/v4l2/v4l2_compat_manager.h | 86 ++++++
> > > > 10 files changed, 1469 insertions(+)
> > > > create mode 100644 src/v4l2/meson.build
> > > > create mode 100644 src/v4l2/v4l2_camera.cpp
> > > > create mode 100644 src/v4l2/v4l2_camera.h
> > > > create mode 100644 src/v4l2/v4l2_camera_proxy.cpp
> > > > create mode 100644 src/v4l2/v4l2_camera_proxy.h
> > > > create mode 100644 src/v4l2/v4l2_compat.cpp
> > > > create mode 100644 src/v4l2/v4l2_compat_manager.cpp
> > > > create mode 100644 src/v4l2/v4l2_compat_manager.h
> > > >
> > > > diff --git a/meson_options.txt b/meson_options.txt
> > > > index 1a328045..b06dd494 100644
> > > > --- a/meson_options.txt
> > > > +++ b/meson_options.txt
> > > > @@ -10,3 +10,8 @@ option('documentation',
> > > > option('test',
> > > > type : 'boolean',
> > > > description: 'Compile and include the tests')
> > > > +
> > > > +option('v4l2',
> > > > + type : 'boolean',
> > > > + value : false,
> > > > + description : 'Compile libcamera with V4L2 compatibility layer')
> >
> > How about 'Compile the V4L2 adaptation layer', as it's not part of
> > libcamera.so ?
>
> ack
>
> Although, I wrote "compatibility" instead of "adaptation" everywhere
> because that's what the documentation on libcamera.org says (V4L2
> Compatibility Layer).
Then please disregard my comment. I misremembered that we called the
component V4L2 adaptation everywhere, but I was wrong.
[snip]
> > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > > > new file mode 100644
> > > > index 00000000..90416b35
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > > > @@ -0,0 +1,382 @@
[snip]
> > > > +void V4L2CompatManager::run()
> > > > +{
> > > > + cm_ = new CameraManager();
> > > > +
> > > > + int ret = cm_->start();
> > > > + if (ret) {
> > > > + LOG(V4L2Compat, Error) << "Failed to start camera manager: "
> > > > + << strerror(-ret);
> > > > + return;
> > > > + }
> > > > +
> > > > + LOG(V4L2Compat, Debug) << "Started camera manager";
> > > > +
> > > > + /*
> > > > + * For each Camera registered in the system, a V4L2CameraProxy
> > > > + * gets created here to wraps a camera device.
> >
> > s/wraps/wrap/
>
> This was copied from CameraHalManager::run(), which means that that has
> the same typo :)
Please send a patch to fix that too :-)
> > and you can go up to 80 columns before wrapping.
>
> ack
>
> > > > + */
> > > > + unsigned int index = 0;
> > > > + for (auto &camera : cm_->cameras()) {
> > > > + V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);
> > > > + proxies_.emplace_back(proxy);
> > > > + ++index;
> > > > + }
> > > > +
> > > > + /*
> > > > + * libcamera has been initialized. Unlock the init() caller
> > > > + * as we're now ready to handle calls from the framework.
> >
> > You can go up to 80 columns before wrapping here too.
>
> ack
>
> > > > + */
> >
> > mutex_.lock();
> > initialized_ = true;
> > mutex_.unlock();
>
> ack
>
> > > > + cv_.notify_one();
> > > > +
> > > > + /* Now start processing events and messages. */
> > > > + exec();
> > > > +
> > > > + cm_->stop();
> > > > + delete cm_;
> > > > + cm_ = nullptr;
> > > > +}
[snip]
> > > > +int V4L2CompatManager::getCameraIndex(int fd)
> > > > +{
> > > > + struct stat statbuf;
> > > > + fstat(fd, &statbuf);
> > > > + unsigned int dev_major = major(statbuf.st_rdev);
> > > > + unsigned int dev_minor = minor(statbuf.st_rdev);
> >
> > devMajor and devMinor.
>
> ack
>
> > > > +
> > > > + std::string name;
> > > > + std::ifstream nameFile;
> > > > + nameFile.open("/sys/dev/char/" + std::to_string(dev_major) + ":" +
> > > > + std::to_string(dev_minor) + "/name");
> > > > + if (!nameFile)
> > > > + return -1;
> > > > +
> > > > + std::getline(nameFile, name);
> > > > + nameFile.close();
> > > > +
> > > > + unsigned int index = 0;
> > > > + for (auto &camera : cm_->cameras()) {
> > > > + if (!camera->name().compare(name))
> > > > + break;
> > > > + ++index;
> > > > + }
> >
> > As commented on v1, matching on the video node name isn't a good idea,
> > as we want to support all cameras, and not all cameras are named based
> > on the video node name. I wrote
> >
> > "One option would be to add a method to the camera manager to retrieve a
> > camera by device node path. This wouldn't expose any V4L2-specific
> > information towards applications, the map of device nodes to cameras
> > would be private."
> >
> > but I don't think that's right, we should instead retrieve a camera by
> > major:minor, as multiple device nodes in the system could
>
> Doesn't the major:minor devnum map uniquely to device node paths?
# mknod /dev/video0 c 81 0
# mknod /dev/video0-alias c 81 0
and you have two device nodes for the same devnums :-) Without going to
those extremes, I have on my system
$ ls -l /dev/video*
crw-rw----+ 1 root video 81, 0 Dec 16 13:19 /dev/video0
crw-rw----+ 1 root video 81, 1 Dec 16 13:19 /dev/video1
crw-rw----+ 1 root video 81, 2 Dec 16 13:19 /dev/video2
crw-rw----+ 1 root video 81, 3 Dec 16 13:19 /dev/video3
$ ls -l /dev/v4l/by-id/
total 0
lrwxrwxrwx 1 root root 12 Dec 16 13:19 usb-CN0FFMHCLOG008AAB2XTA01_Integrated_Webcam_HD_200901010001-video-index0 -> ../../video0
lrwxrwxrwx 1 root root 12 Dec 16 13:19 usb-CN0FFMHCLOG008AAB2XTA01_Integrated_Webcam_HD_200901010001-video-index1 -> ../../video3
And finally, open("/dev/video0") and open("video0") with the current
directory being "/dev" (or the equivalent with openat()) both map to the
same file, but pass a different path to openat(). You would at the very
least need to determine the canonical path byt dereferencing symlinks
and making the path absolute, which would be more complicated than
looking up based on devnum.
> What do we do when different device nodes/devnums map to the same
> libcamera camera but different stream etc? Simply map to the libcamera
> camera at the simplest setting?
To start with, we can limit the support to a single stream per camera.
Let's have pipelines report to the CameraManager one devnum per Camera,
and then offer a method to get a camera by devnum. That should be enough
to move forward.
> > > > +
> > > > + if (index >= cm_->cameras().size())
> > > > + return -1;
> > > > +
> > > > + return index;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)
> > > > +{
> > > > + int fd = openat_func_(dirfd, path, oflag, mode);
> > > > + if (fd < 0)
> > > > + return fd;
> > > > +
> > > > + if (std::string(path).find("video") == std::string::npos)
> > > > + return fd;
> >
> > So opening "/home/user/myvideo.mp4" will match :-)
>
> Oh, oops :p
>
> > You can't match by name here, you need to fstat() and check that the
> > file is a character device node whose major is equal to the V4L2 major.
>
> I thought I did that a long time ago... I guess it disappeared.
>
> > Or, if you add a method to the camera manager to retrieve a camera by
> > major:minor, I think you could even skip this check completely as it
> > would be handled in CameraManager (but the fstat is needed nonetheless).
>
> ack
>
> > > > +
> > > > + if (!isRunning())
> > > > + init();
> > > > +
> > > > + int ret = getCameraIndex(fd);
> > > > + if (ret < 0) {
> > > > + LOG(V4L2Compat, Error) << "No camera found for " << path;
> >
> > I'd turn that into an Info or Debug message as it's not necessarily an
> > error, not all cameras are supported by libcamera.
>
> True.
>
> > > > + return fd;
> > > > + }
> >
> > You need to close_func_(fd) here.
>
> Why? The application has called open() on a V4L2 device node that we
> don't support, so shouldn't we just let it through? Should we block it
> instead?
I meant after the }, sorry for not being clear. Otherwise you leak fd
for devices that map to a camera.
> > > > +
> > > > + unsigned int camera_index = static_cast<unsigned int>(ret);
> > > > +
> > > > + std::shared_ptr<V4L2CameraProxy> proxy = proxies_[camera_index];
> > >
> > > You here share the ownership of the managed object owned by
> > > proxies_[camera_index] with a local variable, increasing its
> > > reference count
> > >
> > > > + ret = proxy->open(mode & O_NONBLOCK);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + int efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));
> > > > + if (efd < 0)
> > > > + return efd;
> > >
> > > I'm still not convinced about this, I'll go read your reply to v1
> > > again.
> >
> > What's wrong with it ? We need an fd that applications can use in a
> > select() or poll() call, so we can't return the fd for the video node,
> > we need an eventfd that we can signal when a request completes.
> >
> > > > +
> > > > + devices_.emplace(efd, proxy);
> > >
> > > And here you store it permanently in a map, making its reference count
> > > permanently = 2 (it goes up to 3, then the local variable goes out of
> > > scope and gest back to 2).
> >
> > I don't think this is a big issue, and reference counting is needed to
> > support hot-unplug. As we don't support it yet we could turn proxies_
> > into a std::vector<std::unique_ptr<V4L2CameraProxy>> and devices_ into a
> > std::map<int, V4L2CameraProxy *>>, knowing that the device won't be
> > deleted until the V4L2CompatManager itself is deleted. The above lookup
> > in proxies_ would then become
> >
> > V4L2CameraProxy *proxy = proxies_[camera_index].get();
> >
> > I think the mmaps_ could then map from void * to V4L2CameraProxy *,
> > which would avoid double look-ups in munmap().
>
> Good idea.
>
> > > > +
> > > > + return efd;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::dup(int oldfd)
> > > > +{
> > > > + int newfd = dup_func_(oldfd);
> > > > + if (getCamera(oldfd))
> > > > + devices_[newfd] = devices_[oldfd];
> > >
> > > Then here you create a new copy, with reference count = 3
> > >
> > > At delete time you clear all the entries in devices_, causing the
> > > shared pointer stored in proxies_ to get back to 1, then proxies_ goes
> > > out of scope, and the managed object gets deleted.
> > >
> > > Why all this dance ? Is the ownership of the CameraProxy shared
> > > between different componentes? Is it created or destoryed outside of
> > > this class ? I would save all this complexity and store
> > > V4L2CameraProxy instances in a vector, and point to them in the map at
> > > openat() time.
> > >
> > > The only thing you care about is to increase the reference count to
> > > the Camera, but you have a shared_ptr<> class instance in V4L2Camera,
> > > so that's fine.
> >
> > To support hot-unplug we'll likely need shared pointers, for now we can
> > do without them and use unique_ptr as explained above.
> >
> > > > +
> > > > + return newfd;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::close(int fd)
> > > > +{
> > > > + V4L2CameraProxy *proxy = getCamera(fd);
> > > > + if (proxy)
> > > > + return proxy->close();
> >
> > This will break if the fd has been dup'ed. Closing one of the two fds
> > will close the proxy, the second fd will then be unusable. You need to
> > refcount opens in V4L2CameraProxy.
>
> Oh yeah...
>
> > You also need to remove fd from the devices_ map here.
> >
> > > > +
> > > > + int ret = close_func_(fd);
> > > > + return ret;
> >
> > return close_func_(fd);
> >
> > > > +}
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list