October 2016 - Wine-Devel - list.winehq.org

Re: ntdll: Forward EtwEventRegister() from EventRegister().
by Nikolay Sivov Oct. 27, 2016

Oct. 27, 2016

> --- a/dlls/ntdll/ntdll.spec > +++ b/dlls/ntdll/ntdll.spec > @@ -41,6 +41,7 @@ > # @ stub DbgUiStopDebugging > @ stub DbgUiWaitStateChange > @ stdcall DbgUserBreakPoint() > +@ stdcall EtwEventRegister(ptr ptr ptr ptr) advapi32.EventRegister This won't do it. Advapi32 should forward to ntdll, not the other way around.

1 0

Re: [PATCH 1/2] edit: Test that EM_REPLACESEL works after EM_GETHANDLE.
by Sebastian Lackner Oct. 26, 2016

Oct. 26, 2016

On 27.10.2016 00:41, Roman Pišl wrote: > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> > --- > dlls/user32/tests/edit.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) I haven't reviewed the patch yet, but please note that the subject should start with the component (here user32), not with the filename. It was also wrong in http://source.winehq.org/git/wine.git/patch/ddfc5275af0081cba6e834bf9d9e35d…, but seems like Alexandre didn't notice it. ;) Best regards, Sebastian

1 0

d3dx9: d3dx9_base_effect_get_pass_desc
by Luis C. Busquets Pérez Oct. 26, 2016

Oct. 26, 2016

Dear developers, I have tried to run Civ4BeyondtheSword.exe with the internal d3dx9_33.dll I can run the programme but when I start a game, it hangs. I saw that a lot of FIXME's with the message: fixme:d3dx:d3dx9_base_effect_get_pass_desc Pixel shader and vertex shader are not supported, yet. Therefore, I tried to finishing implementing the function. Find attached a patch that works. With this patch, the programme does not hang. Fell free to study, redo it or rewrite it but please I look forward to somebody who corrects it and manages to integrate that function in the wine tree so that I do not need to compile the wine. On the other hand, find attached an image, where you can see that the settler image seems to something in the middle. Furthermore, the leaders also seem to have something in the middle. Actually, it seems that the image that should be behind the leader in the bottom is in the middle of them and bend vertically when they bend. I assume this is not because of the patch but I guess that this information could serve to locate and correct another bug.

1 0

Re: [PATCH] hid: implement HidD_SetOutputReport
by Sebastian Lackner Oct. 26, 2016

Oct. 26, 2016

On 24.10.2016 16:06, Aric Stewart wrote: > Also fixes an issue with IOCTL_HID_SET_FEATURE where the wrong buffers where being sent. > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/hid/hid.spec | 2 +- > dlls/hid/hidd.c | 6 ++++++ > dlls/hidclass.sys/device.c | 11 +++++++---- > include/ddk/hidsdi.h | 1 + > 4 files changed, 15 insertions(+), 5 deletions(-) > While you are just at it, shouldn't the first char in SystemBuffer be skipped? "packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;" Also, it looks like the first assignment of "irp->IoStatus.Information = 0;" is not really required, its also assigned later. Regards, Sebastian

1 0

Re: [6/6] webservices: Implement WsCall.
by Marvin Oct. 26, 2016

Oct. 26, 2016

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 https://testbot.winehq.org/JobDetails.pl?Key=26297 Your paranoid android. === build (build) === Patch failed to apply

1 0

Re: [PATCH 1/4] d2d1: Implement FillContainsPoint for rectangles
by Nikolay Sivov Oct. 26, 2016

Oct. 26, 2016

On Fri, Oct 21, 2016 at 7:28 PM, Andrew Eikum <aeikum(a)codeweavers.com> wrote: > > + *contains = probe.x > geometry->u.rectangle.rect.left - tolerance && > + probe.x < geometry->u.rectangle.rect.right + tolerance && > + probe.y > geometry->u.rectangle.rect.top - tolerance && > + probe.y < geometry->u.rectangle.rect.bottom + tolerance; > + > Does it really work like that? I'd expect tolerance to be irrelevant for paths constructed purely from lines. So basically in general case path is approximated with lines (or enclosed area with triangles) with given tolerance, and then point is tested against this flattened path. While in rectangular case it's already flat to begin with.

2 1

Product Registration Bug
by Jonathan Allen Oct. 25, 2016

Oct. 25, 2016

Dear List, Do we have any movement on bug 29354? I'm trying to move to an all-Linux environment and am blocked on two (old, venerable) XP (but not MS) applications that go to sleep in the ... fixme:msi:msi_validate_product_id partial stud: template ... ... code and can't move beyond that point. Completely stuck - help! Jonathan

1 0

