Re: [PATCH hwc v2 05/18] drm_hwcomposer: Enable resource manager support

[Sean Paul <seanpaul@chromium.org>]

On Wed, Apr 11, 2018 at 04:22:16PM +0100, Alexandru Gheorghe wrote:
> Use the newly added ResourceManager for creating and detecting all the
> drm devices instead of assuming that there is only one device.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmhwctwo.cpp    | 34 +++++++++++++---------------------
>  drmhwctwo.h      |  4 +---
>  drmresources.cpp | 25 ++++++++++++++++++-------
>  drmresources.h   | 14 +++++++++++---
>  4 files changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index dfca1a6..cddd5da 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -58,40 +58,32 @@ DrmHwcTwo::DrmHwcTwo() {
>  }
>  
>  HWC2::Error DrmHwcTwo::Init() {
> -  int ret = drm_.Init();
> +  int ret = resource_manager_.Init();
>    if (ret) {
> -    ALOGE("Can't initialize drm object %d", ret);
> +    ALOGE("Can't initialize the resource manager %d", ret);
>      return HWC2::Error::NoResources;
>    }
>  
> -  importer_.reset(Importer::CreateInstance(&drm_));
> -  if (!importer_) {
> -    ALOGE("Failed to create importer instance");
> +  DrmResources *drm = resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
> +  std::shared_ptr<Importer> importer =
> +      resource_manager_.GetImporter(HWC_DISPLAY_PRIMARY);
> +  if (!drm || !importer) {
> +    ALOGE("Failed to get a valid drmresource and importer");
>      return HWC2::Error::NoResources;
>    }
> +  displays_.emplace(
> +      std::piecewise_construct, std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
> +      std::forward_as_tuple(drm, importer, resource_manager_.GetGralloc(),
> +                            HWC_DISPLAY_PRIMARY, HWC2::DisplayType::Physical));
>  
> -  ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> -                      (const hw_module_t **)&gralloc_);
> -  if (ret) {
> -    ALOGE("Failed to open gralloc module %d", ret);
> -    return HWC2::Error::NoResources;
> -  }
> -
> -  displays_.emplace(std::piecewise_construct,
> -                    std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
> -                    std::forward_as_tuple(&drm_, importer_, gralloc_,
> -                                          HWC_DISPLAY_PRIMARY,
> -                                          HWC2::DisplayType::Physical));
> -
> -  DrmCrtc *crtc = drm_.GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
> +  DrmCrtc *crtc = drm->GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
>    if (!crtc) {
>      ALOGE("Failed to get crtc for display %d",
>            static_cast<int>(HWC_DISPLAY_PRIMARY));
>      return HWC2::Error::BadDisplay;
>    }
> -
>    std::vector<DrmPlane *> display_planes;
> -  for (auto &plane : drm_.planes()) {
> +  for (auto &plane : drm->planes()) {
>      if (plane->GetCrtcSupported(*crtc))
>        display_planes.push_back(plane.get());
>    }
> diff --git a/drmhwctwo.h b/drmhwctwo.h
> index 0490e2a..beb5d2d 100644
> --- a/drmhwctwo.h
> +++ b/drmhwctwo.h
> @@ -262,9 +262,7 @@ class DrmHwcTwo : public hwc2_device_t {
>    HWC2::Error RegisterCallback(int32_t descriptor, hwc2_callback_data_t data,
>                                 hwc2_function_pointer_t function);
>  
> -  DrmResources drm_;
> -  std::shared_ptr<Importer> importer_;  // Shared with HwcDisplay
> -  const gralloc_module_t *gralloc_;
> +  ResourceManager resource_manager_;
>    std::map<hwc2_display_t, HwcDisplay> displays_;
>    std::map<HWC2::Callback, HwcCallback> callbacks_;
>  };
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 32dd376..a5ddda0 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -42,10 +42,9 @@ DrmResources::~DrmResources() {
>    event_listener_.Exit();
>  }
>  
> -int DrmResources::Init() {
> -  char path[PROPERTY_VALUE_MAX];
> -  property_get("hwc.drm.device", path, "/dev/dri/card0");
> -
> +int DrmResources::Init(ResourceManager *resource_manager, char *path,
> +                       int start_display_index) {
> +  resource_manager_ = resource_manager;

You can avoid the backpointer if you just pass the RM to the right places (looks
like compositor + composition). Bonus points if you can remove drm_ from those
objects once you've done that.

>    /* TODO: Use drmOpenControl here instead */
>    fd_.Set(open(path, O_RDWR));
>    if (fd() < 0) {
> @@ -76,8 +75,8 @@ int DrmResources::Init() {
>    max_resolution_ =
>        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
>  
> -  bool found_primary = false;
> -  int display_num = 1;
> +  bool found_primary = start_display_index != 0;
> +  int display_num = found_primary ? start_display_index : 1;

This could use a comment. AFAICT, you're assuming the primary display will
always be in the first drm device, which is fine, but should be explained
_somewhere_.

>  
>    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
>      drmModeCrtcPtr c = drmModeGetCrtc(fd(), res->crtcs[i]);
> @@ -161,9 +160,11 @@ int DrmResources::Init() {
>    for (auto &conn : connectors_) {
>      if (conn->internal() && !found_primary) {
>        conn->set_display(0);
> +      displays_[0] = 0;
>        found_primary = true;
>      } else {
>        conn->set_display(display_num);
> +      displays_[display_num] = display_num;
>        ++display_num;
>      }
>    }
> @@ -171,7 +172,9 @@ int DrmResources::Init() {
>    // Then look for primary amongst external connectors
>    for (auto &conn : connectors_) {
>      if (conn->external() && !found_primary) {
> +      displays_.erase(conn->display());
>        conn->set_display(0);
> +      displays_[0] = 0;
>        found_primary = true;
>      }
>    }
> @@ -226,7 +229,11 @@ int DrmResources::Init() {
>        return ret;
>      }
>    }
> -  return 0;
> +  return displays_.size() ? displays_.rbegin()->first : -EINVAL;

I'd rather not change the meaning of the return value (especially without a
comment somewhere to let readers know this function doesn't follow the 0 ||
-ERRNO convention). Consider returning a pair of ret,display.

> +}
> +
> +bool DrmResources::HandlesDisplay(int display) const {
> +  return displays_.find(display) != displays_.end();
>  }
>  
>  DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
> @@ -349,6 +356,10 @@ DrmEventListener *DrmResources::event_listener() {
>    return &event_listener_;
>  }
>  
> +ResourceManager *DrmResources::resource_manager() {
> +  return resource_manager_;
> +}
> +
>  int DrmResources::GetProperty(uint32_t obj_id, uint32_t obj_type,
>                                const char *prop_name, DrmProperty *property) {
>    drmModeObjectPropertiesPtr props;
> diff --git a/drmresources.h b/drmresources.h
> index 4cca48c..4cdcd87 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -17,22 +17,26 @@
>  #ifndef ANDROID_DRM_H_
>  #define ANDROID_DRM_H_
>  
> +#include <stdint.h>
>  #include "drmconnector.h"
>  #include "drmcrtc.h"
>  #include "drmencoder.h"
>  #include "drmeventlistener.h"
>  #include "drmplane.h"
> -
> -#include <stdint.h>

Why this change?

> +#include "platform.h"
> +#include "resourcemanager.h"
>  
>  namespace android {
>  
> +class ResourceManager;
> +
>  class DrmResources {
>   public:
>    DrmResources();
>    ~DrmResources();
>  
> -  int Init();
> +  int Init(ResourceManager *resource_manager, char *path,
> +           int start_display_index);
>  
>    int fd() const {
>      return fd_.get();
> @@ -58,6 +62,7 @@ class DrmResources {
>    DrmCrtc *GetCrtcForDisplay(int display) const;
>    DrmPlane *GetPlane(uint32_t id) const;
>    DrmEventListener *event_listener();
> +  ResourceManager *resource_manager();
>  
>    int GetPlaneProperty(const DrmPlane &plane, const char *prop_name,
>                         DrmProperty *property);
> @@ -71,6 +76,7 @@ class DrmResources {
>  
>    int CreatePropertyBlob(void *data, size_t length, uint32_t *blob_id);
>    int DestroyPropertyBlob(uint32_t blob_id);
> +  bool HandlesDisplay(int display) const;
>  
>   private:
>    int TryEncoderForDisplay(int display, DrmEncoder *enc);
> @@ -90,6 +96,8 @@ class DrmResources {
>  
>    std::pair<uint32_t, uint32_t> min_resolution_;
>    std::pair<uint32_t, uint32_t> max_resolution_;
> +  std::map<int, int> displays_;
> +  ResourceManager *resource_manager_;
>  };
>  }
>  
> -- 
> 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