On Tue, Dec 08, 2015 at 09:56:45AM -0800, Keith Packard wrote: > Michel Dänzer <[email protected]> writes: > > > This commit broke input for me with xf86-input-libinput. No keyboard or > > mouse input is received. There are no errors in the log file. > > > > xf86-input-evdev works. > > Ok, I spent some time yesterday figuring out what happened here. The > issue is that xf86-input-libinput is using AddEnabledDevice to register > file descriptors for input devices, and expecting that xf86Wakeup would > call the driver when input became available. > > The problem is that without passing the set of ready file descriptors to > xf86Wakeup (the whole point of this series), there's no way for > xf86Wakeup to discover that input is available on a file descriptor > registered behind it's back through AddEnabledDevice. > > Once the select mask is gone from the wakeup handler, AddEnabledDevice > and AddGeneralSocket are no longer as useful -- there's no way to > discover when the associated socket is ready for I/O without polling on > the descriptor each time the server wakes up (which is actually done in > several places in the server, including DMX and Xnest). > > With the NotifyFD interfaces in place, I think it would be best if we > eliminated the AddEnabledDevice and AddGeneralSocket interfaces from the > OS layer. Most code using these in the core server has been changed to > use the NotifyFD interface instead and I've got patches for the > remaining cases. > > This leaves open the question about how to deal with xf86 input drivers > though. The reason they're using AddEnabledDevice directly and not > calling xf86AddEnabledDevice is that they want to be handled in the > normal execution flow of the X server and not asynchronously from the > SIGIO handler. > > Instead of using that kludge, I'd like to suggest that we fix the > xf86AddEnabledDevice function to do what the devices need instead, by > adding a flag to the InputInfo indicating when the driver can tolerate > reading events at SIGIO time. Then, xf86AddEnabledDevice can either > register for SIGIO or call the NotifyFD intefaces as appropriate. > > Here's a patch which adds a new flag (set by default, to match the > current interface) signaling which drivers can support device I/O at > SIGIO time: >
> From 70f75a9d5e196b1722a04060f7947f1f2832e011 Mon Sep 17 00:00:00 2001 > From: Keith Packard <[email protected]> > Date: Mon, 7 Dec 2015 15:10:13 -0800 > Subject: [PATCH xserver 03/15] hw/xfree86: Add XI86_SIGNAL_IO > > This flags devices which can perform I/O at SIGIO time; other devices > will have I/O performed in the normal flow of execution. > > Signed-off-by: Keith Packard <[email protected]> > --- > hw/xfree86/common/xf86Events.c | 14 ++++++++------ > hw/xfree86/common/xf86Xinput.c | 1 + > hw/xfree86/common/xf86Xinput.h | 1 + > 3 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c > index 709afd6..0417c72 100644 > --- a/hw/xfree86/common/xf86Events.c > +++ b/hw/xfree86/common/xf86Events.c > @@ -312,9 +312,10 @@ xf86SigioReadInput(int fd, void *closure) > void > xf86AddEnabledDevice(InputInfoPtr pInfo) > { > - if (!xf86InstallSIGIOHandler(pInfo->fd, xf86SigioReadInput, pInfo)) { > - AddEnabledDevice(pInfo->fd); > - } > + if (pInfo->flags & XI86_SIGNAL_IO) > + if (xf86InstallSIGIOHandler(pInfo->fd, xf86SigioReadInput, pInfo)) > + return; > + AddEnabledDevice(pInfo->fd); > } > > /* > @@ -324,9 +325,10 @@ xf86AddEnabledDevice(InputInfoPtr pInfo) > void > xf86RemoveEnabledDevice(InputInfoPtr pInfo) > { > - if (!xf86RemoveSIGIOHandler(pInfo->fd)) { > - RemoveEnabledDevice(pInfo->fd); > - } > + if (pInfo->flags & XI86_SIGNAL_IO) > + if (xf86RemoveSIGIOHandler(pInfo->fd)) > + return; > + RemoveEnabledDevice(pInfo->fd); > } > > static int *xf86SignalIntercept = NULL; > diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c > index c56a2b9..a8dff0b 100644 > --- a/hw/xfree86/common/xf86Xinput.c > +++ b/hw/xfree86/common/xf86Xinput.c > @@ -727,6 +727,7 @@ xf86AllocateInput(void) > > pInfo->fd = -1; > pInfo->type_name = "UNKNOWN"; > + pInfo->flags = XI86_SIGNAL_IO; > > return pInfo; > } > diff --git a/hw/xfree86/common/xf86Xinput.h b/hw/xfree86/common/xf86Xinput.h > index 0024053..d00251a 100644 > --- a/hw/xfree86/common/xf86Xinput.h > +++ b/hw/xfree86/common/xf86Xinput.h > @@ -66,6 +66,7 @@ > /* server-internal only */ > #define XI86_DEVICE_DISABLED 0x10 /* device was disabled before vt > switch */ > #define XI86_SERVER_FD 0x20 /* fd is managed by xserver */ > +#define XI86_SIGNAL_IO 0x40 /* driver can handle I/O at SIGIO > time */ > > /* Input device driver capabilities */ > #define XI86_DRV_CAP_SERVER_FD 0x01 > -- > 2.6.2 If it indicates a driver capability, it should be be XI86_DRV_CAP_SIGNAL_IO flag, with the per-device set as the XI86_SIGNAL_IO, similar to the server fd parts. Then you can pre-set the flag depending on the driver capability and only unset it in the driver in the special case. but imo we don't get any benefit of stuffing this into the flags. the server-fd parts are a bit of a special case because we open the device before PreInit and thus need to know ahead of time wheter we need this. For this use-case having a separate API call would be sufficient - and as the libinput patch below shows you need to ifdef it anyway so you don't get any advantage *not* having the new API. And having a xf86AddEnabledDeviceNotifyFd() is more expressive in the code than having this hidden in the flag. Naming this is the hardest bit... Maybe deprecate the current call and add xf86AddInputDeviceSIGIO and xf86AddInputDeviceFd or so? > And here's the change to xf86-input-libinput that matches. > [...] > > In terms of sequencing and validating this series, here's what I > suggest: > > 1) Add the X server support. No API or ABI breakages are involved > 2) Update input drivers > 3) Switch X server to use NotifyFd everywhere > 4) Remove AddEnabledDevice and AddGeneralSocket from X server > > At this point, drivers not using the new interface will not run with the > changed X server as they will fail to find AddEnabledDevices, so we'll > be able to catch them quickly. > > I'm willing to make the changes for each of the input drivers that are > 'interesting'; only those currently using AddEnabledDevice would need > changing. Here's the list of drivers on my machine which currently call > AddEnabledDevice; I haven't actually looked inside them to know if > they actually need changing or not: > > xf86-input-calcomp > xf86-input-citron > xf86-input-digitaledge > xf86-input-dmc > xf86-input-dynapro > xf86-input-elo2300 > xf86-input-elographics > xf86-input-hyperpen > xf86-input-keyboard > xf86-input-libinput > xf86-input-magellan > xf86-input-magictouch > xf86-input-microtouch > xf86-input-mutouch > xf86-input-palmax > xf86-input-penmount > xf86-input-spaceorb > xf86-input-summa > xf86-input-ur98 of these, only keyboard and libinput need updating. elographics still sees the odd use-case but I've been pushing people to use the kernel driver instead. All the other ones are truly dead. Cheers, Peter _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
