From a573c1344cfadcb99b15f080f55ce7b363b1b672 Mon Sep 17 00:00:00 2001 From: narzoul Date: Mon, 18 Jan 2016 23:23:50 +0100 Subject: [PATCH] Fixed GDI interworking deadlocks Because both the DirectDraw and GDI thread locks are held by the thread that initially calls beginGdiRendering, a deadlock can occur if a complex rendering operation uses synchronized worker threads for some subtasks and they also need access to the resources locked by the initial thread. To resolve this, the GDI thread lock is no longer held after beginGdiRendering returns, and the DirectDraw thread lock is only taken during the initial entry. However, this introduces another problem, because now the final endGdiRendering might not be called by the same thread that initially called beginGdiRendering. If this happens, a deadlock will occur because the initial thread is still holding the DirectDraw thread lock while the other thread is trying to acquire it to unlock the primary surface. To resolve this, the initial thread will always be the one to release the lock on the primary surface, waiting for other threads to finish using GDI DCs if necessary. This also means that other threads won't be able to create new cached DCs (as they would need the DD thread lock), so to prevent yet another deadlock, the initial thread always preallocates a number of DCs in the cache, and only the initial thread is allowed to extend the cache. --- DDrawCompat/CompatGdi.cpp | 122 +++++++++++++++++++++++++------ DDrawCompat/CompatGdi.h | 3 + DDrawCompat/CompatGdiDc.cpp | 13 +++- DDrawCompat/CompatGdiDcCache.cpp | 45 +++++++++++- DDrawCompat/CompatGdiDcCache.h | 1 + DDrawCompat/Config.h | 1 + 6 files changed, 157 insertions(+), 28 deletions(-) diff --git a/DDrawCompat/CompatGdi.cpp b/DDrawCompat/CompatGdi.cpp index fd4b374..8d2c040 100644 --- a/DDrawCompat/CompatGdi.cpp +++ b/DDrawCompat/CompatGdi.cpp @@ -10,7 +10,12 @@ namespace { - DWORD g_renderingDepth = 0; + DWORD g_renderingRefCount = 0; + DWORD g_ddLockThreadRenderingRefCount = 0; + DWORD g_ddLockThreadId = 0; + HANDLE g_ddUnlockBeginEvent = nullptr; + HANDLE g_ddUnlockEndEvent = nullptr; + bool g_isDelayedUnlockPending = false; FARPROC getProcAddress(HMODULE module, const char* procName) { @@ -60,6 +65,35 @@ namespace } return TRUE; } + + bool lockPrimarySurface() + { + Compat::origProcs.AcquireDDThreadLock(); + + DDSURFACEDESC2 desc = {}; + desc.dwSize = sizeof(desc); + if (FAILED(CompatDirectDrawSurface::s_origVtable.Lock( + CompatPrimarySurface::surface, nullptr, &desc, DDLOCK_WAIT, nullptr))) + { + Compat::origProcs.ReleaseDDThreadLock(); + return false; + } + + g_ddLockThreadId = GetCurrentThreadId(); + CompatGdiDcCache::setDdLockThreadId(g_ddLockThreadId); + CompatGdiDcCache::setSurfaceMemory(desc.lpSurface, desc.lPitch); + return true; + } + + void unlockPrimarySurface() + { + GdiFlush(); + CompatDirectDrawSurface::s_origVtable.Unlock( + CompatPrimarySurface::surface, nullptr); + RealPrimarySurface::update(); + + Compat::origProcs.ReleaseDDThreadLock(); + } } namespace CompatGdi @@ -67,14 +101,23 @@ namespace CompatGdi CRITICAL_SECTION g_gdiCriticalSection; std::unordered_map g_funcNames; - GdiScopedThreadLock::GdiScopedThreadLock() + GdiScopedThreadLock::GdiScopedThreadLock() : m_isLocked(true) { EnterCriticalSection(&g_gdiCriticalSection); } GdiScopedThreadLock::~GdiScopedThreadLock() { - LeaveCriticalSection(&g_gdiCriticalSection); + unlock(); + } + + void GdiScopedThreadLock::unlock() + { + if (m_isLocked) + { + LeaveCriticalSection(&g_gdiCriticalSection); + m_isLocked = false; + } } bool beginGdiRendering() @@ -84,40 +127,63 @@ namespace CompatGdi return false; } - Compat::origProcs.AcquireDDThreadLock(); - EnterCriticalSection(&g_gdiCriticalSection); + GdiScopedThreadLock gdiLock; - if (0 == g_renderingDepth) + if (0 == g_renderingRefCount) { - DDSURFACEDESC2 desc = {}; - desc.dwSize = sizeof(desc); - if (FAILED(CompatDirectDrawSurface::s_origVtable.Lock( - CompatPrimarySurface::surface, nullptr, &desc, DDLOCK_WAIT, nullptr))) + if (!lockPrimarySurface()) { - LeaveCriticalSection(&g_gdiCriticalSection); - Compat::origProcs.ReleaseDDThreadLock(); return false; } - CompatGdiDcCache::setSurfaceMemory(desc.lpSurface, desc.lPitch); + ++g_ddLockThreadRenderingRefCount; + } + else if (GetCurrentThreadId() == g_ddLockThreadId) + { + ++g_ddLockThreadRenderingRefCount; } - ++g_renderingDepth; + ++g_renderingRefCount; return true; } void endGdiRendering() { - --g_renderingDepth; - if (0 == g_renderingDepth) - { - GdiFlush(); - CompatDirectDrawSurface::s_origVtable.Unlock( - CompatPrimarySurface::surface, nullptr); - RealPrimarySurface::update(); - } + CompatGdi::GdiScopedThreadLock gdiLock; - LeaveCriticalSection(&g_gdiCriticalSection); - Compat::origProcs.ReleaseDDThreadLock(); + if (GetCurrentThreadId() == g_ddLockThreadId) + { + if (1 == g_renderingRefCount) + { + unlockPrimarySurface(); + g_ddLockThreadRenderingRefCount = 0; + g_renderingRefCount = 0; + } + else if (1 == g_ddLockThreadRenderingRefCount) + { + g_isDelayedUnlockPending = true; + gdiLock.unlock(); + WaitForSingleObject(g_ddUnlockBeginEvent, INFINITE); + unlockPrimarySurface(); + g_ddLockThreadRenderingRefCount = 0; + g_renderingRefCount = 0; + SetEvent(g_ddUnlockEndEvent); + } + else + { + --g_ddLockThreadRenderingRefCount; + --g_renderingRefCount; + } + } + else + { + --g_renderingRefCount; + if (1 == g_renderingRefCount && g_isDelayedUnlockPending) + { + SetEvent(g_ddUnlockBeginEvent); + WaitForSingleObject(g_ddUnlockEndEvent, INFINITE); + g_isDelayedUnlockPending = false; + } + } } void hookGdiFunction(const char* moduleName, const char* funcName, void*& origFuncPtr, void* newFuncPtr) @@ -146,6 +212,14 @@ namespace CompatGdi InitializeCriticalSection(&g_gdiCriticalSection); if (CompatGdiDcCache::init()) { + g_ddUnlockBeginEvent = CreateEvent(nullptr, FALSE, FALSE, nullptr); + g_ddUnlockEndEvent = CreateEvent(nullptr, FALSE, FALSE, nullptr); + if (!g_ddUnlockBeginEvent || !g_ddUnlockEndEvent) + { + Compat::Log() << "Failed to create the unlock events for GDI"; + return; + } + CompatGdiFunctions::installHooks(); CompatGdiWinProc::installHooks(); CompatGdiCaret::installHooks(); diff --git a/DDrawCompat/CompatGdi.h b/DDrawCompat/CompatGdi.h index df9e7fa..fd70667 100644 --- a/DDrawCompat/CompatGdi.h +++ b/DDrawCompat/CompatGdi.h @@ -17,6 +17,9 @@ namespace CompatGdi public: GdiScopedThreadLock(); ~GdiScopedThreadLock(); + void unlock(); + private: + bool m_isLocked; }; extern CRITICAL_SECTION g_gdiCriticalSection; diff --git a/DDrawCompat/CompatGdiDc.cpp b/DDrawCompat/CompatGdiDc.cpp index e2479e0..d606e8c 100644 --- a/DDrawCompat/CompatGdiDc.cpp +++ b/DDrawCompat/CompatGdiDc.cpp @@ -1,6 +1,7 @@ #include #include +#include "CompatGdi.h" #include "CompatGdiDc.h" #include "CompatGdiDcCache.h" #include "DDrawLog.h" @@ -110,8 +111,14 @@ namespace CompatGdiDc { HDC getDc(HDC origDc) { - if (!origDc || OBJ_DC != GetObjectType(origDc) || DT_RASDISPLAY != GetDeviceCaps(origDc, TECHNOLOGY) || - g_origDcToCompatDc.end() != std::find_if(g_origDcToCompatDc.begin(), g_origDcToCompatDc.end(), + if (!origDc || OBJ_DC != GetObjectType(origDc) || DT_RASDISPLAY != GetDeviceCaps(origDc, TECHNOLOGY)) + { + return nullptr; + } + + CompatGdi::GdiScopedThreadLock gdiLock; + + if (g_origDcToCompatDc.end() != std::find_if(g_origDcToCompatDc.begin(), g_origDcToCompatDc.end(), [=](const CompatDcMap::value_type& compatDc) { return compatDc.second.dc == origDc; })) { return nullptr; @@ -145,6 +152,8 @@ namespace CompatGdiDc void releaseDc(HDC origDc) { + CompatGdi::GdiScopedThreadLock gdiLock; + auto it = g_origDcToCompatDc.find(origDc); if (it == g_origDcToCompatDc.end()) { diff --git a/DDrawCompat/CompatGdiDcCache.cpp b/DDrawCompat/CompatGdiDcCache.cpp index 550bc72..5a8479b 100644 --- a/DDrawCompat/CompatGdiDcCache.cpp +++ b/DDrawCompat/CompatGdiDcCache.cpp @@ -13,6 +13,9 @@ namespace using CompatGdiDcCache::CachedDc; std::vector g_cache; + DWORD g_cacheSize = 0; + DWORD g_maxUsedCacheSize = 0; + DWORD g_ddLockThreadId = 0; IDirectDraw7* g_directDraw = nullptr; void* g_surfaceMemory = nullptr; @@ -97,6 +100,30 @@ namespace return surface; } + void extendCache() + { + if (g_cacheSize >= Config::preallocatedGdiDcCount) + { + LOG_ONCE("Warning: Preallocated GDI DC count is insufficient. This may lead to graphical issues."); + } + + if (GetCurrentThreadId() != g_ddLockThreadId) + { + return; + } + + for (DWORD i = 0; i < Config::preallocatedGdiDcCount; ++i) + { + CachedDc cachedDc = createCachedDc(); + if (!cachedDc.dc) + { + return; + } + g_cache.push_back(cachedDc); + ++g_cacheSize; + } + } + void releaseCachedDc(CachedDc cachedDc) { // Reacquire DD critical section that was temporarily released after IDirectDrawSurface7::GetDC @@ -122,6 +149,7 @@ namespace CompatGdiDcCache releaseCachedDc(cachedDc); } g_cache.clear(); + g_cacheSize = 0; } CachedDc getDc() @@ -134,12 +162,20 @@ namespace CompatGdiDcCache if (g_cache.empty()) { - cachedDc = createCachedDc(); + extendCache(); } - else + + if (!g_cache.empty()) { cachedDc = g_cache.back(); g_cache.pop_back(); + + const DWORD usedCacheSize = g_cacheSize - g_cache.size(); + if (usedCacheSize > g_maxUsedCacheSize) + { + g_maxUsedCacheSize = usedCacheSize; + Compat::Log() << "GDI used DC cache size: " << g_maxUsedCacheSize; + } } return cachedDc; @@ -156,6 +192,11 @@ namespace CompatGdiDcCache g_cache.push_back(cachedDc); } + void setDdLockThreadId(DWORD ddLockThreadId) + { + g_ddLockThreadId = ddLockThreadId; + } + void setSurfaceMemory(void* surfaceMemory, LONG pitch) { if (g_surfaceMemory == surfaceMemory && g_pitch == pitch) diff --git a/DDrawCompat/CompatGdiDcCache.h b/DDrawCompat/CompatGdiDcCache.h index 366155e..28aec75 100644 --- a/DDrawCompat/CompatGdiDcCache.h +++ b/DDrawCompat/CompatGdiDcCache.h @@ -18,5 +18,6 @@ namespace CompatGdiDcCache CachedDc getDc(); bool init(); void releaseDc(const CachedDc& cachedDc); + void setDdLockThreadId(DWORD ddLockThreadId); void setSurfaceMemory(void* surfaceMemory, LONG pitch); } diff --git a/DDrawCompat/Config.h b/DDrawCompat/Config.h index e976559..08d07cb 100644 --- a/DDrawCompat/Config.h +++ b/DDrawCompat/Config.h @@ -6,4 +6,5 @@ namespace Config { const DWORD minRefreshInterval = 1000 / 60; const DWORD minRefreshIntervalAfterFlip = 1000 / 10; + const DWORD preallocatedGdiDcCount = 4; }