This also fixes what I think is a bug in the custom event handler... right now we have:
handler(DequeueScreen(e->pDev)->myNum, e->events->event,
e->pDev, e->nevents);
if (!e->pDev->isMaster && e->pDev->u.master) {
handler(DequeueScreen(e->pDev->u.master)->myNum,
e->events->event, e->pDev->u.master, e->nevents);
}
But that fails if e->nevents > 1 ... granted all current custom event
handlers have nevents=1, but it's worth noting.
Also, why do we call the handler twice here?
--Jeremy
diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..3e1d2f8 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -305,8 +305,9 @@ void
mieqProcessInputEvents(void)
{
mieqHandler handler;
- EventRec *e = NULL;
+ EventRec e;
int x = 0, y = 0;
+ int i;
xEvent* event,
*master_event = NULL;
@@ -321,43 +322,44 @@ mieqProcessInputEvents(void)
DPMSSet(serverClient, DPMSModeOn);
#endif
- e = &miEventQueue.events[miEventQueue.head];
+ memcpy(&e, &miEventQueue.events[miEventQueue.head],
sizeof(EventRec));
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+ event = xcalloc(e.nevents, sizeof(xEvent));
+
+ /* FIXME: Bad hack. The only event where we actually get
multiple
+ * events at once is a DeviceMotionNotify followed by + * DeviceValuators. For now it's safe enough to just take the + * event directly or copy the bunch of events and pass in the+ * copy. Eventually the interface for the processInputProc needs
+ * to be changed. (whot)
+ */
+
+ if (!event)
+ FatalError("[mi] No memory left for event processing.\n");
+ for (i = 0; i < e.nevents; i++) {
+ memcpy(&event[i], e.events[i].event, sizeof(xEvent));
+ }
+
+ /* Just using event array, null this out now since we don't
want to
+ * rely on it (another thread can change it during an enqueue
and we
+ * just copied all we care about)
+ */
+ e.events = NULL;
/* Custom event handler */
- handler = miEventQueue.handlers[e->events->event->u.u.type];
+ handler = miEventQueue.handlers[event[0].u.u.type];
- if (e->pScreen != DequeueScreen(e->pDev) && !handler) {
+ if (e.pScreen != DequeueScreen(e.pDev) && !handler) {
/* Assumption - screen switching can only occur on
motion events. */
- DequeueScreen(e->pDev) = e->pScreen;
- x = e->events[0].event->u.keyButtonPointer.rootX;
- y = e->events[0].event->u.keyButtonPointer.rootY;
- NewCurrentScreen (e->pDev, DequeueScreen(e->pDev), x, y);
- }
- else {
- /* FIXME: Bad hack. The only event where we actually get
multiple
- * events at once is a DeviceMotionNotify followed by- * DeviceValuators. For now it's safe enough to just take the - * event directly or copy the bunch of events and pass in the - * copy. Eventually the interface for the processInputProc needs
- * to be changed. (whot)
- */
- if (e->nevents > 1)
- {
- int i;
- event = xcalloc(e->nevents, sizeof(xEvent));
- if (!event)
- FatalError("[mi] No memory left for event
processing.\n");
- for (i = 0; i < e->nevents; i++)
- {
- memcpy(&event[i], e->events[i].event,
sizeof(xEvent));
- }
- } else
- event = e->events->event;
- if (!e->pDev->isMaster && e->pDev->u.master)
+ DequeueScreen(e.pDev) = e.pScreen;
+ x = event[0].u.keyButtonPointer.rootX;
+ y = event[0].u.keyButtonPointer.rootY;
+ NewCurrentScreen (e.pDev, DequeueScreen(e.pDev), x, y);
+ } else {
+ if (!e.pDev->isMaster && e.pDev->u.master)
{
- CopyGetMasterEvent(e->pDev->u.master, event,
- &master_event, e->nevents);
+ CopyGetMasterEvent(e.pDev->u.master, event,
+ &master_event, e.nevents);
} else
master_event = NULL;
@@ -365,35 +367,33 @@ mieqProcessInputEvents(void)
* steal it. */
if (handler)
{
- handler(DequeueScreen(e->pDev)->myNum, e->events-
>event,
- e->pDev, e->nevents);
- if (!e->pDev->isMaster && e->pDev->u.master)
+ handler(DequeueScreen(e.pDev)->myNum, event,
+ e.pDev, e.nevents);
+ if (!e.pDev->isMaster && e.pDev->u.master)
{
- handler(DequeueScreen(e->pDev->u.master)->myNum,
- e->events->event, e->pDev->u.master, e-
>nevents);
+ handler(DequeueScreen(e.pDev->u.master)->myNum,
+ event, e.pDev->u.master, e.nevents);
}
} else
{
/* process slave first, then master */
- e->pDev->public.processInputProc(event, e->pDev, e-
>nevents);
+ e.pDev->public.processInputProc(event, e.pDev,
e.nevents);
- if (!e->pDev->isMaster && e->pDev->u.master)
+ if (!e.pDev->isMaster && e.pDev->u.master)
{
- e->pDev->u.master-
>public.processInputProc(master_event,
- e->pDev->u.master, e->nevents);+ e.pDev->u.master- >public.processInputProc(master_event,
+ e.pDev->u.master, e.nevents);
}
}
- if (e->nevents > 1)
- xfree(event);
+ xfree(event);
xfree(master_event);
}
/* Update the sprite now. Next event may be from different
device. */
- if (e->events->event[0].u.u.type == DeviceMotionNotify - && e->pDev->coreEvents)+ if (event[0].u.u.type == DeviceMotionNotify && e.pDev- >coreEvents)
{
- miPointerUpdateSprite(e->pDev);
+ miPointerUpdateSprite(e.pDev);
}
}
}
On Oct 23, 2008, at 14:08, Jeremy Huddleston wrote:
Whoops... I forgot the diff to show you what I meant... here's what I'm going to push to 1.4-apple to address our problem.--Jeremy diff --git a/mi/mieq.c b/mi/mieq.c index 3f50f27..543b7e7 100644 --- a/mi/mieq.c +++ b/mi/mieq.c @@ -231,7 +231,7 @@ mieqSetHandler(int event, mieqHandler handler) void mieqProcessInputEvents(void) { - EventRec *e = NULL; + EventRec e; int x = 0, y = 0; DeviceIntPtr dev = NULL; @@ -250,45 +250,46 @@ mieqProcessInputEvents(void) DPMSSet(DPMSModeOn); #endif - e = &miEventQueue.events[miEventQueue.head];+ memcpy(&e, &miEventQueue.events[miEventQueue.head], sizeof(EventRec));miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE; - if (miEventQueue.handlers[e->event->u.u.type]) { +#ifdef XQUARTZ + pthread_mutex_unlock(&miEventQueueMutex); +#endif + + if (miEventQueue.handlers[e.event->u.u.type]) {/* If someone's registered a custom event handler, let them* steal it. */- miEventQueue.handlers[e->event->u.u.type] (miEventQueue.pDequeueScreen->myNum,- e->event, dev, - e->nevents);+ miEventQueue.handlers[e.event->u.u.type](e.pScreen- >myNum, + e.event, e.pDev,+ e.nevents); } - else if (e->pScreen != miEventQueue.pDequeueScreen) { + else if (e.pScreen != miEventQueue.pDequeueScreen) {/* Assumption - screen switching can only occur on motion events. */- miEventQueue.pDequeueScreen = e->pScreen; - x = e->event[0].u.keyButtonPointer.rootX; - y = e->event[0].u.keyButtonPointer.rootY; - NewCurrentScreen (miEventQueue.pDequeueScreen, x, y); + miEventQueue.pDequeueScreen = e.pScreen; + x = e.event[0].u.keyButtonPointer.rootX; + y = e.event[0].u.keyButtonPointer.rootY; + NewCurrentScreen(e.pScreen, x, y); } else {/* If this is a core event, make sure our keymap, et al, is* changed to suit. */ - if (e->event[0].u.u.type == KeyPress || - e->event[0].u.u.type == KeyRelease) { - SwitchCoreKeyboard(e->pDev); + if (e.event[0].u.u.type == KeyPress || + e.event[0].u.u.type == KeyRelease) { + SwitchCoreKeyboard(e.pDev); dev = inputInfo.keyboard; } - else if (e->event[0].u.u.type == MotionNotify || - e->event[0].u.u.type == ButtonPress || - e->event[0].u.u.type == ButtonRelease) { - SwitchCorePointer(e->pDev); + else if (e.event[0].u.u.type == MotionNotify || + e.event[0].u.u.type == ButtonPress || + e.event[0].u.u.type == ButtonRelease) { + SwitchCorePointer(e.pDev); dev = inputInfo.pointer; } else { - dev = e->pDev; + dev = e.pDev; } - dev->public.processInputProc(e->event, dev, e->nevents); + dev->public.processInputProc(e.event, dev, e.nevents); } } -#ifdef XQUARTZ - pthread_mutex_unlock(&miEventQueueMutex); -#endif } On Oct 23, 2008, at 13:59, Jeremy Huddleston wrote:Hey Tiago,I hope things are going well for you. I've recently hit an issue using locks in miEq. We're doing it the same way in mieq.c as your proposal (patch 2/4) and this causes us to hit a deadlock when the enqueueing thread is waiting for the lock to push an event and the dequeuing thread (the server thread) is processing an event that requires a message to be sent to the enqueueing thread.I am going to try solving this by making the lock a bit more conservative in mieqProcessInputEvents. Our current implementation (we're still on 1.4) passes the event to the handler as a pointer to the event within the queue. In 1.5, mieqProcessInputEvents now copies the nevents first... so we might be able to release that a bit sooner...The thing is that we have: e = &miEventQueue.events[miEventQueue.head];Then e->events is copied, but 'e' itself is still directly referenced throughout mieqProcessInputEvents. Could we actually just copy all of miEventQueue.events[miEventQueue.head] to a local copy and release the lock after that? I see us using:e->pDev e->nevents e->events (already copied in 1.5.x and later if nevents > 1) --Jeremy Begin forwarded message:From: Jeremy Huddleston <[EMAIL PROTECTED]> Date: October 19, 2008 12:59:16 PDTTo: Kevin Van Vechten <[EMAIL PROTECTED]>, Jordan Hubbard <[EMAIL PROTECTED]>Cc: George Peter Staplin <[EMAIL PROTECTED]> Subject: mach_msg async? thread communication question So... I've made some progress with fullscreen...It now renders the "weaved" background and only crashes if there are windows other than the root window... but if you just want to stare at a black and white X11 weave background, we're there!Aside from this crash, I noticed a thread communication deadlock issue (see the stack traces below):Thread 2's mieqProcessInputEvents and mieqEnqueue lock the input event queue... but some of the handlers for mieqProcessInputEvents actually need to msg the Appkit thread which appears to cause the deadlock.Is there a way to allow mach_msg to be async? In these cases, we don't care about return values.Or do I need to do something clever (read: annoying) to address this issue?Call graph: 4783 Thread_2503 4783 start 4783 main 4783 mach_msg_server 4783 mach_startup_server 4783 _Xstart_x11_server 4783 do_start_x11_server 4783 server_main 4783 X11ControllerMain 4783 X11ApplicationMain 4783 -[NSApplication run] 4783 -[X11Application sendEvent:] 4783 -[NSApplication sendEvent:]4783 -[NSApplication _handleKeyEquivalent:]4783 -[NSMenu performKeyEquivalent:]4783 -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:] 4783 -[NSMenu performActionForItemAtIndex:] 4783 -[NSApplication sendAction:to:from:] 4783 -[X11Controller toggle_fullscreen:]4783 DarwinSendDDXEvent 4783 mieqEnqueue 4768 pthread_mutex_lock4768 semaphore_wait_trap 4768 semaphore_wait_trap15 0xffffffff 15 _sigtramp 15 _sigtramp 4783 Thread_2703 4783 thread_start 4783 _pthread_start 4783 server_thread 4783 dix_main 4783 Dispatch 4783 ProcessInputEvents 4783 mieqProcessInputEvents 4783 DarwinEventHandler 4783 QuartzHide 4783 QuartzSetFullscreen 4783 X11ApplicationShowHideMenubar 4783 message_kit_thread 4783 mach_msg 4783 mach_msg_trap 4783 mach_msg_trap_______________________________________________ xorg mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/xorg
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ xorg mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/xorg
