[13/13] DDraw: Make the IDirectDrawImpl structure thread safe
Stefan Dösinger
stefan at codeweavers.com
Tue Feb 20 16:04:16 CST 2007
Another try on starting Direct3D thread safety.
This version of the patch avoids holding any locks while doing any calls that
could send Messages to applications. See the comment added to
IDirectDrawImpl_SetCooperativeLevel
-------------- next part --------------
From 5baa4d3a000974e1a3d53589c95ea921c3817e91 Mon Sep 17 00:00:00 2001
From: Stefan Doesinger <stefan at codeweavers.com>
Date: Tue, 13 Feb 2007 01:57:37 +0100
Subject: [PATCH] DDraw: Make the IDirectDrawImpl structure thread safe
This patch protects the IDirectDraw implementation against race conditions if the application sets the
DDSCL_MULTITHREADED cooperative level flag. Whenever a member of the implementation structure is
accessed then the lock is held. Holding the lock while calling wined3d calls that operate on windows is
avoided to prevent deadlocks due to ddraw calls from the message handler.
Other objects like surfaces and the 3ddevice will use the lock of the ddraw object.
---
dlls/ddraw/ddraw.c | 111 ++++++++++++++++++++++++++++++++++++-------
dlls/ddraw/ddraw_private.h | 8 +++
dlls/ddraw/direct3d.c | 15 ++++++-
dlls/ddraw/main.c | 2 +
4 files changed, 117 insertions(+), 19 deletions(-)
diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c
index 3270e41..5e23735 100644
--- a/dlls/ddraw/ddraw.c
+++ b/dlls/ddraw/ddraw.c
@@ -145,6 +145,11 @@ IDirectDrawImpl_QueryInterface(IDirectDraw7 *iface,
IsEqualGUID( &IID_IDirect3D3 , refiid ) ||
IsEqualGUID( &IID_IDirect3D7 , refiid ) )
{
+ /* Lock the object, we access and change private members when querying
+ * IDirect3D interfaces
+ */
+ DDOBJ_LOCK(This);
+
/* Check the surface implementation */
if(This->ImplType == SURFACE_UNKNOWN)
{
@@ -189,6 +194,7 @@ IDirectDrawImpl_QueryInterface(IDirectDraw7 *iface,
*obj = ICOM_INTERFACE(This, IDirect3D7);
TRACE(" returning Direct3D7 interface at %p.\n", *obj);
}
+ DDOBJ_UNLOCK(This);
}
/* Unknown interface */
@@ -248,8 +254,8 @@ IDirectDrawImpl_Destroy(IDirectDrawImpl *This)
{
/* Clear the cooplevel to restore window and display mode */
IDirectDraw7_SetCooperativeLevel(ICOM_INTERFACE(This, IDirectDraw7),
- NULL,
- DDSCL_NORMAL);
+ NULL,
+ DDSCL_NORMAL);
/* Destroy the device window if we created one */
if(This->devicewindow != 0)
@@ -268,6 +274,9 @@ IDirectDrawImpl_Destroy(IDirectDrawImpl *This)
IWineD3DDevice_Release(This->wineD3DDevice);
IWineD3D_Release(This->wineD3D);
+ if(This->hasCrit) DeleteCriticalSection(&This->crit);
+ This->hasCrit = FALSE;
+
/* Now free the object */
HeapFree(GetProcessHeap(), 0, This);
}
@@ -335,7 +344,15 @@ IDirectDrawImpl_Release(IDirectDraw7 *iface)
*
* These seem not really imporant for wine
* DDSCL_ALLOWREBOOT, DDSCL_NOWINDOWCHANGES, DDSCL_ALLOWMODEX,
- * DDSCL_MULTITHREDED
+ *
+ * There is no need to hold the lock in this method. It is required to be called
+ * from the thread that created the destination window, the same as SetDisplayMode
+ * and GetDisplayMode, two methods which depend on cooperative levels in theory.
+ * Creator functions also read the cooperative level, but they only determine
+ * if any cooplevel has been set. The interface is not thread safe until
+ * DDSCL_MULTIHREADED has been set. So if there is any race between this method
+ * and Create* methods then this is inherited by the design and not solvable
+ * with locking.
*
* Returns:
* DD_OK if the cooperative level was set successfully
@@ -397,11 +414,6 @@ IDirectDrawImpl_SetCooperativeLevel(IDirectDraw7 *iface,
}
This->focuswindow = hwnd;
- /* Won't use the hwnd param for anything else */
- hwnd = NULL;
-
- /* Use the focus window for drawing too */
- IWineD3DDevice_SetHWND(This->wineD3DDevice, This->focuswindow);
/* Destroy the device window, if we have one */
if(This->devicewindow)
@@ -409,6 +421,14 @@ IDirectDrawImpl_SetCooperativeLevel(IDirectDraw7 *iface,
DestroyWindow(This->devicewindow);
This->devicewindow = NULL;
}
+
+ /* Use the focus window for drawing too. SetHWND may change window parameters,
+ * and send signals, thus do not hold the lock
+ */
+ IWineD3DDevice_SetHWND(This->wineD3DDevice, hwnd);
+
+ /* Won't use the hwnd param for anything else */
+ hwnd = NULL;
}
/* DDSCL_NORMAL or DDSCL_FULLSCREEN | DDSCL_EXCLUSIVE */
if(cooplevel & DDSCL_NORMAL)
@@ -423,12 +443,12 @@ IDirectDrawImpl_SetCooperativeLevel(IDirectDraw7 *iface,
/* Switching from fullscreen? */
if(This->cooperative_level & DDSCL_FULLSCREEN)
{
- /* Restore the display mode */
- IDirectDraw7_RestoreDisplayMode(iface);
-
This->cooperative_level &= ~DDSCL_FULLSCREEN;
This->cooperative_level &= ~DDSCL_EXCLUSIVE;
This->cooperative_level &= ~DDSCL_ALLOWMODEX;
+
+ /* Restore the display mode */
+ IDirectDraw7_RestoreDisplayMode(iface);
}
/* Don't override focus windows or private device windows */
@@ -501,13 +521,23 @@ IDirectDrawImpl_SetCooperativeLevel(IDirectDraw7 *iface,
}
}
+ if(cooplevel & DDSCL_MULTITHREADED && !(This->cooperative_level & DDSCL_MULTITHREADED))
+ {
+ if(This->surfaces != 0)
+ {
+ FIXME("Setting DDSCL_MULTITHREADED, but surfaces exist already\n");
+ }
+
+ TRACE("(%p): Creating a critical section for the IDirectDrawImpl object\n", This);
+ InitializeCriticalSection(&This->crit);
+ This->hasCrit = TRUE;
+ }
+
/* Unhandled flags */
if(cooplevel & DDSCL_ALLOWREBOOT)
WARN("(%p) Unhandled flag DDSCL_ALLOWREBOOT, harmless\n", This);
if(cooplevel & DDSCL_ALLOWMODEX)
WARN("(%p) Unhandled flag DDSCL_ALLOWMODEX, harmless\n", This);
- if(cooplevel & DDSCL_MULTITHREADED)
- FIXME("(%p) Unhandled flag DDSCL_MULTITHREADED, Uh Oh...\n", This);
if(cooplevel & DDSCL_FPUSETUP)
WARN("(%p) Unhandled flag DDSCL_FPUSETUP, harmless\n", This);
if(cooplevel & DDSCL_FPUPRESERVE)
@@ -531,6 +561,8 @@ IDirectDrawImpl_SetCooperativeLevel(IDirectDraw7 *iface,
* It could mean that the current video mode should be left as-is. (But why
* call it then?)
*
+ * Does not need locking because it does not access any impl members.
+ *
* Params:
* Height, Width: Screen dimension
* BPP: Color depth in Bits per pixel
@@ -620,6 +652,7 @@ IDirectDrawImpl_RestoreDisplayMode(IDirectDraw7 *iface)
ICOM_THIS_FROM(IDirectDrawImpl, IDirectDraw7, iface);
TRACE("(%p)\n", This);
+ /* No need to lock for that, orig_width is set only at creation time */
return IDirectDraw7_SetDisplayMode(ICOM_INTERFACE(This, IDirectDraw7),
This->orig_width,
This->orig_height,
@@ -650,6 +683,7 @@ IDirectDrawImpl_GetCaps(IDirectDraw7 *iface,
{
ICOM_THIS_FROM(IDirectDrawImpl, IDirectDraw7, iface);
TRACE("(%p)->(%p,%p)\n", This, DriverCaps, HELCaps);
+ /* The caps are only set at startup and not changed, no need to lock */
/* One structure must be != NULL */
if( (!DriverCaps) && (!HELCaps) )
@@ -722,6 +756,7 @@ IDirectDrawImpl_GetDisplayMode(IDirectDraw7 *iface,
WINED3DDISPLAYMODE Mode;
DWORD Size;
TRACE("(%p)->(%p): Relay\n", This, DDSD);
+ /* Does not use structure members, no need to lock */
/* This seems sane */
if(!DDSD)
@@ -844,8 +879,10 @@ IDirectDrawImpl_GetVerticalBlankStatus(IDirectDraw7 *iface,
/* This looks sane, the MSDN suggests it too */
if(!status) return DDERR_INVALIDPARAMS;
+ DDOBJ_LOCK(This);
*status = This->fake_vblank;
This->fake_vblank = !This->fake_vblank;
+ DDOBJ_UNLOCK(This);
return DD_OK;
}
@@ -869,6 +906,7 @@ IDirectDrawImpl_GetAvailableVidMem(IDirectDraw7 *iface, DDSCAPS2 *Caps, DWORD *t
{
ICOM_THIS_FROM(IDirectDrawImpl, IDirectDraw7, iface);
TRACE("(%p)->(%p, %p, %p)\n", This, Caps, total, free);
+ /* Total vidmem is const, free comes from wineD3D, no need to lock */
if(TRACE_ON(ddraw))
{
@@ -1005,11 +1043,13 @@ static HRESULT WINAPI IDirectDrawImpl_GetScanLine(IDirectDraw7 *iface, DWORD *Sc
&Mode);
/* Fake the line sweeping of the monitor */
- /* FIXME: We should synchronize with a source to keep the refresh rate */
+ /* FIXME: We should synchronize with a source to keep the refresh rate */
+ DDOBJ_LOCK(This);
*Scanline = This->cur_scanline++;
/* Assume 20 scan lines in the vertical blank */
if (This->cur_scanline >= Mode.Height + 20)
This->cur_scanline = 0;
+ DDOBJ_UNLOCK(This);
return DD_OK;
}
@@ -1032,6 +1072,7 @@ IDirectDrawImpl_TestCooperativeLevel(IDirectDraw7 *iface)
ICOM_THIS_FROM(IDirectDrawImpl, IDirectDraw7, iface);
HRESULT hr;
TRACE("(%p)\n", This);
+ /* Only data from wineD3D is used, no need to lock */
/* Description from MSDN:
* For fullscreen apps return DDERR_NOEXCLUSIVEMODE if the user switched
@@ -1097,6 +1138,7 @@ IDirectDrawImpl_GetGDISurface(IDirectDraw7 *iface,
HRESULT hr;
DDSCAPS2 ddsCaps;
TRACE("(%p)->(%p)\n", This, GDISurface);
+ /* No need to lock, only data from wineD3D is used */
/* Get the back buffer from the wineD3DDevice and search its
* attached surfaces for the front buffer
@@ -1273,6 +1315,8 @@ IDirectDrawImpl_GetDeviceIdentifier(IDirectDraw7 *iface,
/* The DDGDI_GETHOSTIDENTIFIER returns the information about the 2D
* host adapter, if there's a secondary 3D adapter. This doesn't apply
* to any modern hardware, nor is it interesting for Wine, so ignore it
+ *
+ * The device identifier is global and const, no need to lock anything
*/
*DDDI = deviceidentifier;
@@ -1290,7 +1334,7 @@ IDirectDrawImpl_GetDeviceIdentifier(IDirectDraw7 *iface,
* Surface: Address to write the surface pointer to
*
* Returns:
- * Always returns DD_OK because it's a stub
+ * Always returns DDERR_NOTFOUND because it's a stub
*
*****************************************************************************/
static HRESULT WINAPI
@@ -1382,6 +1426,8 @@ IDirectDrawImpl_StartModeTest(IDirectDraw7 *iface,
* Enumeration callback for IDirectDrawImpl_RecreateAllSurfaces.
* It re-recreates the WineD3DSurface. It's pretty straightforward
*
+ * Assumes that the lock is held properly by the caller
+ *
*****************************************************************************/
HRESULT WINAPI
IDirectDrawImpl_RecreateSurfacesCallback(IDirectDrawSurface7 *surf,
@@ -1490,7 +1536,9 @@ IDirectDrawImpl_RecreateSurfacesCallback(IDirectDrawSurface7 *surf,
*
* A function, that converts all wineD3DSurfaces to the new implementation type
* It enumerates all surfaces with IWineD3DDevice::EnumSurfaces, creates a
- * new WineD3DSurface, copies the content and releases the old surface
+ * new WineD3DSurface, copies the content and releases the old surface.
+ *
+ * Assumes that the lock is held properly by the caller
*
*****************************************************************************/
static HRESULT
@@ -1525,6 +1573,8 @@ IDirectDrawImpl_RecreateAllSurfaces(IDirectDrawImpl *This)
* correct mipmap sublevel, and returns it to WineD3D.
* The surfaces are created already by IDirectDraw7::CreateSurface
*
+ * Assumes that the lock is held properly by IDirectDraw7::CreateSurface
+ *
* Params:
* With, Height: With and height of the surface
* Format: The requested format
@@ -1586,6 +1636,9 @@ ULONG WINAPI D3D7CB_DestroyDepthStencilSurface(IWineD3DSurface *pSurface) {
* A helper function for IDirectDraw7::CreateSurface. It creates a new surface
* with the passed parameters.
*
+ * Assumes that the lock on the IDirectDrawImpl object is held by
+ * IDirectDraw7::CreateSurface.
+ *
* Params:
* DDSD: Description of the surface to create
* Surf: Address to store the interface pointer at
@@ -2053,6 +2106,7 @@ IDirectDrawImpl_CreateSurface(IDirectDraw7 *iface,
&Mode);
if(FAILED(hr))
{
+ /* This->orig_bpp is never changed, no need to lock */
ERR("Failed to read display mode from wined3d\n");
switch(This->orig_bpp)
{
@@ -2195,7 +2249,9 @@ IDirectDrawImpl_CreateSurface(IDirectDraw7 *iface,
}
/* Create the first surface */
+ DDOBJ_LOCK(This);
hr = IDirectDrawImpl_CreateNewSurface(This, &desc2, &object, 0);
+ DDOBJ_UNLOCK(This);
if( hr != DD_OK)
{
ERR("IDirectDrawImpl_CreateNewSurface failed with %08x\n", hr);
@@ -2236,10 +2292,12 @@ IDirectDrawImpl_CreateSurface(IDirectDraw7 *iface,
if(desc2.dwHeight > 1) desc2.dwHeight /= 2;
}
+ DDOBJ_LOCK(This);
hr = IDirectDrawImpl_CreateNewSurface(This,
&desc2,
&object2,
level);
+ DDOBJ_UNLOCK(This);
if(hr != DD_OK)
{
/* This destroys and possibly created surfaces too */
@@ -2248,11 +2306,13 @@ IDirectDrawImpl_CreateSurface(IDirectDraw7 *iface,
}
/* Add the new surface to the complex attachment list */
+ DDOBJ_LOCK(This);
object2->first_complex = object;
object2->next_complex = NULL;
iterator = object;
while(iterator->next_complex) iterator = iterator->next_complex;
iterator->next_complex = object2;
+ DDOBJ_UNLOCK(This);
/* Remove the (possible) back buffer cap from the new surface description,
* because only one surface in the flipping chain is a back buffer, one
@@ -2307,6 +2367,7 @@ IDirectDrawImpl_CreateSurface(IDirectDraw7 *iface,
WINED3DFORMAT Format;
WINED3DPOOL Pool = WINED3DPOOL_DEFAULT;
+ DDOBJ_LOCK(This);
This->tex_root = object;
if(desc2.ddsCaps.dwCaps & DDSCAPS_MIPMAP)
@@ -2344,6 +2405,7 @@ IDirectDrawImpl_CreateSurface(IDirectDraw7 *iface,
(IUnknown *) ICOM_INTERFACE(object, IDirectDrawSurface7),
D3D7CB_CreateSurface );
This->tex_root = NULL;
+ DDOBJ_UNLOCK(This);
}
return hr;
@@ -2510,6 +2572,7 @@ IDirectDrawImpl_EnumSurfaces(IDirectDraw7 *iface,
return DDERR_INVALIDPARAMS;
/* Use the _SAFE enumeration, the app may destroy enumerated surfaces */
+ DDOBJ_LOCK(This);
LIST_FOR_EACH_SAFE(entry, entry2, &This->surface_list)
{
surf = LIST_ENTRY(entry, IDirectDrawSurfaceImpl, surface_list_entry);
@@ -2518,9 +2581,13 @@ IDirectDrawImpl_EnumSurfaces(IDirectDraw7 *iface,
desc = surf->surface_desc;
IDirectDrawSurface7_AddRef(ICOM_INTERFACE(surf, IDirectDrawSurface7));
if(Callback( ICOM_INTERFACE(surf, IDirectDrawSurface7), &desc, Context) != DDENUMRET_OK)
+ {
+ DDOBJ_UNLOCK(This);
return DD_OK;
+ }
}
}
+ DDOBJ_UNLOCK(This);
return DD_OK;
}
@@ -2530,7 +2597,9 @@ IDirectDrawImpl_EnumSurfaces(IDirectDraw7 *iface,
* Callback called by WineD3D to create Surfaces for render target usage
* This function takes the D3D target from the IDirectDrawImpl structure,
* and returns the WineD3DSurface. To avoid double usage, the surface
- * is marked as render target afterwards
+ * is marked as render target afterwards.
+ *
+ * Assumes that the lock is properly held by IDirectDraw7::CreateSurface
*
* Params
* device: The WineD3DDevice's parent
@@ -2638,7 +2707,10 @@ D3D7CB_CreateDepthStencilSurface(IUnknown *device,
* Callback function for WineD3D which creates a new WineD3DSwapchain
* interface. It also creates an IParent interface to store that pointer,
* so the WineD3DSwapchain has a parent and can be released when the D3D
- * device is destroyed
+ * device is destroyed.
+ *
+ * Assumes that the lock is properly held by IDirectDraw7::CreateSurface
+ *
*****************************************************************************/
static HRESULT WINAPI
D3D7CB_CreateAdditionalSwapChain(IUnknown *device,
@@ -2716,6 +2788,7 @@ IDirectDrawImpl_AttachD3DDevice(IDirectDrawImpl *This,
return hr;
}
+ DDOBJ_LOCK(This);
/* If there's no window, create a hidden window. WineD3D needs it */
if(window == 0)
{
@@ -2768,8 +2841,10 @@ IDirectDrawImpl_AttachD3DDevice(IDirectDrawImpl *This,
if(FAILED(hr))
{
This->wineD3DDevice = NULL;
+ DDOBJ_UNLOCK(This);
return hr;
}
+ DDOBJ_UNLOCK(This);
/* Create an Index Buffer parent */
TRACE("(%p) Successfully initialized 3D\n", This);
diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h
index 85f8ab2..28e5464 100644
--- a/dlls/ddraw/ddraw_private.h
+++ b/dlls/ddraw/ddraw_private.h
@@ -149,6 +149,10 @@ struct IDirectDrawImpl
*/
struct list surface_list;
LONG surfaces;
+
+ /* Multithreading synchronisation */
+ CRITICAL_SECTION crit;
+ BOOL hasCrit;
};
/* Declare the VTables. They can be found ddraw.c */
@@ -193,6 +197,10 @@ remove_ddraw_object(IDirectDrawImpl *ddraw);
/* The default surface type */
extern WINED3DSURFTYPE DefaultSurfaceType;
+/* Helper macro for multithreading, for all objects */
+#define DDOBJ_LOCK(obj) if(obj->hasCrit) EnterCriticalSection(&obj->crit)
+#define DDOBJ_UNLOCK(obj) if(obj->hasCrit) LeaveCriticalSection(&obj->crit)
+
/*****************************************************************************
* IDirectDrawSurface implementation structure
*****************************************************************************/
diff --git a/dlls/ddraw/direct3d.c b/dlls/ddraw/direct3d.c
index b716a60..4b011cf 100644
--- a/dlls/ddraw/direct3d.c
+++ b/dlls/ddraw/direct3d.c
@@ -743,12 +743,16 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
*Device = NULL;
+ /* Lock to be sure */
+ DDOBJ_LOCK(This);
/* Fail device creation if non-opengl surfaces are used */
if(This->ImplType != SURFACE_OPENGL)
{
+ DDOBJ_UNLOCK(This);
ERR("The application wants to create a Direct3D device, but non-opengl surfaces are set in the registry. Please set the surface implementation to opengl or autodetection to allow 3D rendering\n");
- /* We only hit this path if a default surface is set in the registry. Incorrect autodetection
+ /* We only hit this path if a default
+ * surface is set in the registry. Incorrect autodetection
* is caught in CreateSurface or QueryInterface
*/
return DDERR_NO3D;
@@ -757,6 +761,7 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
/* So far we can only create one device per ddraw object */
if(This->d3ddevice)
{
+ DDOBJ_UNLOCK(This);
FIXME("(%p): Only one Direct3D device per DirectDraw object supported\n", This);
return DDERR_INVALIDPARAMS;
}
@@ -764,6 +769,7 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
object = HeapAlloc(GetProcessHeap(), 0, sizeof(IDirect3DDeviceImpl));
if(!object)
{
+ DDOBJ_UNLOCK(This);
ERR("Out of memory when allocating a IDirect3DDevice implementation\n");
return DDERR_OUTOFMEMORY;
}
@@ -790,6 +796,7 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
IndexBufferParent = HeapAlloc(GetProcessHeap(), 0, sizeof(IParentImpl *));
if(!IndexBufferParent)
{
+ DDOBJ_UNLOCK(This);
ERR("Allocating memory for an index buffer parent failed\n");
HeapFree(GetProcessHeap(), 0, object);
return DDERR_OUTOFMEMORY;
@@ -813,6 +820,7 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
if(FAILED(hr))
{
+ DDOBJ_UNLOCK(This);
ERR("Failed to create an index buffer\n");
HeapFree(GetProcessHeap(), 0, object);
return hr;
@@ -853,7 +861,11 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
This->d3d_target->WineD3DSurface,
target->WineD3DSurface);
if(hr != D3D_OK)
+ {
+ DDOBJ_UNLOCK(This);
ERR("(%p) Error %08x setting the front and back buffer\n", This, hr);
+ return hr;
+ }
object->OffScreenTarget = TRUE;
}
@@ -887,6 +899,7 @@ IDirect3DImpl_7_CreateDevice(IDirect3D7 *iface,
IDirectDrawSurface7_Release(depthbuffer);
}
+ DDOBJ_UNLOCK(This);
return D3D_OK;
}
diff --git a/dlls/ddraw/main.c b/dlls/ddraw/main.c
index c754036..d87897e 100644
--- a/dlls/ddraw/main.c
+++ b/dlls/ddraw/main.c
@@ -753,6 +753,8 @@ HRESULT WINAPI DllCanUnloadNow(void)
* Callback function for the EnumSurfaces call in DllMain.
* Dumps some surface info and releases the surface
*
+ * No need to hold the lock because this is called when the last thread detaches
+ *
* Params:
* surf: The enumerated surface
* desc: it's description
--
1.4.4.3
More information about the wine-patches
mailing list