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
