Log when creating a second buffer mapping in a process
Bug: 8468756
Change-Id: Ia883f459ea9e2648ca4a0b5a6f09ded4f46f13b3
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 {
struct native_handle nativeHandle;
#endif
-
+
enum {
PRIV_FLAGS_FRAMEBUFFER = 0x00000001
};
@@ -74,16 +74,18 @@
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 @@
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 @@
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);
}