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

[Sean Paul <seanpaul@chromium.org>]

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
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel