Re: [PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class

[Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>]

On Tue, Apr 17, 2018 at 06:08:06PM +0200, Robert Foss wrote:
> Hey,
> 
> On 04/17/2018 05:33 PM, Sean Paul wrote:
> >On Wed, Apr 11, 2018 at 04:22:15PM +0100, Alexandru Gheorghe wrote:
> >>Add a resource manager object that is responsible for detecting all
> >>kms devices and allocates unique display numbers for every detected
> >>display.
> >>
> >>This is controlled by the value of hwc.drm.device property, if it ends
> >>with a %, it will try to open minor devices until and error is detected.
> >>E.g: /dev/dri/card%
> >
> >I'm a bit on the fence as to whether to use the %, or add a new
> >hwc.drm.scan_devices property. I guess since we need to convey the path anyways
> >this is fine, but it really needs to be better documented.
> 
> I'm looking at this stuff in another series about DRM Node probing[1],
> and I'll look into using properties to define what we are looking for.
> 
> But those properties won't be paths, but rather PCI IDs and driver vendor names.
> 
> As for what to do in the series, I don't have much of an opinion. But I'm
> likely to try to change it in the not too distant future.
> 
> 
> [1] https://www.spinics.net/lists/dri-devel/msg172496.html

Aren't this two complementary? 
This series try to go through all available nodes and yours provides a
mechanism to check if file descriptor match some expectations.
You still have to open them somehow.

> 
> >
> >>
> >>Additionally, this will be used for finding an available writeback
> >>connector that will be used for the flattening of the currently
> >>displayed scene.
> >>
> >>Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> >>---
> >>  Android.mk          |  1 +
> >>  resourcemanager.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  resourcemanager.h   | 29 ++++++++++++++++++++++
> >>  3 files changed, 101 insertions(+)
> >>  create mode 100644 resourcemanager.cpp
> >>  create mode 100644 resourcemanager.h
> >>
> >>diff --git a/Android.mk b/Android.mk
> >>index 1add286..736fe24 100644
> >>--- a/Android.mk
> >>+++ b/Android.mk
> >>@@ -52,6 +52,7 @@ LOCAL_C_INCLUDES := \
> >>  LOCAL_SRC_FILES := \
> >>  	autolock.cpp \
> >>+	resourcemanager.cpp \
> >>  	drmresources.cpp \
> >>  	drmconnector.cpp \
> >>  	drmcrtc.cpp \
> >>diff --git a/resourcemanager.cpp b/resourcemanager.cpp
> >>new file mode 100644
> >>index 0000000..e7b654e
> >>--- /dev/null
> >>+++ b/resourcemanager.cpp
> >>@@ -0,0 +1,71 @@
> >>+#include "resourcemanager.h"
> >>+#include <cutils/log.h>
> >>+#include <cutils/properties.h>
> >>+
> >>+namespace android {
> >>+
> >>+ResourceManager::ResourceManager() : gralloc_(NULL) {
> >>+}
> >>+
> >>+int ResourceManager::Init() {
> >>+  char path_pattern[PROPERTY_VALUE_MAX];
> >>+  property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
> >
> >We probably also don't want to default to scanning, since that might change
> >behavior in existing boards.
> >
> >>+
> >>+  uint8_t minor = 0;
> >
> >Please use unsigned/int instead of fixed-size types. Unless the number of bits
> >is relevant to use of the variable, let the compiler handle it.
> >
> >>+  int last_display_index = 0;
> >
> >Could we just call this num_displays? It was kind of hard for me to reason
> >through this, especially when it's call "start_display_index" when you jump into
> >drm::Init(). I also think drm->Init's return pair should return
> ><ret, displays_added> instead of <ret, display_index>, so each time you call
> >Init(), you're adding to num_displays.
> >
> >>+  int last_char = strlen(path_pattern) - 1;
> >>+  do {
> >>+    char path[PROPERTY_VALUE_MAX];
> >
> >Please use string
> >
> >>+    if (path_pattern[last_char] == '%') {
> >>+      path_pattern[last_char] = '\0';
> >>+      snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor);
> >>+      path_pattern[last_char] = '%';
> >
> >This doesn't work for minor > 10.
> >
> >>+    } else {
> >>+      snprintf(path, PROPERTY_VALUE_MAX, "%s", path_pattern);
> >>+    }
> >>+    std::unique_ptr<DrmResources> drm = std::make_unique<DrmResources>();
> >>+    last_display_index = drm->Init(this, path, last_display_index);
> >>+    if (last_display_index < 0) {
> >>+      break;
> >>+    }
> >>+    std::shared_ptr<Importer> importer;
> >>+    importer.reset(Importer::CreateInstance(drm.get()));
> >>+    if (!importer) {
> >>+      ALOGE("Failed to create importer instance");
> >>+      break;
> >>+    }
> >>+    importers_.push_back(importer);
> >>+    drms_.push_back(std::move(drm));
> >>+    minor++;
> >>+    last_display_index++;
> >>+  } while (path_pattern[last_char] == '%');
> >>+
> >>+  if (!drms_.size()) {
> >>+    ALOGE("Failed to find any working drm device");
> >>+    return -EINVAL;
> >>+  }
> >>+
> >>+  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> >>+                       (const hw_module_t **)&gralloc_);
> >>+}
> >
> >Consider the following:
> >
> >int ResourceManager::AddDrmDevice(std::string path) {
> >   std::unique_ptr<DrmDevice> drm = std::make_unique<DrmDevice>();
> >   int displays_added, ret;
> >   std::tie(displays_added, ret) = drm->Init(path.c_str(), num_displays_);
> >   if (ret)
> >     return ret;
> >
> >   std::shared_ptr<Importer> importer;
> >   importer.reset(Importer::CreateInstance(drm.get()));
> >   if (!importer) {
> >     ALOGE("Failed to create importer instance");
> >     return -ENODEV;
> >   }
> >
> >   importers_.push_back(importer);
> >   drms_.push_back(std::move(drm));
> >   num_displays_ += displays_added;
> >   return 0;
> >}
> >
> >int ResourceManager::Init() {
> >   char path_pattern[PROPERTY_VALUE_MAX];
> >   int path_len = property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
> >
> >   if (path_pattern[path_len - 1] != '%')
> >     return AddDrmDevice(std::string(path_pattern);
> >
> >   path_pattern[path_len - 1] = '\0';
> >   for (int ret = 0, idx = 0; !ret; ++idx) {
> >     ostringstream path;
> >     path << path_pattern << idx;
> >     ret = AddDrmDevice(path.str());
> >   }
> >   if (!num_displays_) {
> >     ALOGE("Failed to initialize any displays");
> >     return -EINVAL;
> >   }
> >
> >   return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> >                        (const hw_module_t **)&gralloc_);
> >}
> >
> >I think resolves the issues from the original patches and incorporates the
> >suggestions of drm->Init() returning the tuple of added displays, as well as
> >eliminating the backpointer.
> >
> >
> >>+
> >>+DrmResources *ResourceManager::GetDrmResources(int display) {
> >>+  for (uint32_t i = 0; i < drms_.size(); i++) {
> >
> >for (auto &drm_: drms_) {
> >
> >>+    if (drms_[i]->HandlesDisplay(display))
> >>+      return drms_[i].get();
> >>+  }
> >>+  return NULL;
> >>+}
> >>+
> >>+std::shared_ptr<Importer> ResourceManager::GetImporter(int display) {
> >>+  for (uint32_t i = 0; i < drms_.size(); i++) {
> >
> >Same here
> >
> >>+    if (drms_[i]->HandlesDisplay(display))
> >>+      return importers_[i];
> >>+  }
> >>+  return NULL;
> >>+}
> >>+
> >>+const gralloc_module_t *ResourceManager::GetGralloc() {
> >
> >I think this should be called gralloc()
> >
> >>+  return gralloc_;
> >>+}
> >>+}
> >>diff --git a/resourcemanager.h b/resourcemanager.h
> >>new file mode 100644
> >>index 0000000..b8caa9a
> >>--- /dev/null
> >>+++ b/resourcemanager.h
> >>@@ -0,0 +1,29 @@
> >>+#ifndef RESOURCEMANAGER_H
> >>+#define RESOURCEMANAGER_H
> >>+
> >>+#include "drmresources.h"
> >>+#include "platform.h"
> >>+
> >>+namespace android {
> >>+
> >>+class DrmResources;
> >>+class Importer;
> >
> >I think you need either the forward declarations or the includes, but not both?
> >
> >>+
> >>+class ResourceManager {
> >>+ public:
> >>+  ResourceManager();
> >>+  ResourceManager(const ResourceManager &) = delete;
> >>+  ResourceManager &operator=(const ResourceManager &) = delete;
> >>+  int Init();
> >>+  DrmResources *GetDrmResources(int display);
> >>+  std::shared_ptr<Importer> GetImporter(int display);
> >>+  const gralloc_module_t *GetGralloc();
> >>+
> >>+ private:
> >>+  std::vector<std::unique_ptr<DrmResources>> drms_;
> >>+  std::vector<std::shared_ptr<Importer>> importers_;
> >>+  const gralloc_module_t *gralloc_;
> >>+};
> >>+}
> >>+
> >>+#endif  // RESOURCEMANAGER_H
> >>-- 
> >>2.7.4
> >>
> >

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel