st/wgl: refactor framebuffer locking code

Split the old stw_framebuffer_reference() function into two new
functions: stw_framebuffer_reference_locked() which increments
the refcount and stw_framebuffer_release_locked() which decrements
the refcount and destroys the buffer when the count hits zero.

Original patch by Jose.  Modified by Brian (clean-ups, lock assertion
checks, etc).

Reviewed-by: José Fonseca <jfonseca@vmware.com>

diff --git a/src/gallium/state_trackers/wgl/stw_context.c b/src/gallium/state_trackers/wgl/stw_context.c
index 9971f95..b1e5f5e 100644 (file)
--- a/src/gallium/state_trackers/wgl/stw_context.c
+++ b/src/gallium/state_trackers/wgl/stw_context.c
@@ -390,7 +390,6 @@ stw_make_current(HDC hdc, DHGLRC dhglrc)
 {
    struct stw_context *old_ctx = NULL;
    struct stw_context *ctx = NULL;
-   struct stw_framebuffer *fb = NULL;
    BOOL ret = FALSE;
 
    if (!stw_dev)
@@ -409,6 +408,7 @@ stw_make_current(HDC hdc, DHGLRC dhglrc)
    }
 
    if (dhglrc) {
+      struct stw_framebuffer *fb = NULL;
       stw_lock_contexts(stw_dev);
       ctx = stw_lookup_context_locked( dhglrc );
       stw_unlock_contexts(stw_dev);
@@ -416,6 +416,7 @@ stw_make_current(HDC hdc, DHGLRC dhglrc)
          goto fail;
       }
 
+      /* This call locks fb's mutex */
       fb = stw_framebuffer_from_hdc( hdc );
       if (fb) {
          stw_framebuffer_update(fb);
@@ -434,6 +435,7 @@ stw_make_current(HDC hdc, DHGLRC dhglrc)
       }
 
       if (fb->iPixelFormat != ctx->iPixelFormat) {
+         stw_framebuffer_unlock(fb);
          SetLastError(ERROR_INVALID_PIXEL_FORMAT);
          goto fail;
       }
@@ -441,36 +443,53 @@ stw_make_current(HDC hdc, DHGLRC dhglrc)
       /* Bind the new framebuffer */
       ctx->hdc = hdc;
 
+      struct stw_framebuffer *old_fb = ctx->current_framebuffer;
+      if (old_fb != fb) {
+         stw_framebuffer_reference_locked(fb);
+         ctx->current_framebuffer = fb;
+      }
+      stw_framebuffer_unlock(fb);
+
       /* Note: when we call this function we will wind up in the
        * stw_st_framebuffer_validate_locked() function which will incur
        * a recursive fb->mutex lock.
        */
       ret = stw_dev->stapi->make_current(stw_dev->stapi, ctx->st,
                                          fb->stfb, fb->stfb);
-      stw_framebuffer_reference(&ctx->current_framebuffer, fb);
-   } else {
-      ret = stw_dev->stapi->make_current(stw_dev->stapi, NULL, NULL, NULL);
-   }
 
-fail:
+      if (old_fb && old_fb != fb) {
+         stw_lock_framebuffers(stw_dev);
+         stw_framebuffer_lock(old_fb);
+         stw_framebuffer_release_locked(old_fb);
+         stw_unlock_framebuffers(stw_dev);
+      }
 
-   if (fb) {
-      stw_framebuffer_unlock(fb);
-   }
+fail:
+      /* fb must be unlocked at this point. */
+      assert(!stw_own_mutex(&fb->mutex));
 
-   /* On failure, make the thread's current rendering context not current
-    * before returning.
-    */
-   if (!ret) {
-      stw_dev->stapi->make_current(stw_dev->stapi, NULL, NULL, NULL);
-      ctx = NULL;
+      /* On failure, make the thread's current rendering context not current
+       * before returning.
+       */
+      if (!ret) {
+         stw_make_current(NULL, 0);
+      }
+   } else {
+      ret = stw_dev->stapi->make_current(stw_dev->stapi, NULL, NULL, NULL);
    }
 
    /* Unreference the previous framebuffer if any. It must be done after
     * make_current, as it can be referenced inside.
     */
    if (old_ctx && old_ctx != ctx) {
-      stw_framebuffer_reference(&old_ctx->current_framebuffer, NULL);
+      struct stw_framebuffer *old_fb = old_ctx->current_framebuffer;
+      if (old_fb) {
+         old_ctx->current_framebuffer = NULL;
+         stw_lock_framebuffers(stw_dev);
+         stw_framebuffer_lock(old_fb);
+         stw_framebuffer_release_locked(old_fb);
+         stw_unlock_framebuffers(stw_dev);
+      }
    }
 
    return ret;

diff --git a/src/gallium/state_trackers/wgl/stw_device.h b/src/gallium/state_trackers/wgl/stw_device.h
index 3f0dffe..15d66a2 100644 (file)
--- a/src/gallium/state_trackers/wgl/stw_device.h
+++ b/src/gallium/state_trackers/wgl/stw_device.h
@@ -67,6 +67,10 @@ struct stw_device
    CRITICAL_SECTION ctx_mutex;
    struct handle_table *ctx_table;
    
