xserver: Branch 'master' - 16 commits

[Peter Hutterer]

On Tue, Dec 08, 2015 at 09:56:45AM -0800, Keith Packard wrote:
> Michel Dänzer <michel at daenzer.net> 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 <keithp at keithp.com>
> 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 <keithp at keithp.com>
> ---
>  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