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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <alexandru-cosmin.gheorghe@xxxxxxx>
> > ---
> >  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.

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

Will do.

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

I avoided pair because it looks so ugly, but suppose it's better than
not knowing if it's an error code or something else. 
I will update it to return a pair.

> 
> > +}
> > +
> > +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?

I blame clang-format-diff-3.8 -i. I suppose it should be in a different
commit.
> 
> > +#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

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux