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.
>
> 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
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
