OSDN Git Service

graphics: fix potential leaks for IAllocator
authorChia-I Wu <olv@google.com>
Tue, 22 Nov 2016 03:37:04 +0000 (11:37 +0800)
committerChia-I Wu <olv@google.com>
Wed, 23 Nov 2016 03:58:23 +0000 (11:58 +0800)
Introduce IAllocatorClient to manage resources owned by a client (e.g., SF
or VTS).  This makes sure there is no resource leak when SF or VTS
crashes.

This also fixes two unrelated bugs

 - sizeof(Buffer) != sizeof(void*) on 32-bit impl.
 - layerCount was not set to 1 in tests

Test: builds and boots
Change-Id: I67f5cdd64b97fb3ce1b931099c25f59cc8517f21

graphics/allocator/2.0/Android.bp
graphics/allocator/2.0/IAllocator.hal
graphics/allocator/2.0/IAllocatorClient.hal [new file with mode: 0644]
graphics/allocator/2.0/default/Gralloc.cpp
graphics/allocator/2.0/vts/functional/graphics_allocator_hidl_hal_test.cpp

index e26e9a3..f823570 100644 (file)
@@ -7,10 +7,12 @@ genrule {
     srcs: [
         "types.hal",
         "IAllocator.hal",
+        "IAllocatorClient.hal",
     ],
     out: [
         "android/hardware/graphics/allocator/2.0/types.cpp",
         "android/hardware/graphics/allocator/2.0/AllocatorAll.cpp",
+        "android/hardware/graphics/allocator/2.0/AllocatorClientAll.cpp",
     ],
 }
 