Re: [PATCH 01/10 v2] server: Introduced iosb struct for server-side IO_STATUS_BLOCK representation and use it in irp_call.
by Sebastian Lackner Oct. 25, 2016

Oct. 25, 2016

On 19.10.2016 19:05, Jacek Caban wrote: > v2: Don't store async queues in a list. > > Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> > --- > server/async.c | 34 +++++++++++++++++++++++++++++++++ > server/device.c | 58 > +++++++++++++++++++++++++-------------------------------- > server/file.h | 14 ++++++++++++++ > 3 files changed, 73 insertions(+), 33 deletions(-) > I'm not sure if Alexandre has already agreed to this approach, but from my point of view it would make sense to do some measurements. How fast is the proposed named pipe approach compared to alternatives (unpatched Wine, or the Linux-only implementation included in Wine Staging)? In general I like the idea, but I fear that adding significant optimizations afterwards will be very difficult. Regards, Sebastian

3 6

Re: Polish translation
by Nikolay Sivov Oct. 25, 2016

Oct. 25, 2016

On 24.10.2016 21:55, m4sk1n wrote: > Here is translated .po file from my GlotPress. Hope it will be used. > Hi. Thank you for your contribution. Translations should be submitted as regular patches, generated using git format-patch or similar. Also make sure to use your real name when submitting.

1 0

Testing: gcc -foutline-msabi-xlogues patch
by Daniel Santos Oct. 24, 2016

Oct. 24, 2016

