Re: Direct3D thread safety review

2007-05-19 Thread H. Verbeet

On 18/05/07, Stefan Dösinger [EMAIL PROTECTED] wrote:

* When a method needs locking, the lock is aquired after printing the trace,
but before checking error conditions(like windows).
* If a method doesn't need locking(e.g. unimplemented, or just a getter for
static info(GetCaps), or QI / AddRef / Release), then the lock is not held

Since ddraw, d3d8 and d3d9 need their own locking anyway, my plan is to
implement locking in them and assume synchronised calls in wined3d - ie no
wined3d locking. No 2 threads may call wined3d at the same time, and
ddraw/d3d8/d3d9 have to take care of that. That keeps the code simpler.


Makes sense to me.




Direct3D thread safety review

2007-05-18 Thread Stefan Dösinger
Hi,

After writing some tests for synchronisation in ddraw, I have new versions of 
my thread safety patches for ddraw. I want to show them for some ideas / 
comments before sending them in.

Basically windows ddraw seems to hold a DLL global lock whenever some code of 
the DLL is executed even in functions that wouldn't need it(like AddRef, can 
use interlocked addref), unrelated calls(2 different ddraw objects or 
surfaces) or before checking errornous params.

As per Alexandre's comments, we don't want to copy all this insane locking 
exactly(until we find an app that really needs this. Basically my 2 patches 
handle it like this(for the ddraw object only - all others will follow):

* When a method needs locking, the lock is aquired after printing the trace, 
but before checking error conditions(like windows).
* If a method doesn't need locking(e.g. unimplemented, or just a getter for 
static info(GetCaps), or QI / AddRef / Release), then the lock is not held

Since ddraw, d3d8 and d3d9 need their own locking anyway, my plan is to 
implement locking in them and assume synchronised calls in wined3d - ie no 
wined3d locking. No 2 threads may call wined3d at the same time, and 
ddraw/d3d8/d3d9 have to take care of that. That keeps the code simpler.

Attached is my test(threading.c), the locking for main and ddraw(merged it 
accidentally - need to split it up again. The plan is to continue like that 
for the rest of ddraw and d3d8/9

From e8a3a4850d6c21fffbb16af45a969bd92aa59e05 Mon Sep 17 00:00:00 2001
From: Stefan Doesinger [EMAIL PROTECTED]
Date: Fri, 18 May 2007 15:31:47 +0200
Subject: [PATCH] DDraw: Make the ddraw list lock a global dll lock

---
 dlls/ddraw/ddraw_private.h |3 +++
 dlls/ddraw/main.c  |   22 +++---
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h
index 9845102..da16aba 100644
--- a/dlls/ddraw/ddraw_private.h
+++ b/dlls/ddraw/ddraw_private.h
@@ -80,6 +80,9 @@ extern ULONG WINAPI D3D7CB_DestroySwapChain(IWineD3DSwapChain *pSwapChain);
 
 extern ULONG WINAPI D3D7CB_DestroyDepthStencilSurface(IWineD3DSurface *pSurface);
 
+/* Global critical section */
+CRITICAL_SECTION ddraw_cs;
+
 /*
  * IDirectDraw implementation structure
  */
diff --git a/dlls/ddraw/main.c b/dlls/ddraw/main.c
index 46c383a..6882ccf 100644
--- a/dlls/ddraw/main.c
+++ b/dlls/ddraw/main.c
@@ -59,15 +59,15 @@ WINED3DSURFTYPE DefaultSurfaceType = SURFACE_UNKNOWN;
 /* DDraw list and critical section */
 static struct list global_ddraw_list = LIST_INIT(global_ddraw_list);
 
-static CRITICAL_SECTION ddraw_list_cs;
-static CRITICAL_SECTION_DEBUG ddraw_list_cs_debug =
+CRITICAL_SECTION ddraw_cs;
+CRITICAL_SECTION_DEBUG ddraw_cs_debug =
 {
-0, 0, ddraw_list_cs,
-{ ddraw_list_cs_debug.ProcessLocksList, 
-ddraw_list_cs_debug.ProcessLocksList },
-0, 0, { (DWORD_PTR)(__FILE__ : ddraw_list_cs) }
+0, 0, ddraw_cs,
+{ ddraw_cs_debug.ProcessLocksList, 
+ddraw_cs_debug.ProcessLocksList },
+0, 0, { (DWORD_PTR)(__FILE__ : ddraw_cs) }
 };
-static CRITICAL_SECTION ddraw_list_cs = { ddraw_list_cs_debug, -1, 0, 0, 0, 0 };
+CRITICAL_SECTION ddraw_cs = { ddraw_cs_debug, -1, 0, 0, 0, 0 };
 
 /***
  *
@@ -319,9 +319,9 @@ DDRAW_Create(const GUID *guid,
 
 list_init(This-surface_list);
 
-EnterCriticalSection(ddraw_list_cs);
+EnterCriticalSection(ddraw_cs);
 list_add_head(global_ddraw_list, This-ddraw_list_entry);
-LeaveCriticalSection(ddraw_list_cs);
+LeaveCriticalSection(ddraw_cs);
 
 This-decls = HeapAlloc(GetProcessHeap(), 0, 0);
 if(!This-decls)
@@ -948,7 +948,7 @@ DllMain(HINSTANCE hInstDLL,
 void
 remove_ddraw_object(IDirectDrawImpl *ddraw)
 {
-EnterCriticalSection(ddraw_list_cs);
+EnterCriticalSection(ddraw_cs);
 list_remove(ddraw-ddraw_list_entry);
-LeaveCriticalSection(ddraw_list_cs);
+LeaveCriticalSection(ddraw_cs);
 }
-- 
1.5.0.7

From 7585984da84cfde23774e5d3b1272a9a52759013 Mon Sep 17 00:00:00 2001
From: Stefan Doesinger [EMAIL PROTECTED]
Date: Fri, 18 May 2007 17:06:11 +0200
Subject: [PATCH] DDraw: Hold the lock in IDirectDrawX methods

---
 dlls/ddraw/ddraw.c |  107 +---
 dlls/ddraw/ddraw_private.h |3 -
 dlls/ddraw/main.c  |   42 +++--
 3 files changed, 127 insertions(+), 25 deletions(-)

diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c
index 2d8dc8a..ce591f1 100644
--- a/dlls/ddraw/ddraw.c
+++ b/dlls/ddraw/ddraw.c
@@ -97,11 +97,17 @@ IDirectDrawImpl_QueryInterface(IDirectDraw7 *iface,
 
 TRACE((%p)-(%s,%p)\n, This, debugstr_guid(refiid), obj);
 
+/* Can change surface impl type */
+EnterCriticalSection(ddraw_cs);
+
 /* According to COM docs, if