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

[Robert Foss <robert.foss@collabora.com>]

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