I've been working on a gcc patch targeting 64-bit Wine to reduce the impact of the implicit register clobbers when a Microsoft's x64 function calls a System V ABI function and I appear to finally have a working implementation. However, I cannot get recent Wine tests to complete with an unpatched compiler (I have 30 failures, 4 which are crashes and I even get some dmesgs from drivers.) While the same Wine tests pass both with and without the new optimization, I could use some external testing to make sure there's no flaws that I've missed (especially a full test with no failures). The patch set is not actually complete as my unit tests didn't catch many of the flaws (now fixed) that I caught when building real Wine, so I'm re-working them. I have my current version up on github: https://github.com/daniel-santos/gcc/tree/gcc-5_4_0-outline-msabi-xlogues. Background For those unfamiliar, the differences between the two ABIs requires an ms_abi function to save and restore 12 registers: RSI, RDI and XMM6-15. The size of pushing/popping RSI and RDI is insignificant, but saving and restoring 10 SSE registers takes between 94 and 174 bytes each function (dependent upon the SP offset), thereby bloating ".text" size quite a bit. Details This patch uses out-of-line stubs for register for saves/restores to reduce .text size. The *theory* (not yet proven) is that the reduction of instruction cache misses will offset the extra few instructions required to facilitate this. But while we're going this far, the optimization also saves/restores an additional 6 non-volatile ms_abi registers if they are clobbered in the function: RBX, RBP and R12-15. This extra step actually saves very little ".text" code, but the stub implements them using MOVs which the CPU is better able to parallelize. While MOV instructions are larger (4 bytes vs 1 or 2), they only appear in the stubs, theoretically gaining the performance benefits of MOVs without paying for the cost of the extra size. This optimization works both with and without forced stack realignment (https://bugs.winehq.org/show_bug.cgi?id=27680) although more can be done to produce better code in the realigned case. In the forced realignment case, this optimization reduces .text size Wine 64 by 19% and in the normal (aligned stack) case by 22%. As an example, here is a test function (with stubs) built with -foutline-msabi-xlogues (but no realignment): __attribute__ ((noinline)) long sysv_fn0 () { return 42; } __attribute__ ((ms_abi, noinline)) long msabifn0_6 () { __asm__ __volatile__ ("" ::: "rbx", "rbp", "r12", "r13", "r14", "r15"); return sysv_fn0 (); } 0000000000401000 <msabifn0_6>: 401000: 48 8d 44 24 88 lea -0x78(%rsp),%rax 401005: 48 81 ec f8 00 00 00 sub $0xf8,%rsp 40100c: e8 09 e8 01 00 callq 41f81a <__msabi_save_18> 401011: 31 c0 xor %eax,%eax 401013: e8 48 fc ff ff callq 400c60 <sysv_fn0> 401018: 48 8d b4 24 80 00 00 lea 0x80(%rsp),%rsi 40101f: 00 401020: 4c 8d 56 78 lea 0x78(%rsi),%r10 401024: e9 41 e8 01 00 jmpq 41f86a <__msabi_restore_ret_18> 000000000041f81a <__msabi_save_18>: 41f81a: 4c 89 78 90 mov %r15,-0x70(%rax) 000000000041f81e <__msabi_save_17>: 41f81e: 4c 89 70 98 mov %r14,-0x68(%rax) 000000000041f822 <__msabi_save_16>: 41f822: 4c 89 68 a0 mov %r13,-0x60(%rax) 000000000041f826 <__msabi_save_15>: 41f826: 4c 89 60 a8 mov %r12,-0x58(%rax) 000000000041f82a <__msabi_save_14>: 41f82a: 48 89 68 b0 mov %rbp,-0x50(%rax) 000000000041f82e <__msabi_save_13>: 41f82e: 48 89 58 b8 mov %rbx,-0x48(%rax) 000000000041f832 <__msabi_save_12>: 41f832: 48 89 78 c0 mov %rdi,-0x40(%rax) 41f836: 48 89 70 c8 mov %rsi,-0x38(%rax) 41f83a: 44 0f 29 78 d0 movaps %xmm15,-0x30(%rax) 41f83f: 44 0f 29 70 e0 movaps %xmm14,-0x20(%rax) 41f844: 44 0f 29 68 f0 movaps %xmm13,-0x10(%rax) 41f849: 44 0f 29 20 movaps %xmm12,(%rax) 41f84d: 44 0f 29 58 10 movaps %xmm11,0x10(%rax) 41f852: 44 0f 29 50 20 movaps %xmm10,0x20(%rax) 41f857: 44 0f 29 48 30 movaps %xmm9,0x30(%rax) 41f85c: 44 0f 29 40 40 movaps %xmm8,0x40(%rax) 41f861: 0f 29 78 50 movaps %xmm7,0x50(%rax) 41f865: 0f 29 70 60 movaps %xmm6,0x60(%rax) 41f869: c3 retq 000000000041f86a <__msabi_restore_ret_18>: 41f86a: 4c 8b 7e 90 mov -0x70(%rsi),%r15 000000000041f86e <__msabi_restore_ret_17>: 41f86e: 4c 8b 76 98 mov -0x68(%rsi),%r14 000000000041f872 <__msabi_restore_ret_16>: 41f872: 4c 8b 6e a0 mov -0x60(%rsi),%r13 000000000041f876 <__msabi_restore_ret_15>: 41f876: 4c 8b 66 a8 mov -0x58(%rsi),%r12 000000000041f87a <__msabi_restore_ret_14>: 41f87a: 48 8b 6e b0 mov -0x50(%rsi),%rbp 000000000041f87e <__msabi_restore_ret_13>: 41f87e: 48 8b 5e b8 mov -0x48(%rsi),%rbx 000000000041f882 <__msabi_restore_ret_12>: 41f882: 48 8b 7e c0 mov -0x40(%rsi),%rdi 41f886: 44 0f 28 7e d0 movaps -0x30(%rsi),%xmm15 41f88b: 44 0f 28 76 e0 movaps -0x20(%rsi),%xmm14 41f890: 44 0f 28 6e f0 movaps -0x10(%rsi),%xmm13 41f895: 44 0f 28 26 movaps (%rsi),%xmm12 41f899: 44 0f 28 5e 10 movaps 0x10(%rsi),%xmm11 41f89e: 44 0f 28 56 20 movaps 0x20(%rsi),%xmm10 41f8a3: 44 0f 28 4e 30 movaps 0x30(%rsi),%xmm9 41f8a8: 44 0f 28 46 40 movaps 0x40(%rsi),%xmm8 41f8ad: 0f 28 7e 50 movaps 0x50(%rsi),%xmm7 41f8b1: 0f 28 76 60 movaps 0x60(%rsi),%xmm6 41f8b5: 48 8b 76 c8 mov -0x38(%rsi),%rsi 41f8b9: 4c 89 d4 mov %r10,%rsp 41f8bc: c3 retq Here is a similar function with forced realignment. This can be improved with an additional "have frame pointer" set of stubs. __attribute__ ((ms_abi, noinline, force_align_arg_pointer)) long msabifn0_5 () { __asm__ __volatile__ ( "" ::: "rbx", "r12", "r13", "r14", "r15" ); return sysv_fn0 (); } 0000000000400fc0 <msabifn0_5>: 400fc0: 55 push %rbp 400fc1: 48 8d 44 24 90 lea -0x70(%rsp),%rax 400fc6: 48 89 e5 mov %rsp,%rbp 400fc9: 48 83 e0 f0 and $0xfffffffffffffff0,%rax 400fcd: 48 8d 60 b0 lea -0x50(%rax),%rsp 400fd1: e8 78 e8 01 00 callq 41f84e <__msabi_save_13> 400fd6: 41 57 push %r15 400fd8: 41 56 push %r14 400fda: 41 55 push %r13 400fdc: 41 54 push %r12 400fde: 31 c0 xor %eax,%eax 400fe0: e8 7b fc ff ff callq 400c60 <sysv_fn0> 400fe5: 48 8d a5 20 ff ff ff lea -0xe0(%rbp),%rsp 400fec: 41 5c pop %r12 400fee: 41 5d pop %r13 400ff0: 41 5e pop %r14 400ff2: 41 5f pop %r15 400ff4: 48 8d 74 24 50 lea 0x50(%rsp),%rsi 400ff9: e8 a0 e8 01 00 callq 41f89e <__msabi_restore_13> 400ffe: 48 81 c4 c0 00 00 00 add $0xc0,%rsp 401005: 5d pop %rbp 401006: c3 retq Daniel

