Re: [AD] X11 unresponsiveness

[Elias Pschernig <elias@xxxxxxxxxx>]

On Tue, 2004-07-13 at 04:51 -0700, Chris wrote: 
> I think I figured out the best solution for this problem. We seem to be 
> hacking the input signal handler, trying to make it do something it's 
> not really designed to handle.. but we seem to have been overlooking 
> some Allegro functions: keyboard_needs_poll/poll_keyboard (and it's 
> mouse and joystick kin).
> 
> Here's what I propose.. since we seem to have a semi-working patch for 
> the signal based handler (Elias says the last patch I posted works 
> good), but not the best, we could use the signal-based aproach by 
> default. The only reason for that is that some people didn't really take 
> the warning seriously that some systems would require polling. However, 
> the *_needs_poll functions would return TRUE regardless, and when the 
> poll_* functions are called, that device is no longer auto-polled in the 
> handler.
> 
> What do you think?
> 

Well, the patch works perfectly here so far. I'm only concerned about
possible race conditions. A signal can interrupt at any place, even
inside a C instructions. And with that in mind, using a simple lock
variable like we do with the signals driver isn't 100% safe. (We would
need to disable signals, use some special software-mutex-construct, or
rely on asm support, to be on the safe side.)

So, encouraging polling with the signals driver is a good idea in any
case. And it will prevent code written as badly as expal or the demo in
the first place :)

To clarify this all, the current problem is code like:

while (1)
{
   check key[]
   do_intense_gfx_stuff() with XLOCK XUNLOCK
}

It would get:

while (1)
{
   poll_keyboard ();
   check key[]
   do_intense_gfx_stuff() with XLOCK XUNLOCK
}

And so the responsiveness problem is solved. But I still want to apply
the patch first, updated version attached (thanks to SF who finally let
me made it).

-- 
Elias Pschernig

Index: src/x/xwin.c
===================================================================
RCS file: /cvsroot/alleg/allegro/src/x/xwin.c,v
retrieving revision 1.55
diff -u -p -r1.55 xwin.c
--- src/x/xwin.c	29 Oct 2003 12:37:57 -0000	1.55
+++ src/x/xwin.c	13 Jul 2004 13:23:52 -0000
@@ -137,6 +137,10 @@ struct _xwin_type _xwin =
 int _xwin_last_line = -1;
 int _xwin_in_gfx_call = 0;
 
+#ifndef ALLEGRO_MULTITHREADED
+int _xwin_missed_input;
+#endif
+
 #ifdef ALLEGRO_XWINDOWS_WITH_XF86DGA
 int _xdga_last_line = -1;
 static int _xdga_installed_colormap; 
@@ -190,7 +194,6 @@ static void _xwin_private_flush_buffers(
 static void _xwin_private_vsync(void);
 static void _xwin_private_resize_window(int w, int h);
 static void _xwin_private_process_event(XEvent *event);
-static void _xwin_private_handle_input(void);
 static void _xwin_private_set_warped_mouse_mode(int permanent);
 static void _xwin_private_redraw_window(int x, int y, int w, int h);
 static int _xwin_private_scroll_screen(int x, int y);
@@ -2205,7 +2208,7 @@ static void _xwin_private_process_event(
 /* _xwin_handle_input:
  *  Handle events from the queue.
  */
-static void _xwin_private_handle_input(void)
+void _xwin_private_handle_input(void)
 {
    int i, events;
    static XEvent event[5];
@@ -2252,7 +2255,12 @@ static void _xwin_private_handle_input(v
 
 void _xwin_handle_input(void)
 {
-   if (_xwin.lock_count) return;
+#ifndef ALLEGRO_MULTITHREADED
+   if (_xwin.lock_count) {
+      ++_xwin_missed_input;
+      return;
+   }
+#endif
 
    XLOCK();
 
Index: include/allegro/platform/aintunix.h
===================================================================
RCS file: /cvsroot/alleg/allegro/include/allegro/platform/aintunix.h,v
retrieving revision 1.6
diff -u -p -r1.6 aintunix.h
--- include/allegro/platform/aintunix.h	7 Feb 2003 13:29:06 -0000	1.6
+++ include/allegro/platform/aintunix.h	13 Jul 2004 13:23:52 -0000
@@ -83,28 +83,51 @@ extern "C" {
    AL_ARRAY(_DRIVER_INFO, _xwin_timer_driver_list);
 
    AL_FUNC(void, _xwin_handle_input, (void));
+   AL_FUNC(void, _xwin_private_handle_input, (void));
 
+#ifndef ALLEGRO_MULTITHREADED
+
+   AL_VAR(int, _xwin_missed_input);
 
    #define XLOCK()                              \
       do {                                      \
-         if (_unix_bg_man->multi_threaded) {    \
-            if (_xwin.display)                  \
-               XLockDisplay(_xwin.display);     \
-         }                                      \
          _xwin.lock_count++;                    \
       } while (0)
 
    #define XUNLOCK()                            \
       do {                                      \
-         if (_unix_bg_man->multi_threaded) {    \
-            if (_xwin.display)                  \
-               XUnlockDisplay(_xwin.display);   \
+         if (_xwin.lock_count == 1) {           \
+            while(_xwin_missed_input) {         \
+               if (_xwin_input_handler)         \
+                  _xwin_input_handler();        \
+               else                             \
+                  _xwin_private_handle_input(); \
+               --_xwin_missed_input;            \
+            }                                   \
          }                                      \
+         _xwin.lock_count--;                    \
+      } while (0)
+
+#else
+
+   #define XLOCK()                              \
+      do {                                      \
+         if (_xwin.display)                     \
+            XLockDisplay(_xwin.display);        \
+         _xwin.lock_count++;                    \
+      } while (0)
+
+   #define XUNLOCK()                            \
+      do {                                      \
+         if (_xwin.display)                     \
+            XUnlockDisplay(_xwin.display);      \
          _xwin.lock_count--;                    \
       } while (0)
 
 #endif
 
+#endif
+
 
 #ifdef DIGI_OSS
    /* So the setup program can read what we detected */