[PATCH v6 0/3] MR4452: d2d1: Acquire lock before attempt to draw to device context
Fixes periodic crashes in PowerPoint 365 -- v6: d2d1: Acquire lock before attempt to draw to device context d2d1: Add test for singlethreaded draw d2d1: Add test for multithreaded draw https://gitlab.winehq.org/wine/wine/-/merge_requests/4452
From: Brendan McGrath <bmcgrath(a)codeweavers.com> Tests that Direct2D can not access D2D exclusive resources whilst the Direct 2D lock is held --- dlls/d2d1/tests/d2d1.c | 50 +++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 58f905ca1dc..1e4d77e1c84 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -1163,13 +1163,13 @@ static IDXGISwapChain *create_d3d10_swapchain(ID3D10Device1 *device, HWND window } static ID2D1RenderTarget *create_render_target_desc(IDXGISurface *surface, - const D2D1_RENDER_TARGET_PROPERTIES *desc, BOOL d3d11) + const D2D1_RENDER_TARGET_PROPERTIES *desc, BOOL d3d11, D2D1_FACTORY_TYPE factory_type) { ID2D1RenderTarget *render_target; ID2D1Factory *factory; HRESULT hr; - hr = D2D1CreateFactory(D2D1_FACTORY_TYPE_SINGLE_THREADED, &IID_ID2D1Factory, NULL, (void **)&factory); + hr = D2D1CreateFactory(factory_type, &IID_ID2D1Factory, NULL, (void **)&factory); ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory_CreateDxgiSurfaceRenderTarget(factory, surface, desc, &render_target); ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); @@ -1178,7 +1178,7 @@ static ID2D1RenderTarget *create_render_target_desc(IDXGISurface *surface, return render_target; } -static ID2D1RenderTarget *create_render_target(IDXGISurface *surface, BOOL d3d11) +static ID2D1RenderTarget *create_render_target(IDXGISurface *surface, BOOL d3d11, D2D1_FACTORY_TYPE factory_type) { D2D1_RENDER_TARGET_PROPERTIES desc; @@ -1190,7 +1190,7 @@ static ID2D1RenderTarget *create_render_target(IDXGISurface *surface, BOOL d3d11 desc.usage = D2D1_RENDER_TARGET_USAGE_NONE; desc.minLevel = D2D1_FEATURE_LEVEL_DEFAULT; - return create_render_target_desc(surface, &desc, d3d11); + return create_render_target_desc(surface, &desc, d3d11, factory_type); } #define release_test_context(ctx) release_test_context_(__LINE__, ctx) @@ -1216,8 +1216,9 @@ static void release_test_context_(unsigned int line, struct d2d1_test_context *c ok_(__FILE__, line)(!ref, "Device has %lu references left.\n", ref); } -#define init_test_context(ctx, d3d11) init_test_context_(__LINE__, ctx, d3d11) -static BOOL init_test_context_(unsigned int line, struct d2d1_test_context *ctx, BOOL d3d11) +#define init_test_context(ctx, d3d11) init_test_context_(__LINE__, ctx, d3d11, D2D1_FACTORY_TYPE_SINGLE_THREADED) +#define init_test_multithreaded_context(ctx, d3d11) init_test_context_(__LINE__, ctx, d3d11, D2D1_FACTORY_TYPE_MULTI_THREADED) +static BOOL init_test_context_(unsigned int line, struct d2d1_test_context *ctx, BOOL d3d11, D2D1_FACTORY_TYPE factory_type) { HRESULT hr; @@ -1237,7 +1238,7 @@ static BOOL init_test_context_(unsigned int line, struct d2d1_test_context *ctx, hr = IDXGISwapChain_GetBuffer(ctx->swapchain, 0, &IID_IDXGISurface, (void **)&ctx->surface); ok_(__FILE__, line)(hr == S_OK, "Failed to get buffer, hr %#lx.\n", hr); - ctx->rt = create_render_target(ctx->surface, d3d11); + ctx->rt = create_render_target(ctx->surface, d3d11, factory_type); if (!ctx->rt && d3d11) { todo_wine win_skip_(__FILE__, line)("Skipping d3d11 tests.\n"); @@ -4887,7 +4888,7 @@ static void test_alpha_mode(BOOL d3d11) rt_desc.dpiY = 0.0f; rt_desc.usage = D2D1_RENDER_TARGET_USAGE_NONE; rt_desc.minLevel = D2D1_FEATURE_LEVEL_DEFAULT; - rt = create_render_target_desc(ctx.surface, &rt_desc, d3d11); + rt = create_render_target_desc(ctx.surface, &rt_desc, d3d11, D2D1_FACTORY_TYPE_SINGLE_THREADED); ok(!!rt, "Failed to create render target.\n"); ID2D1RenderTarget_SetAntialiasMode(rt, D2D1_ANTIALIAS_MODE_ALIASED); @@ -10694,8 +10695,23 @@ static DWORD WINAPI mt_factory_test_thread_func(void *param) return 0; } +static DWORD WINAPI mt_factory_test_thread_draw_func(void *param) { + ID2D1RenderTarget *rt = param; + D2D1_COLOR_F color; + HRESULT hr; + + ID2D1RenderTarget_BeginDraw(rt); + set_color(&color, 1.0f, 1.0f, 0.0f, 1.0f); + ID2D1RenderTarget_Clear(rt, &color); + hr = ID2D1RenderTarget_EndDraw(rt, NULL, NULL); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + return 0; +} + static void test_mt_factory(BOOL d3d11) { + struct d2d1_test_context ctx; ID2D1Multithread *multithread; ID2D1Factory *factory; HANDLE thread; @@ -10750,6 +10766,24 @@ static void test_mt_factory(BOOL d3d11) ID2D1Multithread_Release(multithread); ID2D1Factory_Release(factory); + + if (!init_test_multithreaded_context(&ctx, d3d11)) + return; + + hr = ID2D1Factory_QueryInterface(ctx.factory, &IID_ID2D1Multithread, (void **)&multithread); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + ID2D1Multithread_Enter(multithread); + thread = CreateThread(NULL, 0, mt_factory_test_thread_draw_func, ctx.rt, 0, NULL); + ok(!!thread, "Failed to create a thread.\n"); + ret = WaitForSingleObject(thread, 1000); + todo_wine ok(ret == WAIT_TIMEOUT, "Expected timeout.\n"); + ID2D1Multithread_Leave(multithread); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + + ID2D1Multithread_Release(multithread); + release_test_context(&ctx); } #define check_system_properties(effect, is_builtin) check_system_properties_(__LINE__, effect, is_builtin) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4452
From: Brendan McGrath <bmcgrath(a)codeweavers.com> Tests that Direct2D can access D2D exclusive resources whilst the Direct 2D lock is held if the factory type is single threaded --- dlls/d2d1/tests/d2d1.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 1e4d77e1c84..3a8d97162fa 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -10784,6 +10784,24 @@ static void test_mt_factory(BOOL d3d11) ID2D1Multithread_Release(multithread); release_test_context(&ctx); + + if (!init_test_context(&ctx, d3d11)) + return; + + hr = ID2D1Factory_QueryInterface(ctx.factory, &IID_ID2D1Multithread, (void **)&multithread); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + ID2D1Multithread_Enter(multithread); + thread = CreateThread(NULL, 0, mt_factory_test_thread_draw_func, ctx.rt, 0, NULL); + ok(!!thread, "Failed to create a thread.\n"); + ret = WaitForSingleObject(thread, 1000); + ok(ret == WAIT_OBJECT_0, "Didn't expect timeout.\n"); + ID2D1Multithread_Leave(multithread); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + + ID2D1Multithread_Release(multithread); + release_test_context(&ctx); } #define check_system_properties(effect, is_builtin) check_system_properties_(__LINE__, effect, is_builtin) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4452
From: Brendan McGrath <bmcgrath(a)codeweavers.com> Ensures the Direct 2D lock is held before attempting to access Direct 2D exclusive resources. This fixes periodic crashes in PowerPoint 365 --- dlls/d2d1/d2d1_private.h | 7 ++++++- dlls/d2d1/device.c | 23 +++++++++++++++++++++++ dlls/d2d1/factory.c | 15 +++++++++++++++ dlls/d2d1/tests/d2d1.c | 2 +- 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index 1dd5cdb0a39..6a780c6334f 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -159,6 +159,8 @@ enum d2d_device_context_target_type D2D_TARGET_COMMAND_LIST, }; +struct d2d_factory; + struct d2d_device_context { ID2D1DeviceContext1 ID2D1DeviceContext1_iface; @@ -171,6 +173,7 @@ struct d2d_device_context const struct d2d_device_context_ops *ops; ID2D1Factory *factory; + struct d2d_factory *factory_impl; struct d2d_device *device; ID3D11Device1 *d3d_device; ID3DDeviceContextState *d3d_state; @@ -678,12 +681,14 @@ struct d2d_effect_registration struct d2d_effect_properties properties; }; -struct d2d_factory; void d2d_effects_init_builtins(struct d2d_factory *factory); struct d2d_effect_registration * d2d_factory_get_registered_effect(ID2D1Factory *factory, const GUID *effect_id); void d2d_factory_register_effect(struct d2d_factory *factory, struct d2d_effect_registration *effect); +struct d2d_factory *d2d_factory_from_ID2D1Multithread(ID2D1Multithread *iface); +void d2d_factory_enter_cs(struct d2d_factory *factory); +void d2d_factory_leave_cs(struct d2d_factory *factory); struct d2d_transform_graph { diff --git a/dlls/d2d1/device.c b/dlls/d2d1/device.c index e5548878ee9..efaf4cb0ba6 100644 --- a/dlls/d2d1/device.c +++ b/dlls/d2d1/device.c @@ -115,6 +115,16 @@ static void d2d_clip_stack_pop(struct d2d_clip_stack *stack) --stack->count; } +static inline void d2d_device_context_enter_cs(struct d2d_device_context *context) +{ + if (context->factory_impl) d2d_factory_enter_cs(context->factory_impl); +} + +static inline void d2d_device_context_leave_cs(struct d2d_device_context *context) +{ + if (context->factory_impl) d2d_factory_leave_cs(context->factory_impl); +} + static void d2d_device_context_draw(struct d2d_device_context *render_target, enum d2d_shape_type shape_type, ID3D11Buffer *ib, unsigned int index_count, ID3D11Buffer *vb, unsigned int vb_stride, struct d2d_brush *brush, struct d2d_brush *opacity_brush) @@ -135,6 +145,8 @@ static void d2d_device_context_draw(struct d2d_device_context *render_target, en vp.MinDepth = 0.0f; vp.MaxDepth = 1.0f; + d2d_device_context_enter_cs(render_target); + ID3D11Device1_GetImmediateContext1(device, &context); ID3D11DeviceContext1_SwapDeviceContextState(context, render_target->d3d_state, &prev_state); @@ -188,6 +200,8 @@ static void d2d_device_context_draw(struct d2d_device_context *render_target, en ID3D11DeviceContext1_SwapDeviceContextState(context, prev_state, NULL); ID3D11DeviceContext1_Release(context); ID3DDeviceContextState_Release(prev_state); + + d2d_device_context_leave_cs(render_target); } static void d2d_device_context_set_error(struct d2d_device_context *context, HRESULT code) @@ -3196,6 +3210,7 @@ static HRESULT d2d_device_context_init(struct d2d_device_context *render_target, IDWriteFactory *dwrite_factory; D3D11_RASTERIZER_DESC rs_desc; D3D11_BUFFER_DESC buffer_desc; + ID2D1Multithread *multithread; unsigned int i; HRESULT hr; @@ -4256,6 +4271,14 @@ static HRESULT d2d_device_context_init(struct d2d_device_context *render_target, render_target->device = device; ID2D1Device1_AddRef(&render_target->device->ID2D1Device1_iface); + ID2D1Factory_QueryInterface(render_target->factory, &IID_ID2D1Multithread, (void **)&multithread); + if (ID2D1Multithread_GetMultithreadProtected(multithread)) { + render_target->factory_impl = d2d_factory_from_ID2D1Multithread(multithread); + } else { + render_target->factory_impl = NULL; + } + ID2D1Multithread_Release(multithread); + render_target->outer_unknown = outer_unknown ? outer_unknown : &render_target->IUnknown_iface; render_target->ops = ops; diff --git a/dlls/d2d1/factory.c b/dlls/d2d1/factory.c index 3b2b1f5e4ec..4a3be704a2b 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -68,6 +68,11 @@ static inline struct d2d_factory *impl_from_ID2D1Multithread(ID2D1Multithread *i return CONTAINING_RECORD(iface, struct d2d_factory, ID2D1Multithread_iface); } +struct d2d_factory *d2d_factory_from_ID2D1Multithread(ID2D1Multithread *iface) +{ + return impl_from_ID2D1Multithread(iface); +} + static BOOL WINAPI d2d_factory_builtins_initonce(INIT_ONCE *once, void *param, void **context) { d2d_effects_init_builtins(param); @@ -1173,6 +1178,16 @@ static void STDMETHODCALLTYPE d2d_factory_mt_Leave(ID2D1Multithread *iface) LeaveCriticalSection(&factory->cs); } +void d2d_factory_enter_cs(struct d2d_factory *factory) +{ + EnterCriticalSection(&factory->cs); +} + +void d2d_factory_leave_cs(struct d2d_factory *factory) +{ + LeaveCriticalSection(&factory->cs); +} + static BOOL STDMETHODCALLTYPE d2d_factory_st_GetMultithreadProtected(ID2D1Multithread *iface) { return FALSE; diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 3a8d97162fa..0a4d81fe857 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -10777,7 +10777,7 @@ static void test_mt_factory(BOOL d3d11) thread = CreateThread(NULL, 0, mt_factory_test_thread_draw_func, ctx.rt, 0, NULL); ok(!!thread, "Failed to create a thread.\n"); ret = WaitForSingleObject(thread, 1000); - todo_wine ok(ret == WAIT_TIMEOUT, "Expected timeout.\n"); + ok(ret == WAIT_TIMEOUT, "Expected timeout.\n"); ID2D1Multithread_Leave(multithread); WaitForSingleObject(thread, INFINITE); CloseHandle(thread); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4452
On Thu Nov 23 19:14:15 2023 +0000, Brendan McGrath wrote:
changed this line in [version 6 of the diff](/wine/wine/-/merge_requests/4452/diffs?diff_id=85641&start_sha=40bf7f9c2997b24c826dddd5ac4895c361763f2a#0a5641ba01f61e9eda969890860e19c2269a989a_1168_1168) Agree - that's done
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53585
On Thu Nov 23 19:02:38 2023 +0000, Brendan McGrath wrote:
My apologies for misunderstanding previously, I thought the intent for using `d2d_factory` was just to hide the details of the factory locking mechanism from device. I think I needed a better understanding on the why. When you say "use it for everything": did you want me to remove the existing `ID2D1Factory *factory`? And then replace all uses of it with the `d2d_factory`. So I would effectively mirror the public API, but have it use `d2d_factory` instead (declaring them in `d2d1_private.h`). And then to create inline helpers using the `d2d_factory`, I would need to move the `d2d_factory` definition in to `d2d1_private.h`. Let me know if I've got that right. As an alternative, maybe I could store the `ID2D1Multithread` instead and make `ID2D1Multithread_Enter` and `ID2D1Multithread_Leave` calls in `d2d_device_context_draw`. It does add the overhead of the indirect call to `EnterCriticalSection`, but it would be insignificant compared to the `EnterCriticalSection` call itself (not to mention all the indirect ID3D11 calls that are already in `d2d_device_context_draw`).
This approach has the advantage of leaving the factory implementation in `factory.c` (i.e. not leaking it in to `d2d1_private.h`) and at least removes the call to `ID2D1Factory_QueryInterface` in every draw. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53588
On Thu Nov 23 20:25:26 2023 +0000, Brendan McGrath wrote:
As an alternative, maybe I could store the `ID2D1Multithread` instead and make `ID2D1Multithread_Enter` and `ID2D1Multithread_Leave` calls in `d2d_device_context_draw`. It does add the overhead of the indirect call to `EnterCriticalSection`, but it would be insignificant compared to the `EnterCriticalSection` call itself (not to mention all the indirect ID3D11 calls that are already in `d2d_device_context_draw`). This approach has the advantage of leaving the factory implementation in `factory.c` (i.e. not leaking it in to `d2d1_private.h`) and at least removes the call to `ID2D1Factory_QueryInterface` in every draw. I see now that changing factory field will create a larger diff, affecting every object creation method basically. Doing that we'll have to start with d2d_device I guess. For now let's go back then, and have a cs with unsafe_impl_* on context creation. You can move d2d_factory to the header, it's not a problem. I think it makes sense to have it there anyway.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53681
participants (3)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Nikolay Sivov (＠nsivov)