1 2

Re: [PATCH 09/10] kernel32/tests: Added more named pipe tests.
by Marvin Oct. 24, 2016

Oct. 24, 2016

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 https://testbot.winehq.org/JobDetails.pl?Key=26285 Your paranoid android. === w864 (64 bit pipe) === pipe.c:2575: Test failed: FlushFileBuffers failed: 87 pipe.c:2575: Test failed: FlushFileBuffers failed: 87

1 0

Stack realignment configure option?
by Daniel Santos Oct. 23, 2016

Oct. 23, 2016

I've read up a bit on the issues with realignment on Wine 64 and all of the programs that crash because they luckily get away with violating the ABI on Windows. While not crashing is a pretty good thing, forcing stack realignment also affects optimizations in some less obvious ways, inhibiting sibling calls, ret from function bodies, adding a frame pointer, etc. I don't actually have any bright ideas for this atm, but can we at least add a configure option to disable this for those who don't happen to have such programs crashing? Probably only Gentoo users and geeks will care for it.

1 1

TRACE in wined3d_check_device_format()
by wylda＠volny.cz Oct. 22, 2016

Oct. 22, 2016

Hi, isn't that middle \n in TRACE some kind of leftover? TRACE("wined3d %p, adapter_idx %u, device_type %s, adapter_format %s, usage %s, %s,\n" "resource_type %s, check_format %s.\n", wined3d, adapter_idx, debug_d3ddevicetype(device_type), debug_d3dformat(adapter_format_id), debug_d3dusage(usage), debug_d3dusagequery(usage), debug_d3dresourcetype(resource_type), debug_d3dformat(check_format_id)); Regards, W.

2 1

How to make winedbg listen to same port every time?
by A B Oct. 21, 2016

Oct. 21, 2016

If i run : winedbg --gdb --no-start test.exe it say : target remote localhost:55304 But if I run it second time , it say a different port , for example target remote localhost:111111 is there a command line argument for winedbg so it to listen to same port every time , and to do not change the port ,so I can specity to witch port it to listen , or I have to recompile winedbg ? Thank you!

2 2

Re: [PATCH 6/9] wined3d: Replace wined3d_buffer_load_sysmem() calls with wined3d_buffer_load_location().
by Henri Verbeet Oct. 21, 2016

Oct. 21, 2016

On 20 October 2016 at 12:50, Józef Kucia <jkucia(a)codeweavers.com> wrote: > - /* Download the buffer, but don't permanently enable double buffering */ > - if (!(buffer->flags & WINED3D_BUFFER_DOUBLEBUFFER)) > - { > - wined3d_buffer_load_sysmem(buffer, context); > - buffer->flags &= ~WINED3D_BUFFER_DOUBLEBUFFER; > - } > + /* Download the buffer, but don't permanently enable double buffering. */ > + wined3d_buffer_load_location(buffer, context, WINED3D_LOCATION_SYSMEM); > + buffer->flags &= ~WINED3D_BUFFER_DOUBLEBUFFER; Wouldn't this also clear WINED3D_BUFFER_DOUBLEBUFFER if it was previously set? Longer term, it's probably inappropriate for wined3d_buffer_load_location() to set WINED3D_BUFFER_DOUBLEBUFFER to begin with.

1 0

Re: [PATCH 5/9] wined3d: Implement wined3d_buffer_prepare_location() for WINED3D_LOCATION_BUFFER.
by Henri Verbeet Oct. 21, 2016

Oct. 21, 2016

