June 2011 - Wine-devel - list.winehq.org

deadlock in mmdevapi AudioClient_Stop (winealsa.drv/mmdevdrv.c)
by Joerg-Cyril.Hoehle＠t-systems.com 15 Jun '11

15 Jun '11

Hi, please try and convince me that a deadlock cannot happen. http://source.winehq.org/source//dlls/winealsa.drv/mmdevdrv.c AudioClient_Start calls CreateTimerQueueTimer which creates a new thread periodically invoking the timer callback. The callback invokes alsa_push_buffer_data() which ceases the audio client object lock (This->lock). AudioClient_Stop performs EnterCriticalSection(&This->lock); then DeleteTimerQueueTimer(g_timer_q, This->timer, INVALID_HANDLE_VALUE); Now imagine Stop is invoked, EnterCriticalSection executed then the thread is preempted. Suppose the timer callback kicks in: alsa_push_buffer_data will block on This->lock. When the thread is resumed, DeleteTimerQueue will hang forever waiting for all timer callbacks to come to an end. My opinion is that critical sections are great to prevent concurrent execution while things are running, but they are hell to tear down properly. IMHO, LeaveCriticalSection + Re-EnterCriticalSection around DeleteTimerQueue is not a solution. I consider using Leave+EnterCS around anything that can wait/block an anti-pattern (there are a few places like that in Wine). I believe a "restart op" is needed in general (like a "restart transaction if it failed" in databases) as anything could have happened while waiting. Regards, Jörg Höhle

2 1

Re: [4/6] winex11.drv: Grab registered hotkeys in the X server.
by Ken Thomases 14 Jun '11

14 Jun '11

