[PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class
Alexandru-Cosmin Gheorghe
Alexandru-Cosmin.Gheorghe at arm.com
Wed Apr 18 10:12:09 UTC 2018
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 at 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
More information about the dri-devel
mailing list