On 20 October 2016 at 12:50, Józef Kucia <jkucia(a)codeweavers.com> wrote: > @@ -612,7 +619,7 @@ void buffer_get_memory(struct wined3d_buffer *buffer, struct wined3d_context *co > { > if ((buffer->flags & WINED3D_BUFFER_USE_BO) && !buffer->resource.map_count) > { > - buffer_create_buffer_object(buffer, context); > + wined3d_buffer_prepare_location(buffer, context, WINED3D_LOCATION_BUFFER); > if (buffer->buffer_object) Note that that should be equivalent to "if (wined3d_buffer_prepare_location(buffer, context, WINED3D_LOCATION_BUFFER))".

1 0

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices
by Sebastian Lackner Oct. 21, 2016

Oct. 21, 2016

On 17.10.2016 17:14, Aric Stewart wrote: > v2: Suggestions from Sebastian Lackner > v3: Further suggestions from Sebastian Lackner > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus.h | 3 + > dlls/winebus.sys/bus_udev.c | 73 ++++++++++++++++++++++++ > dlls/winebus.sys/main.c | 133 +++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 208 insertions(+), 1 deletion(-) > > Now that all the handling for last_report is in main.c, I would suggest to split it out into a separate patch for easier reviewing. The first patch could contain IOCTL_HID_READ_REPORT and corresponding platform code only. Caching of the last record can then be implemented in the second patch. > > v3-0003-winebus.sys-Handle-device-reports-for-hidraw-device.txt > > > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h > index 7ce54bf..339449d 100644 > --- a/dlls/winebus.sys/bus.h > +++ b/dlls/winebus.sys/bus.h > @@ -25,6 +25,8 @@ typedef struct > int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); > NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length); > NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length); > + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device); > + void (*end_report_processing)(DEVICE_OBJECT *device); > } platform_vtbl; > > void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; > void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; > +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) DECLSPEC_HIDDEN; > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c > index 243aabb..517a7d3 100644 > --- a/dlls/winebus.sys/bus_udev.c > +++ b/dlls/winebus.sys/bus_udev.c > @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > > #ifdef HAVE_UDEV > > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > + > static struct udev *udev_context = NULL; > static DRIVER_OBJECT *udev_driver_obj = NULL; > > @@ -73,6 +75,11 @@ struct platform_private > { > struct udev_device *udev_device; > int device_fd; > + > + BYTE *report_buffer; > + int buffer_length; > + HANDLE report_thread; > + int control_pipe[2]; > }; > > static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) > @@ -222,11 +229,77 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf > return STATUS_SUCCESS; > } > > +static DWORD CALLBACK device_report_thread(void *args) > +{ > + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args; > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + struct pollfd plfds[2]; > + > + plfds[0].fd = private->device_fd; > + plfds[0].events = POLLIN; > + plfds[0].revents = 0; > + plfds[1].fd = private->control_pipe[0]; > + plfds[1].events = POLLIN; > + plfds[1].revents = 0; > + > + while (1) > + { > + int size; > + if (poll(plfds, 2, -1) <= 0) continue; > + if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0) > + break; > + size = read(plfds[0].fd, private->report_buffer, private->buffer_length); > + if (size == -1) > + TRACE_(hid_report)("Read failed. Likely an unplugged device\n"); > + else if (size == 0) > + TRACE_(hid_report)("Failed to read report\n"); > + else > + process_hid_report(device, private->report_buffer, size); > + } > + return 0; > +} > + > +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + private->buffer_length = 1024; > + private->report_buffer = HeapAlloc(GetProcessHeap(), 0, private->buffer_length); My understanding is that report_buffer is only used from the device_report_thread. Do we really have to store it in the platform private data then? For such a small amount of data, you could probably even allocate it directly on the stack in device_report_thread. > + > + pipe(private->control_pipe); Please also check for success here. > + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL); > + if (!private->report_thread) > + { > + ERR("Unable to create device report thread\n"); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); > + return STATUS_UNSUCCESSFUL; > + } > + else > + return STATUS_SUCCESS; > +} > + > +static void end_report_processing(DEVICE_OBJECT *device) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + if (private->report_thread) > + { > + write(private->control_pipe[1], "q", 1); > + WaitForSingleObject(private->report_thread, INFINITE); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); report_thread should also be closed when no longer needed. > + } > + private->buffer_length = 0; > + HeapFree(GetProcessHeap(), 0, private->report_buffer); > + private->report_buffer = NULL; > +} > + > static const platform_vtbl hidraw_vtbl = > { > compare_platform_device, > hidraw_get_reportdescriptor, > hidraw_get_string, > + begin_report_processing, > + end_report_processing, > }; > > static void try_add_device(struct udev_device *dev) > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c > index 86adbf5..0bd2c76 100644 > --- a/dlls/winebus.sys/main.c > +++ b/dlls/winebus.sys/main.c > @@ -40,6 +40,7 @@ > #include "bus.h" > > WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > > struct pnp_device > { > @@ -58,6 +59,14 @@ struct device_extension > const WCHAR *busid; /* Expected to be a static constant */ > > const platform_vtbl *vtbl; > + > + BOOL processing_started; > + BYTE *last_report; > + DWORD last_report_size; > + DWORD buffer_size; > + LIST_ENTRY irp_queue; > + CRITICAL_SECTION report_cs; > + > BYTE platform_private[1]; > }; > > @@ -214,6 +223,10 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > ext->busid = busidW; > ext->vtbl = vtbl; > It might be better to explicitly initialize all fields here. I don't know if you can rely on the fact that Wines current implementation nulls the driver extension. > + InitializeListHead(&ext->irp_queue); > + InitializeCriticalSection(&ext->report_cs); > + ext->report_cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": report_cs"); > + > /* add to list of pnp devices */ > pnp_dev->device = device; > list_add_tail(&pnp_devset, &pnp_dev->entry); > @@ -271,6 +284,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > { > struct device_extension *ext = (struct device_extension *)device->DeviceExtension; > struct pnp_device *pnp_device = ext->pnp_device; > + LIST_ENTRY *entry; > + IRP *irp; > > TRACE("(%p)\n", device); > > @@ -279,7 +294,28 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > LeaveCriticalSection(&device_list_cs); > > IoInvalidateDeviceRelations(device, RemovalRelations); > + > + if (ext->processing_started) > + ext->vtbl->end_report_processing(device); At this point the device_fd was already closed (too early). Do we really need the callback, or could we also do the platform specific cleanup in try_remove_device()? > + > + /* Cancel pending IRPs */ > + EnterCriticalSection(&ext->report_cs); > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) You could write this loop a bit easier like: while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) { ... } The same also applies to the code below. > + { > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irp->IoStatus.u.Status = STATUS_CANCELLED; > + irp->IoStatus.Information = 0; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > + > + ext->report_cs.DebugInfo->Spare[0] = 0; > + DeleteCriticalSection(&ext->report_cs); > + > HeapFree(GetProcessHeap(), 0, ext->serial); > + HeapFree(GetProcessHeap(), 0, ext->last_report); > IoDeleteDevice(device); > > /* pnp_device must be released after the device is gone */ > @@ -432,6 +468,58 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); > break; > } > + case IOCTL_HID_GET_INPUT_REPORT: > + { > + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); > + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device); > + if (status == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + > + if (packet->reportBufferLen < ext->last_report_size) > + { > + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + > + if (ext->last_report_size) > + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size); > + packet->reportBufferLen = ext->last_report_size; > + irp->IoStatus.Information = ext->last_report_size; > + irp->IoStatus.u.Status = status = STATUS_SUCCESS; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + case IOCTL_HID_READ_REPORT: > + { > + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + if (ext->vtbl->begin_report_processing(device) == STATUS_SUCCESS) Did you want to assign the result to "status" here? Also, it is a bit unfortunate that we have tracking of report processing in both the common code and also in the platform specific code. If you modify begin_report_processing to return STATUS_SUCCESS if it already has been started, you could simplify this code to something like: --- snip --- status = ext->vtbl->begin_report_processing(device); if (status != STATUS_SUCCESS) { ... } --- snip --- It also ensures that the status in both components never gets out of sync. Not sure if it is a problem, but due to the fact that the device has been opened a long time before starting to read from it, couldn't there be a large amount of reports in the kernel buffers? Do you think it makes sense to skip such outdated reports? > + ext->processing_started = TRUE; > + else > + { > + irp->IoStatus.u.Status = status; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); > + status = STATUS_PENDING; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > default: > { > ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; > @@ -441,11 +529,54 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > } > } > > - IoCompleteRequest(irp, IO_NO_INCREMENT); > + if (status != STATUS_PENDING) > + IoCompleteRequest(irp, IO_NO_INCREMENT); > > return status; > } > > +void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) > +{ > + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; > + IRP *irp; > + LIST_ENTRY *entry; > + > + EnterCriticalSection(&ext->report_cs); > + if (length > ext->buffer_size) > + { > + HeapFree(GetProcessHeap(), 0, ext->last_report); > + ext->last_report = HeapAlloc(GetProcessHeap(), 0, length); It would be better to handle failures here. > + ext->buffer_size = length; > + } > + > + memcpy(ext->last_report, report, length); > + ext->last_report_size = length; > + > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + IO_STACK_LOCATION *irpsp; > + TRACE_(hid_report)("Processing Request\n"); > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irpsp = IoGetCurrentIrpStackLocation(irp); > + > + if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < length) > + { > + irp->IoStatus.Information = 0; > + irp->IoStatus.u.Status = STATUS_BUFFER_TOO_SMALL; > + } > + else > + { > + memcpy(irp->UserBuffer, report, length); > + irp->IoStatus.Information = length; > + irp->IoStatus.u.Status = STATUS_SUCCESS; > + } > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > +} > + > NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path ) > { > static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0}; > > >

