From 485e69809aef8bf301b6bf19c03dc2d7693aaa1a Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 5 May 2009 20:21:57 -0700 Subject: [PATCH] better documentation and implementation for lock/unlock --- include/hardware/gralloc.h | 12 ++++++ modules/gralloc/gralloc_priv.h | 12 ++++-- modules/gralloc/mapper.cpp | 94 +++++++++++++++++++++++++++++++----------- 3 files changed, 89 insertions(+), 29 deletions(-) diff --git a/include/hardware/gralloc.h b/include/hardware/gralloc.h index 6bde8f7..dba6fda 100644 --- 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 --- 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 --- 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; } -- 2.11.0