better documentation and implementation for lock/unlock

diff --git a/include/hardware/gralloc.h b/include/hardware/gralloc.h
index 6bde8f7..dba6fda 100644 (file)
--- a/include/hardware/gralloc.h
+++ b/include/hardware/gralloc.h
@@ -131,6 +131,18 @@ struct gralloc_module_t {
      * If usage specifies GRALLOC_USAGE_SW_*, vaddr is filled with the address
      * of the buffer in virtual memory.
      *
+     * THREADING CONSIDERATIONS:
+     *
+     * It is legal for several different threads to lock a buffer from 
+     * read access, none of the threads are blocked.
+     * 
+     * However, locking a buffer simultaneously for write or read/write is
+     * undefined, but:
+     * - shall not result in termination of the process
+     * - shall not block the caller
+     * It is acceptable to return an error or to leave the buffer's content
+     * into an indeterminate state.
+     *
      * If the buffer was created with a usage mask incompatible with the
      * requested usage flags here, -EINVAL is returned. 
      * 

diff --git a/modules/gralloc/gralloc_priv.h b/modules/gralloc/gralloc_priv.h
index b0f2c6c..117999d 100644 (file)
--- a/modules/gralloc/gralloc_priv.h
+++ b/modules/gralloc/gralloc_priv.h
@@ -78,21 +78,25 @@ struct private_handle_t : public native_handle
         PRIV_FLAGS_FRAMEBUFFER = 0x00000001,
         PRIV_FLAGS_USES_PMEM   = 0x00000002,
         PRIV_FLAGS_MAPPED      = 0x00000004,    // FIXME: should be out-of-line
-        PRIV_FLAGS_LOCKED      = 0x00000008     // FIXME: should be out-of-line
     };
 
     int     fd;
     int     magic;
-    int     base;   // FIXME: should be out-of-line (meaningless with ipc)
     int     flags;
     int     size;
+    // FIXME: the attributes below should be out-of-line
+    int     base;
+    int     lockState;
+    int     writeOwner;
 
-    static const int sNumInts = 4;
+    static const int sNumInts = 6;
     static const int sNumFds = 1;
     static const int sMagic = 0x3141592;
 
     private_handle_t(int fd, int size, int flags) :