@@ -21,6 +23,7 @@ genrule {
     srcs: [
         "types.hal",
         "IAllocator.hal",
+        "IAllocatorClient.hal",
     ],
     out: [
         "android/hardware/graphics/allocator/2.0/types.h",
@@ -29,6 +32,11 @@ genrule {
         "android/hardware/graphics/allocator/2.0/BnAllocator.h",
         "android/hardware/graphics/allocator/2.0/BpAllocator.h",
         "android/hardware/graphics/allocator/2.0/BsAllocator.h",
+        "android/hardware/graphics/allocator/2.0/IAllocatorClient.h",
+        "android/hardware/graphics/allocator/2.0/IHwAllocatorClient.h",
+        "android/hardware/graphics/allocator/2.0/BnAllocatorClient.h",
+        "android/hardware/graphics/allocator/2.0/BpAllocatorClient.h",
+        "android/hardware/graphics/allocator/2.0/BsAllocatorClient.h",
     ],
 }
 
index 9a45444..00d07d5 100644 (file)
@@ -16,7 +16,7 @@
 
 package android.hardware.graphics.allocator@2.0;
 
-import android.hardware.graphics.common@1.0::PixelFormat;
+import IAllocatorClient;
 
 interface IAllocator {
     enum Capability : int32_t {
@@ -24,53 +24,18 @@ interface IAllocator {
         INVALID = 0,
 
         /*
-         * testAllocate will always return UNDEFINED unless this capability
-         * is supported.
+         * IAllocatorClient::testAllocate must always return UNDEFINED unless
+         * this capability is supported.
          */
         TEST_ALLOCATE = 1,
 
         /*
-         * layerCount must be 1 unless this capability is supported.
+         * IAllocatorClient::BufferDescriptorInfo::layerCount must be 1 unless
+         * this capability is supported.
          */
         LAYERED_BUFFERS = 2,
     };
 
-    struct BufferDescriptorInfo {
-        /*
-         * The width specifies how many columns of pixels must be in the
-         * allocated buffer, but does not necessarily represent the offset in
-         * columns between the same column in adjacent rows. The rows may be
-         * padded.
-         */
-        uint32_t width;
-
-       /*
-        * The height specifies how many rows of pixels must be in the
-        * allocated buffer.
-        */
-        uint32_t height;
-
-       /*
-        * The number of image layers that must be in the allocated buffer.
-        */
-        uint32_t layerCount;
-
-        /* Buffer pixel format. */
-        PixelFormat format;
-
-        /*
-         * Buffer producer usage mask; valid flags can be found in the
-         * definition of ProducerUsage.
-         */
-        uint64_t producerUsageMask;
-
-        /*
-         * Buffer consumer usage mask; valid flags can be found in the
-         * definition of ConsumerUsage.
-         */
-        uint64_t consumerUsageMask;
-    };
-
     /*
      * Provides a list of supported capabilities (as described in the
      * definition of Capability above). This list must not change after
@@ -95,110 +60,15 @@ interface IAllocator {
     dumpDebugInfo() generates (string debugInfo);
 
     /*
-     * Creates a new, opaque buffer descriptor.
+     * Creates a client of the allocator. All resources created by the client
+     * are owned by the client and are only visible to the client, unless they
+     * are exported by exportHandle.
      *
-     * @param descriptorInfo specifies the attributes of the buffer
-     *        descriptor.
      * @return error is NONE upon success. Otherwise,
-     *         BAD_VALUE when any attribute in descriptorInfo is invalid.
-     *         NO_RESOURCES when no more descriptors can currently be created.
-     * @return descriptor is the newly created buffer descriptor.
+     *         NO_RESOURCES when no more client can currently be created.
+     * @return client is the newly created client.
      */
     @entry
     @callflow(next="*")
-    createDescriptor(BufferDescriptorInfo descriptorInfo)
-          generates (Error error,
-                     BufferDescriptor descriptor);
-
-    /*
-     * Destroys an existing buffer descriptor.
-     *
-     * @param descriptor is the descriptor to destroy.
-     * @return error is either NONE or BAD_DESCRIPTOR.
-     */
-    @exit
-    @callflow(next="*")
-    destroyDescriptor(BufferDescriptor descriptor) generates (Error error);
-
-    /*
-     * Tests whether a buffer allocation can succeed, ignoring potential
-     * resource contention which might lead to a NO_RESOURCES error.
-     *
-     * @param descriptors is a list of buffer descriptors.
-     * @return error is NONE or NOT_SHARED upon success;
-     *         NONE when buffers can be created and share a backing store.
-     *         NOT_SHARED when buffers can be created but require more than a
-     *                    backing store.
-     *         Otherwise,
-     *         BAD_DESCRIPTOR when any of the descriptors is invalid.
-     *         UNSUPPORTED when any of the descriptors can never be satisfied.
-     *         UNDEFINED when TEST_ALLOCATE is not listed in getCapabilities.
-     */
-    @callflow(next="allocate")
-    testAllocate(vec<BufferDescriptor> descriptors) generates (Error error);
-
-    /*
-     * Attempts to allocate a list of buffers sharing a backing store.
-     *
-     * Each buffer will correspond to one of the descriptors passed into the
-     * function and will hold a reference to its backing store. If the device
-     * is unable to share the backing store between the buffers, it must
-     * attempt to allocate the buffers with different backing stores and
-     * return NOT_SHARED if it is successful.
-     *
-     * @param descriptors is the buffer descriptors to attempt to allocate.
-     * @return error is NONE or NOT_SHARED upon success;
-     *         NONE when buffers can be created and share a backing store.
-     *         NOT_SHARED when buffers can be created but require more than a
-     *                    backing store.
-     *         Otherwise,
-     *         BAD_DESCRIPTOR when any of the descriptors is invalid.
-     *         UNSUPPORTED when any of the descriptors can never be satisfied.
-     *         NO_RESOURCES when any of the buffers cannot be created at this
-     *                      time.
-     * @return buffers is the allocated buffers.
-     */
-    @callflow(next="exportHandle")
-    allocate(vec<BufferDescriptor> descriptors)
-        generates (Error error,
-                   vec<Buffer> buffers);
-
-    /*
-     * Frees a buffer.
-     *
-     * @param buffer is the buffer to be freed.
-     * @return error is NONE upon success. Otherwise,
-     *         BAD_BUFFER when the buffer is invalid.
-     */
-    @exit
-    @callflow(next="*")
-    free(Buffer buffer) generates (Error error);
-
-    /*
-     * Exports a buffer for use in other client libraries or for cross-process
-     * sharing.
-     *
-     * The exported handle is a handle to the backing store of the buffer, not
-     * to the buffer itself. It however may not hold any reference to the
-     * backing store and may be considered invalid by client libraries. To use
-     * it and, in most cases, to save it for later use, a client must make a
-     * clone of the handle and have the cloned handle hold a reference to the
-     * backing store. Such a cloned handle will stay valid even after the
-     * original buffer is freed. Refer to native_handle_clone and IMapper for
-     * how a handle is cloned and how a reference is added.
-     *
-     * @param descriptor is the descriptor used to allocate the buffer.
-     * @param buffer is the buffer to be exported.
-     * @return error is NONE upon success. Otherwise,
-     *         BAD_DESCRIPTOR when the descriptor is invalid.
-     *         BAD_BUFFER when the buffer is invalid.
-     *         BAD_VALUE when descriptor and buffer do not match.
-     *         NO_RESOURCES when the buffer cannot be exported at this time.
-     * @return bufferHandle is the exported handle.
-     */
-    @callflow(next="free")
-    exportHandle(BufferDescriptor descriptor,
-                 Buffer buffer)
-      generates (Error error,
-                 handle bufferHandle);
+    createClient() generates (Error error, IAllocatorClient client);
 };
diff --git a/graphics/allocator/2.0/IAllocatorClient.hal b/graphics/allocator/2.0/IAllocatorClient.hal
new file mode 100644 (file)
index 0000000..080e3ea
--- /dev/null
@@ -0,0 +1,165 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package android.hardware.graphics.allocator@2.0;
+
+import android.hardware.graphics.common@1.0::PixelFormat;
+
+interface IAllocatorClient {
+    struct BufferDescriptorInfo {
+        /*
+         * The width specifies how many columns of pixels must be in the
+         * allocated buffer, but does not necessarily represent the offset in
+         * columns between the same column in adjacent rows. The rows may be
+         * padded.
+         */
+        uint32_t width;
+
+       /*
+        * The height specifies how many rows of pixels must be in the
+        * allocated buffer.
+        */
+        uint32_t height;
+
+       /*
+        * The number of image layers that must be in the allocated buffer.
+        */
+        uint32_t layerCount;
+
+        /* Buffer pixel format. */
+        PixelFormat format;
+
+        /*
+         * Buffer producer usage mask; valid flags can be found in the
+         * definition of ProducerUsage.
+         */
+        uint64_t producerUsageMask;
+
+        /*
+         * Buffer consumer usage mask; valid flags can be found in the
+         * definition of ConsumerUsage.
+         */
+        uint64_t consumerUsageMask;
+    };
+
+    /*
+     * Creates a new, opaque buffer descriptor.
+     *
+     * @param descriptorInfo specifies the attributes of the buffer
+     *        descriptor.
+     * @return error is NONE upon success. Otherwise,
+     *         BAD_VALUE when any attribute in descriptorInfo is invalid.
+     *         NO_RESOURCES when no more descriptors can currently be created.
+     * @return descriptor is the newly created buffer descriptor.
+     */
+    @entry
+    @callflow(next="*")
+    createDescriptor(BufferDescriptorInfo descriptorInfo)
+          generates (Error error,
+                     BufferDescriptor descriptor);
+
+    /*
+     * Destroys an existing buffer descriptor.
+     *
+     * @param descriptor is the descriptor to destroy.
+     * @return error is either NONE or BAD_DESCRIPTOR.
+     */
+    @exit
+    @callflow(next="*")
+    destroyDescriptor(BufferDescriptor descriptor) generates (Error error);
+
+    /*
+     * Tests whether a buffer allocation can succeed, ignoring potential
+     * resource contention which might lead to a NO_RESOURCES error.
+     *
+     * @param descriptors is a list of buffer descriptors.
+     * @return error is NONE or NOT_SHARED upon success;
+     *         NONE when buffers can be created and share a backing store.
+     *         NOT_SHARED when buffers can be created but require more than a
+     *                    backing store.
+     *         Otherwise,
+     *         BAD_DESCRIPTOR when any of the descriptors is invalid.
+     *         UNSUPPORTED when any of the descriptors can never be satisfied.
+     *         UNDEFINED when TEST_ALLOCATE is not listed in getCapabilities.
+     */
+    @callflow(next="allocate")
+    testAllocate(vec<BufferDescriptor> descriptors) generates (Error error);
+
+    /*
+     * Attempts to allocate a list of buffers sharing a backing store.
+     *
+     * Each buffer must correspond to one of the descriptors passed into the
+     * function and must hold a reference to its backing store. If the device
+     * is unable to share the backing store between the buffers, it must
+     * attempt to allocate the buffers with different backing stores and
+     * return NOT_SHARED if it is successful.
+     *
+     * @param descriptors is the buffer descriptors to attempt to allocate.
+     * @return error is NONE or NOT_SHARED upon success;
+     *         NONE when buffers can be created and share a backing store.
+     *         NOT_SHARED when buffers can be created but require more than a
+     *                    backing store.
+     *         Otherwise,
+     *         BAD_DESCRIPTOR when any of the descriptors is invalid.
+     *         UNSUPPORTED when any of the descriptors can never be satisfied.
+     *         NO_RESOURCES when any of the buffers cannot be created at this
+     *                      time.
+     * @return buffers is the allocated buffers.
+     */
+    @callflow(next="exportHandle")
+    allocate(vec<BufferDescriptor> descriptors)
+        generates (Error error,
+                   vec<Buffer> buffers);
+
+    /*
+     * Frees a buffer.
+     *
+     * @param buffer is the buffer to be freed.
+     * @return error is NONE upon success. Otherwise,
+     *         BAD_BUFFER when the buffer is invalid.
+     */
+    @exit
+    @callflow(next="*")
+    free(Buffer buffer) generates (Error error);
+
+    /*
+     * Exports a buffer for use in other client libraries or for cross-process
+     * sharing.
+     *
+     * The exported handle is a handle to the backing store of the buffer, not
+     * to the buffer itself. It however may not hold any reference to the
+     * backing store and may be considered invalid by client libraries. To use
+     * it and, in most cases, to save it for later use, a client must make a
+     * clone of the handle and have the cloned handle hold a reference to the
+     * backing store. Such a cloned handle will stay valid even after the
+     * original buffer is freed. Refer to native_handle_clone and IMapper for
+     * how a handle is cloned and how a reference is added.
+     *
+     * @param descriptor is the descriptor used to allocate the buffer.
+     * @param buffer is the buffer to be exported.
+     * @return error is NONE upon success. Otherwise,
+     *         BAD_DESCRIPTOR when the descriptor is invalid.
+     *         BAD_BUFFER when the buffer is invalid.
+     *         BAD_VALUE when descriptor and buffer do not match.
+     *         NO_RESOURCES when the buffer cannot be exported at this time.
+     * @return bufferHandle is the exported handle.
+     */
+    @callflow(next="free")
+    exportHandle(BufferDescriptor descriptor,
+                 Buffer buffer)
+      generates (Error error,
+                 handle bufferHandle);
+};
index 8a74661..e3d2703 100644 (file)
@@ -16,6 +16,7 @@
 
 #define LOG_TAG "GrallocPassthrough"
 
+#include <mutex>
 #include <type_traits>
 #include <unordered_set>
 #include <vector>
@@ -42,18 +43,19 @@ public:
     // IAllocator interface
     Return<void> getCapabilities(getCapabilities_cb hidl_cb) override;
     Return<void> dumpDebugInfo(dumpDebugInfo_cb hidl_cb) override;
-    Return<void> createDescriptor(const BufferDescriptorInfo& descriptorInfo,
-            createDescriptor_cb hidl_cb) override;
-    Return<Error> destroyDescriptor(BufferDescriptor descriptor) override;
+    Return<void> createClient(createClient_cb hidl_cb) override;
 
-    Return<Error> testAllocate(
-            const hidl_vec<BufferDescriptor>& descriptors) override;
-    Return<void> allocate(const hidl_vec<BufferDescriptor>& descriptors,
-            allocate_cb hidl_cb) override;
-    Return<Error> free(Buffer buffer) override;
+    Error createDescriptor(
+            const IAllocatorClient::BufferDescriptorInfo& descriptorInfo,
+            BufferDescriptor& outDescriptor);
+    Error destroyDescriptor(BufferDescriptor descriptor);
 
-    Return<void> exportHandle(BufferDescriptor descriptor,
-            Buffer buffer, exportHandle_cb hidl_cb) override;
+    Error testAllocate(const hidl_vec<BufferDescriptor>& descriptors);
+    Error allocate(const hidl_vec<BufferDescriptor>& descriptors,
+            hidl_vec<Buffer>& outBuffers);
+    Error free(Buffer buffer);
+
+    Error exportHandle(Buffer buffer, const native_handle_t*& outHandle);
 
 private:
     void initCapabilities();
@@ -79,12 +81,36 @@ private:
         GRALLOC1_PFN_SET_PRODUCER_USAGE setProducerUsage;
         GRALLOC1_PFN_ALLOCATE allocate;
         GRALLOC1_PFN_RELEASE release;
-        GRALLOC1_PFN_GET_BACKING_STORE getBackingStore;
-        GRALLOC1_PFN_GET_STRIDE getStride;
-        GRALLOC1_PFN_GET_NUM_FLEX_PLANES getNumFlexPlanes;
     } mDispatch;
 };
 
