From 1068c15c61a6c76a2da04ed3ca136f0d49abed1d Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jos=C3=A9=20Fonseca?= Date: Mon, 6 Jul 2009 18:23:37 +0100 Subject: [PATCH] wgl: Make the stw_framebuffer destructions threadsafe. Ensure no other thread is accessing a framebuffer when it is being destroyed by acquiring both the global and per-framebuffer mutexes. Normal access only needs the global lock to walk the linked list and acquire the per-framebuffer mutex. --- .../state_trackers/wgl/shared/stw_context.c | 46 ++++--- .../state_trackers/wgl/shared/stw_device.c | 15 +-- .../state_trackers/wgl/shared/stw_device.h | 4 +- .../wgl/shared/stw_framebuffer.c | 120 ++++++++++++------ .../wgl/shared/stw_framebuffer.h | 83 ++++++++++-- 5 files changed, 186 insertions(+), 82 deletions(-) diff --git a/src/gallium/state_trackers/wgl/shared/stw_context.c b/src/gallium/state_trackers/wgl/shared/stw_context.c index 8393efbccf5..4968ecc692d 100644 --- a/src/gallium/state_trackers/wgl/shared/stw_context.c +++ b/src/gallium/state_trackers/wgl/shared/stw_context.c @@ -80,7 +80,7 @@ stw_copy_context( struct stw_context *dst; BOOL ret = FALSE; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->ctx_mutex ); src = stw_lookup_context_locked( hglrcSrc ); dst = stw_lookup_context_locked( hglrcDst ); @@ -93,7 +93,7 @@ stw_copy_context( (void) mask; } - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); return ret; } @@ -107,7 +107,7 @@ stw_share_lists( struct stw_context *ctx2; BOOL ret = FALSE; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->ctx_mutex ); ctx1 = stw_lookup_context_locked( hglrc1 ); ctx2 = stw_lookup_context_locked( hglrc2 ); @@ -117,7 +117,7 @@ stw_share_lists( ret = _mesa_share_state(ctx2->st->ctx, ctx1->st->ctx); } - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); return ret; } @@ -130,8 +130,10 @@ stw_viewport(GLcontext * glctx, GLint x, GLint y, struct stw_framebuffer *fb; fb = stw_framebuffer_from_hdc( ctx->hdc ); - if(fb) + if(fb) { stw_framebuffer_update(fb); + stw_framebuffer_release(fb); + } } UINT_PTR @@ -195,9 +197,9 @@ stw_create_layer_context( ctx->st->ctx->DriverCtx = ctx; ctx->st->ctx->Driver.Viewport = stw_viewport; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->ctx_mutex ); ctx->hglrc = handle_table_add(stw_dev->ctx_table, ctx); - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); if (!ctx->hglrc) goto no_hglrc; @@ -224,10 +226,10 @@ stw_delete_context( if (!stw_dev) return FALSE; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->ctx_mutex ); ctx = stw_lookup_context_locked(hglrc); handle_table_remove(stw_dev->ctx_table, hglrc); - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); if (ctx) { struct stw_context *curctx = stw_current_context(); @@ -254,9 +256,9 @@ stw_release_context( if (!stw_dev) return FALSE; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->ctx_mutex ); ctx = stw_lookup_context_locked( hglrc ); - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); if (!ctx) return FALSE; @@ -304,9 +306,9 @@ stw_make_current( HDC hdc, UINT_PTR hglrc ) { - struct stw_context *curctx; - struct stw_context *ctx; - struct stw_framebuffer *fb; + struct stw_context *curctx = NULL; + struct stw_context *ctx = NULL; + struct stw_framebuffer *fb = NULL; if (!stw_dev) goto fail; @@ -328,13 +330,13 @@ stw_make_current( return st_make_current( NULL, NULL, NULL ); } - pipe_mutex_lock( stw_dev->mutex ); - + pipe_mutex_lock( stw_dev->ctx_mutex ); ctx = stw_lookup_context_locked( hglrc ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); if(!ctx) goto fail; - fb = stw_framebuffer_from_hdc_locked( hdc ); + fb = stw_framebuffer_from_hdc( hdc ); if(!fb) { /* Applications should call SetPixelFormat before creating a context, * but not all do, and the opengl32 runtime seems to use a default pixel @@ -342,13 +344,11 @@ stw_make_current( */ int iPixelFormat = GetPixelFormat(hdc); if(iPixelFormat) - fb = stw_framebuffer_create_locked( hdc, iPixelFormat ); + fb = stw_framebuffer_create( hdc, iPixelFormat ); if(!fb) goto fail; } - pipe_mutex_unlock( stw_dev->mutex ); - if(fb->iPixelFormat != ctx->iPixelFormat) goto fail; @@ -367,12 +367,16 @@ stw_make_current( success: assert(fb); - if(fb) + if(fb) { stw_framebuffer_update(fb); + stw_framebuffer_release(fb); + } return TRUE; fail: + if(fb) + stw_framebuffer_release(fb); st_make_current( NULL, NULL, NULL ); return FALSE; } diff --git a/src/gallium/state_trackers/wgl/shared/stw_device.c b/src/gallium/state_trackers/wgl/shared/stw_device.c index ce466241463..0b6954915a6 100644 --- a/src/gallium/state_trackers/wgl/shared/stw_device.c +++ b/src/gallium/state_trackers/wgl/shared/stw_device.c @@ -69,8 +69,6 @@ stw_flush_frontbuffer(struct pipe_screen *screen, fb = stw_framebuffer_from_hdc( hdc ); /* fb can be NULL if window was destroyed already */ if (fb) { - pipe_mutex_lock( fb->mutex ); - #if DEBUG { struct pipe_surface *surface2; @@ -94,8 +92,7 @@ stw_flush_frontbuffer(struct pipe_screen *screen, if(fb) { stw_framebuffer_update(fb); - - pipe_mutex_unlock( fb->mutex ); + stw_framebuffer_release(fb); } } @@ -138,7 +135,8 @@ stw_init(const struct stw_winsys *stw_winsys) stw_dev->screen->flush_frontbuffer = &stw_flush_frontbuffer; - pipe_mutex_init( stw_dev->mutex ); + pipe_mutex_init( stw_dev->ctx_mutex ); + pipe_mutex_init( stw_dev->fb_mutex ); stw_dev->ctx_table = handle_table_create(); if (!stw_dev->ctx_table) { @@ -179,7 +177,7 @@ stw_cleanup(void) if (!stw_dev) return; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->ctx_mutex ); { /* Ensure all contexts are destroyed */ i = handle_table_get_first_handle(stw_dev->ctx_table); @@ -189,11 +187,12 @@ stw_cleanup(void) } handle_table_destroy(stw_dev->ctx_table); } - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->ctx_mutex ); stw_framebuffer_cleanup(); - pipe_mutex_destroy( stw_dev->mutex ); + pipe_mutex_destroy( stw_dev->fb_mutex ); + pipe_mutex_destroy( stw_dev->ctx_mutex ); stw_dev->screen->destroy(stw_dev->screen); diff --git a/src/gallium/state_trackers/wgl/shared/stw_device.h b/src/gallium/state_trackers/wgl/shared/stw_device.h index e097f1f71e0..e1bb9518dd1 100644 --- a/src/gallium/state_trackers/wgl/shared/stw_device.h +++ b/src/gallium/state_trackers/wgl/shared/stw_device.h @@ -57,10 +57,10 @@ struct stw_device unsigned pixelformat_count; unsigned pixelformat_extended_count; - pipe_mutex mutex; - + pipe_mutex ctx_mutex; struct handle_table *ctx_table; + pipe_mutex fb_mutex; struct stw_framebuffer *fb_head; #ifdef DEBUG diff --git a/src/gallium/state_trackers/wgl/shared/stw_framebuffer.c b/src/gallium/state_trackers/wgl/shared/stw_framebuffer.c index c0a42c1b7d4..b8956bb5509 100644 --- a/src/gallium/state_trackers/wgl/shared/stw_framebuffer.c +++ b/src/gallium/state_trackers/wgl/shared/stw_framebuffer.c @@ -45,6 +45,10 @@ #include "stw_tls.h" +/** + * Search the framebuffer with the matching HWND while holding the + * stw_dev::fb_mutex global lock. + */ static INLINE struct stw_framebuffer * stw_framebuffer_from_hwnd_locked( HWND hwnd ) @@ -52,13 +56,20 @@ stw_framebuffer_from_hwnd_locked( struct stw_framebuffer *fb; for (fb = stw_dev->fb_head; fb != NULL; fb = fb->next) - if (fb->hWnd == hwnd) + if (fb->hWnd == hwnd) { + pipe_mutex_lock(fb->mutex); break; + } return fb; } +/** + * Destroy this framebuffer. Both stw_dev::fb_mutex and stw_framebuffer::mutex + * must be held, by this order. Obviously no further access to fb can be done + * after this. + */ static INLINE void stw_framebuffer_destroy_locked( struct stw_framebuffer *fb ) @@ -74,12 +85,23 @@ stw_framebuffer_destroy_locked( st_unreference_framebuffer(fb->stfb); + pipe_mutex_unlock( fb->mutex ); + pipe_mutex_destroy( fb->mutex ); FREE( fb ); } +void +stw_framebuffer_release( + struct stw_framebuffer *fb) +{ + assert(fb); + pipe_mutex_unlock( fb->mutex ); +} + + static INLINE void stw_framebuffer_get_size( struct stw_framebuffer *fb ) { @@ -134,38 +156,29 @@ stw_call_window_proc( LPWINDOWPOS lpWindowPos = (LPWINDOWPOS)pParams->lParam; if((lpWindowPos->flags & SWP_SHOWWINDOW) || !(lpWindowPos->flags & SWP_NOSIZE)) { - pipe_mutex_lock( stw_dev->mutex ); - fb = stw_framebuffer_from_hwnd_locked( pParams->hwnd ); - pipe_mutex_unlock( stw_dev->mutex ); - + fb = stw_framebuffer_from_hwnd( pParams->hwnd ); if(fb) { - pipe_mutex_lock( fb->mutex ); /* Size in WINDOWPOS includes the window frame, so get the size * of the client area via GetClientRect. */ stw_framebuffer_get_size(fb); - pipe_mutex_unlock( fb->mutex ); + stw_framebuffer_release(fb); } } } else if (pParams->message == WM_DESTROY) { - pipe_mutex_lock( stw_dev->mutex ); - + pipe_mutex_lock( stw_dev->fb_mutex ); fb = stw_framebuffer_from_hwnd_locked( pParams->hwnd ); if(fb) stw_framebuffer_destroy_locked(fb); - - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->fb_mutex ); } return CallNextHookEx(tls_data->hCallWndProcHook, nCode, wParam, lParam); } -/** - * Create a new framebuffer object which will correspond to the given HDC. - */ struct stw_framebuffer * -stw_framebuffer_create_locked( +stw_framebuffer_create( HDC hdc, int iPixelFormat ) { @@ -194,8 +207,16 @@ stw_framebuffer_create_locked( pipe_mutex_init( fb->mutex ); + /* This is the only case where we lock the stw_framebuffer::mutex before + * stw_dev::fb_mutex, since no other thread can know about this framebuffer + * and we must prevent any other thread from destroying it before we return. + */ + pipe_mutex_lock( fb->mutex ); + + pipe_mutex_lock( stw_dev->fb_mutex ); fb->next = stw_dev->fb_head; stw_dev->fb_head = fb; + pipe_mutex_unlock( stw_dev->fb_mutex ); return fb; } @@ -205,8 +226,8 @@ BOOL stw_framebuffer_allocate( struct stw_framebuffer *fb) { - pipe_mutex_lock( fb->mutex ); - + assert(fb); + if(!fb->stfb) { const struct stw_pixelformat_info *pfi = fb->pfi; enum pipe_format colorFormat, depthFormat, stencilFormat; @@ -242,8 +263,6 @@ stw_framebuffer_allocate( fb->must_resize = TRUE; } - pipe_mutex_unlock( fb->mutex ); - return fb->stfb ? TRUE : FALSE; } @@ -280,24 +299,27 @@ stw_framebuffer_cleanup( void ) struct stw_framebuffer *fb; struct stw_framebuffer *next; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->fb_mutex ); fb = stw_dev->fb_head; while (fb) { next = fb->next; + + pipe_mutex_lock(fb->mutex); stw_framebuffer_destroy_locked(fb); + fb = next; } stw_dev->fb_head = NULL; - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->fb_mutex ); } /** * Given an hdc, return the corresponding stw_framebuffer. */ -struct stw_framebuffer * +static INLINE struct stw_framebuffer * stw_framebuffer_from_hdc_locked( HDC hdc ) { @@ -314,8 +336,10 @@ stw_framebuffer_from_hdc_locked( return stw_framebuffer_from_hwnd_locked(hwnd); for (fb = stw_dev->fb_head; fb != NULL; fb = fb->next) - if (fb->hDC == hdc) + if (fb->hDC == hdc) { + pipe_mutex_lock(fb->mutex); break; + } return fb; } @@ -330,9 +354,26 @@ stw_framebuffer_from_hdc( { struct stw_framebuffer *fb; - pipe_mutex_lock( stw_dev->mutex ); + pipe_mutex_lock( stw_dev->fb_mutex ); fb = stw_framebuffer_from_hdc_locked(hdc); - pipe_mutex_unlock( stw_dev->mutex ); + pipe_mutex_unlock( stw_dev->fb_mutex ); + + return fb; +} + + +/** + * Given an hdc, return the corresponding stw_framebuffer. + */ +struct stw_framebuffer * +stw_framebuffer_from_hwnd( + HWND hwnd ) +{ + struct stw_framebuffer *fb; + + pipe_mutex_lock( stw_dev->fb_mutex ); + fb = stw_framebuffer_from_hwnd_locked(hwnd); + pipe_mutex_unlock( stw_dev->fb_mutex ); return fb; } @@ -352,22 +393,19 @@ stw_pixelformat_set( if (index >= count) return FALSE; - pipe_mutex_lock( stw_dev->mutex ); - fb = stw_framebuffer_from_hdc_locked(hdc); if(fb) { /* SetPixelFormat must be called only once */ - pipe_mutex_unlock( stw_dev->mutex ); + stw_framebuffer_release( fb ); return FALSE; } - fb = stw_framebuffer_create_locked(hdc, iPixelFormat); + fb = stw_framebuffer_create(hdc, iPixelFormat); if(!fb) { - pipe_mutex_unlock( stw_dev->mutex ); return FALSE; } - pipe_mutex_unlock( stw_dev->mutex ); + stw_framebuffer_release( fb ); /* Some applications mistakenly use the undocumented wglSetPixelFormat * function instead of SetPixelFormat, so we call SetPixelFormat here to @@ -384,13 +422,16 @@ int stw_pixelformat_get( HDC hdc ) { + int iPixelFormat = 0; struct stw_framebuffer *fb; fb = stw_framebuffer_from_hdc(hdc); - if(!fb) - return 0; + if(fb) { + iPixelFormat = fb->iPixelFormat; + stw_framebuffer_release(fb); + } - return fb->iPixelFormat; + return iPixelFormat; } @@ -406,10 +447,10 @@ stw_swap_buffers( if (fb == NULL) return FALSE; - if (!(fb->pfi->pfd.dwFlags & PFD_DOUBLEBUFFER)) + if (!(fb->pfi->pfd.dwFlags & PFD_DOUBLEBUFFER)) { + stw_framebuffer_release(fb); return TRUE; - - pipe_mutex_lock( fb->mutex ); + } /* If we're swapping the buffer associated with the current context * we have to flush any pending rendering commands first. @@ -420,7 +461,7 @@ stw_swap_buffers( if(!st_get_framebuffer_surface( fb->stfb, ST_SURFACE_BACK_LEFT, &surface )) { /* FIXME: this shouldn't happen, but does on glean */ - pipe_mutex_unlock( fb->mutex ); + stw_framebuffer_release(fb); return FALSE; } @@ -434,8 +475,7 @@ stw_swap_buffers( stw_dev->stw_winsys->flush_frontbuffer( screen, surface, hdc ); stw_framebuffer_update(fb); - - pipe_mutex_unlock( fb->mutex ); + stw_framebuffer_release(fb); return TRUE; } diff --git a/src/gallium/state_trackers/wgl/shared/stw_framebuffer.h b/src/gallium/state_trackers/wgl/shared/stw_framebuffer.h index 759e06b8914..13d29f37e48 100644 --- a/src/gallium/state_trackers/wgl/shared/stw_framebuffer.h +++ b/src/gallium/state_trackers/wgl/shared/stw_framebuffer.h @@ -41,6 +41,23 @@ struct stw_pixelformat_info; */ struct stw_framebuffer { + /** + * This mutex has two purposes: + * - protect the access to the mutable data members below + * - prevent the the framebuffer from being deleted while being accessed. + * + * It is OK to lock this mutex while holding the stw_device::fb_mutex lock, + * but the opposite must never happen. + */ + pipe_mutex mutex; + + /* + * Immutable members. + * + * Note that even access to immutable members implies acquiring the mutex + * above, to prevent the framebuffer from being destroyed. + */ + HDC hDC; HWND hWnd; @@ -48,7 +65,10 @@ struct stw_framebuffer const struct stw_pixelformat_info *pfi; GLvisual visual; - pipe_mutex mutex; + /* + * Mutable members. + */ + struct st_framebuffer *stfb; /* FIXME: Make this work for multiple contexts bound to the same framebuffer */ @@ -56,15 +76,52 @@ struct stw_framebuffer unsigned width; unsigned height; - /** This is protected by stw_device::mutex, not the mutex above */ + /** + * This is protected by stw_device::fb_mutex, not the mutex above. + * + * Deletions must be done by first acquiring stw_device::fb_mutex, and then + * acquiring the stw_framebuffer::mutex of the framebuffer to be deleted. + * This ensures that nobody else is reading/writing to the. + * + * It is not necessary to aquire the mutex above to navigate the linked list + * given that deletions are done with stw_device::fb_mutex held, so no other + * thread can delete. + */ struct stw_framebuffer *next; }; + +/** + * Create a new framebuffer object which will correspond to the given HDC. + * + * This function will acquire stw_framebuffer::mutex. stw_framebuffer_release + * must be called when done + */ struct stw_framebuffer * -stw_framebuffer_create_locked( +stw_framebuffer_create( HDC hdc, int iPixelFormat ); +/** + * Search a framebuffer with a matching HWND. + * + * This function will acquire stw_framebuffer::mutex. stw_framebuffer_release + * must be called when done + */ +struct stw_framebuffer * +stw_framebuffer_from_hwnd( + HWND hwnd ); + +/** + * Search a framebuffer with a matching HDC. + * + * This function will acquire stw_framebuffer::mutex. stw_framebuffer_release + * must be called when done + */ +struct stw_framebuffer * +stw_framebuffer_from_hdc( + HDC hdc ); + BOOL stw_framebuffer_allocate( struct stw_framebuffer *fb ); @@ -73,15 +130,19 @@ void stw_framebuffer_update( struct stw_framebuffer *fb); +/** + * Release stw_framebuffer::mutex lock. This framebuffer must not be accessed + * after calling this function, as it may have been deleted by another thread + * in the meanwhile. + */ void -stw_framebuffer_cleanup(void); - -struct stw_framebuffer * -stw_framebuffer_from_hdc_locked( - HDC hdc ); +stw_framebuffer_release( + struct stw_framebuffer *fb); -struct stw_framebuffer * -stw_framebuffer_from_hdc( - HDC hdc ); +/** + * Cleanup any existing framebuffers when exiting application. + */ +void +stw_framebuffer_cleanup(void); #endif /* STW_FRAMEBUFFER_H */ -- 2.30.2