-        fd(fd), magic(sMagic), base(0), flags(flags), size(size) {
+        fd(fd), magic(sMagic), flags(flags), size(size), base(0),
+        lockState(0), writeOwner(0) 
+    {
         version = sizeof(native_handle);
         numInts = sNumInts;
         numFds = sNumFds;

diff --git a/modules/gralloc/mapper.cpp b/modules/gralloc/mapper.cpp
index 94be43b..bbcffac 100644 (file)
--- a/modules/gralloc/mapper.cpp
+++ b/modules/gralloc/mapper.cpp
@@ -220,8 +220,9 @@ int gralloc_register_buffer(gralloc_module_t const* module,
      */ 
     private_handle_t* hnd = (private_handle_t*)(handle);
     hnd->base = 0;
-    hnd->flags &= ~(private_handle_t::PRIV_FLAGS_LOCKED | 
-                    private_handle_t::PRIV_FLAGS_MAPPED);
+    hnd->flags &= ~private_handle_t::PRIV_FLAGS_MAPPED;
+    hnd->lockState = 0;
+    hnd->writeOwner = 0;
     
     return 0;
 }
@@ -240,8 +241,8 @@ int gralloc_unregister_buffer(gralloc_module_t const* module,
 
     private_handle_t* hnd = (private_handle_t*)(handle);
     
-    LOGE_IF(hnd->flags & private_handle_t::PRIV_FLAGS_LOCKED,
-            "handle %p still locked", hnd);
+    LOGE_IF(hnd->lockState, "handle %p still locked (state=%08x)",
+            hnd, hnd->lockState);
 
     if (hnd->flags & private_handle_t::PRIV_FLAGS_MAPPED) {
         if (!(hnd->flags & private_handle_t::PRIV_FLAGS_FRAMEBUFFER)) {
@@ -260,23 +261,51 @@ int gralloc_lock(gralloc_module_t const* module,
         int l, int t, int w, int h,
         void** vaddr)
 {
-    // FIXME: gralloc_lock() needs implementation
-
     if (private_handle_t::validate(handle) < 0)
         return -EINVAL;
 
     int err = 0;
     private_handle_t* hnd = (private_handle_t*)(handle);
+    int32_t current_value, new_value;
+    int retry;
+
+    do {
+        current_value = hnd->lockState;
+        new_value = current_value;
+        
+        if (current_value>>31) {
+            // already locked for write 
+            LOGE("handle %p already locked for write", handle);
+            return -EBUSY;
+        } else if (current_value<<1) {
+            // already locked for read
+            if (usage & (GRALLOC_USAGE_SW_WRITE_MASK | GRALLOC_USAGE_HW_RENDER)) {
+                LOGE("handle %p already locked for read", handle);
+                return -EBUSY;
+            } else {
+                // this is not an error, but for now we want to know
+                LOGD("%p already locked for read... count = %d", 
+                        handle, (current_value & ~(1<<31)));
+            }
+        }
 
-    // already locked
-    if (hnd->flags & private_handle_t::PRIV_FLAGS_LOCKED) {
-        LOGE("handle %p already locked", handle);
-        return -EBUSY;
-    }
+        // not currently locked
+        if (usage & (GRALLOC_USAGE_SW_WRITE_MASK | GRALLOC_USAGE_HW_RENDER)) {
+            // locking for write
+            new_value |= (1<<31);
+        }
+        new_value++;
+
+        retry = android_atomic_cmpxchg(current_value, new_value, 
+                (volatile int32_t*)&hnd->lockState);
+    } while (retry);
     
-    uint32_t mask = GRALLOC_USAGE_SW_READ_MASK | 
-                    GRALLOC_USAGE_SW_WRITE_MASK;
-    if ((usage & mask) && vaddr){
+    if (new_value>>31) {
+        // locking for write, store the tid
+        hnd->writeOwner = gettid();
+    }
+
+    if (usage & (GRALLOC_USAGE_SW_READ_MASK | GRALLOC_USAGE_SW_WRITE_MASK)) {
         if (hnd->flags & private_handle_t::PRIV_FLAGS_MAPPED) {
             *vaddr = (void*)hnd->base;
         } else {
@@ -284,25 +313,41 @@ int gralloc_lock(gralloc_module_t const* module,
             err = gralloc_map(module, handle, vaddr);
         }
     }
-    
-    hnd->flags |= private_handle_t::PRIV_FLAGS_LOCKED;
+
     return err;
 }
 
 int gralloc_unlock(gralloc_module_t const* module, 
         buffer_handle_t handle)
 {
-    // FIXME: gralloc_unlock() needs implementation
     if (private_handle_t::validate(handle) < 0)
         return -EINVAL;
 
     private_handle_t* hnd = (private_handle_t*)(handle);
+    int32_t current_value, new_value;
+
+    do {
+        current_value = hnd->lockState;
+        new_value = current_value;
+
+        if (current_value>>31) {
+            // locked for write
+            if (hnd->writeOwner == gettid()) {
+                hnd->writeOwner = 0;
+                new_value &= ~(1<<31);
+            }
+        }
+
+        if ((new_value<<1) == 0) {
+            LOGE("handle %p not locked", handle);
+            return -EINVAL;
+        }
+
+        new_value--;
+
+    } while (android_atomic_cmpxchg(current_value, new_value, 
+            (volatile int32_t*)&hnd->lockState));
 
-    // not locked
-    if (!(hnd->flags & private_handle_t::PRIV_FLAGS_LOCKED)) {
-        LOGE("handle %p is not locked", handle);
-        return -EINVAL;
-    }
 
     /* FOR DEBUGGING
     if (hnd->flags & private_handle_t::PRIV_FLAGS_MAPPED) {
@@ -310,8 +355,7 @@ int gralloc_unlock(gralloc_module_t const* module,
             hnd->flags &= ~private_handle_t::PRIV_FLAGS_MAPPED;
         }
     }
-    */
-    
-    hnd->flags &= ~private_handle_t::PRIV_FLAGS_LOCKED;
+     */
+
     return 0;
 }