+class GrallocClient : public IAllocatorClient {
+public:
+    GrallocClient(GrallocHal& hal);
+    virtual ~GrallocClient();
+
+    // IAllocatorClient interface
+    Return<void> createDescriptor(const BufferDescriptorInfo& descriptorInfo,
+            createDescriptor_cb hidl_cb) override;
+    Return<Error> destroyDescriptor(BufferDescriptor descriptor) override;
+
+    Return<Error> testAllocate(
+            const hidl_vec<BufferDescriptor>& descriptors) override;
+    Return<void> allocate(const hidl_vec<BufferDescriptor>& descriptors,
+            allocate_cb hidl_cb) override;
+    Return<Error> free(Buffer buffer) override;
+
+    Return<void> exportHandle(BufferDescriptor descriptor,
+            Buffer buffer, exportHandle_cb hidl_cb) override;
+
+private:
+    GrallocHal& mHal;
+
+    std::mutex mMutex;
+    std::unordered_set<BufferDescriptor> mDescriptors;
+    std::unordered_set<Buffer> mBuffers;
+};
+
 GrallocHal::GrallocHal(const hw_module_t* module)
     : mDevice(nullptr), mDispatch()
 {
@@ -182,24 +208,37 @@ Return<void> GrallocHal::dumpDebugInfo(dumpDebugInfo_cb hidl_cb)
     return Void();
 }
 
