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

Re: [PATCH 1/4] d2d1: Implement FillContainsPoint for rectangles

by Nikolay Sivov

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.

Oct. 21, 2016

Re: [PATCH 6/9] wined3d: Replace wined3d_buffer_load_sysmem() calls with wined3d_buffer_load_location().

by Henri Verbeet

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.

Oct. 21, 2016

Re: [PATCH 5/9] wined3d: Implement wined3d_buffer_prepare_location() for WINED3D_LOCATION_BUFFER.

by Henri Verbeet

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))".

Oct. 21, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Aric Stewart

On 10/21/16 7:21 AM, Sebastian Lackner wrote: > On 21.10.2016 05:39, Aric Stewart wrote: >>>> >>>> Another idea we could consider (at least for Linux) would be to >>>> use wine_server_fd_to_handle(), and then NtReadFile(). This would >>>> allow to get rid of the thread, and we could use an APC to run >>>> IoCompleteRequest(). Have you actually looked into that? And >>>> which mechanism is required on other platforms, do they also >>>> require a separate thread? >>>> >> >> I have not explored wine_server_fd_to_handle as I was not really >> aware of it, Mac and IOHID uses a callback, so a run loop thread is >> needed, but no read of any sort. If you thinking doing that wine >> magic is correct then I would be up for exploring it, but it seems >> like the sort of thing that Alexandre would frown on. >> >> -aric > > Well, using async NtReadFile would have the advantage that we can > start listening for reports at any time, without spawning a thread. > Nevertheless, if other platforms do not offer some similar mechanism, > it probably does not make any sense to implement it like that. I'm > also fine with a thread based approach. > > I would still suggest to do further investigations regarding report > loss though. If there is not sufficient evidence that this happens > on Windows aswell, I would still prefer a more reliable Wine > implementation. MSDN suggests that in order to avoid dropped HID > reports, applications can try to increase HidD_SetNumInputBuffers - > but that will obviously not help if we already drop them in the > lower level drivers. The idea with a simple flags only works if we > can be sure that the thread runs fast enough to ensure records > are never dropped. Otherwise a more complicated approach would be > required. > Yes HidD_SetNumInputBuffers only affects the size of the ring buffer at the upper level HID device. I my preferences is to go forward with the flag and wait to see if this is really an issue or not and then revisit it if we really see an issue. -aric > Regards, > Sebastian >

Oct. 21, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 21.10.2016 05:39, Aric Stewart wrote: >>> >>> Another idea we could consider (at least for Linux) would be to >>> use wine_server_fd_to_handle(), and then NtReadFile(). This would >>> allow to get rid of the thread, and we could use an APC to run >>> IoCompleteRequest(). Have you actually looked into that? And >>> which mechanism is required on other platforms, do they also >>> require a separate thread? >>> > > I have not explored wine_server_fd_to_handle as I was not really > aware of it, Mac and IOHID uses a callback, so a run loop thread is > needed, but no read of any sort. If you thinking doing that wine > magic is correct then I would be up for exploring it, but it seems > like the sort of thing that Alexandre would frown on. > > -aric Well, using async NtReadFile would have the advantage that we can start listening for reports at any time, without spawning a thread. Nevertheless, if other platforms do not offer some similar mechanism, it probably does not make any sense to implement it like that. I'm also fine with a thread based approach. I would still suggest to do further investigations regarding report loss though. If there is not sufficient evidence that this happens on Windows aswell, I would still prefer a more reliable Wine implementation. MSDN suggests that in order to avoid dropped HID reports, applications can try to increase HidD_SetNumInputBuffers - but that will obviously not help if we already drop them in the lower level drivers. The idea with a simple flags only works if we can be sure that the thread runs fast enough to ensure records are never dropped. Otherwise a more complicated approach would be required. Regards, Sebastian

Oct. 21, 2016

Re: [PATCH] hidclass.sys: Use IoSetCompletionRoutine

by Aric Stewart

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> >

Oct. 21, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Aric Stewart

