From 0d8f81a9848b47afd7f4a75cda9955a5cc77b465 Mon Sep 17 00:00:00 2001 From: Jesse Hall Date: Thu, 28 Mar 2013 16:51:25 -0700 Subject: Log when creating a second buffer mapping in a process Bug: 8468756 Change-Id: Ia883f459ea9e2648ca4a0b5a6f09ded4f46f13b3 --- modules/gralloc/gralloc_priv.h | 12 +++++++----- modules/gralloc/mapper.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) (limited to 'modules/gralloc') diff --git a/modules/gralloc/gralloc_priv.h b/modules/gralloc/gralloc_priv.h index 9d14fe0..86a5d52 100644 --- a/modules/gralloc/gralloc_priv.h +++ b/modules/gralloc/gralloc_priv.h @@ -61,7 +61,7 @@ struct private_handle_t : public native_handle { struct private_handle_t { struct native_handle nativeHandle; #endif - + enum { PRIV_FLAGS_FRAMEBUFFER = 0x00000001 }; @@ -74,16 +74,18 @@ struct private_handle_t { int size; int offset; - // FIXME: this should be out-of-line + // FIXME: the attributes below should be out-of-line int base; + int pid; #ifdef __cplusplus - static const int sNumInts = 5; + 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), flags(flags), size(size), offset(0), base(0) + fd(fd), magic(sMagic), flags(flags), size(size), offset(0), + base(0), pid(getpid()) { version = sizeof(native_handle); numInts = sNumInts; @@ -97,7 +99,7 @@ struct private_handle_t { const private_handle_t* hnd = (const private_handle_t*)h; if (!h || h->version != sizeof(native_handle) || h->numInts != sNumInts || h->numFds != sNumFds || - hnd->magic != sMagic) + hnd->magic != sMagic) { ALOGE("invalid gralloc handle (at %p)", h); return -EINVAL; diff --git a/modules/gralloc/mapper.cpp b/modules/gralloc/mapper.cpp index 8aadb4a..e57dba9 100644 --- a/modules/gralloc/mapper.cpp +++ b/modules/gralloc/mapper.cpp @@ -92,6 +92,46 @@ int gralloc_register_buffer(gralloc_module_t const* module, if (private_handle_t::validate(handle) < 0) return -EINVAL; + // *** WARNING WARNING WARNING *** + // + // If a buffer handle is passed from the process that allocated it to a + // different process, and then back to the allocator process, we will + // create a second mapping of the buffer. If the process reads and writes + // through both mappings, normal memory ordering guarantees may be + // violated, depending on the processor cache implementation*. + // + // If you are deriving a new gralloc implementation from this code, don't + // do this. A "real" gralloc should provide a single reference-counted + // mapping for each buffer in a process. + // + // In the current system, there is one case that needs a buffer to be + // registered in the same process that allocated it. The SurfaceFlinger + // process acts as the IGraphicBufferAlloc Binder provider, so all gralloc + // allocations happen in its process. After returning the buffer handle to + // the IGraphicBufferAlloc client, SurfaceFlinger free's its handle to the + // buffer (unmapping it from the SurfaceFlinger process). If + // SurfaceFlinger later acts as the producer end of the buffer queue the + // buffer belongs to, it will get a new handle to the buffer in response + // to IGraphicBufferProducer::requestBuffer(). Like any buffer handle + // received through Binder, the SurfaceFlinger process will register it. + // Since it already freed its original handle, it will only end up with + // one mapping to the buffer and there will be no problem. + // + // Currently SurfaceFlinger only acts as a buffer producer for a remote + // consumer when taking screenshots and when using virtual displays. + // + // Eventually, each application should be allowed to make its own gralloc + // allocations, solving the problem. Also, this ashmem-based gralloc + // should go away, replaced with a real ion-based gralloc. + // + // * Specifically, associative virtually-indexed caches are likely to have + // problems. Most modern L1 caches fit that description. + + private_handle_t* hnd = (private_handle_t*)handle; + ALOGD_IF(hnd->pid == getpid(), + "Registering a buffer in the process that created it. " + "This may cause memory ordering problems."); + void *vaddr; return gralloc_map(module, handle, &vaddr); } -- cgit v1.1