On Jun 14, 2011, at 12:40 AM, Vincent Povirk wrote: >> Is it necessary to test QS_HOTKEY in dlls/winex11.drv/event.c:filter_event, where QS_KEY is currently tested? If somebody calls MsgWaitForMultipleObjectsEx with QS_HOTKEY but not QS_KEY, shouldn't KeyPress events be accepted? >> > > I guess? Wouldn't we have to actually set QS_HOTKEY before that does any good? You mean your patch 5? Well, it's a bit of a chicken-and-egg situation. I don't think it matters much which order the two things are added. They'll both ultimately be necessary. In some sense, doing winex11.drv first makes sense, since it's the first "gate" for the event to make it into the system. (Admittedly, it's probably very unlikely that MsgWaitForMultipleObjectsEx will be called with QS_HOTKEY and not QS_KEY.) >> You may be able to use the non-left-right-sensitive vkeys (e.g. VK_MENU) here, and thus eliminate the alt_vkey parameter. And if you can't, then update_modifier_state doesn't do the right thing when alt_vkey is the one that's pressed in the keystate but it's no longer pressed in event->state. In that case, you send a key-up for vkey, when you should send it for alt_vkey. If both were down and now neither is, you'll need to send a key-up for both. Note, to send keyboard input for a right-hand modifier, you'll need to include KEYEVENTF_EXTENDEDKEY in the flags and different scan codes. > > There's no such combined vkey for the windows key. Even if there were, > releasing VK_MENU when VK_LMENU was pressed doesn't seem like a good > idea. I was thinking that maybe the send-input machinery would take a side-generic key and do the right thing. A little more research reveals that it doesn't. However, that still leaves you doing the wrong thing with your side-sensitive code. If event->state doesn't include MetaMask, then you send a key-up for VK_LMENU when it may have been VK_RMENU that was previously down. You need to send a key-up for whichever was previously down, and that might even be both. Note that the important thing is not that you have sent a key event for the wrong key, it's that you haven't properly transitioned to a state which matches event->state. That is, you sent the key-up but the overall key state will still have a menu key down. > Now you've got me worried about what happens when we press VK_LWIN and > it later turns out VK_RWIN was really pressed (and has been released). > :/ For when you send a key-down, it's not so important. In that case, you're properly transitioning from neither being down to at least one of them being down. If it later turns out that you synthesized a VK_LWIN-down when the user had actually pressed VK_RWIN, then that will get corrected on the next KeymapNotify. -Ken

1 0

Re: [PATCH 1/6] server: Add server-side raw input functions definitions
by Austin English 14 Jun '11

14 Jun '11

2011/6/14 Vincas Miliūnas <vincas.miliunas(a)gmail.com>: > This is a partial raw input implementation for mouse and keyboard. > > I've read in the IRC chat, that products of tools/make_request should not be included in a patch, so I left them out. > > My first attempts were by leeching input events from queue_mouse_message and queue_keyboard_message functions, but I couldn't get mouse cursor movement right. Instead, I copied the way dinput receives input - by installing low level hooks and forwarding input to the server. This is just to get "try 1" out of the door. Of course, it would be better if the input feed would be server-side only. > > I hit a very weird bug, where in peek_message function, info.msg.lParam was impossible to set with the proper lParam (it was correct in reply->lparam) value, instead is was overwritten by some constant. That was happening only for the messages sent by the patch using post_message. I guess the tales of server-side dragons hold some truth... If anyone wants to try to reproduce it, just recompile without the unused #include "user.h" in raw_input.c and run user32 tests. > > This patch game-tested and works for Quake Live (in_mouse 2 for raw input), World of Tanks and Risen. > > Feedback is very much appreciated. Thanks. > > --- > server/protocol.def | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 79 insertions(+), 0 deletions(-) Howdy Vincas, This patch breaks the compile: $ patch -p1 < ~/75274 $ ./tools/make_requests $ ./configure --disable-tests $ make make[1]: Entering directory `/home/austin/wine-git/server' gcc -o wineserver async.o atom.o change.o class.o clipboard.o completion.o console.o debugger.o device.o directory.o event.o fd.o file.o handle.o hook.o mach.o mailslot.o main.o mapping.o mutex.o named_pipe.o object.o process.o procfs.o ptrace.o queue.o region.o registry.o request.o semaphore.o serial.o signal.o snapshot.o sock.o symlink.o thread.o timer.o token.o trace.o unicode.o user.o window.o winstation.o -L../libs/wine -lwine ../libs/port/libwine_port.a -Wl,--rpath,\$ORIGIN/../libs/wine request.o:(.rodata+0x494): undefined reference to `req_get_raw_input_device_list' request.o:(.rodata+0x498): undefined reference to `req_register_raw_input_device' request.o:(.rodata+0x49c): undefined reference to `req_unregister_raw_input_device' request.o:(.rodata+0x4a0): undefined reference to `req_is_raw_input_data' request.o:(.rodata+0x4a4): undefined reference to `req_get_raw_input_data' request.o:(.rodata+0x4a8): undefined reference to `req_get_registered_raw_input_devices' request.o:(.rodata+0x4ac): undefined reference to `req_add_raw_input_data' request.o:(.rodata+0x4b0): undefined reference to `req_get_raw_input_device_info' collect2: ld returned 1 exit status make[1]: *** [wineserver] Error 1 make[1]: Leaving directory `/home/austin/wine-git/server' make: *** [server] Error 2 which you implement in patch 5. Every patch needs to stand on its own, without breaking the tests/compile. For such a complicated changeset, it's probably best to send the tests in on their own first, with the failing tests wrapped in todo_wine(). See also http://wiki.winehq.org/SubmittingPatches BTW, thanks for working on this, I used your earlier patches to play a bit of Brink ;-). -- -Austin

1 0

Re: [PATCH 6/6] user32/tests: Add client-side raw input functions tests
by Marvin 14 Jun '11

14 Jun '11

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11737 Your paranoid android. === W7PROX64 (64 bit input) === No test summary line found

1 0

wined3d performance patches
by Stefan Dösinger 14 Jun '11

14 Jun '11

Hi, This is intended mostly for the other d3d developers, but since we have quite a number of them now so individual CCs are a lot of work :-) I attached the patches I currently have in my tree to give an update on what I've been working on recently. The main aim of those patches is to reduce draw overhead a bit, thus improving game performance. The patches need some cleanup, but for that I first need a patch Matteo is working on. Feedback is welcome. I'm also interested in test results, e.g. if the changes break a game, or the performance impact. If those patches cause a 5% performance increase I am happy. Patches 1-3: Mostly unrelated. I haven't sent them yet because patch 3 breaks Unigine Heaven, and patches 1 and 2 make little sense without 3. Patch 4: This removes a hack for a driver bug workaround. I have to do more testing on my old machines to find out if the bug is really fixed in newer nvidia drivers. Patches 5, 6: They keep track of changes to the framebuffer setup so we don't have to run through the code that figures out which FBO to bind every draw. Patch 5 gets rid of the ordering assumption. Patch 6 applies the FBO only when needed. They aren't ready yet. In patch 6 the FBO may have to be reapplied when the pixelshader changes. To implement that I need some draw buffer tracking infrastructure Matteo is working on. Also clears can be integrated. fbo- clear.diff is a half-baked attempt to do this. I dropped it when I realized I was duplicating Matteos work. After that I have to double-check that I took care of all situations where the FBO may have to be updated. Furthermore, Matteo says that not calling context_apply_draw_buffers every time framebuffer() is run is a noticeable performance improvement too. Matteo, did you test this with just patch 0005, or both 0005 and 0006? Patch 0007: Sampler map optimization, it has a lengthy description in the patch file Patch 0008: A tiny fix, it results in a pretty small improvement on OSX. On Linux+Nvidia it is not noticeable. Patch 0009: At first I tried to skip the render target dirtification entirely via a flag in the d3ddevice, but it was pretty tricky and ugly. Just making it cheaper gets us ~2/3rd of the way too. (Draw overhead tester performance without this: 259 fps. Complete disabling of the dirtification calls via a hack: 275. With this patch: 269) 0010: An unrelated cleanup Patches 11, 12: Preparation for including clears in the fbo dirtification patches. See fbo-clear.diff. More work on performance is obviously required, for example *) Separate vertex declaration, vertex shader and pixel shader states *) Speed up sampler preloading. This will be easier once we have a tree-like state structure. *) Write more tests for other common operations, like clears, blits, shader changes, texture changes, vertex buffer changes, dynamic resource loading *) Test our shader's GPU-side execution performance *) See if we can do something about locking *) Isolate bottlenecks in the GPU drivers and get them fixed.

