From: José Fonseca <jfonseca@vmware.com>
Date: Mon, 6 Jul 2009 17:23:37 +0000 (+0100)
Subject: wgl: Make the stw_framebuffer destructions threadsafe.
X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=1068c15c61a6c76a2da04ed3ca136f0d49abed1d;p=mesa.git

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.
---

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 */