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

[Sean Paul]

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 
> ---



> index 4cca48c..4cdcd87 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -17,22 +17,26 @@
>  #ifndef ANDROID_DRM_H_
>  #define ANDROID_DRM_H_
>  
> +#include 
>  #include "drmconnector.h"
>  #include "drmcrtc.h"
>  #include "drmencoder.h"
>  #include "drmeventlistener.h"
>  #include "drmplane.h"
> -
> -#include 
> +#include "platform.h"
> +#include "resourcemanager.h"
>  
>  namespace android {
>  
> +class ResourceManager;
> +
>  class DrmResources {

One more thing I've been thinking about. Let's rename this to DrmDevice now that
we can have more than one. It's immediately obvious what a collection of
DrmDevices is, it's less obvious if they're DrmResources. I think
ResourceManager is Ok to keep, but if you think there's a better name I'm open
to that.

Sean

>   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 , 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 min_resolution_;
>std::pair max_resolution_;
> +  std::map 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

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

[Sean Paul]

On Tue, Apr 17, 2018 at 02:43:17PM +0100, Alexandru-Cosmin Gheorghe wrote:
> Hi Sean,
> 
> On Mon, Apr 16, 2018 at 03:54:02PM -0400, Sean Paul wrote:
> > 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 
> > > ---



> > > 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.
> 
> That's the thing Compositor/Composition already had drm_, hence the
> need of the backpointer. Didn't want to touch that as well. I suppose
> there is no strong reason why both Compositor & Composition shouldn't
> have just a ResourceManager object.

Yeah, exactly what I was thinking. It'll be a bit more refactoring, but worth it
IMO.



> > > 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 
> > >  #include "drmconnector.h"
> > >  #include "drmcrtc.h"
> > >  #include "drmencoder.h"
> > >  #include "drmeventlistener.h"
> > >  #include "drmplane.h"
> > > -
> > > -#include 
> > 
> > Why this change?
> 
> I blame clang-format-diff-3.8 -i. I suppose it should be in a different
> commit.

I'd rather fix it at the source. If stdint.h include needs to move, that
probably means one of our headers needs to include it. So let's figure out which
needs it and add it there instead of shuffling things here.

Sean

> > 
> > > +#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 , 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 min_resolution_;
> > >std::pair max_resolution_;
> > > +  std::map displays_;
> > > +  ResourceManager *resource_manager_;
> > >  };
> > >  }
> > >  
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

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

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

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 03:54:02PM -0400, Sean Paul wrote:
> 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 
> > ---
> >  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(_));
> > -  if (!importer_) {
> > -ALOGE("Failed to create importer instance");
> > +  DrmResources *drm = 
> > resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
> > +  std::shared_ptr 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 **)_);
> > -  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(_, importer_, gralloc_,
> > -  HWC_DISPLAY_PRIMARY,
> > -  HWC2::DisplayType::Physical));
> > -
> > -  DrmCrtc *crtc = 
> > drm_.GetCrtcForDisplay(static_cast(HWC_DISPLAY_PRIMARY));
> > +  DrmCrtc *crtc = 
> > drm->GetCrtcForDisplay(static_cast(HWC_DISPLAY_PRIMARY));
> >if (!crtc) {
> >  ALOGE("Failed to get crtc for display %d",
> >static_cast(HWC_DISPLAY_PRIMARY));
> >  return HWC2::Error::BadDisplay;
> >}
> > -
> >std::vector display_planes;
> > -  for (auto  : drm_.planes()) {
> > +  for (auto  : 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_;  // Shared with HwcDisplay
> > -  const gralloc_module_t *gralloc_;
> > +  ResourceManager resource_manager_;
> >std::map displays_;
> >std::map 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.

That's the thing Compositor/Composition already had drm_, hence the
need of the backpointer. Didn't want to touch that as well. I suppose
there is no strong reason why both Compositor & Composition shouldn't
have just a ResourceManager object.

> 
> >/* 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(res->max_width, res->max_height);
> >  
> > -  bool found_primary = false;
> > -  int display_num = 1;
> > +  bool 

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

[Sean Paul]

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 
> ---
>  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(_));
> -  if (!importer_) {
> -ALOGE("Failed to create importer instance");
> +  DrmResources *drm = resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
> +  std::shared_ptr 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 **)_);
> -  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(_, importer_, gralloc_,
> -  HWC_DISPLAY_PRIMARY,
> -  HWC2::DisplayType::Physical));
> -
> -  DrmCrtc *crtc = 
> drm_.GetCrtcForDisplay(static_cast(HWC_DISPLAY_PRIMARY));
> +  DrmCrtc *crtc = 
> drm->GetCrtcForDisplay(static_cast(HWC_DISPLAY_PRIMARY));
>if (!crtc) {
>  ALOGE("Failed to get crtc for display %d",
>static_cast(HWC_DISPLAY_PRIMARY));
>  return HWC2::Error::BadDisplay;
>}
> -
>std::vector display_planes;
> -  for (auto  : drm_.planes()) {
> +  for (auto  : 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_;  // Shared with HwcDisplay
> -  const gralloc_module_t *gralloc_;
> +  ResourceManager resource_manager_;
>std::map displays_;
>std::map 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(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  : connectors_) {
>  if (conn->internal() && !found_primary) {
>conn->set_display(0);
> +  

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

[Alexandru Gheorghe]

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 
---
 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(_));
-  if (!importer_) {
-ALOGE("Failed to create importer instance");
+  DrmResources *drm = resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
+  std::shared_ptr 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 **)_);
-  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(_, importer_, gralloc_,
-  HWC_DISPLAY_PRIMARY,
-  HWC2::DisplayType::Physical));
-
-  DrmCrtc *crtc = 
drm_.GetCrtcForDisplay(static_cast(HWC_DISPLAY_PRIMARY));
+  DrmCrtc *crtc = 
drm->GetCrtcForDisplay(static_cast(HWC_DISPLAY_PRIMARY));
   if (!crtc) {
 ALOGE("Failed to get crtc for display %d",
   static_cast(HWC_DISPLAY_PRIMARY));
 return HWC2::Error::BadDisplay;
   }
-
   std::vector display_planes;
-  for (auto  : drm_.planes()) {
+  for (auto  : 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_;  // Shared with HwcDisplay
-  const gralloc_module_t *gralloc_;
+  ResourceManager resource_manager_;
   std::map displays_;
   std::map 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;
   /* 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(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;
 
   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  : 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  : 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;
+}
+
+bool