[PATCH v2 0/4] MR4452: Draft: d2d1: Acquire lock before attempt draw to device context
CW bug 22597 -- v2: d2d1: Optimise acquisition of the lock 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/device.c | 8 ++++++++ dlls/d2d1/tests/d2d1.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/d2d1/device.c b/dlls/d2d1/device.c index e5548878ee9..677968a980c 100644 --- a/dlls/d2d1/device.c +++ b/dlls/d2d1/device.c @@ -121,6 +121,8 @@ static void d2d_device_context_draw(struct d2d_device_context *render_target, en { struct d2d_shape_resources *shape_resources = &render_target->shape_resources[shape_type]; ID3DDeviceContextState *prev_state; + ID2D1Factory *factory = render_target->factory; + ID2D1Multithread *multithread; ID3D11Device1 *device = render_target->d3d_device; ID3D11DeviceContext1 *context; ID3D11Buffer *vs_cb = render_target->vs_cb, *ps_cb = render_target->ps_cb; @@ -135,6 +137,9 @@ static void d2d_device_context_draw(struct d2d_device_context *render_target, en vp.MinDepth = 0.0f; vp.MaxDepth = 1.0f; + ID2D1Factory_QueryInterface(factory, &IID_ID2D1Multithread, (void **)&multithread); + ID2D1Multithread_Enter(multithread); + ID3D11Device1_GetImmediateContext1(device, &context); ID3D11DeviceContext1_SwapDeviceContextState(context, render_target->d3d_state, &prev_state); @@ -188,6 +193,9 @@ 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); + + ID2D1Multithread_Leave(multithread); + ID2D1Multithread_Release(multithread); } static void d2d_device_context_set_error(struct d2d_device_context *context, HRESULT code) 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
From: Brendan McGrath <bmcgrath(a)codeweavers.com> Place the CRITICAL_SECTION pointer in the d2d_device_context struct so we don't have to QueryInterface(), Release(), and go through function pointers everytime we draw --- dlls/d2d1/d2d1_private.h | 2 ++ dlls/d2d1/device.c | 25 +++++++++++++++++++------ dlls/d2d1/factory.c | 5 +++++ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index 1dd5cdb0a39..4ad22a8a223 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -171,6 +171,7 @@ struct d2d_device_context const struct d2d_device_context_ops *ops; ID2D1Factory *factory; + CRITICAL_SECTION *cs; struct d2d_device *device; ID3D11Device1 *d3d_device; ID3DDeviceContextState *d3d_state; @@ -684,6 +685,7 @@ struct d2d_effect_registration * d2d_factory_get_registered_effect(ID2D1Factory const GUID *effect_id); void d2d_factory_register_effect(struct d2d_factory *factory, struct d2d_effect_registration *effect); +CRITICAL_SECTION* d2d_factory_mt_get_cs(ID2D1Multithread *iface); struct d2d_transform_graph { diff --git a/dlls/d2d1/device.c b/dlls/d2d1/device.c index 677968a980c..4127ea8f633 100644 --- a/dlls/d2d1/device.c +++ b/dlls/d2d1/device.c @@ -115,14 +115,20 @@ 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->cs) EnterCriticalSection(context->cs); +} + +static inline void d2d_device_context_leave_cs(struct d2d_device_context *context) { + if (context->cs) LeaveCriticalSection(context->cs); +} + 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) { struct d2d_shape_resources *shape_resources = &render_target->shape_resources[shape_type]; ID3DDeviceContextState *prev_state; - ID2D1Factory *factory = render_target->factory; - ID2D1Multithread *multithread; ID3D11Device1 *device = render_target->d3d_device; ID3D11DeviceContext1 *context; ID3D11Buffer *vs_cb = render_target->vs_cb, *ps_cb = render_target->ps_cb; @@ -137,8 +143,7 @@ static void d2d_device_context_draw(struct d2d_device_context *render_target, en vp.MinDepth = 0.0f; vp.MaxDepth = 1.0f; - ID2D1Factory_QueryInterface(factory, &IID_ID2D1Multithread, (void **)&multithread); - ID2D1Multithread_Enter(multithread); + d2d_device_context_enter_cs(render_target); ID3D11Device1_GetImmediateContext1(device, &context); ID3D11DeviceContext1_SwapDeviceContextState(context, render_target->d3d_state, &prev_state); @@ -194,8 +199,7 @@ static void d2d_device_context_draw(struct d2d_device_context *render_target, en ID3D11DeviceContext1_Release(context); ID3DDeviceContextState_Release(prev_state); - ID2D1Multithread_Leave(multithread); - ID2D1Multithread_Release(multithread); + d2d_device_context_leave_cs(render_target); } static void d2d_device_context_set_error(struct d2d_device_context *context, HRESULT code) @@ -3204,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; @@ -4264,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->cs = d2d_factory_mt_get_cs(multithread); + } else { + render_target->cs = 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..063fa6a8543 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -1155,6 +1155,11 @@ static BOOL STDMETHODCALLTYPE d2d_factory_mt_GetMultithreadProtected(ID2D1Multit return TRUE; } +CRITICAL_SECTION* d2d_factory_mt_get_cs(ID2D1Multithread *iface) { + struct d2d_factory *factory = impl_from_ID2D1Multithread(iface); + return &factory->cs; +} + static void STDMETHODCALLTYPE d2d_factory_mt_Enter(ID2D1Multithread *iface) { struct d2d_factory *factory = impl_from_ID2D1Multithread(iface); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4452
@hverbeet @zfigura. Thanks for the feedback. I've addressed both. I've added two new commits: 1. adds a test for when the Factory is created single threaded (as that shouldn't block); and 2. added the optimization suggested by @hverbeet I've also rebased and force pushed (as I removed the CodeWeaver bug references) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53232
Nikolay Sivov (@nsivov) commented about dlls/d2d1/device.c:
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->cs = d2d_factory_mt_get_cs(multithread); + } else { + render_target->cs = NULL; + } + ID2D1Multithread_Release(multithread); +
I would store "struct d2d_factory *" instead, and then you can have some helper to leave/enter with it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53464
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4452#note_53465
participants (3)
-
Brendan McGrath -
Brendan McGrath (＠redmcg)
-
Nikolay Sivov (＠nsivov)