2 8

Re: [PATCH] hidclass.sys: Use IoSetCompletionRoutine
by Aric Stewart Oct. 21, 2016

Oct. 21, 2016

On 10/18/16 5:40 PM, Sebastian Lackner wrote: > On 18.10.2016 17:07, Aric Stewart wrote: >> Signed-off-by: Aric Stewart <aric(a)codeweavers.com> >> --- >> dlls/hidclass.sys/device.c | 20 +++++++------------- >> dlls/hidclass.sys/main.c | 13 +++++-------- >> dlls/hidclass.sys/pnp.c | 10 +++++----- >> 3 files changed, 17 insertions(+), 26 deletions(-) > > Thanks! While you are just at it, could you please also check the two > occurances of "if (irp->IoStatus.u.Status == STATUS_PENDING)"? They > look wrong to me, the correct way would be to check the return value > of IoCallDriver. Also, it looks like call_minidriver is leaking "buffer". Actually that was intentional at the time. There was code in ntoskrnl that was freeing that buffer already so I was getting a double free issue. Now you have been doing a lot of work there so maybe that has changed. I have not gone back and revisited it. -aric > > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de> >

1 0

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v6).
by Marvin Oct. 20, 2016

Oct. 20, 2016

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 https://testbot.winehq.org/JobDetails.pl?Key=26243 Your paranoid android. === w2008s64 (64 bit sock) === sock: unhandled exception c0000005 at 000007FEFF1220C0

