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

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

by Aric Stewart

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. On 10/19/16 12:32 PM, Sebastian Lackner wrote: > On 19.10.2016 10:50, Aric Stewart wrote: >> >> I dont think that is a big problem. Not that it wont happen but that >> it is ok to have happen. The 2 ways to read from the >> minidriver/device layer is to either get the last report from the >> device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to >> wait for the next report from the device(IOCTL_HID_READ_REPORT). >> >> There is not specification of a need for a ring buffer or anything >> like that in the MSDN docs that I can see so it seem like keeping it >> simple and direct is the best. > > Even if missing a report is not a problem, doesn't it cause an arbitrary > long delay when we have to wait for the next report? I have not been able > to find much information about buffering in HID drivers, but at least > some drivers seem to implement that. For example: > 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. 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. -aric

Oct. 19, 2016

Re: [PATCH] d2d1: Initial implementation of bitmap render target

by Nikolay Sivov

On 10/19/2016 01:13 PM, Henri Verbeet wrote: > 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? > > Sure, just sent v2. Initially I did it in the same orderit's documented.

Oct. 19, 2016

How to make winedbg listen to same port every time?

by A B

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!

Oct. 19, 2016

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().

by Henri Verbeet

On 19 October 2016 at 13:52, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > Yes, it's mainly for consistency with buffer_clear_dirty_areas(), but > additionally: > * having two functions with very similar names is confusing > (wined3d_buffer_invalidate_range() and buffer_invalidate_bo_range()), Somewhat. It would be good to make that less confusing somehow, but I don't think using different terminology makes it better. One is a more specific helper for the other, and I imagine ultimately wined3d_buffer_invalidate_range() would be the only caller of buffer_invalidate_bo_range(). I.e., similar to how e.g. surface_load_location() and surface_load_texture() are related. > * dirty areas are also used for flushing modified areas when BO is > being unmapped (see wined3d_buffer_unmap()). Yes, but I don't think that conflicts with the current name?

Oct. 19, 2016

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().

by Józef Kucia

On Wed, Oct 19, 2016 at 1:32 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > 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. Yes, it's mainly for consistency with buffer_clear_dirty_areas(), but additionally: * having two functions with very similar names is confusing (wined3d_buffer_invalidate_range() and buffer_invalidate_bo_range()), * dirty areas are also used for flushing modified areas when BO is being unmapped (see wined3d_buffer_unmap()).

Oct. 19, 2016

Re: [PATCH 6/6] wined3d: Rename buffer_invalidate_bo_range() to buffer_mark_dirty_area().

by Henri Verbeet

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.

Oct. 19, 2016

Re: [PATCH 3/6] wined3d: Rename buffer_internal_preload() to wined3d_buffer_load().

by Henri Verbeet

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

Oct. 19, 2016

Re: [PATCH 2/6] wined3d: Clean up buffer_internal_preload().

by Henri Verbeet

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.

Oct. 19, 2016

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

by Sebastian Lackner

On 19.10.2016 10:50, Aric Stewart wrote: > > I dont think that is a big problem. Not that it wont happen but that > it is ok to have happen. The 2 ways to read from the > minidriver/device layer is to either get the last report from the > device or current device state (IOCTL_HID_GET_INPUT_REPORT) or to > wait for the next report from the device(IOCTL_HID_READ_REPORT). > > There is not specification of a need for a ring buffer or anything > like that in the MSDN docs that I can see so it seem like keeping it > simple and direct is the best. Even if missing a report is not a problem, doesn't it cause an arbitrary long delay when we have to wait for the next report? I have not been able to find much information about buffering in HID drivers, but at least some drivers seem to implement that. For example: 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.""" 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 --- Regards, Sebastian

Oct. 19, 2016

Re: [PATCH] d2d1: Initial implementation of bitmap render target

by Henri Verbeet

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

Oct. 19, 2016