-Return<void> GrallocHal::createDescriptor(
-        const BufferDescriptorInfo& descriptorInfo,
-        createDescriptor_cb hidl_cb)
+Return<void> GrallocHal::createClient(createClient_cb hidl_cb)
 {
-    BufferDescriptor descriptor;
+    sp<IAllocatorClient> client = new GrallocClient(*this);
+    hidl_cb(Error::NONE, client);
+
+    return Void();
+}
+
+Error GrallocHal::createDescriptor(
+        const IAllocatorClient::BufferDescriptorInfo& descriptorInfo,
+        BufferDescriptor& outDescriptor)
+{
+    gralloc1_buffer_descriptor_t descriptor;
     int32_t err = mDispatch.createDescriptor(mDevice, &descriptor);
-    if (err == GRALLOC1_ERROR_NONE) {
-        err = mDispatch.setDimensions(mDevice, descriptor,
-                descriptorInfo.width, descriptorInfo.height);
+    if (err != GRALLOC1_ERROR_NONE) {
+        return static_cast<Error>(err);
     }
+
+    err = mDispatch.setDimensions(mDevice, descriptor,
+            descriptorInfo.width, descriptorInfo.height);
     if (err == GRALLOC1_ERROR_NONE) {
         err = mDispatch.setFormat(mDevice, descriptor,
                 static_cast<int32_t>(descriptorInfo.format));
     }
-    if (err == GRALLOC1_ERROR_NONE &&
-            hasCapability(Capability::LAYERED_BUFFERS)) {
-        err = mDispatch.setLayerCount(mDevice, descriptor,
-                descriptorInfo.layerCount);
+    if (err == GRALLOC1_ERROR_NONE) {
+        if (hasCapability(Capability::LAYERED_BUFFERS)) {
+            err = mDispatch.setLayerCount(mDevice, descriptor,
+                    descriptorInfo.layerCount);
+        } else if (descriptorInfo.layerCount != 1) {
+            err = GRALLOC1_ERROR_BAD_VALUE;
+        }
     }
     if (err == GRALLOC1_ERROR_NONE) {
         uint64_t producerUsageMask = descriptorInfo.producerUsageMask;
@@ -221,64 +260,189 @@ Return<void> GrallocHal::createDescriptor(
                 consumerUsageMask);
     }
 
-    hidl_cb(static_cast<Error>(err), descriptor);
+    if (err == GRALLOC1_ERROR_NONE) {
+        outDescriptor = descriptor;
+    } else {
+        mDispatch.destroyDescriptor(mDevice, descriptor);
+    }
 
-    return Void();
+    return static_cast<Error>(err);
 }
 
-Return<Error> GrallocHal::destroyDescriptor(
-        BufferDescriptor descriptor)
+Error GrallocHal::destroyDescriptor(BufferDescriptor descriptor)
 {
     int32_t err = mDispatch.destroyDescriptor(mDevice, descriptor);
     return static_cast<Error>(err);
 }
 
-Return<Error> GrallocHal::testAllocate(
-        const hidl_vec<BufferDescriptor>& descriptors)
+Error GrallocHal::testAllocate(const hidl_vec<BufferDescriptor>& descriptors)
 {
     if (!hasCapability(Capability::TEST_ALLOCATE)) {
         return Error::UNDEFINED;
     }
 
     int32_t err = mDispatch.allocate(mDevice, descriptors.size(),
-            &descriptors[0], nullptr);
+            descriptors.data(), nullptr);
     return static_cast<Error>(err);
 }
 
-Return<void> GrallocHal::allocate(
-        const hidl_vec<BufferDescriptor>& descriptors,
-        allocate_cb hidl_cb) {
+Error GrallocHal::allocate(const hidl_vec<BufferDescriptor>& descriptors,
+        hidl_vec<Buffer>& outBuffers)
+{
     std::vector<buffer_handle_t> buffers(descriptors.size());
     int32_t err = mDispatch.allocate(mDevice, descriptors.size(),
-            &descriptors[0], buffers.data());
-    if (err != GRALLOC1_ERROR_NONE && err != GRALLOC1_ERROR_NOT_SHARED) {
-        buffers.clear();
+            descriptors.data(), buffers.data());
+    if (err == GRALLOC1_ERROR_NONE || err == GRALLOC1_ERROR_NOT_SHARED) {
+        outBuffers.resize(buffers.size());
+        for (size_t i = 0; i < outBuffers.size(); i++) {
+            outBuffers[i] = static_cast<Buffer>(
+                    reinterpret_cast<uintptr_t>(buffers[i]));
+        }
     }
 
-    hidl_vec<Buffer> reply;
-    reply.setToExternal(
-            reinterpret_cast<Buffer*>(buffers.data()),
-            buffers.size());
-    hidl_cb(static_cast<Error>(err), reply);
-
-    return Void();
+    return static_cast<Error>(err);
 }
 
-Return<Error> GrallocHal::free(Buffer buffer)
+Error GrallocHal::free(Buffer buffer)
 {
-    buffer_handle_t handle = reinterpret_cast<buffer_handle_t>(buffer);
+    buffer_handle_t handle = reinterpret_cast<buffer_handle_t>(
+            static_cast<uintptr_t>(buffer));
+
     int32_t err = mDispatch.release(mDevice, handle);
     return static_cast<Error>(err);
 }
 
-Return<void> GrallocHal::exportHandle(BufferDescriptor /*descriptor*/,
+Error GrallocHal::exportHandle(Buffer buffer,
+        const native_handle_t*& outHandle)
+{
+    // we rely on the caller to validate buffer here
+    outHandle = reinterpret_cast<buffer_handle_t>(
+            static_cast<uintptr_t>(buffer));
+    return Error::NONE;
+}
+
+GrallocClient::GrallocClient(GrallocHal& hal)
+    : mHal(hal)
+{
+}
+
+GrallocClient::~GrallocClient()
+{
+    if (!mBuffers.empty()) {
+        ALOGW("client destroyed with valid buffers");
+        for (auto buf : mBuffers) {
+            mHal.free(buf);
+        }
+    }
+
+    if (!mDescriptors.empty()) {
+        ALOGW("client destroyed with valid buffer descriptors");
+        for (auto desc : mDescriptors) {
+            mHal.destroyDescriptor(desc);
+        }
+    }
+}
+
+Return<void> GrallocClient::createDescriptor(
+        const BufferDescriptorInfo& descriptorInfo,
+        createDescriptor_cb hidl_cb)
+{
+    BufferDescriptor descriptor;
+    Error err = mHal.createDescriptor(descriptorInfo, descriptor);
+
+    if (err == Error::NONE) {
+        std::lock_guard<std::mutex> lock(mMutex);
+
+        auto result = mDescriptors.insert(descriptor);
+        if (!result.second) {
+            ALOGW("duplicated buffer descriptor id returned");
+            mHal.destroyDescriptor(descriptor);
+            err = Error::NO_RESOURCES;
+        }
+    }
+
+    hidl_cb(err, descriptor);
+    return Void();
+}
+
+Return<Error> GrallocClient::destroyDescriptor(BufferDescriptor descriptor)
+{
+    {
+        std::lock_guard<std::mutex> lock(mMutex);
+        if (!mDescriptors.erase(descriptor)) {
+            return Error::BAD_DESCRIPTOR;
+        }
+    }
+
+    return mHal.destroyDescriptor(descriptor);
+}
+
+Return<Error> GrallocClient::testAllocate(
+        const hidl_vec<BufferDescriptor>& descriptors)
+{
+    return mHal.testAllocate(descriptors);
+}
+
+Return<void> GrallocClient::allocate(
+        const hidl_vec<BufferDescriptor>& descriptors,
+        allocate_cb hidl_cb) {
+    hidl_vec<Buffer> buffers;
+    Error err = mHal.allocate(descriptors, buffers);
+
+    if (err == Error::NONE || err == Error::NOT_SHARED) {
+        std::lock_guard<std::mutex> lock(mMutex);
+
+        for (size_t i = 0; i < buffers.size(); i++) {
+            auto result = mBuffers.insert(buffers[i]);
+            if (!result.second) {
+                ALOGW("duplicated buffer id returned");
+
+                for (size_t j = 0; j < buffers.size(); j++) {
+                    if (j < i) {
+                        mBuffers.erase(buffers[i]);
+                    }
+                    mHal.free(buffers[i]);
+                }
+
+                buffers = hidl_vec<Buffer>();
+                err = Error::NO_RESOURCES;
+                break;
+            }
+        }
+    }
+
+    hidl_cb(err, buffers);
+    return Void();
+}
+
+Return<Error> GrallocClient::free(Buffer buffer)
+{
+    {
+        std::lock_guard<std::mutex> lock(mMutex);
+        if (!mBuffers.erase(buffer)) {
+            return Error::BAD_BUFFER;
+        }
+    }
+
+    return mHal.free(buffer);
+}
+
+Return<void> GrallocClient::exportHandle(BufferDescriptor /*descriptor*/,
         Buffer buffer, exportHandle_cb hidl_cb)
 {
-    // do we want to validate?
-    buffer_handle_t handle = reinterpret_cast<buffer_handle_t>(buffer);
+    const native_handle_t* handle = nullptr;
+
+    {
+        std::lock_guard<std::mutex> lock(mMutex);
+        if (mBuffers.count(buffer) == 0) {
+            hidl_cb(Error::BAD_BUFFER, handle);
+            return Void();
+        }
+    }
 
-    hidl_cb(Error::NONE, handle);
+    Error err = mHal.exportHandle(buffer, handle);
 
+    hidl_cb(err, handle);
     return Void();
 }
 
index 54369a4..bd846c4 100644 (file)
@@ -43,10 +43,10 @@ using android::hardware::graphics::common::V1_0::PixelFormat;
 
 class TempDescriptor {
  public:
-  TempDescriptor(const sp<IAllocator>& allocator,
-                 const IAllocator::BufferDescriptorInfo& info)
-      : mAllocator(allocator), mError(Error::NO_RESOURCES) {
-    mAllocator->createDescriptor(
+  TempDescriptor(const sp<IAllocatorClient>& client,
+                 const IAllocatorClient::BufferDescriptorInfo& info)
+      : mClient(client), mError(Error::NO_RESOURCES) {
+    mClient->createDescriptor(
         info, [&](const auto& tmpError, const auto& tmpDescriptor) {
           mError = tmpError;
           mDescriptor = tmpDescriptor;
@@ -55,7 +55,7 @@ class TempDescriptor {
 
   ~TempDescriptor() {
     if (mError == Error::NONE) {
-      mAllocator->destroyDescriptor(mDescriptor);
+      mClient->destroyDescriptor(mDescriptor);
     }
   }
 
@@ -64,7 +64,7 @@ class TempDescriptor {
   operator BufferDescriptor() const { return mDescriptor; }
 
  private:
-  sp<IAllocator> mAllocator;
+  sp<IAllocatorClient> mClient;
   Error mError;
   BufferDescriptor mDescriptor;
 };
@@ -75,10 +75,18 @@ class GraphicsAllocatorHidlTest : public ::testing::Test {
     mAllocator = IAllocator::getService("gralloc");
     ASSERT_NE(mAllocator, nullptr);
 
+    mAllocator->createClient([this](const auto& error, const auto& client) {
+      if (error == Error::NONE) {
+        mClient = client;
+      }
+    });
+    ASSERT_NE(mClient, nullptr);
+
     initCapabilities();
 
     mDummyDescriptorInfo.width = 64;
     mDummyDescriptorInfo.height = 64;
+    mDummyDescriptorInfo.layerCount = 1;
     mDummyDescriptorInfo.format = PixelFormat::RGBA_8888;
     mDummyDescriptorInfo.producerUsageMask =
         static_cast<uint64_t>(ProducerUsage::CPU_WRITE);
@@ -106,7 +114,8 @@ class GraphicsAllocatorHidlTest : public ::testing::Test {
   }
 
   sp<IAllocator> mAllocator;
-  IAllocator::BufferDescriptorInfo mDummyDescriptorInfo{};
+  sp<IAllocatorClient> mClient;
+  IAllocatorClient::BufferDescriptorInfo mDummyDescriptorInfo{};
 
  private:
   std::unordered_set<IAllocator::Capability> mCapabilities;
@@ -134,7 +143,7 @@ TEST_F(GraphicsAllocatorHidlTest, DumpDebugInfo) {
 TEST_F(GraphicsAllocatorHidlTest, CreateDestroyDescriptor) {
   Error error;
   BufferDescriptor descriptor;
-  auto ret = mAllocator->createDescriptor(
+  auto ret = mClient->createDescriptor(
       mDummyDescriptorInfo,
       [&](const auto& tmpError, const auto& tmpDescriptor) {
         error = tmpError;
@@ -144,7 +153,7 @@ TEST_F(GraphicsAllocatorHidlTest, CreateDestroyDescriptor) {
   ASSERT_TRUE(ret.getStatus().isOk());
   ASSERT_EQ(Error::NONE, error);
 
-  auto err_ret = mAllocator->destroyDescriptor(descriptor);
+  auto err_ret = mClient->destroyDescriptor(descriptor);
   ASSERT_TRUE(err_ret.getStatus().isOk());
   ASSERT_EQ(Error::NONE, static_cast<Error>(err_ret));
 }
@@ -155,14 +164,14 @@ TEST_F(GraphicsAllocatorHidlTest, CreateDestroyDescriptor) {
 TEST_F(GraphicsAllocatorHidlTest, TestAllocateBasic) {
   CHECK_FEATURE_OR_SKIP(IAllocator::Capability::TEST_ALLOCATE);
 
-  TempDescriptor descriptor(mAllocator, mDummyDescriptorInfo);
+  TempDescriptor descriptor(mClient, mDummyDescriptorInfo);
   ASSERT_TRUE(descriptor.isValid());
 
   hidl_vec<BufferDescriptor> descriptors;
   descriptors.resize(1);
   descriptors[0] = descriptor;
 
-  auto ret = mAllocator->testAllocate(descriptors);
+  auto ret = mClient->testAllocate(descriptors);
   ASSERT_TRUE(ret.getStatus().isOk());
 
   auto error = static_cast<Error>(ret);
@@ -175,7 +184,7 @@ TEST_F(GraphicsAllocatorHidlTest, TestAllocateBasic) {
 TEST_F(GraphicsAllocatorHidlTest, TestAllocateArray) {
   CHECK_FEATURE_OR_SKIP(IAllocator::Capability::TEST_ALLOCATE);
 
-  TempDescriptor descriptor(mAllocator, mDummyDescriptorInfo);
+  TempDescriptor descriptor(mClient, mDummyDescriptorInfo);
   ASSERT_TRUE(descriptor.isValid());
 
   hidl_vec<BufferDescriptor> descriptors;
@@ -183,7 +192,7 @@ TEST_F(GraphicsAllocatorHidlTest, TestAllocateArray) {
   descriptors[0] = descriptor;
   descriptors[1] = descriptor;
 
-  auto ret = mAllocator->testAllocate(descriptors);
+  auto ret = mClient->testAllocate(descriptors);
   ASSERT_TRUE(ret.getStatus().isOk());
 
   auto error = static_cast<Error>(ret);
@@ -194,7 +203,7 @@ TEST_F(GraphicsAllocatorHidlTest, TestAllocateArray) {
  * Test allocate/free with a single buffer descriptor.
  */
 TEST_F(GraphicsAllocatorHidlTest, AllocateFreeBasic) {
-  TempDescriptor descriptor(mAllocator, mDummyDescriptorInfo);
+  TempDescriptor descriptor(mClient, mDummyDescriptorInfo);
   ASSERT_TRUE(descriptor.isValid());
 
   hidl_vec<BufferDescriptor> descriptors;
@@ -203,7 +212,7 @@ TEST_F(GraphicsAllocatorHidlTest, AllocateFreeBasic) {
 
   Error error;
   std::vector<Buffer> buffers;
-  auto ret = mAllocator->allocate(
+  auto ret = mClient->allocate(
       descriptors, [&](const auto& tmpError, const auto& tmpBuffers) {
         error = tmpError;
         buffers = tmpBuffers;
@@ -214,7 +223,7 @@ TEST_F(GraphicsAllocatorHidlTest, AllocateFreeBasic) {
   EXPECT_EQ(1u, buffers.size());
 
   if (!buffers.empty()) {
-    auto err_ret = mAllocator->free(buffers[0]);
+    auto err_ret = mClient->free(buffers[0]);
     EXPECT_TRUE(err_ret.getStatus().isOk());
     EXPECT_EQ(Error::NONE, static_cast<Error>(err_ret));
   }
@@ -224,10 +233,10 @@ TEST_F(GraphicsAllocatorHidlTest, AllocateFreeBasic) {
  * Test allocate/free with an array of buffer descriptors.
  */
 TEST_F(GraphicsAllocatorHidlTest, AllocateFreeArray) {
-  TempDescriptor descriptor1(mAllocator, mDummyDescriptorInfo);
+  TempDescriptor descriptor1(mClient, mDummyDescriptorInfo);
   ASSERT_TRUE(descriptor1.isValid());
 
-  TempDescriptor descriptor2(mAllocator, mDummyDescriptorInfo);
+  TempDescriptor descriptor2(mClient, mDummyDescriptorInfo);
   ASSERT_TRUE(descriptor2.isValid());
 
   hidl_vec<BufferDescriptor> descriptors;
@@ -238,7 +247,7 @@ TEST_F(GraphicsAllocatorHidlTest, AllocateFreeArray) {
 
   Error error;
   std::vector<Buffer> buffers;
-  auto ret = mAllocator->allocate(
+  auto ret = mClient->allocate(
       descriptors, [&](const auto& tmpError, const auto& tmpBuffers) {
         error = tmpError;
         buffers = tmpBuffers;
@@ -249,14 +258,14 @@ TEST_F(GraphicsAllocatorHidlTest, AllocateFreeArray) {
   EXPECT_EQ(descriptors.size(), buffers.size());
 
   for (auto buf : buffers) {
-    auto err_ret = mAllocator->free(buf);
+    auto err_ret = mClient->free(buf);
     EXPECT_TRUE(err_ret.getStatus().isOk());
     EXPECT_EQ(Error::NONE, static_cast<Error>(err_ret));
   }
 }
 
 TEST_F(GraphicsAllocatorHidlTest, ExportHandle) {
-  TempDescriptor descriptor(mAllocator, mDummyDescriptorInfo);
+  TempDescriptor descriptor(mClient, mDummyDescriptorInfo);
   ASSERT_TRUE(descriptor.isValid());
 
   hidl_vec<BufferDescriptor> descriptors;
@@ -265,7 +274,7 @@ TEST_F(GraphicsAllocatorHidlTest, ExportHandle) {
 
   Error error;
   std::vector<Buffer> buffers;
-  auto ret = mAllocator->allocate(
+  auto ret = mClient->allocate(
       descriptors, [&](const auto& tmpError, const auto& tmpBuffers) {
         error = tmpError;
         buffers = tmpBuffers;
@@ -275,13 +284,13 @@ TEST_F(GraphicsAllocatorHidlTest, ExportHandle) {
   ASSERT_TRUE(error == Error::NONE || error == Error::NOT_SHARED);
   ASSERT_EQ(1u, buffers.size());
 
-  ret = mAllocator->exportHandle(
+  ret = mClient->exportHandle(
       descriptors[0], buffers[0],
       [&](const auto& tmpError, const auto&) { error = tmpError; });
   EXPECT_TRUE(ret.getStatus().isOk());
   EXPECT_EQ(Error::NONE, error);
 
-  auto err_ret = mAllocator->free(buffers[0]);
+  auto err_ret = mClient->free(buffers[0]);
   EXPECT_TRUE(err_ret.getStatus().isOk());
   EXPECT_EQ(Error::NONE, static_cast<Error>(err_ret));
 }