1 0

Re: [PATCH] d2d1: Initial implementation of bitmap render target
by Henri Verbeet Oct. 19, 2016

Oct. 19, 2016

On 14 October 2016 at 18:12, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > + if (!pixel_size && size) > + { > + texture_desc.Width = ceilf((size->width * parent_target->desc.dpiX) / 96.0f); > + texture_desc.Height = ceilf((size->height * parent_target->desc.dpiY) / 96.0f); > + dxgi_rt_desc.dpiX = (texture_desc.Width * 96.0f) / size->width; > + dxgi_rt_desc.dpiY = (texture_desc.Height * 96.0f) / size->height; > + } > + else if (pixel_size && !size) > + { > + texture_desc.Width = pixel_size->width; > + texture_desc.Height = pixel_size->height; > + dxgi_rt_desc.dpiX = parent_target->desc.dpiX; > + dxgi_rt_desc.dpiY = parent_target->desc.dpiY; > + } > + else if (pixel_size && size) > + { > + texture_desc.Width = pixel_size->width; > + texture_desc.Height = pixel_size->height; > + dxgi_rt_desc.dpiX = (pixel_size->width * 96.0f) / size->width; > + dxgi_rt_desc.dpiY = (pixel_size->height * 96.0f) / size->height; > + } > + else > + { > + texture_desc.Width = parent_target->pixel_size.width; > + texture_desc.Height = parent_target->pixel_size.height; > + dxgi_rt_desc.dpiX = parent_target->desc.dpiX; > + dxgi_rt_desc.dpiY = parent_target->desc.dpiY; > + } It may just be taste, but I think this is a bit harder to follow than it needs to be. What do you think about the following? if (pixel_size) { texture_desc.Width = pixel_size->width; texture_desc.Height = pixel_size->height; } else if (size) { texture_desc.Width = ceilf((size->width * parent_target->desc.dpiX) / 96.0f); texture_desc.Height = ceilf((size->height * parent_target->desc.dpiY) / 96.0f); } else { texture_desc.Width = parent_target->pixel_size.width; texture_desc.Height = parent_target->pixel_size.height; } if (size) { dxgi_rt_desc.dpiX = (texture_desc.Width * 96.0f) / size->width; dxgi_rt_desc.dpiY = (texture_desc.Height * 96.0f) / size->height; } else { dxgi_rt_desc.dpiX = parent_target->desc.dpiX; dxgi_rt_desc.dpiY = parent_target->desc.dpiY; }

2 1

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().
by Henri Verbeet Oct. 19, 2016

Oct. 19, 2016

On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -static void buffer_invalidate_bo_range(struct wined3d_buffer *buffer, unsigned int offset, unsigned int size) > +static void buffer_mark_dirty_area(struct wined3d_buffer *buffer, unsigned int offset, unsigned int size) I assume this is for consistency with buffer_clear_dirty_areas() and the like, but in the larger context we're using validate/invalidate for terminology.

2 2

Re: [PATCH 3/6] wined3d: Rename buffer_internal_preload() to wined3d_buffer_load().
by Henri Verbeet Oct. 19, 2016

Oct. 19, 2016

