From patch 1/5:
Helpfully, I haven't taken note of which game this helped but it did in fact fix a game crash...
I remember discussing this change before... I think this was for FFXIV. While the change is fine in terms of reducing the scope of "offset" and avoiding a calculation we may end up not needing, it wasn't clear why this would avoid a crash, and that seems somewhat concerning.
We also discussed it further, it did not seem like a huge mystery that it fixes a crash. get_vertex_attrib_pointer() dereferences a buffer object that could have been freed unless we make sure to only access it when the element is in use AND element->stride is not 0.
Quoting the relevant chat:
``` (11:08:52 PM) Mystral: I think it's because sometimes there are stray, unused buffers in the stream_info structure (11:12:59 PM) Mystral: I haven't checked but I guess element->stride is 0 or something (11:13:05 PM) henri: oh, that probably makes sense, yeah (11:13:43 PM) henri: well, stream_info->use_map in particular I'd think (11:14:03 PM) Mystral: or that (11:14:37 PM) henri: e.g. wined3d_stream_info_from_declaration() only updates used elements ```
From patch 4/5:
Acquiring a GL context might be problematic during teardown (e.g. the CS thread might end up running this code after wined3d_device_uninit_3d() has already completed and device->swapchains has become NULL).
This too is a patch I've seen before. From last time:
On Wed, 9 Feb 2022 at 20:25, Matteo Bruni [matteo.mystral@gmail.com](mailto:matteo.mystral@gmail.com) wrote:
On Wed, Feb 9, 2022 at 7:44 PM Henri Verbeet [hverbeet@gmail.com](mailto:hverbeet@gmail.com) wrote:
I suppose this is probably fine in practice, but glFinish() always makes me a bit uncomfortable; I'd prefer just checking for the availability of fences, like in 5/5. Taking that one step further, if fences aren't supported, I don't think there's anything we would need to wait for in the first place; we can probably just avoid the glFinish() in that case as well.
Good point, I'll give that a try.
Not quite. There was a follow up to that which became commit ab72e336411a17e0108eb0fbc927e2c1ffb98978 and fixed that specific case.
It turns out that there is at least one more case where the same code is a problem, i.e. what I mention in the commit message. To explain it further, in specific circumstances wined3d_context_gl_wait_command_fence() attempts to acquire a different context from the one that's current at that time and, if that happens after wined3d_device_uninit_3d() has completed, that will end up accessing a NULL device->swapchain[0].
I guess we could potentially complicate the code further to address this case but honestly I don't see the point. Especially if we get rid of multiple contexts in the first place (i.e. the next patch).
From patch 5/5:
Originally written to avoid triggering a Mesa slowpath introduced by Mesa commit e7f3a8d6959c74f63c877dd8776fe519d54f946f. It seems to me like this would be a good idea for more reasons than just that though.
It may be worth mentioning those reasons then.
I guess. Avoiding multiple contexts in the most common case (which is, CSMT enabled) avoids a lot of unnecessary complication, like the context reacquire thing for event queries. Also it should fix bug 43773 (see https://bugs.winehq.org/show_bug.cgi?id=43773#c3 in particular). I suspect there are more bugs due to the same root cause (I seem to have a vague recollection of at least another application incurring the same issue but as usual I didn't keep notes, or I can't find them anyway :/)
For reference, from last time:
2020-12-03 18:52:45 @henri Mystral: 197149 caught my eye; do we actually hit that? 2020-12-03 18:56:47 @henri looking at https://cgit.freedesktop.org/mesa/mesa/commit/?id=e7f3a8d6959c74f63c877dd8776fe519d54f946f my guess would be the issue would mostly go away if we did event queries the Vulkan way 2020-12-03 18:57:03 @henri (i.e., wined3d_query_event_vk_issue()/wined3d_query_event_vk_poll()) 2020-12-03 20:31:10 @Mystral I don't know if it matters with the new event queries stuff but it did destroy perf with my branch 2020-12-03 20:32:38 @Mystral IIUC it flushes all the pending commands every time you FenceSync 2020-12-03 20:34:03 @Mystral if there are more than 1 contexts around, which is usually the case with current Wine and d3d11
Yeah, we do hit it and it was bad last time I checked (which was with upstream Wine as of a few months ago IIRC). I don't think I followed up with the event queries rework, as I understand it it wouldn't solve the issue and there seem to be more reason than just that to remove the per-swapchain constraint to our GL contexts.
I can put some of this discussion into the relevant commit messages, if that's useful.