Re: [PATCH 5/5] input: use a thread for the generation of input events

2011-01-19 Thread Daniel Stone
Hi,

On Thu, Dec 16, 2010 at 03:31:40PM -0500, Adam Jackson wrote:
 If configured, and enabled at runtime, use a separate thread for
 handling input devices.  Use pipes to communicate plug events from main
 thread to input thread, and to communicate event queue updates from
 input thread to main thread.

The locking in this isn't quite sufficient.  Pretty much all
Get*Events() users will need locking around their event-handling body,
as the following can go badly wrong:
  * One master device with an XTest and a physical slave device
  * Main thread gets a request to generate an XTest event, calls
GetPointerEvents which sends a DeviceChangedEvent to switch the MD
to the XTest SD's caps, as well as the pointer event itself
  * Main thread posts DCE through mieqEnqueue
  * Input thread turns up and griefs the entire process by calling
GetPointerEvents, generating a DCE to switch the MD's caps to the
physical SD's, posts its DCE
  * XTest posts its pointer event
  * Input thread posts its pointer event

(Took me three goes to type GetPointerEvents instead of GetTouchEvents.
 Ugh.)

Mind you, we have the same race with SIGIO, so it's not a strictly a
regression, but still not ideal.

Cheers,
Daniel


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 5/5] input: use a thread for the generation of input events

2010-12-17 Thread Tiago Vignatti
On Thu, Dec 16, 2010 at 03:31:40PM -0500, ext Adam Jackson wrote:
 From: Tiago Vignatti tiago.vigna...@nokia.com
 
 If configured, and enabled at runtime, use a separate thread for
 handling input devices.  Use pipes to communicate plug events from main
 thread to input thread, and to communicate event queue updates from
 input thread to main thread.
 
 For the xfree86 DDX, change the SIGIO handler to simply raise enough of
 a dispatch exception to make the main loop interrupt request processing
 and return to input processing.  This has no effect when the input
 thread is active but is a minor optimization when it's not.
 
 v2:
 - Fix memory leak in InputThreadUnregisterDev
 - Fix build with --disable-input-thread

you left MaxInputDevices out of the disabling macro.

Also, I liked in the previous patchset when you used the second commit to
explain that now we have two input processing methods:

- input thread
- main loop dispatch
 
where the SIGIO handler is merely an optimization for the second case.

I'd say to attach this kind of information somewhere in the code, which would
help a lot upcoming developers.

Thanks

Tiago
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel


Re: [PATCH 5/5] input: use a thread for the generation of input events

2010-12-17 Thread Adam Jackson
On Fri, 2010-12-17 at 15:24 +0200, Tiago Vignatti wrote:

 you left MaxInputDevices out of the disabling macro.
 
 Also, I liked in the previous patchset when you used the second commit to
 explain that now we have two input processing methods:
 
 - input thread
 - main loop dispatch
  
 where the SIGIO handler is merely an optimization for the second case.
 
 I'd say to attach this kind of information somewhere in the code, which would
 help a lot upcoming developers.

It's sort of there in the comment above time_to_yield().  I can
elaborate on it there though.

- ajax

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel


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

2010-12-16 Thread Adam Jackson
From: Tiago Vignatti tiago.vigna...@nokia.com

If configured, and enabled at runtime, use a separate thread for
handling input devices.  Use pipes to communicate plug events from main
thread to input thread, and to communicate event queue updates from
input thread to main thread.

For the xfree86 DDX, change the SIGIO handler to simply raise enough of
a dispatch exception to make the main loop interrupt request processing
and return to input processing.  This has no effect when the input
thread is active but is a minor optimization when it's not.

v2:
- Fix memory leak in InputThreadUnregisterDev
- Fix build with --disable-input-thread
- Merge SIGIO strength reduction

Co-authored-by: Fernando Carrijo fcarr...@freedesktop.org
Signed-off-by: Tiago Vignatti tiago.vigna...@nokia.com
Signed-off-by: Adam Jackson a...@redhat.com
---
 configure.ac   |   30 +++
 dix/main.c |3 +
 hw/xfree86/common/xf86Events.c |   35 ++--
 hw/xfree86/common/xf86Init.c   |6 +-
 include/Makefile.am|1 +
 include/dix-config.h.in|3 +
 include/inputthread.h  |   36 
 include/opaque.h   |2 +
 mi/mieq.c  |   76 
 os/Makefile.am |1 +
 os/connection.c|4 +
 os/inputthread.c   |  404 
 12 files changed, 540 insertions(+), 61 deletions(-)
 create mode 100644 include/inputthread.h
 create mode 100644 os/inputthread.c

