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@xxxxxxx> > >>--- > >> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
