Hello,
Trying to solve the problem reported in Bugzilla #301 I found that the root
of the problem is an order of timeout processing in WaitForSomething subroutine.
The reported problem is that under some conditions Xserver duplicates key
press events. For example if one run setxkbmap program it can happen that
the Enter key pressed as the end of the command line produces two key press
events.
It happens when XKB uses a software emulated autorepeat and a machine is
relatively weak or is heavy loaded and so some request performing consumes
much time.
When XKB emulates an autorepeat in a software it uses Xserver's timers.
each key press sets a timer and a key release cancels the action the timer
has to do when it is expired. If no key release event comes before the timer
expires a timers callback function emulates a key press event.
The problem is that WaitForSomething checks timers (and runs thir callbacks)
twice. Once before it calls select and the second time after a select
finishing and a reading of input devices events.
It works correctly when an Xserver is 'almost idle' and sometime processes
short (in time) requests. In such case it spends most time sleeping on a select
call and even the autorepeat timer expires near the time a real key is released,
WaitForSomething code after a select finishing firstly reads keyboard events
and cancels uneeded emulated autorepeat events.
But if a server performs a request such as 'load new keyboard map' that
consumes much time things become worse. During that time it can happens
that the key pressed before the request processing start is already released
but the autorepeat timer is already expired too. When the server calls
WaitForSomething it firstly checks timers and although the key is already
released and a corresponded event is ready to be read the subroutine knows
nothing about this event until it calls Select and reads input devices.
Therefore it runs callback function that emits autorepeated key press event
and when WaitForSomething comes to Select and then to a real key events reading
the real key release event can't cancel wrongly emited autorepeat event.
The solution is obvious. WaitForSomething should not run any callbacks before
the Select but just check timers and if there are expired timers call select
with a zero timeout. Also the code before the select checks screensaver and
DPMS timeouts. I don't see reason why those timeouts are processed separately
from timers and don't use timers facility.
(Strictly speaking to process those timeouts before the select is incorrect
too. Because some timeouts can expire and the subroutine can run a screensaver
or switch DPMS although some input device events are alreday happened and will
be read immediately when WaitForSomteting comes to Select.)
I made a patch that changes the first timers check as I said and moves
the screensaver and DPMS timeouts processing into timers and their callbacks.
But I don't know why the first timers check was put before the select (except
for a making a positive timeout for the select call) and why screensaver and
DPMS don't use timers. And I can't guess can my patch cause any problems
somewhere else. Please review it.
--
Ivan U. Pascal | e-mail: [EMAIL PROTECTED]
Administrator of | Tomsk State University
University Network | Tomsk, Russia
--- xc/programs/Xserver/os/WaitFor.c.orig Sat Sep 20 19:07:25 2003
+++ xc/programs/Xserver/os/WaitFor.c Sat Sep 20 19:07:34 2003
@@ -131,17 +131,12 @@
* pClientsReady is an array to store ready client->index values into.
*****************/
-static INT32 timeTilFrob = 0; /* while screen saving */
-
int
WaitForSomething(int *pClientsReady)
{
int i;
struct timeval waittime, *wt;
INT32 timeout = 0;
-#ifdef DPMSExtension
- INT32 standbyTimeout = 0, suspendTimeout = 0, offTimeout = 0;
-#endif
fd_set clientsReadable;
fd_set clientsWritable;
int curclient;
@@ -188,138 +183,17 @@
else
{
#endif
-#ifdef DPMSExtension
- if (ScreenSaverTime > 0 || DPMSEnabled || timers)
-#else
- if (ScreenSaverTime > 0 || timers)
-#endif
- now = GetTimeInMillis();
- wt = NULL;
+ wt = NULL;
if (timers)
- {
- while (timers && (int) (timers->expires - now) <= 0)
- DoTimer(timers, now, &timers);
- if (timers)
- {
- timeout = timers->expires - now;
- waittime.tv_sec = timeout / MILLI_PER_SECOND;
- waittime.tv_usec = (timeout % MILLI_PER_SECOND) *
- (1000000 / MILLI_PER_SECOND);
- wt = &waittime;
- }
- }
- if (ScreenSaverTime > 0
-#ifdef DPMSExtension
- || (DPMSEnabled &&
- (DPMSStandbyTime > 0 || DPMSSuspendTime > 0 || DPMSOffTime > 0))
-#endif
- ) {
-#ifdef DPMSExtension
- if (ScreenSaverTime > 0)
-#endif
- timeout = (ScreenSaverTime -
- (now - lastDeviceEventTime.milliseconds));
-#ifdef DPMSExtension
- if (DPMSStandbyTime > 0)
- standbyTimeout = (DPMSStandbyTime -
- (now - lastDeviceEventTime.milliseconds));
- if (DPMSSuspendTime > 0)
- suspendTimeout = (DPMSSuspendTime -
- (now - lastDeviceEventTime.milliseconds));
- if (DPMSOffTime > 0)
- offTimeout = (DPMSOffTime -
- (now - lastDeviceEventTime.milliseconds));
-#endif /* DPMSExtension */
-
- if (
- timeout <= 0
-#ifdef DPMSExtension
- && ScreenSaverTime > 0
-#endif /* DPMSExtension */
- ) {
- INT32 timeSinceSave;
-
- timeSinceSave = -timeout;
- if (timeSinceSave >= timeTilFrob && timeTilFrob >= 0)
- {
- ResetOsBuffers(); /* not ideal, but better than nothing */
- SaveScreens(SCREEN_SAVER_ON, ScreenSaverActive);
-#ifdef DPMSExtension
- if (ScreenSaverInterval > 0 &&
- DPMSPowerLevel == DPMSModeOn)
-#else
- if (ScreenSaverInterval)
-#endif /* DPMSExtension */
- /* round up to the next ScreenSaverInterval */
- timeTilFrob = ScreenSaverInterval *
- ((timeSinceSave + ScreenSaverInterval) /
- ScreenSaverInterval);
- else
- timeTilFrob = -1;
- }
- timeout = timeTilFrob - timeSinceSave;
- }
- else
- {
- if (ScreenSaverTime > 0 && timeout > ScreenSaverTime)
- timeout = ScreenSaverTime;
- timeTilFrob = 0;
- }
-#ifdef DPMSExtension
- if (DPMSEnabled)
- {
- if (standbyTimeout > 0
- && (timeout <= 0 || timeout > standbyTimeout))
- timeout = standbyTimeout;
- if (suspendTimeout > 0
- && (timeout <= 0 || timeout > suspendTimeout))
- timeout = suspendTimeout;
- if (offTimeout > 0
- && (timeout <= 0 || timeout > offTimeout))
- timeout = offTimeout;
- }
-#endif
- if (timeout > 0 && (!wt || timeout < (int) (timers->expires - now)))
- {
- waittime.tv_sec = timeout / MILLI_PER_SECOND;
- waittime.tv_usec = (timeout % MILLI_PER_SECOND) *
- (1000000 / MILLI_PER_SECOND);
- wt = &waittime;
- }
-#ifdef DPMSExtension
- /* don't bother unless it's switched on */
- if (DPMSEnabled) {
- /*
- * If this mode's enabled, and if the time's come
- * and if we're still at a lesser mode, do it now.
- */
- if (DPMSStandbyTime > 0) {
- if (standbyTimeout <= 0) {
- if (DPMSPowerLevel < DPMSModeStandby) {
- DPMSSet(DPMSModeStandby);
- }
- }
- }
- /*
- * and ditto. Note that since these modes can have the
- * same timeouts, they can happen at the same time.
- */
- if (DPMSSuspendTime > 0) {
- if (suspendTimeout <= 0) {
- if (DPMSPowerLevel < DPMSModeSuspend) {
- DPMSSet(DPMSModeSuspend);
- }
- }
- }
- if (DPMSOffTime > 0) {
- if (offTimeout <= 0) {
- if (DPMSPowerLevel < DPMSModeOff) {
- DPMSSet(DPMSModeOff);
- }
- }
- }
- }
-#endif
+ {
+ now = GetTimeInMillis();
+ timeout = timers->expires - now;
+ if (timeout < 0)
+ timeout = 0;
+ waittime.tv_sec = timeout / MILLI_PER_SECOND;
+ waittime.tv_usec = (timeout % MILLI_PER_SECOND) *
+ (1000000 / MILLI_PER_SECOND);
+ wt = &waittime;
}
XFD_COPYSET(&AllSockets, &LastSelectMask);
#ifdef SMART_SCHEDULE
@@ -646,3 +520,108 @@
xfree(timer);
}
}
+
+static CARD32
+ScreenSaverTimeoutExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < ScreenSaverTime) {
+ return ScreenSaverTime - timeout;
+ }
+
+ ResetOsBuffers(); /* not ideal, but better than nothing */
+ SaveScreens(SCREEN_SAVER_ON, ScreenSaverActive);
+
+#ifdef DPMSExtension
+ if (ScreenSaverInterval > 0 && DPMSPowerLevel == DPMSModeOn)
+#else
+ if (ScreenSaverInterval > 0)
+#endif /* DPMSExtension */
+ return ScreenSaverInterval;
+
+ return 0;
+}
+
+static OsTimerPtr ScreenSaverTimer = NULL;
+
+void
+SetScreenSaverTimer(void)
+{
+ if (ScreenSaverTime > 0) {
+ ScreenSaverTimer = TimerSet(ScreenSaverTimer, 0, ScreenSaverTime,
+ ScreenSaverTimeoutExpire, NULL);
+ } else if (ScreenSaverTimer) {
+ TimerFree(ScreenSaverTimer);
+ ScreenSaverTimer = NULL;
+ }
+}
+
+#ifdef DPMSExtension
+
+static OsTimerPtr DPMSStandbyTimer = NULL;
+static OsTimerPtr DPMSSuspendTimer = NULL;
+static OsTimerPtr DPMSOffTimer = NULL;
+
+static CARD32
+DPMSStandbyTimerExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < DPMSStandbyTime) {
+ return DPMSStandbyTime - timeout;
+ }
+ if (DPMSPowerLevel < DPMSModeStandby) {
+ DPMSSet(DPMSModeStandby);
+ }
+ return DPMSStandbyTime;
+}
+
+static CARD32
+DPMSSuspendTimerExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < DPMSSuspendTime) {
+ return DPMSSuspendTime - timeout;
+ }
+ if (DPMSPowerLevel < DPMSModeSuspend) {
+ DPMSSet(DPMSModeSuspend);
+ }
+ return DPMSSuspendTime;
+}
+
+static CARD32
+DPMSOffTimerExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < DPMSOffTime) {
+ return DPMSOffTime - timeout;
+ }
+ if (DPMSPowerLevel < DPMSModeOff) {
+ DPMSSet(DPMSModeOff);
+ }
+ return DPMSOffTime;
+}
+
+void
+SetDPMSTimers(void)
+{
+ if (!DPMSEnabled)
+ return;
+
+ if (DPMSStandbyTime > 0) {
+ DPMSStandbyTimer = TimerSet(DPMSStandbyTimer, 0, DPMSStandbyTime,
+ DPMSStandbyTimerExpire, NULL);
+ }
+ if (DPMSSuspendTime > 0) {
+ DPMSStandbyTimer = TimerSet(DPMSSuspendTimer, 0, DPMSSuspendTime,
+ DPMSSuspendTimerExpire, NULL);
+ }
+ if (DPMSOffTime > 0) {
+ DPMSOffTimer = TimerSet(DPMSOffTimer, 0, DPMSOffTime,
+ DPMSOffTimerExpire, NULL);
+ }
+}
+#endif
--- xc/programs/Xserver/include/os.h.orig Sat Sep 20 19:33:39 2003
+++ xc/programs/Xserver/include/os.h Sat Sep 20 19:34:32 2003
@@ -220,6 +220,12 @@
extern void TimerFree(OsTimerPtr /* pTimer */);
+extern void SetScreenSaverTimer(void);
+
+#ifdef DPMSExtension
+extern void SetDPMSTimers(void);
+#endif
+
extern SIGVAL AutoResetServer(int /*sig*/);
extern SIGVAL GiveUp(int /*sig*/);
--- xc/programs/Xserver/dix/main.c.orig Sat Sep 20 19:36:09 2003
+++ xc/programs/Xserver/dix/main.c Sat Sep 20 19:36:31 2003
@@ -420,6 +420,9 @@
InitRootWindow(WindowTable[i]);
DefineInitialRootWindow(WindowTable[0]);
SaveScreens(SCREEN_SAVER_FORCER, ScreenSaverReset);
+#ifdef DPMSExtension
+ SetDPMSTimers();
+#endif
#ifdef PANORAMIX
if (!noPanoramiXExtension) {
--- xc/programs/Xserver/dix/window.c.orig Sat Sep 20 19:36:18 2003
+++ xc/programs/Xserver/dix/window.c Sat Sep 20 19:36:39 2003
@@ -3487,6 +3487,8 @@
}
}
screenIsSaved = what;
+ if (mode == ScreenSaverReset)
+ SetScreenSaverTimer();
}
static Bool