+   /* TODO: use an atomic counter to track the number of locked
+    * stw_framebuffer objects.  Assert that the counter is zero when
+    * trying to lock this mutex.
+    */
    CRITICAL_SECTION fb_mutex;
    struct stw_framebuffer *fb_head;
    

diff --git a/src/gallium/state_trackers/wgl/stw_framebuffer.c b/src/gallium/state_trackers/wgl/stw_framebuffer.c
index b49bc22..1263615 100644 (file)
--- a/src/gallium/state_trackers/wgl/stw_framebuffer.c
+++ b/src/gallium/state_trackers/wgl/stw_framebuffer.c
@@ -67,14 +67,18 @@ stw_framebuffer_from_hwnd_locked(HWND hwnd)
  * Decrement the reference count on the given stw_framebuffer object.
  * If the reference count hits zero, destroy the object.
  *
- * Note: Both stw_dev::fb_mutex and stw_framebuffer::mutex must already
- * be locked.
+ * Note: Both stw_dev::fb_mutex and stw_framebuffer::mutex must already be
+ * locked.  After this function completes, the fb's mutex will be unlocked.
  */
-static void
-stw_framebuffer_destroy_locked(struct stw_framebuffer *fb)
+void
+stw_framebuffer_release_locked(struct stw_framebuffer *fb)
 {
    struct stw_framebuffer **link;
 
+   assert(fb);
+   assert(stw_own_mutex(&fb->mutex));
+   assert(stw_own_mutex(&stw_dev->fb_mutex));
+
    /* check the reference count */
    fb->refcnt--;
    if (fb->refcnt) {
@@ -223,7 +227,7 @@ stw_call_window_proc(int nCode, WPARAM wParam, LPARAM lParam)
       stw_lock_framebuffers(stw_dev);
       fb = stw_framebuffer_from_hwnd_locked( pParams->hwnd );
       if (fb)
-         stw_framebuffer_destroy_locked(fb);
+         stw_framebuffer_release_locked(fb);
       stw_unlock_framebuffers(stw_dev);
    }
 
@@ -304,33 +308,6 @@ stw_framebuffer_create(HDC hdc, int iPixelFormat)
 
 
 /**
- * Have ptr reference fb.  The referenced framebuffer should be locked.
- */
-void
-stw_framebuffer_reference(struct stw_framebuffer **ptr,
-                          struct stw_framebuffer *fb)
-{
-   struct stw_framebuffer *old_fb = *ptr;
-
-   if (old_fb == fb)
-      return;
-
-   if (fb)
-      fb->refcnt++;
-   if (old_fb) {
-      stw_lock_framebuffers(stw_dev);
-
-      stw_framebuffer_lock(old_fb);
-      stw_framebuffer_destroy_locked(old_fb);
-
-      stw_unlock_framebuffers(stw_dev);
-   }
-
-   *ptr = fb;
-}
-
-
-/**
  * Update the framebuffer's size if necessary.
  */
 void
@@ -369,7 +346,7 @@ stw_framebuffer_cleanup(void)
       next = fb->next;
 
       stw_framebuffer_lock(fb);
-      stw_framebuffer_destroy_locked(fb);
+      stw_framebuffer_release_locked(fb);
 
       fb = next;
    }

diff --git a/src/gallium/state_trackers/wgl/stw_framebuffer.h b/src/gallium/state_trackers/wgl/stw_framebuffer.h
index 0e2c61f..029fb9f 100644 (file)
--- a/src/gallium/state_trackers/wgl/stw_framebuffer.h
+++ b/src/gallium/state_trackers/wgl/stw_framebuffer.h
@@ -34,6 +34,7 @@
 #include <GL/wglext.h>
 
 #include "util/u_debug.h"
+#include "stw_st.h"
 
 
 struct pipe_resource;
@@ -131,9 +132,24 @@ struct stw_framebuffer
 struct stw_framebuffer *
 stw_framebuffer_create(HDC hdc, int iPixelFormat);
 
+
+/**
+ * Increase fb reference count.  The referenced framebuffer should be locked.
+ *
+ * It's not necessary to hold stw_dev::fb_mutex global lock.
+ */
+static inline void
+stw_framebuffer_reference_locked(struct stw_framebuffer *fb)
+{
+   if (fb) {
+      assert(stw_own_mutex(&fb->mutex));
+      fb->refcnt++;
+   }
+}
+
+
 void
-stw_framebuffer_reference(struct stw_framebuffer **ptr,
-                          struct stw_framebuffer *fb);
+stw_framebuffer_release_locked(struct stw_framebuffer *fb);
 
 /**
  * Search a framebuffer with a matching HWND.
@@ -179,6 +195,7 @@ static inline void
 stw_framebuffer_unlock(struct stw_framebuffer *fb)
 {
    assert(fb);
+   assert(stw_own_mutex(&fb->mutex));
    LeaveCriticalSection(&fb->mutex);
 }