Track GemHandles and FB separately.

We might be having cases where we don't end up creating
FB as fb creation failed or layers go through offscreen
composition pass. In this case we worked around it by
closing the gem handles in gralloc handler. Instead, we
now track gem handle registration and fb registration
separately in FrameBufferManager and use GemHandles as
key for both.

Jira: None.
Test: 1) Don't see rapid increase in memory usage when using
         task switcher in Android IA.
      2) No errors complaining about us trying to close invalid
         handles in multi monitor case.

Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com>

diff --git a/common/compositor/compositorthread.cpp b/common/compositor/compositorthread.cpp
index fcb483b..584a2fd 100644 (file)
--- a/common/compositor/compositorthread.cpp
+++ b/common/compositor/compositorthread.cpp
@@ -179,7 +179,7 @@ void CompositorThread::HandleReleaseRequest() {
       purged_gl_resources, purged_media_resources, &has_gpu_resource);
   size_t purged_size = purged_gl_resources.size();
 
-  FrameBufferManager *pFBManager = FrameBufferManager::GetInstance(gpu_fd_);
+  FrameBufferManager *pFBManager = FrameBufferManager::GetInstance();
 
   if (purged_size != 0) {
     if (has_gpu_resource) {
@@ -192,21 +192,14 @@ void CompositorThread::HandleReleaseRequest() {
 
     for (size_t i = 0; i < purged_size; i++) {
       const ResourceHandle &handle = purged_gl_resources.at(i);
-      bool fd_created = false;
-      if (handle.drm_fd_)
-        fd_created = true;
-
-      if (fd_created &&
-          pFBManager->RemoveFB(handle.drm_fd_,
-                               handler->CanReleaseGemHandles(handle.handle_))) {
-        ETRACE("Failed to remove fb %s", PRINTERROR());
-      }
-
       if (!handle.handle_) {
         continue;
       }
 
-      handler->ReleaseBuffer(handle.handle_, !fd_created);
+      pFBManager->RemoveFB(handle.handle_->meta_data_.num_planes_,
+                           handle.handle_->meta_data_.gem_handles_);
+
+      handler->ReleaseBuffer(handle.handle_);
       handler->DestroyHandle(handle.handle_);
     }
   }
@@ -222,16 +215,12 @@ void CompositorThread::HandleReleaseRequest() {
 
     for (size_t i = 0; i < purged_size; i++) {
       const MediaResourceHandle &handle = purged_media_resources.at(i);
-      if (handle.drm_fd_ &&
-          pFBManager->RemoveFB(handle.drm_fd_,
-                               handler->CanReleaseGemHandles(handle.handle_))) {
-        ETRACE("Failed to remove fb %s", PRINTERROR());
-      }
-
       if (!handle.handle_) {
         continue;
       }
 
+      pFBManager->RemoveFB(handle.handle_->meta_data_.num_planes_,
+                           handle.handle_->meta_data_.gem_handles_);
       handler->ReleaseBuffer(handle.handle_);
       handler->DestroyHandle(handle.handle_);
     }

diff --git a/common/core/framebuffermanager.cpp b/common/core/framebuffermanager.cpp
index 4c11a7c..929e563 100644 (file)
--- a/common/core/framebuffermanager.cpp
+++ b/common/core/framebuffermanager.cpp
@@ -22,10 +22,31 @@ namespace hwcomposer {
 
 FrameBufferManager *FrameBufferManager::pInstance = NULL;
 
-FrameBufferManager *FrameBufferManager::GetInstance(uint32_t gpu_fd) {
+FrameBufferManager *FrameBufferManager::GetInstance() {
+  return pInstance;
+}
+
+void FrameBufferManager::CreateInstance(uint32_t gpu_fd) {
   if (pInstance == NULL)
     pInstance = new FrameBufferManager(gpu_fd);
-  return pInstance;
+}
+
+void FrameBufferManager::RegisterGemHandles(const uint32_t &num_planes,
+                                            const uint32_t (&igem_handles)[4]) {
+  lock_.lock();
+  FBKey key(num_planes, igem_handles);
+  auto it = fb_map_.find(key);
+  if (it != fb_map_.end()) {
+    it->second.fb_ref++;
+  } else {
+    FBValue value;
+    value.fb_ref = 1;
+    value.fb_id = 0;
+    value.fb_created = false;
+    fb_map_.emplace(std::make_pair(key, value));
+  }
+
+  lock_.unlock();
 }
 
 uint32_t FrameBufferManager::FindFB(const uint32_t &iwidth,
@@ -37,49 +58,48 @@ uint32_t FrameBufferManager::FindFB(const uint32_t &iwidth,
                                     const uint32_t (&ioffsets)[4]) {
   lock_.lock();
   FBKey key(num_planes, igem_handles);
+  uint32_t fb_id = 0;
   auto it = fb_map_.find(key);
   if (it != fb_map_.end()) {
-    it->second.fb_ref++;
-    lock_.unlock();
-    return it->second.fb_id;
-  } else {
-    uint32_t fb_id = 0;
-    int ret =
-        CreateFrameBuffer(iwidth, iheight, iframe_buffer_format, igem_handles,
-                          ipitches, ioffsets, gpu_fd_, &fb_id);
-    if (ret) {
-      lock_.unlock();
-      return fb_id;
+    if (it->second.fb_created) {
+      it->second.fb_ref++;
+    } else {
+      it->second.fb_created = true;
+      CreateFrameBuffer(iwidth, iheight, iframe_buffer_format, igem_handles,
+                        ipitches, ioffsets, gpu_fd_, &it->second.fb_id);
     }
 
-    FBValue value;
-    value.fb_id = fb_id;
-    value.fb_ref = 1;
-    fb_map_.emplace(std::make_pair(key, value));
-    lock_.unlock();
-    return fb_id;
+    fb_id = it->second.fb_id;
+  } else {
+    ETRACE("Handle not found in Cache \n");
   }
+
+  lock_.unlock();
+  return fb_id;
 }
 
-int FrameBufferManager::RemoveFB(const uint32_t &fb, bool release_gem_handles) {
+int FrameBufferManager::RemoveFB(uint32_t num_planes,
+                                 const uint32_t (&igem_handles)[4]) {
   lock_.lock();
-  auto it = fb_map_.begin();
+
   int ret = 0;
+  FBKey key(num_planes, igem_handles);
 
-  while (it != fb_map_.end()) {
-    if (it->second.fb_id == fb) {
-      it->second.fb_ref -= 1;
-      if (it->second.fb_ref == 0) {
-        ret = ReleaseFrameBuffer(it->first, fb, gpu_fd_, release_gem_handles);
-        fb_map_.erase(it);
-      }
-      break;
+  auto it = fb_map_.find(key);
+  if (it != fb_map_.end()) {
+    it->second.fb_ref -= 1;
+    if (it->second.fb_ref == 0) {
+      ret = ReleaseFrameBuffer(it->first, it->second.fb_id, gpu_fd_);
+      fb_map_.erase(it);
     }
-    it++;
   }
 
-  if(it == fb_map_.end()) {
-    ETRACE("RemoveFB not meet!");
+  if (it == fb_map_.end()) {
+    if (igem_handles[0] != 0 || igem_handles[1] != 0 || igem_handles[2] != 0 ||
+        igem_handles[3] != 0) {
+      ETRACE("Unable to find fb in cache. %d %d %d %d \n", igem_handles[0],
+             igem_handles[1], igem_handles[2], igem_handles[3]);
+    }
   }
 
   lock_.unlock();
@@ -92,7 +112,7 @@ void FrameBufferManager::PurgeAllFBs() {
   auto it = fb_map_.begin();
 
   while (it != fb_map_.end()) {
-    ReleaseFrameBuffer(it->first, it->second.fb_id, gpu_fd_, false);
+    ReleaseFrameBuffer(it->first, it->second.fb_id, gpu_fd_);
   }
 
   lock_.unlock();

diff --git a/common/core/framebuffermanager.h b/common/core/framebuffermanager.h
index 81fbacf..bc6d86c 100644 (file)
--- a/common/core/framebuffermanager.h
+++ b/common/core/framebuffermanager.h
@@ -56,6 +56,7 @@ class NativeBufferHandler;
 typedef struct {
   uint32_t fb_id;
   uint32_t fb_ref;
+  bool fb_created;
 } FBValue;
 
 struct FBHash {
@@ -76,12 +77,15 @@ struct FBEqual {
 
 class FrameBufferManager {
  public:
-  static FrameBufferManager *GetInstance(uint32_t gpu_fd);
+  static FrameBufferManager *GetInstance();
+  static void CreateInstance(uint32_t gpu_fd);
+  void RegisterGemHandles(const uint32_t &num_planes,
+                          const uint32_t (&igem_handles)[4]);
   uint32_t FindFB(const uint32_t &iwidth, const uint32_t &iheight,
                   const uint32_t &iframe_buffer_format,
                   const uint32_t &num_planes, const uint32_t (&igem_handles)[4],
                   const uint32_t (&ipitches)[4], const uint32_t (&ioffsets)[4]);
-  int RemoveFB(const uint32_t &fb, bool release_gem_handles);
+  int RemoveFB(uint32_t num_planes, const uint32_t (&igem_handles)[4]);
 
  private:
   static FrameBufferManager *pInstance;

diff --git a/os/android/gralloc1bufferhandler.cpp b/os/android/gralloc1bufferhandler.cpp
index 1fbd7f2..9b8addb 100644 (file)
--- a/os/android/gralloc1bufferhandler.cpp
+++ b/os/android/gralloc1bufferhandler.cpp
@@ -170,56 +170,13 @@ bool Gralloc1BufferHandler::CreateBuffer(uint32_t w, uint32_t h, int format,
   return true;
 }
 
-bool Gralloc1BufferHandler::CanReleaseGemHandles(HWCNativeHandle handle) const {
-  if (!handle) {
-    return false;
-  }
-
-  if (handle->hwc_buffer_) {
-    return true;
-  }
-
-  if (handle->imported_handle_) {
-    return true;
-  }
-
-  return false;
-}
-
-bool Gralloc1BufferHandler::ReleaseBuffer(HWCNativeHandle handle,
-                                          bool release_gem_handles) const {
+bool Gralloc1BufferHandler::ReleaseBuffer(HWCNativeHandle handle) const {
   gralloc1_device_t *gralloc1_dvc =
       reinterpret_cast<gralloc1_device_t *>(device_);
 
   if (handle->hwc_buffer_) {
     release_(gralloc1_dvc, handle->handle_);
   } else if (handle->imported_handle_) {
-    if (release_gem_handles) {
-      uint32_t total_planes = handle->meta_data_.num_planes_;
-      struct drm_gem_close gem_close;
-      int last_gem_handle = -1;
-
-      for (uint32_t plane = 0; plane < total_planes; plane++) {
-        uint32_t current_gem_handle = handle->meta_data_.gem_handles_[plane];
-        if ((last_gem_handle != -1) &&
-            (current_gem_handle == static_cast<uint32_t>(last_gem_handle))) {
-          break;
-        }
-
-        memset(&gem_close, 0, sizeof(gem_close));
-        last_gem_handle = current_gem_handle;
-        gem_close.handle = current_gem_handle;
-
-        int ret = drmIoctl(fd_, DRM_IOCTL_GEM_CLOSE, &gem_close);
-        if (ret) {
-          ETRACE(
-              "Failed to close gem handle ErrorCode: %d "
-              "GemHandle: %d  \n",
-              ret, current_gem_handle);
-        }
-      }
-    }
-
     release_(gralloc1_dvc, handle->imported_handle_);
   }
 

diff --git a/os/android/gralloc1bufferhandler.h b/os/android/gralloc1bufferhandler.h
index 1bd0e77..fc1e914 100644 (file)
--- a/os/android/gralloc1bufferhandler.h
+++ b/os/android/gralloc1bufferhandler.h
@@ -36,9 +36,7 @@ class Gralloc1BufferHandler : public NativeBufferHandler {
 
   bool CreateBuffer(uint32_t w, uint32_t h, int format, HWCNativeHandle *handle,
                     uint32_t layer_type) const override;
-  bool CanReleaseGemHandles(HWCNativeHandle handle) const;
-  bool ReleaseBuffer(HWCNativeHandle handle,
-                     bool release_gem_handles = false) const override;
+  bool ReleaseBuffer(HWCNativeHandle handle) const override;
   void DestroyHandle(HWCNativeHandle handle) const override;
   bool ImportBuffer(HWCNativeHandle handle) const override;
   void CopyHandle(HWCNativeHandle source,

diff --git a/os/android/platformdefines.cpp b/os/android/platformdefines.cpp
index eef8b0b..ca678d9 100644 (file)
--- a/os/android/platformdefines.cpp
+++ b/os/android/platformdefines.cpp
@@ -67,17 +67,12 @@ VkFormat NativeToVkFormat(int native_format) {
 }
 #endif
 
-int ReleaseFrameBuffer(const FBKey& key, uint32_t fd, uint32_t gpu_fd,
-                       bool release_gem_handle) {
-  int ret = drmModeRmFB(gpu_fd, fd);
+int ReleaseFrameBuffer(const FBKey &key, uint32_t fd, uint32_t gpu_fd) {
+  int ret = fd > 0 ? drmModeRmFB(gpu_fd, fd) : 0;
   if (ret) {
     ETRACE("Failed to Remove FD ErrorCode: %d FD: %d \n", ret, fd);
   }
 
-  if (!release_gem_handle) {
-    return 0;
-  }
-
   uint32_t total_planes = key.num_planes_;
   struct drm_gem_close gem_close;
   int last_gem_handle = -1;

diff --git a/os/linux/gbmbufferhandler.cpp b/os/linux/gbmbufferhandler.cpp
index 8518217..54d9899 100644 (file)
--- a/os/linux/gbmbufferhandler.cpp
+++ b/os/linux/gbmbufferhandler.cpp
@@ -150,12 +150,7 @@ bool GbmBufferHandler::CreateBuffer(uint32_t w, uint32_t h, int format,
   return true;
 }
 
-bool GbmBufferHandler::CanReleaseGemHandles(HWCNativeHandle handle) const {
-  return false;
-}
-
-bool GbmBufferHandler::ReleaseBuffer(HWCNativeHandle handle,
-                                     bool /*release_gem_handles*/) const {
+bool GbmBufferHandler::ReleaseBuffer(HWCNativeHandle handle) const {
   if (handle->bo || handle->imported_bo) {
     if (handle->bo && handle->hwc_buffer_) {
       gbm_bo_destroy(handle->bo);

diff --git a/os/linux/gbmbufferhandler.h b/os/linux/gbmbufferhandler.h
index bf823a3..4f9af3a 100644 (file)
--- a/os/linux/gbmbufferhandler.h
+++ b/os/linux/gbmbufferhandler.h
@@ -34,9 +34,7 @@ class GbmBufferHandler : public NativeBufferHandler {
 
   bool CreateBuffer(uint32_t w, uint32_t h, int format, HWCNativeHandle *handle,
                     uint32_t layer_type) const override;
-  bool CanReleaseGemHandles(HWCNativeHandle handle) const override;
-  bool ReleaseBuffer(HWCNativeHandle handle,
-                     bool release_gem_handles = false) const override;
+  bool ReleaseBuffer(HWCNativeHandle handle) const override;
   void DestroyHandle(HWCNativeHandle handle) const override;
   void CopyHandle(HWCNativeHandle source,
                   HWCNativeHandle *target) const override;

diff --git a/os/linux/platformdefines.cpp b/os/linux/platformdefines.cpp
index aa2b5fb..7d613b6 100644 (file)
--- a/os/linux/platformdefines.cpp
+++ b/os/linux/platformdefines.cpp
@@ -67,9 +67,8 @@ VkFormat NativeToVkFormat(int native_format) {
 }
 #endif
 
-int ReleaseFrameBuffer(const FBKey& /*key*/, uint32_t fd, uint32_t gpu_fd,
-                       bool /*release_gem_handle*/) {
-  return drmModeRmFB(gpu_fd, fd);
+int ReleaseFrameBuffer(const FBKey & /*key*/, uint32_t fd, uint32_t gpu_fd) {
+  return fd > 0 ? drmModeRmFB(gpu_fd, fd) : 0;
 }
 
 int CreateFrameBuffer(const uint32_t &iwidth, const uint32_t &iheight,

diff --git a/os/platformcommondefines.h b/os/platformcommondefines.h
index 019efdf..0f229f5 100644 (file)
--- a/os/platformcommondefines.h
+++ b/os/platformcommondefines.h
@@ -59,7 +59,6 @@ int CreateFrameBuffer(const uint32_t &iwidth, const uint32_t &iheight,
                       const uint32_t (&ioffsets)[4], uint32_t gpu_fd,
                       uint32_t *fb_id);
 
-int ReleaseFrameBuffer(const FBKey &key, uint32_t fd, uint32_t gpu_fd,
-                       bool release_gem_handle);
+int ReleaseFrameBuffer(const FBKey &key, uint32_t fd, uint32_t gpu_fd);
 
 #endif  // OS_LINUX_PLATFORMCOMMONDEFINES_H_

diff --git a/public/nativebufferhandler.h b/public/nativebufferhandler.h
index ffcf296..04c527a 100644 (file)
--- a/public/nativebufferhandler.h
+++ b/public/nativebufferhandler.h
@@ -35,10 +35,7 @@ class NativeBufferHandler {
                             HWCNativeHandle *handle = NULL,
                             uint32_t layer_type = kLayerNormal) const = 0;
 
-  virtual bool CanReleaseGemHandles(HWCNativeHandle handle) const = 0;
-
-  virtual bool ReleaseBuffer(HWCNativeHandle handle,
-                             bool release_gem_handles = false) const = 0;
+  virtual bool ReleaseBuffer(HWCNativeHandle handle) const = 0;
 
   virtual void DestroyHandle(HWCNativeHandle handle) const = 0;
 

diff --git a/wsi/drm/drmbuffer.cpp b/wsi/drm/drmbuffer.cpp
index 4a7a2d3..d00afb9 100644 (file)
--- a/wsi/drm/drmbuffer.cpp
+++ b/wsi/drm/drmbuffer.cpp
@@ -72,6 +72,10 @@ void DrmBuffer::Initialize(const HwcBuffer& bo) {
   } else {
     frame_buffer_format_ = format_;
   }
+
+  FrameBufferManager::GetInstance()->RegisterGemHandles(
+      image_.handle_->meta_data_.num_planes_,
+      image_.handle_->meta_data_.gem_handles_);
 }
 
 void DrmBuffer::InitializeFromNativeHandle(HWCNativeHandle handle,
@@ -367,7 +371,7 @@ bool DrmBuffer::CreateFrameBuffer(uint32_t gpu_fd) {
   image_.drm_fd_ = 0;
   media_image_.drm_fd_ = 0;
 
-  image_.drm_fd_ = FrameBufferManager::GetInstance(gpu_fd)->FindFB(
+  image_.drm_fd_ = FrameBufferManager::GetInstance()->FindFB(
       width_, height_, frame_buffer_format_,
       image_.handle_->meta_data_.num_planes_, gem_handles_, pitches_, offsets_);
   media_image_.drm_fd_ = image_.drm_fd_;

diff --git a/wsi/drm/drmdisplaymanager.cpp b/wsi/drm/drmdisplaymanager.cpp
index b1a206c..6d68630 100644 (file)
--- a/wsi/drm/drmdisplaymanager.cpp
+++ b/wsi/drm/drmdisplaymanager.cpp
@@ -36,6 +36,8 @@
 
 #include <nativebufferhandler.h>
 
+#include "framebuffermanager.h"
+
 namespace hwcomposer {
 
 DrmDisplayManager::DrmDisplayManager(GpuDevice *device)
@@ -168,6 +170,8 @@ void DrmDisplayManager::HandleWait() {
 
 void DrmDisplayManager::InitializeDisplayResources() {
   buffer_handler_.reset(NativeBufferHandler::CreateInstance(fd_));
+  // FIXME: Remove this once #303 is fixed.
+  FrameBufferManager::CreateInstance(fd_);
   if (!buffer_handler_) {
     ETRACE("Failed to create native buffer handler instance");
     return;