4 7

Re: [2/3] cmd/tests: Add support for todo_wine constructions (try 2)
by Jacek Caban 14 Jun '11

14 Jun '11

Hi Frédéric, On 6/13/11 10:19 PM, Frédéric Delanoy wrote: > + }else if(err) { > + if (!is_todo_wine) > + ok(0, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", > + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), > + out_ptr, (int)(exp_nl-exp_ptr), exp_ptr); > + else > + todo_wine > + ok(0, "unexpected char 0x%x position %d in line %d (got '%.*s', wanted '%.*s')\n", > + *err, (int)(err-out_ptr), line, (int)(out_nl-out_ptr), out_ptr, > + (int)(exp_nl-exp_ptr-sizeof(todo_wine_cmd)), exp_ptr+sizeof(todo_wine_cmd)); You may change tests to ok(!err, ....) here and make this else unconditional to avoid having ok(TRUE, ...) tests later. Jacek

2 6

Re: [PATCH 2/2] d3dx9: Return E_FAIL for unsupported ascii effects.
by Alexandre Julliard 14 Jun '11

14 Jun '11

Rico Schüller <kgbricola(a)web.de> writes: > --- > dlls/d3dx9_36/effect.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) It breaks the tests: ../../../tools/runtest -q -P wine -M d3dx9_36.dll -T ../../.. -p d3dx9_36_test.exe.so effect.c && touch effect.ok effect.c:52: Test failed: Got result 80004005, expected 0 (D3D_OK) wine: Unhandled page fault on execute access to 0x00000000 at address (nil) (thread 0023), starting debugger... -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: winex11.drv: Only set MWM hints if they have not been modified
by Alexandre Julliard 14 Jun '11

14 Jun '11

Scott Talbert <talbert(a)techie.net> writes: > Hello, > > The below patch resolves an issue (BZ# 27473) where the window manager > removes window decorations and Wine restores them. The code in the > patch checks to see whether the window decorations have been modified > and if so, > it does not modify them further. That's a Unity bug. The window manager is not supposed to change the MWM hints, they are controlled by the client. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: [1/3] wined3d: Add GL_ARB_draw_elements_base_vertex
by Henri Verbeet 14 Jun '11

14 Jun '11

On 13 June 2011 22:59, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > +typedef void (WINE_GLAPI *PGLFNDRAWELEMENTSBASEVERTEX)(GLenum mode, GLsizei count, GLenum type, > + const GLvoid *indices, GLint basevertex); With the usual naming convention this would be called "PGLFNDRAWELEMENTSBASEVERTEXPROC".

1 0

Re: [4/6] winex11.drv: Grab registered hotkeys in the X server.
by Ken Thomases 14 Jun '11

14 Jun '11

On Jun 13, 2011, at 2:07 PM, Vincent Povirk wrote: > <0005-winex11.drv-Grab-registered-hotkeys-in-the-X-server.txt> Is it necessary to test QS_HOTKEY in dlls/winex11.drv/event.c:filter_event, where QS_KEY is currently tested? If somebody calls MsgWaitForMultipleObjectsEx with QS_HOTKEY but not QS_KEY, shouldn't KeyPress events be accepted? > +static void update_modifier_state(int modifier_mask, int actual_mask, > + BYTE *keystate, WORD vkey, WORD alt_vkey, WORD scan, DWORD event_time) > +{ > + int key_pressed = (actual_mask & modifier_mask) != 0; > + int vkey_pressed = (keystate[vkey] & 0x80) == 0x80 || > + (keystate[alt_vkey] & 0x80) == 0x80; > + DWORD flags; > + > + if (key_pressed != vkey_pressed) > + { > + if (key_pressed) > + flags = 0; > + else > + flags = KEYEVENTF_KEYUP; > + TRACE_(key)("Adjusting modifier state vkey=0x%x, state=%i\n", vkey, key_pressed); > + X11DRV_send_keyboard_input( NULL, vkey, scan, flags, event_time ); > + } > +} > + update_modifier_state(MetaMask, event->state, keystate, VK_LMENU, VK_RMENU, 0x38, event_time); > + update_modifier_state(ControlMask, event->state, keystate, VK_LCONTROL, VK_RCONTROL, 0x1D, event_time); > + update_modifier_state(ShiftMask, event->state, keystate, VK_LSHIFT, VK_RSHIFT, 0x2A, event_time); > + update_modifier_state(SuperMask, event->state, keystate, VK_LWIN, VK_RWIN, 0x15b, event_time); You may be able to use the non-left-right-sensitive vkeys (e.g. VK_MENU) here, and thus eliminate the alt_vkey parameter. And if you can't, then update_modifier_state doesn't do the right thing when alt_vkey is the one that's pressed in the keystate but it's no longer pressed in event->state. In that case, you send a key-up for vkey, when you should send it for alt_vkey. If both were down and now neither is, you'll need to send a key-up for both. Note, to send keyboard input for a right-hand modifier, you'll need to include KEYEVENTF_EXTENDEDKEY in the flags and different scan codes. -Ken

1 0