[PATCH] xfree86: use a thread for the generation of input events

[Peter Hutterer]

On Mon, Aug 23, 2010 at 11:17:10AM +0300, Tiago Vignatti wrote:
> The current SIGIO signal handler method, used at generation of input events,
> has a bunch of oddities. This patch introduces an alternative way using a
> thread, which is used to select()s all input device file descriptors.
> 
> A mutex was used to control the access of the mi queue by the main and input
> threads. Two pipes to emit alert events (such hotplug ones) and guarantee the
> proper communication between them was also used.
> 

thanks. just a few style comments in this one, ajax already covered enough
in his reply.

> Co-authored-by: Fernando Carrijo <fcarrijo at freedesktop.org>
> Signed-off-by: Tiago Vignatti <tiago.vignatti at nokia.com>
> ---
>  configure.ac                   |    9 +
>  dix/main.c                     |   13 ++
>  hw/xfree86/common/xf86Events.c |   23 +++
>  include/dix-config.h.in        |    3 +
>  include/opaque.h               |    4 +
>  include/os.h                   |   18 ++
>  mi/mieq.c                      |   70 ++++-----
>  os/Makefile.am                 |    5 +
>  os/WaitFor.c                   |    5 +
>  os/connection.c                |    8 +
>  os/inputthread.c               |  368 ++++++++++++++++++++++++++++++++++++++++
>  11 files changed, 487 insertions(+), 39 deletions(-)
>  create mode 100644 os/inputthread.c
> 
> diff --git a/configure.ac b/configure.ac
> index 9884fa7..bfdf6ac 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -477,6 +477,10 @@ AC_ARG_ENABLE(unit-tests,    AS_HELP_STRING([--enable-unit-tests],
>  AC_ARG_ENABLE(use-sigio-by-default, AS_HELP_STRING([--enable-use-sigio-by-default]
>    [Enable SIGIO input handlers by default (default: $USE_SIGIO_BY_DEFAULT)]),
>                                  [USE_SIGIO_BY_DEFAULT=$enableval], [])
> +AC_ARG_ENABLE(input-thread,  AS_HELP_STRING([--enable-input-thread],
> +  [Use a separate thread for input event generation (default: yes)]),
> +                                [INPUT_THREAD=$enableval],
> +                                [INPUT_THREAD=yes])

Can we use this unconditionally? If we can't assume the server is using
threads, none of the drivers can really benefit from it because we always
have to assume the event handling is during a signal handler.

>  AC_ARG_WITH(int10,           AS_HELP_STRING([--with-int10=BACKEND], [int10 backend: vm86, x86emu or stub]),
>  				[INT10="$withval"],
>  				[INT10="$DEFAULT_INT10"])
> @@ -1126,6 +1130,11 @@ if test "x$DPMSExtension" = xyes; then
>  	AC_DEFINE(DPMSExtension, 1, [Support DPMS extension])
>  fi
>  
> +AM_CONDITIONAL(INPUT_THREAD, [test "x$INPUT_THREAD" = xyes])
> +if test "x$INPUT_THREAD" = xyes; then
> +       AC_DEFINE(INPUT_THREAD, 1, [Use a separate thread for input event generation])
> +fi
> +
>  if test "x$XCALIBRATE" = xyes && test "$KDRIVE" = yes; then
>     AC_DEFINE(XCALIBRATE, 1, [Build XCalibrate extension])
>     REQUIRED_MODULES="$REQUIRED_MODULES $XCALIBRATEPROTO"
> diff --git a/dix/main.c b/dix/main.c
> index 47a932f..f79f16f 100644
> --- a/dix/main.c
> +++ b/dix/main.c
> @@ -111,6 +111,12 @@ Equipment Corporation.
>  #include "dispatch.h"		/* InitProcVectors() */
>  #endif
>  
> +#ifndef INPUT_THREAD
> +static inline void threaded_input_pre_init(void) {}
> +static inline void threaded_input_init(void) {}
> +static inline void threaded_input_fini(void) {}
> +#endif

input_threaded_... probably better, given that it is input related.
also, I wish we could stop mixing kernel-style and old X camelcase style,
this is getting more and more confusing.

[...]
> +static void*
> +threaded_input_do_work(void *arg)
> +{
> +    fd_set ready_fds;
> +    threaded_input_device *dev;
> +
> +    FD_ZERO(&ready_fds);
> +
> +    while (1)
> +    {
> +        XFD_COPYSET(&threaded_input->fds, &ready_fds);
> +        FD_SET(hotplugPipeRead, &ready_fds);
> +
> +        DebugF("threaded-input: do_work waiting for devices\n");
> +
> +        if (Select(MaxInputDevices, &ready_fds, NULL, NULL, NULL) < 0)
> +        {
> +            if (errno == EINVAL)
> +            {
> +                FatalError("threaded-input: do_work (%s)", strerror(errno));
> +            }
> +            else if (errno != EINTR)
> +            {
> +                ErrorF("threaded-input: do_work (%s)\n", strerror(errno));
> +            }

no {} for single-line blocks. for other comments I refer to ajax' email.

Cheers,
  Peter