On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -void buffer_internal_preload(struct wined3d_buffer *buffer, struct wined3d_context *context, > +void wined3d_buffer_load(struct wined3d_buffer *buffer, struct wined3d_context *context, > const struct wined3d_state *state) This makes the naming consistent with wined3d_texture_load(), so that's good. Still, I don't think that one necessarily covers what the function does very well either, so if anyone has good suggestions...

1 0

Re: [PATCH 2/6] wined3d: Clean up buffer_internal_preload().
by Henri Verbeet Oct. 19, 2016

Oct. 19, 2016

On 16 October 2016 at 21:46, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -static inline void fixup_transformed_pos(float *p) > +static inline unsigned int fixup_transformed_pos(float *p) > { > /* rhw conversion like in position_float4(). */ > if (p[3] != 1.0f && p[3] != 0.0f) > @@ -452,6 +454,8 @@ static inline void fixup_transformed_pos(float *p) > p[2] *= w; > p[3] = w; > } > + > + return 4 * sizeof(*p); > } Since you're cleaning things up, note that this should probably use struct wined3d_vec4. > - for (i = start / buffer->stride; i < min((end / buffer->stride) + 1, vertices); ++i) > + for (i = start / buffer->stride; i < min((end / buffer->stride) + 1, vertex_count); ++i) > { > - for (j = 0; j < buffer->stride; ++j) > + for (j = 0; j < buffer->stride;) > { > switch (buffer->conversion_map[j]) > { In general this conversion code shouldn't be used a lot, but I could imagine it being better to loop over the vertices in the fixup functions instead of in the outer loop.

1 0

Re: [PATCH 1/2] taskschd: Added ITriggerCollection::Create implementation.
by Dmitry Timoshkov Oct. 19, 2016

Oct. 19, 2016

Jacek Caban <jacek(a)codeweavers.com> wrote: > + if(IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_IDispatch, riid) || IsEqualGUID(&IID_ITrigger, riid) > + || IsEqualGUID(&IID_IDailyTrigger, riid)) { > + *ppv = &This->IDailyTrigger_iface; > + }else { Please try to follow the style of the existing code for placing braces, add space between if and (, } and else, and split the above line of comparisons. > @@ -144,8 +373,29 @@ static HRESULT WINAPI TriggerCollection_get__NewEnum(ITriggerCollection *iface, > static HRESULT WINAPI TriggerCollection_Create(ITriggerCollection *iface, TASK_TRIGGER_TYPE2 type, ITrigger **trigger) > { > trigger_collection *This = impl_from_ITriggerCollection(iface); > - FIXME("(%p)->(%d %p)\n", This, type, trigger); > - return E_NOTIMPL; > + > + TRACE("(%p)->(%d %p)\n", This, type, trigger); > + > + switch(type) { > + case TASK_TRIGGER_DAILY: { > + DailyTrigger *daily_trigger; > + > + daily_trigger = heap_alloc(sizeof(*daily_trigger)); > + if(!daily_trigger) > + return E_OUTOFMEMORY; > + > + daily_trigger->IDailyTrigger_iface.lpVtbl = &DailyTrigger_vtbl; > + daily_trigger->ref = 1; > + > + *trigger = (ITrigger*)&daily_trigger->IDailyTrigger_iface; > + break; > + } > + default: > + FIXME("Unimplemented type %d\n", type); > + return E_NOTIMPL; > + } Please add a global constructor for the triggger, and use it here. -- Dmitry.

1 0

Re: [PATCH v2] kernel32: Improved GetUserPreferredUILanguages stub.
by Marvin Oct. 18, 2016

Oct. 18, 2016

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 https://testbot.winehq.org/JobDetails.pl?Key=26213 Your paranoid android. === w1064 (32 bit locale) === locale.c:2650: Test failed: LCMapStringEx should fail with bad locale name locale.c:2651: Test failed: unexpected error code -559038737 locale.c:2742: Test failed: Expected lcid == 0, got 00001000, error -559038737 locale.c:4478: Test failed: IsValidLocaleName should have failed locale.c:4480: Test failed: IsValidLocaleName should have failed locale.c:4863: Test failed: For id 7, expected ret == 4, got 1, error 87 locale.c:4865: Test failed: For id 7, Expected IVC, got '' === w1064 (64 bit locale) === locale.c:2650: Test failed: LCMapStringEx should fail with bad locale name locale.c:2651: Test failed: unexpected error code -559038737 locale.c:2742: Test failed: Expected lcid == 0, got 00001000, error -559038737 locale.c:4478: Test failed: IsValidLocaleName should have failed locale.c:4480: Test failed: IsValidLocaleName should have failed locale.c:4863: Test failed: For id 7, expected ret == 4, got 1, error 87 locale.c:4865: Test failed: For id 7, Expected IVC, got ''

1 0