On 10/19/16 10:21 PM, Sebastian Lackner wrote: > On 19.10.2016 21:32, Aric Stewart wrote: >> Hi, >> >> It is important to remember that there is a big difference between >> the user level communications with the HID device and the minidriver >> level communications. The top of the stack, the HID device, has a >> ring buffer and such. >> > [...] >> >> Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should >> always return immediately, if there is no report then it returns 0. >> If there is a report it returns the last report from the device. >> >> IOCTL_HID_READ_REPORT is expected to pend until a report is >> available. In the grand scheme of things IOCTL_HID_READ_REPORT is >> expected to block until a report is available because the general use >> is that hidclass will be in a loop where it does a >> IOCTL_HID_READ_REPORT call and when the call succeeds it places that >> report into the ring buffer and then immediately calls again to get >> the next report. >> >> IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding >> IOCTL_HID_GET_INPUT_REPORT in the class driver and >> HidD_GetInputReport. >> >> IOCTL_HID_READ_REPORT is like doing a FileRead. > > I'm aware of those differences. The part I'm more interested in is how > IOCTL_HID_READ_REPORT is implemented in Windows drivers. ;) > Even if Windows does not explicitly mention any kind of buffering, I > believe that data loss is much more unlikely than with the proposed Wine > implementation. > > Usually hardware events trigger an interrupt service routine, which can > immediately complete the request. The code responsible for queuing a new > IOCTL_HID_READ_REPORT ioctl also runs in kernel mode, maybe even with a > higher priority to avoid any interruptions by user mode processes. > In addition, devices might also do their own buffering, to ensure that > the transmission works reliable. > >> >> So they need to behave quite a bit differently. >> >>>> https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf >>> >>>> >> """In case the Wiimote state changes when there is no read request >>>> currently buffered, a flag is used to signal whether a newly >>>> received read request shall be processed and completed >>>> immediately.""" >>>> >> That would not be hard, just a flag that showed if the last reports >> has been read or not. In all practical terms the minidriver only has >> 1 consumer, the hidclass class driver devices, so in many ways the >> whole IRP queue is overkill as there should only ever be 1 client. >> It would not be hard to add that. >> >>>> In Wine we have to cache the last report anyway, so we could >>>> implement something similar very easy. Do you think it makes >>>> sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and >>>> IOCTL_HID_READ_REPORT would also not be much more difficult, as >>>> shown in the following pseudocode: >>>> >>>> --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case >>>> IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start >>>> processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT >>>> && (!last_report || last_report_seen)) { ... async handling ... >>>> break; } ... synchronous handlng ... last_report_seen = TRUE; >>>> LeaveCriticalSection(report_cs); break; --- snip --- >> I will have to explore this when I have more time to code things but >> the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT >> uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that >> complicates things a fair bit more. > > You are right, it would be a bit more complicated. Probably we could > move some of the code to a helper function, but I guess we will have > to try it out. ;) > > Another idea we could consider (at least for Linux) would be to use > wine_server_fd_to_handle(), and then NtReadFile(). This would allow > to get rid of the thread, and we could use an APC to run IoCompleteRequest(). > Have you actually looked into that? And which mechanism is required > on other platforms, do they also require a separate thread? > I have not explored wine_server_fd_to_handle as I was not really aware of it, Mac and IOHID uses a callback, so a run loop thread is needed, but no read of any sort. If you thinking doing that wine magic is correct then I would be up for exploring it, but it seems like the sort of thing that Alexandre would frown on. -aric >> >> -aric >> >

Oct. 21, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v6).

by Marvin

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

Oct. 20, 2016

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

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

Oct. 20, 2016

Re: [PATCH v3] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 19.10.2016 21:32, Aric Stewart wrote: > Hi, > > It is important to remember that there is a big difference between > the user level communications with the HID device and the minidriver > level communications. The top of the stack, the HID device, has a > ring buffer and such. > [...] > > Depends on the call that is made. IOCTL_HID_GET_INPUT_REPORT should > always return immediately, if there is no report then it returns 0. > If there is a report it returns the last report from the device. > > IOCTL_HID_READ_REPORT is expected to pend until a report is > available. In the grand scheme of things IOCTL_HID_READ_REPORT is > expected to block until a report is available because the general use > is that hidclass will be in a loop where it does a > IOCTL_HID_READ_REPORT call and when the call succeeds it places that > report into the ring buffer and then immediately calls again to get > the next report. > > IOCTL_HID_GET_INPUT_REPORT is generated by the corresponding > IOCTL_HID_GET_INPUT_REPORT in the class driver and > HidD_GetInputReport. > > IOCTL_HID_READ_REPORT is like doing a FileRead. I'm aware of those differences. The part I'm more interested in is how IOCTL_HID_READ_REPORT is implemented in Windows drivers. ;) Even if Windows does not explicitly mention any kind of buffering, I believe that data loss is much more unlikely than with the proposed Wine implementation. Usually hardware events trigger an interrupt service routine, which can immediately complete the request. The code responsible for queuing a new IOCTL_HID_READ_REPORT ioctl also runs in kernel mode, maybe even with a higher priority to avoid any interruptions by user mode processes. In addition, devices might also do their own buffering, to ensure that the transmission works reliable. > > So they need to behave quite a bit differently. > >>> https://www.julianloehr.de/wp-content/uploads/2014/07/Paper_HIDWiimote.pdf >> >>> > """In case the Wiimote state changes when there is no read request >>> currently buffered, a flag is used to signal whether a newly >>> received read request shall be processed and completed >>> immediately.""" >>> > That would not be hard, just a flag that showed if the last reports > has been read or not. In all practical terms the minidriver only has > 1 consumer, the hidclass class driver devices, so in many ways the > whole IRP queue is overkill as there should only ever be 1 client. > It would not be hard to add that. > >>> In Wine we have to cache the last report anyway, so we could >>> implement something similar very easy. Do you think it makes >>> sense? The implementation of IOCTL_HID_GET_INPUT_REPORT and >>> IOCTL_HID_READ_REPORT would also not be much more difficult, as >>> shown in the following pseudocode: >>> >>> --- snip --- case IOCTL_HID_GET_INPUT_REPORT: case >>> IOCTL_HID_READ_REPORT: EnterCriticalSection(report_cs); ... start >>> processing if not done yet ... if (ioctl == IOCTL_HID_READ_REPORT >>> && (!last_report || last_report_seen)) { ... async handling ... >>> break; } ... synchronous handlng ... last_report_seen = TRUE; >>> LeaveCriticalSection(report_cs); break; --- snip --- > I will have to explore this when I have more time to code things but > the basic idea looks pretty good though IOCTL_HID_GET_INPUT_REPORT > uses HID_XFER_PACKETs and IOCTL_HID_READ_REPORT does not so that > complicates things a fair bit more. You are right, it would be a bit more complicated. Probably we could move some of the code to a helper function, but I guess we will have to try it out. ;) Another idea we could consider (at least for Linux) would be to use wine_server_fd_to_handle(), and then NtReadFile(). This would allow to get rid of the thread, and we could use an APC to run IoCompleteRequest(). Have you actually looked into that? And which mechanism is required on other platforms, do they also require a separate thread? > > -aric >

Oct. 19, 2016