Differential D6998
input event interface implementation Authored by wulf_cicgroup.ru on Jun 27 2016, 9:17 PM. Tags None Referenced Files
Details Adds input event interface AKA evdev found on most modern Linuxes.
This patch is kernel API only, evdev support for some individual drivers can be found here: https://github.com/wulf7/freebsd/tree/evdev Passes all meaningfull libevdev unit tests (3 still fails)
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions sorry right now I do not have any time to spend on the review, but added people I know interested into that area and eager to get that in Comment Actions Hi, Did you test that your implementation does not interfere with /usr/ports/multimedia/webcamd and the V4L linux compat files? --HPS Comment Actions It tries to choose next device unit number if make_dev returned EEXISTS but real coexistence has not been tested
Both (reviewed and v4linux) headers can not be #included at one time, but they are compatible. Comment Actions Hi, Yes, webcamd currently uses /dev/input/xxx so we need to find a solution there. Webcamd does not jump to the next device name when a EEXISTS happens. So it will need a patch - at least to do that. Can you test that?
Are you aware about the following PR and attached patches: --HPS Comment Actions Hi, Can you propose a solution how webcamd and kernel's evdev can co-exist in /dev/input? For example Use different node names? eventXX for userspace bsdeventYY for the kernel? This is a blocking issue. The cuse module already has a unit management, so that all event nodes created from userspace are unique. Also what to you think about renaming the event header files so that they do not interfere with v4l-compat's port? Or can we use both at the same time? --HPS Comment Actions Yes. I am aware of it. I borrowed some code from it for udev-devd wrapper needed by libinput. Comment Actions Can you propose a patch for webcamd to use the kernel's /dev/input/eventX layer? Many people are using it on their desktops, so this must work smoothly. --HPS Comment Actions Next time please upload with full context -- see https://wiki.freebsd.org/CodeReview. For git, it's just git diff -U9999, for svn svn diff --diff-cmd=diff -x -U999999. No need to re-upload now just to include the context but please do so if/when you have other changes to upload in this review. Comment Actions Could you try following patch? http://195.170.219.74/webcamd-20160628.patch Comment Actions Hi, Thank you for your patch. I'll see if I can give it a spin. Does this also handle when a device registers multiple input event devices or only one? For example wacom tablets register 3 separate input devices. You can attach webcamd to any USB mouse/keyboard if you start it by hand. See: webcamd -l --HPS Comment Actions Thanx for clarification, Ed! Could this link be placed on the start page somewhere before or after bugzilla link? Comment Actions Thank you too. I tried it and it found it working (after some fixes).
I hope, yes, as it fires at every input event device registration.
Unfortunately I was not able to attach any of my USB keyboards/mices to webcamd. Should a special compile or runtime options be used? Next step I faced with another problem: I have to send some events like LEDs in opposite direction (from user(uinput) to hardware(webcamd)) and simplest way to achieve that is to open() cuse device and than write() to it from dedicated broker thread. And than the question is: Is it possible to know which character device (path or opened filehandle) corresponds to current webcamd process/thread? Comment Actions No, it should work out of the box. What does it print when it doesn't work? You may need to run webcamd like super-user. Or perhaps CONFIG_XXX is not enabled in webcamd towards the Linux code for mouse/keyboard ....
Yes, I think this information is available by "fstat -p xxx". I'm a bit worried that your approach will cause a devfs deadlock when webcamd is exiting, when calling forth and back. The reason is that the destroy dev function will sleep until all references are gone. So all file handles opened must be closed before webcamd can detach from the USB device. Comment Actions
I sent output in private mail to not spam the thread Comment Actions Implements passing events from user to userspace drivers like webcamd via uinput interface Comment Actions Done I split it on two parts Comment Actions cuse bug: Can you see if any devices have opened /dev/input handles to evdevs using fstat? There should be no bug there. Comment Actions Yes! You are right! That is not cuse bug, sorry! Xorg was referencing this device nodes, so most probably, the problem is an absence of "Device is gone" notifications from webcamd process to Xorg device driver at least in my setup. Comment Actions When your /dev/uinput device closes, does it wakeup the children devices and make them return an error? That might be the problem. Or is the xorg-event driver not checking read() / poll() for errors. Detach needs to work. Can you investigate? --HPS Comment Actions I found the reason. Cuse returns EINVAL on read() if device is gone but xf86-input-evdev expects ENODEV in that case. Ports version of xf86-input-evdev has patch to handle this but I removed it for testing purposes. Comment Actions Yes it does wakeup and than returns ENODEV on every file operation like Linux does Comment Actions So far it looks good to me. Configurable debugging would be nice. I'm glad there are lots of lock assertions everywhere, thank you for that! What's it been tested on/with ?
Comment Actions I use evdev-only laptop for about a year and run following hw/sw Event receivers Regression Oleksandr Tymoshenko (gonzo@) did some testing on ARM platform also Comment Actions
Comment Actions I need to patch webcamd and figure out how I can dynamically detect if this EVDEV driver is loaded or not. Committing this AS-IS will break /usr/ports/multimedia/webcamd which many people use. --HPS Comment Actions I use following patches to allow webcamd/evdev interoperability in both cuse and uinput ways: Comment Actions
You can use feature_present("evdev") to check inkernel evdev presence or stat("/dev/uinput", statbuf) to check uinput presence to find if particular driver has inkernel evdev support its necessary to do ioctl(EVIOCGPHYS) on every /dev/input/eventXXX file Comment Actions Thank you! I will try to integrate the webcamd patches by end of next week and make a new version of webcamd. Hopefully no show stoppers will pop up. --HPS Comment Actions Hi, What is the motivation behind using IOWINT() for FreeBSD IOCTLs and IOW(xxx, int) for Linux ones. Does this mean we are not binary compatible with Linux? Can this be changed so we avoid patching webcamd? --HPS Comment Actions We are not binary compatible with Linux here as we do automatic copyin()/copyout() on each ioctl but they don`t. As a consequence they can provide arbitrary int data as IOW parameter via casting int to void* but we can`t as we would trigger EFAULT on copyin() of random memory location. We can use any ioctl with VOID direction like IO or IOWINT but not IOW. I prefer IOWINT Following is what libevdev does for grabbing and ungrabbing event device: $ grep EVIOCGRAB libevdev.c libevdev.c: rc = ioctl(dev->fd, EVIOCGRAB, (void *)1); libevdev.c: rc = ioctl(dev->fd, EVIOCGRAB, (void *)0); On Linux EVIOCGRAB is defined in following way: Comment Actions Just now webcamd should return EFAULT on these ioctls So with avoiding of patching you would not make things worse Comment Actions It should be mentioned that only meaningfull IOWINT ioctl is EVIOCGRAB and it is not used widely as you should add extra option to xorg.conf to trigger it. Others are EVIOCRMFF and EVIOCREVOKE. I think no one tried them both on FreeBSD Comment Actions I don't understand this restriction. cxgb(9) and other drivers I've worked on are perfectly capable of handling nested copyins. Would it be better to integrate this driver by way of the linuxkpi where this isn't considered a problem? Comment Actions I think we are talking about other issue. Briefly: Following is call graph of webcamd: User program (compiled with FreeBSD input.h ioctls) -> FreeBSD ioctl syscall -> cuse -> webcamd -> Linux kernel parts inside webcamd (compiled with Linux input.h ioctls) The problem is that we can not just borrow exact ioctl defines from Linux to FreeBSD as they cause EFAULT in FreeBSD ioctl layer. We have to adjust them to be FreeBSD compatible to pass ioctl syscall and after than convert to Linux-compatible values in webcamd layer so they can be used further in userland Linux kernel implementation Comment Actions I understand what you mean. Could you change the Linux IOCTL header file in your webcamd patch instead of redirecting the IOCTLs in the IOCTL handler? I think then you only need to add code for the generic VOID IOCTL copy-in of the argument and you can keep the IOCTL of type VOID? --HPS Comment Actions Here you are: http://195.170.219.74/webcamd-iowint.20160813.patch Comment Actions Hi, I've now updated the SVN version of webcamd with the input.h and ioctl.h patches you've provided. Regarding using /dev/uinput I was wondering of you could add a new IOCTL API, that allows webcamd to only reserve a /dev/input/event X unit number, which is freed when the /dev/uinput file handle is closed. That would be much simpler to implement and maintain. Is there any purpose that all the webcamd events are piped through /dev/uinput ? --HPS
Comment Actions Thank you!
Unfortunately, unit number assignment is handled by devfs not by evdev. That is why the only way to reserve evdev cdev is to create it. char *pool = "input/event%d"; err = devfs_alloc_unit(pool, &softc->unit); if (err) return (err); make_dev(..., &softc->cdev, pool, softc->unit); in device_detach: destroy_dev(softc->cdev); devfs_free_unit(pool, softc->unit); After that it would be possible to reserve eventX node even with existing IOCTL API
That is not strictly necessary for ordinary users but can be useful for evdev driver porters/developers Comment Actions Hi, If kernel drivers are supposed to supply events, then it is a problem to use an SX lock, because many drivers hold a mutex when they generate events. And locking SX after MTX is an issue. If this layer is only intended for loopback of events via user-space it is fine. --HPS Comment Actions Hi, uinput is intended for loopback of events via user-space only. It should not be used from kernel drivers and have no exposed API/KPI except one struct cdevsw provides Comment Actions I guess it's time to commit this change. Jean-Sébastien would you commit it as a maintainer of all things input? Or if you don't have spare cycles I can do it myself. Comment Actions This revision fails to compile with INVARIANTS disabled. I fixed that in my repo and I`m going to update diff in this review in hour or two with couple others minor fixes https://github.com/wulf7/freebsd/commit/64fba04a38c6968a1580f6f5c30e0350898a0d3a Comment Actions Fix compiling with INVARIANTS disabled Comment Actions I will give this a try again, I used it lightly lately. But I'm certainly not a "maintainer of all things input" :-) I barely improved Synaptics support years ago. But I have a lot of interest in this topic. After I tried it again, I will tell you if I feel like I can commit. If someone else is ready to commit, then go ahead! It's a wonderful improvement and there is no need to wait for me. Comment Actions Diff updated. It would be nice to commit https://reviews.freebsd.org/D7588 as well to allow multimedia/webcamd coexistence. 11-servers/xorg-server change can be postponed if someone thinks it too radical Comment Actions It would be nice if someone does review or commit of patch from https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=205690 as it introduces multitouch support in psm.c and is prerequisite to evdev support in that driver. Comment Actions I'm about to commit this change, running final tinderbox build I made three changes to the latest patch:
Comment Actions Vladimir, could you submit devices' evdev patches to differential? I suggest starting with USB devs, one device per review. I talked to Hans, he's OK with reviewing them Comment Actions A little note, you can use the review number by itself (e.g. just D7863) and Phabricator will automatically add the link, and also reference this review from the one mentioned. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||