[PATCH v3 0/3] MR4452: d2d1: Acquire lock before attempt to draw to device context
Fixes periodic crashes in PowerPoint 365 -- v3: 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 | 51 +++++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 58f905ca1dc..e49deea3993 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -1163,13 +1163,14 @@ 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, BOOL multithreaded) { + D2D1_FACTORY_TYPE factory_type = multithreaded ? D2D1_FACTORY_TYPE_MULTI_THREADED : D2D1_FACTORY_TYPE_SINGLE_THREADED; 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 +1179,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, BOOL multithreaded) { D2D1_RENDER_TARGET_PROPERTIES desc; @@ -1190,7 +1191,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, multithreaded); } #define release_test_context(ctx) release_test_context_(__LINE__, ctx) @@ -1216,8 +1217,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, FALSE) +#define init_test_multithreaded_context(ctx, d3d11) init_test_context_(__LINE__, ctx, d3d11, TRUE) +static BOOL init_test_context_(unsigned int line, struct d2d1_test_context *ctx, BOOL d3d11, BOOL multithreaded) { HRESULT hr; @@ -1237,7 +1239,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, multithreaded); if (!ctx->rt && d3d11) { todo_wine win_skip_(__FILE__, line)("Skipping d3d11 tests.\n"); @@ -4887,7 +4889,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, FALSE); ok(!!rt, "Failed to create render target.\n"); ID2D1RenderTarget_SetAntialiasMode(rt, D2D1_ANTIALIAS_MODE_ALIASED); @@ -10694,8 +10696,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 +10767,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 e49deea3993..f0e6f1ffb38 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -10785,6 +10785,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 | 21 +++++++++++++++++++++ dlls/d2d1/factory.c | 14 +++++++++++++- dlls/d2d1/tests/d2d1.c | 2 +- 4 files changed, 41 insertions(+), 3 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..75ac3a5b7c1 100644 --- a/dlls/d2d1/device.c +++ b/dlls/d2d1/device.c @@ -115,6 +115,14 @@ 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 +143,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 +198,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 +3208,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 +4269,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..f35bfe51ea1 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -63,11 +63,15 @@ static inline struct d2d_factory *unsafe_impl_from_ID2D1Factory(ID2D1Factory *if return CONTAINING_RECORD(iface, struct d2d_factory, ID2D1Factory3_iface); } -static inline struct d2d_factory *impl_from_ID2D1Multithread(ID2D1Multithread *iface) +inline struct d2d_factory *impl_from_ID2D1Multithread(ID2D1Multithread *iface) { 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 +1177,14 @@ 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 f0e6f1ffb38..3b7b59ef49f 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -10778,7 +10778,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 Wed Nov 22 23:52:51 2023 +0000, Nikolay Sivov wrote:
I would store "struct d2d_factory *" instead, and then you can have some helper to leave/enter with it. Done
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53478
On Thu Nov 23 03:18:20 2023 +0000, Nikolay Sivov wrote:
We'll need every commit to be meaningful, so in this case you'll need to squash the last two, because current the last one basically reverts what the one before it does. Done
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53479
participants (2)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)