[PATCH hwc v2 05/18] drm_hwcomposer: Enable resource manager support
Sean Paul
seanpaul at chromium.org
Mon Apr 16 19:54:02 UTC 2018
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 at 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
More information about the dri-devel
mailing list