| Message ID | 1523460149-1740-5-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 >
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 > >> >> 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 >> >
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 > >> > >
On 04/18/2018 12:12 PM, Alexandru-Cosmin Gheorghe wrote: > 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. Yes, they are, I'm just noting that I'll prod around this area and change it in not too long, whatever decision is made here I'm likely to tweak it :) Rob. > >> >>> >>>> >>>> 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 >>>> >>> >
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%"); + + uint8_t minor = 0; + int last_display_index = 0; + int last_char = strlen(path_pattern) - 1; + do { + char path[PROPERTY_VALUE_MAX]; + if (path_pattern[last_char] == '%') { + path_pattern[last_char] = '\0'; + snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor); + path_pattern[last_char] = '%'; + } 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_); +} + +DrmResources *ResourceManager::GetDrmResources(int display) { + for (uint32_t i = 0; i < drms_.size(); i++) { + 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++) { + if (drms_[i]->HandlesDisplay(display)) + return importers_[i]; + } + return NULL; +} + +const gralloc_module_t *ResourceManager::GetGralloc() { + 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; + +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
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% 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