diff --git a/configure.ac b/configure.ac
index a80a13f..b516456 100644
--- a/configure.ac
+++ b/configure.ac
@@ -819,6 +819,36 @@ REQUIRED_LIBS=$REQUIRED_LIBS $LIBPIXMAN $LIBXFONT xau
 
 REQUIRED_MODULES=[fixesproto = 4.1] [damageproto = 1.1] [xcmiscproto = 
1.2.0] [xtrans = 1.2.2] [bigreqsproto = 1.1.0] $SDK_REQUIRED_MODULES
 
+# input thread setup
+case $host_os in
+linux*)
+   THREAD_DEFAULT=yes ;;
+*)
+   THREAD_DEFAULT=no ;;
+esac
+AC_ARG_ENABLE(input-thread, AS_HELP_STRING([--enable-input-thread],
+[Enable input threads]),
+[INPUTTHREAD=$enableval], [INPUTTHREAD=$THREAD_DEFAULT])
+if test x$INPUTTHREAD = xyes ; then
+case $host_os in
+linux*|openbsd*|gnu*|k*bsd*-gnu)
+   THREAD_LIB=-lpthread ;;
+netbsd*)
+   THREAD_CFLAGS=-D_POSIX_THREAD_SAFE_FUNCTIONS
+   THREAD_LIB=-lpthread ;;
+freebsd*)
+   THREAD_CFLAGS=-D_THREAD_SAFE
+   THREAD_LIB=-pthread ;;
+dragonfly*)
+   THREAD_LIB=-pthread ;;
+solaris*)
+   THREAD_CFLAGS=-D_REENTRANT -D_POSIX_PTHREAD_SEMANTICS ;;
+esac
+SYS_LIBS=$SYS_LIBS $THREAD_LIB
+CFLAGS=$CFLAGS $THREAD_CFLAGS
+AC_DEFINE(USE_INPUT_THREAD, 1, [Use pthreads for input processing])
+fi
+
 if test x$CONFIG_UDEV = xyes 
  { test x$CONFIG_DBUS_API = xyes || test x$CONFIG_HAL = xyes; }; then
AC_MSG_ERROR([Hotplugging through both libudev and dbus/hal not 
allowed])
diff --git a/dix/main.c b/dix/main.c
index 692bec1..8056892 100644
--- a/dix/main.c
+++ b/dix/main.c
@@ -99,6 +99,7 @@ Equipment Corporation.
 #include opaque.h
 #include servermd.h
 #include hotplug.h
+#include inputthread.h
 #include site.h
 #include dixfont.h
 #include extnsionst.h
@@ -284,6 +285,8 @@ int main(int argc, char *argv[], char *envp[])
 
NotifyParentProcess();
 
+   InputThreadInit();
+
Dispatch();
 
 UndisplayDevices();
diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c
index 6561292..670addb 100644
--- a/hw/xfree86/common/xf86Events.c
+++ b/hw/xfree86/common/xf86Events.c
@@ -71,6 +71,7 @@
 #include X11/extensions/XI.h
 #include X11/extensions/XIproto.h
 #include inputstr.h
+#include inputthread.h
 #include xf86Xinput.h
 
 #include mi.h
@@ -285,42 +286,36 @@ xf86Wakeup(pointer blockData, int err, pointer pReadmask)
 if (xf86VTSwitchPending()) xf86VTSwitch();
 }
 
-
 /*
- * xf86SigioReadInput --
- *signal handler for the SIGIO signal.
+ * The host OS may or may not have working SIGIO handlers.  If they don't,
+ * then the time_to_yield callback never gets called, and input will simply
+ * run out of the main loop whenever it gets around to it.
  */
+
 static void
-xf86SigioReadInput(int fd, void *closure)
+time_to_yield(int fd, void *closure)
 {
-int errno_save = errno;
-InputInfoPtr pInfo = closure;
-
-pInfo-read_input(pInfo);
-
-errno = errno_save;
+isItTimeToYield = 1;
 }
 
-/*
- * xf86AddEnabledDevice --
- *
- */
 void
 xf86AddEnabledDevice(InputInfoPtr pInfo)
 {
-if (!xf86InstallSIGIOHandler (pInfo-fd, xf86SigioReadInput, pInfo)) {
+if (!xf86silkenMouseDisableFlag) {
+   InputThreadRegisterDev(pInfo-fd, (void*) pInfo-read_input, pInfo);
+} else {
+   xf86InstallSIGIOHandler(pInfo-fd, time_to_yield, NULL);
AddEnabledDevice(pInfo-fd);
 }
 }
 
-/*
- * xf86RemoveEnabledDevice --
- *
- */
 void
 xf86RemoveEnabledDevice(InputInfoPtr pInfo)
 {
-if