From 36a7f28516a424648745b8431e3ff43e8228b2b0 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Tue, 5 Oct 2021 18:17:53 +0300 Subject: drm_hwcomposer: Rework display Mode Setting and DPMS handling 1. Remove DPMS logic. As stated in KMS kernel docs: CRTC "activate" property was implemented as a simplified atomic replacement of DPMS. And kernel internally will just update "activate" on DPMS setting. 2. Add SetDisplayActivate(bool state) method to compositor class, which is now replacement for SetDpmsMode(). 3. Move mode settings out of DrmComposition class to DrmCompositor class. From now on DrmComposition describes only layer-to-plane composition as it should be. Signed-off-by: Roman Stratiienko --- DrmHwcTwo.cpp | 25 ++++----- compositor/DrmDisplayComposition.cpp | 35 +------------ compositor/DrmDisplayComposition.h | 34 +------------ compositor/DrmDisplayCompositor.cpp | 98 ++++++++---------------------------- compositor/DrmDisplayCompositor.h | 14 +++--- 5 files changed, 40 insertions(+), 166 deletions(-) diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index 6ec8b31..071f3bf 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -716,7 +716,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) { // TODO(nobody): Don't always assume geometry changed int ret = composition->SetLayers(composition_layers.data(), - composition_layers.size(), true); + composition_layers.size()); if (ret) { ALOGE("Failed to set layers in the composition ret=%d", ret); return HWC2::Error::BadLayer; @@ -793,15 +793,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) { return HWC2::Error::BadConfig; } - auto composition = std::make_unique(crtc_, - planner_.get()); - int ret = composition->SetDisplayMode(*mode); - if (ret) { + if (!compositor_.SetDisplayMode(*mode)) { return HWC2::Error::BadConfig; } - ret = compositor_.ApplyComposition(std::move(composition)); - if (ret) { - ALOGE("Failed to queue dpms composition on %d", ret); + int err = compositor_.ApplyComposition( + compositor_.CreateInitializedComposition()); + if (err != 0) { + ALOGE("Failed to queue mode changing commit %d", err); return HWC2::Error::BadConfig; } @@ -883,14 +881,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetOutputBuffer(buffer_handle_t buffer, HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) { supported(__func__); - uint64_t dpms_value = 0; auto mode = static_cast(mode_in); switch (mode) { case HWC2::PowerMode::Off: - dpms_value = DRM_MODE_DPMS_OFF; + compositor_.SetDisplayActive(false); break; case HWC2::PowerMode::On: - dpms_value = DRM_MODE_DPMS_ON; + compositor_.SetDisplayActive(true); break; case HWC2::PowerMode::Doze: case HWC2::PowerMode::DozeSuspend: @@ -900,10 +897,8 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) { return HWC2::Error::BadParameter; }; - auto composition = std::make_unique(crtc_, - planner_.get()); - composition->SetDpmsMode(dpms_value); - int ret = compositor_.ApplyComposition(std::move(composition)); + int ret = compositor_.ApplyComposition( + compositor_.CreateInitializedComposition()); if (ret) { ALOGE("Failed to apply the dpms composition ret=%d", ret); return HWC2::Error::BadParameter; diff --git a/compositor/DrmDisplayComposition.cpp b/compositor/DrmDisplayComposition.cpp index c0fbba0..cd95267 100644 --- a/compositor/DrmDisplayComposition.cpp +++ b/compositor/DrmDisplayComposition.cpp @@ -37,41 +37,11 @@ DrmDisplayComposition::DrmDisplayComposition(DrmCrtc *crtc, Planner *planner) planner_(planner) { } -bool DrmDisplayComposition::validate_composition_type(DrmCompositionType des) { - return type_ == DRM_COMPOSITION_TYPE_EMPTY || type_ == des; -} - -int DrmDisplayComposition::SetLayers(DrmHwcLayer *layers, size_t num_layers, - bool geometry_changed) { - if (!validate_composition_type(DRM_COMPOSITION_TYPE_FRAME)) - return -EINVAL; - - geometry_changed_ = geometry_changed; - +int DrmDisplayComposition::SetLayers(DrmHwcLayer *layers, size_t num_layers) { for (size_t layer_index = 0; layer_index < num_layers; layer_index++) { layers_.emplace_back(std::move(layers[layer_index])); } - type_ = DRM_COMPOSITION_TYPE_FRAME; - return 0; -} - -int DrmDisplayComposition::SetDpmsMode(uint32_t dpms_mode) { - if (!validate_composition_type(DRM_COMPOSITION_TYPE_DPMS)) - return -EINVAL; - dpms_mode_ = dpms_mode; - type_ = DRM_COMPOSITION_TYPE_DPMS; - return 0; -} - -int DrmDisplayComposition::SetDisplayMode(const DrmMode &display_mode) { - if (!validate_composition_type(DRM_COMPOSITION_TYPE_MODESET)) { - ALOGE("SetDisplayMode() Failed to validate composition type"); - return -EINVAL; - } - display_mode_ = display_mode; - dpms_mode_ = DRM_MODE_DPMS_ON; - type_ = DRM_COMPOSITION_TYPE_MODESET; return 0; } @@ -87,9 +57,6 @@ int DrmDisplayComposition::AddPlaneComposition(DrmCompositionPlane plane) { int DrmDisplayComposition::Plan(std::vector *primary_planes, std::vector *overlay_planes) { - if (type_ != DRM_COMPOSITION_TYPE_FRAME) - return 0; - std::map to_composite; for (size_t i = 0; i < layers_.size(); ++i) diff --git a/compositor/DrmDisplayComposition.h b/compositor/DrmDisplayComposition.h index bbac0af..7b7e668 100644 --- a/compositor/DrmDisplayComposition.h +++ b/compositor/DrmDisplayComposition.h @@ -32,13 +32,6 @@ namespace android { class Importer; class Planner; -enum DrmCompositionType { - DRM_COMPOSITION_TYPE_EMPTY, - DRM_COMPOSITION_TYPE_FRAME, - DRM_COMPOSITION_TYPE_DPMS, - DRM_COMPOSITION_TYPE_MODESET, -}; - class DrmCompositionPlane { public: enum class Type : int32_t { @@ -86,11 +79,9 @@ class DrmDisplayComposition { DrmDisplayComposition(DrmCrtc *crtc, Planner *planner); ~DrmDisplayComposition() = default; - int SetLayers(DrmHwcLayer *layers, size_t num_layers, bool geometry_changed); + int SetLayers(DrmHwcLayer *layers, size_t num_layers); int AddPlaneComposition(DrmCompositionPlane plane); int AddPlaneDisable(DrmPlane *plane); - int SetDpmsMode(uint32_t dpms_mode); - int SetDisplayMode(const DrmMode &display_mode); int Plan(std::vector *primary_planes, std::vector *overlay_planes); @@ -103,22 +94,6 @@ class DrmDisplayComposition { return composition_planes_; } - bool geometry_changed() const { - return geometry_changed_; - } - - DrmCompositionType type() const { - return type_; - } - - uint32_t dpms_mode() const { - return dpms_mode_; - } - - const DrmMode &display_mode() const { - return display_mode_; - } - DrmCrtc *crtc() const { return crtc_; } @@ -130,16 +105,9 @@ class DrmDisplayComposition { UniqueFd out_fence_; private: - bool validate_composition_type(DrmCompositionType desired); - DrmCrtc *crtc_ = NULL; Planner *planner_ = NULL; - DrmCompositionType type_ = DRM_COMPOSITION_TYPE_EMPTY; - uint32_t dpms_mode_ = DRM_MODE_DPMS_ON; - DrmMode display_mode_; - - bool geometry_changed_ = true; std::vector layers_; std::vector composition_planes_; }; diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp index 576c533..a8f8d62 100644 --- a/compositor/DrmDisplayCompositor.cpp +++ b/compositor/DrmDisplayCompositor.cpp @@ -44,8 +44,7 @@ DrmDisplayCompositor::DrmDisplayCompositor() : resource_manager_(nullptr), display_(-1), initialized_(false), - active_(false), - use_hw_overlays_(true) { + active_(false) { } DrmDisplayCompositor::~DrmDisplayCompositor() { @@ -205,43 +204,21 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, } } - if (!test_only && mode_.blob) { - /* TODO: Add dpms to the pset when the kernel supports it */ - ret = ApplyDpms(display_comp); - if (ret) { - ALOGE("Failed to apply DPMS after modeset %d\n", ret); - return ret; + if (!test_only) { + if (mode_.blob) { + connector->set_active_mode(mode_.mode); + mode_.old_blob = std::move(mode_.blob); } + active_changed_ = false; - connector->set_active_mode(mode_.mode); - mode_.old_blob = std::move(mode_.blob); - } - - if (crtc->out_fence_ptr_property()) { - display_comp->out_fence_ = UniqueFd((int)out_fences[crtc->pipe()]); + if (crtc->out_fence_ptr_property()) { + display_comp->out_fence_ = UniqueFd((int)out_fences[crtc->pipe()]); + } } return ret; } -int DrmDisplayCompositor::ApplyDpms(DrmDisplayComposition *display_comp) { - DrmDevice *drm = resource_manager_->GetDrmDevice(display_); - DrmConnector *conn = drm->GetConnectorForDisplay(display_); - if (!conn) { - ALOGE("Failed to get DrmConnector for display %d", display_); - return -ENODEV; - } - - const DrmProperty &prop = conn->dpms_property(); - int ret = drmModeConnectorSetProperty(drm->fd(), conn->id(), prop.id(), - display_comp->dpms_mode()); - if (ret) { - ALOGE("Failed to set DPMS property for connector %d", conn->id()); - return ret; - } - return 0; -} - auto DrmDisplayCompositor::CreateModeBlob(const DrmMode &mode) -> DrmModeUserPropertyBlobUnique { struct drm_mode_modeinfo drm_mode {}; @@ -262,59 +239,20 @@ void DrmDisplayCompositor::ClearDisplay() { active_composition_.reset(nullptr); } -void DrmDisplayCompositor::ApplyFrame( - std::unique_ptr composition, int status) { - int ret = status; - - if (!ret) { - ret = CommitFrame(composition.get(), false); - } +int DrmDisplayCompositor::ApplyComposition( + std::unique_ptr composition) { + int ret = CommitFrame(composition.get(), false); if (ret) { ALOGE("Composite failed for display %d", display_); // Disable the hw used by the last active composition. This allows us to // signal the release fences from that composition to avoid hanging. ClearDisplay(); - return; + return ret; } - active_composition_.swap(composition); -} - -int DrmDisplayCompositor::ApplyComposition( - std::unique_ptr composition) { - int ret = 0; - switch (composition->type()) { - case DRM_COMPOSITION_TYPE_FRAME: - if (composition->geometry_changed()) { - // Send the composition to the kernel to ensure we can commit it. This - // is just a test, it won't actually commit the frame. - ret = CommitFrame(composition.get(), true); - if (ret) { - ALOGE("Commit test failed for display %d, FIXME", display_); - return ret; - } - } - - ApplyFrame(std::move(composition), ret); - break; - case DRM_COMPOSITION_TYPE_DPMS: - active_ = (composition->dpms_mode() == DRM_MODE_DPMS_ON); - ret = ApplyDpms(composition.get()); - if (ret) - ALOGE("Failed to apply dpms for display %d", display_); - return ret; - case DRM_COMPOSITION_TYPE_MODESET: - mode_.mode = composition->display_mode(); - mode_.blob = CreateModeBlob(mode_.mode); - if (!mode_.blob) { - ALOGE("Failed to create mode blob for display %d", display_); - return -EINVAL; - } - return 0; - default: - ALOGE("Unknown composition type %d", composition->type()); - return -EINVAL; + if (composition) { + active_composition_.swap(composition); } return ret; @@ -324,4 +262,10 @@ int DrmDisplayCompositor::TestComposition(DrmDisplayComposition *composition) { return CommitFrame(composition, true); } +auto DrmDisplayCompositor::SetDisplayMode(const DrmMode &display_mode) -> bool { + mode_.mode = display_mode; + mode_.blob = CreateModeBlob(mode_.mode); + return !!mode_.blob; +} + } // namespace android diff --git a/compositor/DrmDisplayCompositor.h b/compositor/DrmDisplayCompositor.h index 3227e12..e76abf7 100644 --- a/compositor/DrmDisplayCompositor.h +++ b/compositor/DrmDisplayCompositor.h @@ -44,8 +44,13 @@ class DrmDisplayCompositor { std::unique_ptr CreateInitializedComposition() const; int ApplyComposition(std::unique_ptr composition); int TestComposition(DrmDisplayComposition *composition); - int Composite(); void ClearDisplay(); + auto SetDisplayMode(const DrmMode &display_mode) -> bool; + auto SetDisplayActive(bool state) -> void { + active_ = state; + active_changed_ = true; + } + UniqueFd TakeOutFence() { if (!active_composition_) { return UniqueFd(); @@ -65,12 +70,8 @@ class DrmDisplayCompositor { DrmDisplayCompositor(const DrmDisplayCompositor &) = delete; int CommitFrame(DrmDisplayComposition *display_comp, bool test_only); - int ApplyDpms(DrmDisplayComposition *display_comp); int DisablePlanes(DrmDisplayComposition *display_comp); - void ApplyFrame(std::unique_ptr composition, - int status); - auto CreateModeBlob(const DrmMode &mode) -> DrmModeUserPropertyBlobUnique; ResourceManager *resource_manager_; @@ -79,8 +80,7 @@ class DrmDisplayCompositor { std::unique_ptr active_composition_; bool initialized_; - bool active_; - bool use_hw_overlays_; + bool active_{true}, active_changed_{true}; ModeState mode_; -- cgit v1.2.3 From 13f61b8bedf0922db0f4c98d71e5e8f7f68bc5aa Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Thu, 7 Oct 2021 11:27:08 +0300 Subject: drm_hwcomposer: Remove DrmDisplayCompositor::GetActiveModeResolution() DrmDisplayCompositor::GetActiveModeResolution() isn't used, remove it. Signed-off-by: Roman Stratiienko --- compositor/DrmDisplayCompositor.cpp | 14 -------------- compositor/DrmDisplayCompositor.h | 2 -- 2 files changed, 16 deletions(-) diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp index a8f8d62..9d3e56d 100644 --- a/compositor/DrmDisplayCompositor.cpp +++ b/compositor/DrmDisplayCompositor.cpp @@ -81,20 +81,6 @@ DrmDisplayCompositor::CreateInitializedComposition() const { return std::make_unique(crtc, planner_.get()); } -std::tuple -DrmDisplayCompositor::GetActiveModeResolution() { - DrmDevice *drm = resource_manager_->GetDrmDevice(display_); - DrmConnector *connector = drm->GetConnectorForDisplay(display_); - if (connector == nullptr) { - ALOGE("Failed to determine display mode: no connector for display %d", - display_); - return std::make_tuple(0, 0, -ENODEV); - } - - const DrmMode &mode = connector->active_mode(); - return std::make_tuple(mode.h_display(), mode.v_display(), 0); -} - int DrmDisplayCompositor::DisablePlanes(DrmDisplayComposition *display_comp) { auto pset = MakeDrmModeAtomicReqUnique(); if (!pset) { diff --git a/compositor/DrmDisplayCompositor.h b/compositor/DrmDisplayCompositor.h index e76abf7..17dc701 100644 --- a/compositor/DrmDisplayCompositor.h +++ b/compositor/DrmDisplayCompositor.h @@ -58,8 +58,6 @@ class DrmDisplayCompositor { return std::move(active_composition_->out_fence_); } - std::tuple GetActiveModeResolution(); - private: struct ModeState { DrmMode mode; -- cgit v1.2.3 From 40b374b1400d668c9975c5c3d239d54c84429dc6 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Fri, 22 Oct 2021 18:13:09 +0300 Subject: drm_hwcomposer: Move CreateModeBlob to DrmMode class CreateModeBlob doesn't architecturally belongs to DrmDisplayCompositor. + align DrmMode internal types with libdrm types to avoid compilation errors. Signed-off-by: Roman Stratiienko --- compositor/DrmDisplayCompositor.cpp | 12 +------ compositor/DrmDisplayCompositor.h | 2 -- drm/DrmMode.cpp | 63 ++++++++++++++++++++----------------- drm/DrmMode.h | 53 +++++++++++++++++-------------- 4 files changed, 65 insertions(+), 65 deletions(-) diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp index 9d3e56d..bb6a33b 100644 --- a/compositor/DrmDisplayCompositor.cpp +++ b/compositor/DrmDisplayCompositor.cpp @@ -205,16 +205,6 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, return ret; } -auto DrmDisplayCompositor::CreateModeBlob(const DrmMode &mode) - -> DrmModeUserPropertyBlobUnique { - struct drm_mode_modeinfo drm_mode {}; - mode.ToDrmModeModeInfo(&drm_mode); - - DrmDevice *drm = resource_manager_->GetDrmDevice(display_); - return drm->RegisterUserPropertyBlob(&drm_mode, - sizeof(struct drm_mode_modeinfo)); -} - void DrmDisplayCompositor::ClearDisplay() { if (!active_composition_) return; @@ -250,7 +240,7 @@ int DrmDisplayCompositor::TestComposition(DrmDisplayComposition *composition) { auto DrmDisplayCompositor::SetDisplayMode(const DrmMode &display_mode) -> bool { mode_.mode = display_mode; - mode_.blob = CreateModeBlob(mode_.mode); + mode_.blob = mode_.mode.CreateModeBlob(*resource_manager_->GetDrmDevice(display_)); return !!mode_.blob; } diff --git a/compositor/DrmDisplayCompositor.h b/compositor/DrmDisplayCompositor.h index 17dc701..434668f 100644 --- a/compositor/DrmDisplayCompositor.h +++ b/compositor/DrmDisplayCompositor.h @@ -70,8 +70,6 @@ class DrmDisplayCompositor { int CommitFrame(DrmDisplayComposition *display_comp, bool test_only); int DisablePlanes(DrmDisplayComposition *display_comp); - auto CreateModeBlob(const DrmMode &mode) -> DrmModeUserPropertyBlobUnique; - ResourceManager *resource_manager_; int display_; diff --git a/drm/DrmMode.cpp b/drm/DrmMode.cpp index c714458..971c327 100644 --- a/drm/DrmMode.cpp +++ b/drm/DrmMode.cpp @@ -49,24 +49,6 @@ bool DrmMode::operator==(const drmModeModeInfo &m) const { v_scan_ == m.vscan && flags_ == m.flags && type_ == m.type; } -void DrmMode::ToDrmModeModeInfo(drm_mode_modeinfo *m) const { - m->clock = clock_; - m->hdisplay = h_display_; - m->hsync_start = h_sync_start_; - m->hsync_end = h_sync_end_; - m->htotal = h_total_; - m->hskew = h_skew_; - m->vdisplay = v_display_; - m->vsync_start = v_sync_start_; - m->vsync_end = v_sync_end_; - m->vtotal = v_total_; - m->vscan = v_scan_; - m->vrefresh = v_refresh_; - m->flags = flags_; - m->type = type_; - strncpy(m->name, name_.c_str(), DRM_DISPLAY_MODE_LEN); -} - uint32_t DrmMode::id() const { return id_; } @@ -79,43 +61,43 @@ uint32_t DrmMode::clock() const { return clock_; } -uint32_t DrmMode::h_display() const { +uint16_t DrmMode::h_display() const { return h_display_; } -uint32_t DrmMode::h_sync_start() const { +uint16_t DrmMode::h_sync_start() const { return h_sync_start_; } -uint32_t DrmMode::h_sync_end() const { +uint16_t DrmMode::h_sync_end() const { return h_sync_end_; } -uint32_t DrmMode::h_total() const { +uint16_t DrmMode::h_total() const { return h_total_; } -uint32_t DrmMode::h_skew() const { +uint16_t DrmMode::h_skew() const { return h_skew_; } -uint32_t DrmMode::v_display() const { +uint16_t DrmMode::v_display() const { return v_display_; } -uint32_t DrmMode::v_sync_start() const { +uint16_t DrmMode::v_sync_start() const { return v_sync_start_; } -uint32_t DrmMode::v_sync_end() const { +uint16_t DrmMode::v_sync_end() const { return v_sync_end_; } -uint32_t DrmMode::v_total() const { +uint16_t DrmMode::v_total() const { return v_total_; } -uint32_t DrmMode::v_scan() const { +uint16_t DrmMode::v_scan() const { return v_scan_; } @@ -135,4 +117,29 @@ uint32_t DrmMode::type() const { std::string DrmMode::name() const { return name_; } + +auto DrmMode::CreateModeBlob(const DrmDevice &drm) + -> DrmModeUserPropertyBlobUnique { + struct drm_mode_modeinfo drm_mode = { + .clock = clock_, + .hdisplay = h_display_, + .hsync_start = h_sync_start_, + .hsync_end = h_sync_end_, + .htotal = h_total_, + .hskew = h_skew_, + .vdisplay = v_display_, + .vsync_start = v_sync_start_, + .vsync_end = v_sync_end_, + .vtotal = v_total_, + .vscan = v_scan_, + .vrefresh = v_refresh_, + .flags = flags_, + .type = type_, + }; + strncpy(drm_mode.name, name_.c_str(), DRM_DISPLAY_MODE_LEN); + + return drm.RegisterUserPropertyBlob(&drm_mode, + sizeof(struct drm_mode_modeinfo)); +} + } // namespace android diff --git a/drm/DrmMode.h b/drm/DrmMode.h index 313a8ea..0974b5a 100644 --- a/drm/DrmMode.h +++ b/drm/DrmMode.h @@ -22,32 +22,35 @@ #include +#include "DrmUnique.h" + namespace android { +class DrmDevice; + class DrmMode { public: DrmMode() = default; DrmMode(drmModeModeInfoPtr m); bool operator==(const drmModeModeInfo &m) const; - void ToDrmModeModeInfo(drm_mode_modeinfo *m) const; uint32_t id() const; void set_id(uint32_t id); uint32_t clock() const; - uint32_t h_display() const; - uint32_t h_sync_start() const; - uint32_t h_sync_end() const; - uint32_t h_total() const; - uint32_t h_skew() const; - - uint32_t v_display() const; - uint32_t v_sync_start() const; - uint32_t v_sync_end() const; - uint32_t v_total() const; - uint32_t v_scan() const; + uint16_t h_display() const; + uint16_t h_sync_start() const; + uint16_t h_sync_end() const; + uint16_t h_total() const; + uint16_t h_skew() const; + + uint16_t v_display() const; + uint16_t v_sync_start() const; + uint16_t v_sync_end() const; + uint16_t v_total() const; + uint16_t v_scan() const; float v_refresh() const; uint32_t flags() const; @@ -55,23 +58,25 @@ class DrmMode { std::string name() const; + auto CreateModeBlob(const DrmDevice &drm) -> DrmModeUserPropertyBlobUnique; + private: uint32_t id_ = 0; uint32_t clock_ = 0; - uint32_t h_display_ = 0; - uint32_t h_sync_start_ = 0; - uint32_t h_sync_end_ = 0; - uint32_t h_total_ = 0; - uint32_t h_skew_ = 0; - - uint32_t v_display_ = 0; - uint32_t v_sync_start_ = 0; - uint32_t v_sync_end_ = 0; - uint32_t v_total_ = 0; - uint32_t v_scan_ = 0; - uint32_t v_refresh_ = 0; + uint16_t h_display_ = 0; + uint16_t h_sync_start_ = 0; + uint16_t h_sync_end_ = 0; + uint16_t h_total_ = 0; + uint16_t h_skew_ = 0; + + uint16_t v_display_ = 0; + uint16_t v_sync_start_ = 0; + uint16_t v_sync_end_ = 0; + uint16_t v_total_ = 0; + uint16_t v_scan_ = 0; + uint16_t v_refresh_ = 0; uint32_t flags_ = 0; uint32_t type_ = 0; -- cgit v1.2.3 From dccc6fb4f364df3a74bdd02c537892eb1a8ac11d Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Sat, 23 Oct 2021 17:35:44 +0300 Subject: drm_hwcomposer: Make single atomic function for all atomic commit ops. ... to allow precise control on atomic commit operations. This should also help to implement dynamic modechange, readback and other useful features. Signed-off-by: Roman Stratiienko --- DrmHwcTwo.cpp | 46 ++++--- compositor/DrmDisplayComposition.h | 2 - compositor/DrmDisplayCompositor.cpp | 238 ++++++++++++++++++------------------ compositor/DrmDisplayCompositor.h | 64 +++++----- 4 files changed, 173 insertions(+), 177 deletions(-) diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index 071f3bf..dada168 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -253,7 +253,8 @@ DrmHwcTwo::HwcDisplay::HwcDisplay(ResourceManager *resource_manager, } void DrmHwcTwo::HwcDisplay::ClearDisplay() { - compositor_.ClearDisplay(); + AtomicCommitArgs a_args = {.clear_active_composition = true}; + compositor_.ExecuteAtomicCommit(a_args); } HWC2::Error DrmHwcTwo::HwcDisplay::Init(std::vector *planes) { @@ -711,7 +712,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) { composition_layers.emplace_back(std::move(layer)); } - auto composition = std::make_unique(crtc_, + auto composition = std::make_shared(crtc_, planner_.get()); // TODO(nobody): Don't always assume geometry changed @@ -740,16 +741,21 @@ HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) { i = overlay_planes.erase(i); } - if (test) { - ret = compositor_.TestComposition(composition.get()); - } else { - ret = compositor_.ApplyComposition(std::move(composition)); - AddFenceToPresentFence(compositor_.TakeOutFence()); - } + AtomicCommitArgs a_args = { + .test_only = test, + .composition = composition, + }; + + ret = compositor_.ExecuteAtomicCommit(a_args); + if (ret) { if (!test) ALOGE("Failed to apply the frame composition ret=%d", ret); return HWC2::Error::BadParameter; + } else { + if (!test) { + AddFenceToPresentFence(std::move(a_args.out_fence)); + } } return HWC2::Error::None; } @@ -793,11 +799,12 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) { return HWC2::Error::BadConfig; } - if (!compositor_.SetDisplayMode(*mode)) { - return HWC2::Error::BadConfig; - } - int err = compositor_.ApplyComposition( - compositor_.CreateInitializedComposition()); + AtomicCommitArgs a_args = { + .display_mode = *mode, + .clear_active_composition = true, + }; + + int err = compositor_.ExecuteAtomicCommit(a_args); if (err != 0) { ALOGE("Failed to queue mode changing commit %d", err); return HWC2::Error::BadConfig; @@ -882,12 +889,14 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetOutputBuffer(buffer_handle_t buffer, HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) { supported(__func__); auto mode = static_cast(mode_in); + AtomicCommitArgs a_args{}; + switch (mode) { case HWC2::PowerMode::Off: - compositor_.SetDisplayActive(false); + a_args.active = false; break; case HWC2::PowerMode::On: - compositor_.SetDisplayActive(true); + a_args.active = true; break; case HWC2::PowerMode::Doze: case HWC2::PowerMode::DozeSuspend: @@ -897,10 +906,9 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetPowerMode(int32_t mode_in) { return HWC2::Error::BadParameter; }; - int ret = compositor_.ApplyComposition( - compositor_.CreateInitializedComposition()); - if (ret) { - ALOGE("Failed to apply the dpms composition ret=%d", ret); + int err = compositor_.ExecuteAtomicCommit(a_args); + if (err) { + ALOGE("Failed to apply the dpms composition err=%d", err); return HWC2::Error::BadParameter; } return HWC2::Error::None; diff --git a/compositor/DrmDisplayComposition.h b/compositor/DrmDisplayComposition.h index 7b7e668..f1958d7 100644 --- a/compositor/DrmDisplayComposition.h +++ b/compositor/DrmDisplayComposition.h @@ -102,8 +102,6 @@ class DrmDisplayComposition { return planner_; } - UniqueFd out_fence_; - private: DrmCrtc *crtc_ = NULL; Planner *planner_ = NULL; diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp index bb6a33b..447d75e 100644 --- a/compositor/DrmDisplayCompositor.cpp +++ b/compositor/DrmDisplayCompositor.cpp @@ -40,20 +40,6 @@ namespace android { -DrmDisplayCompositor::DrmDisplayCompositor() - : resource_manager_(nullptr), - display_(-1), - initialized_(false), - active_(false) { -} - -DrmDisplayCompositor::~DrmDisplayCompositor() { - if (!initialized_) - return; - - active_composition_.reset(); -} - auto DrmDisplayCompositor::Init(ResourceManager *resource_manager, int display) -> int { resource_manager_ = resource_manager; @@ -81,42 +67,30 @@ DrmDisplayCompositor::CreateInitializedComposition() const { return std::make_unique(crtc, planner_.get()); } -int DrmDisplayCompositor::DisablePlanes(DrmDisplayComposition *display_comp) { - auto pset = MakeDrmModeAtomicReqUnique(); - if (!pset) { - ALOGE("Failed to allocate property set"); - return -ENOMEM; - } +auto DrmDisplayCompositor::CommitFrame(AtomicCommitArgs &args) -> int { + ATRACE_CALL(); - int ret = 0; - std::vector &comp_planes = display_comp - ->composition_planes(); - for (DrmCompositionPlane &comp_plane : comp_planes) { - if (comp_plane.plane()->AtomicDisablePlane(*pset) != 0) { - return -EINVAL; - } - } - DrmDevice *drm = resource_manager_->GetDrmDevice(display_); - ret = drmModeAtomicCommit(drm->fd(), pset.get(), 0, drm); - if (ret) { - ALOGE("Failed to commit pset ret=%d\n", ret); - return ret; + if (args.active && *args.active == active_kms_data.active_state) { + /* Don't set the same state twice */ + args.active.reset(); } - return 0; -} + if (!args.HasInputs()) { + /* nothing to do */ + return 0; + } -int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, - bool test_only) { - ATRACE_CALL(); + if (!active_kms_data.active_state) { + /* Force activate display */ + args.active = true; + } - int ret = 0; + if (args.clear_active_composition && args.composition) { + ALOGE("%s: Invalid arguments", __func__); + return -EINVAL; + } - std::vector &layers = display_comp->layers(); - std::vector &comp_planes = display_comp - ->composition_planes(); DrmDevice *drm = resource_manager_->GetDrmDevice(display_); - uint64_t out_fences[drm->crtcs().size()]; DrmConnector *connector = drm->GetConnectorForDisplay(display_); if (!connector) { @@ -135,113 +109,133 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp, return -ENOMEM; } + int64_t out_fence = -1; if (crtc->out_fence_ptr_property() && - !crtc->out_fence_ptr_property() - .AtomicSet(*pset, (uint64_t)&out_fences[crtc->pipe()])) { + !crtc->out_fence_ptr_property().AtomicSet(*pset, (uint64_t)&out_fence)) { return -EINVAL; } - if (mode_.blob && - (!crtc->active_property().AtomicSet(*pset, 1) || - !crtc->mode_property().AtomicSet(*pset, *mode_.blob) || - !connector->crtc_id_property().AtomicSet(*pset, crtc->id()))) { - return -EINVAL; + DrmModeUserPropertyBlobUnique mode_blob; + + if (args.active) { + if (!crtc->active_property().AtomicSet(*pset, *args.active) || + !connector->crtc_id_property().AtomicSet(*pset, crtc->id())) { + return -EINVAL; + } } - for (DrmCompositionPlane &comp_plane : comp_planes) { - DrmPlane *plane = comp_plane.plane(); - std::vector &source_layers = comp_plane.source_layers(); + if (args.display_mode) { + mode_blob = args.display_mode.value().CreateModeBlob( + *resource_manager_->GetDrmDevice(display_)); - if (comp_plane.type() != DrmCompositionPlane::Type::kDisable) { - if (source_layers.size() > 1) { - ALOGE("Can't handle more than one source layer sz=%zu type=%d", - source_layers.size(), comp_plane.type()); - continue; - } + if (!mode_blob) { + ALOGE("Failed to create mode_blob"); + return -EINVAL; + } - if (source_layers.empty() || source_layers.front() >= layers.size()) { - ALOGE("Source layer index %zu out of bounds %zu type=%d", - source_layers.front(), layers.size(), comp_plane.type()); - return -EINVAL; - } - DrmHwcLayer &layer = layers[source_layers.front()]; + if (!crtc->mode_property().AtomicSet(*pset, *mode_blob)) { + return -EINVAL; + } + } - if (plane->AtomicSetState(*pset, layer, source_layers.front(), - crtc->id()) != 0) { - return -EINVAL; + if (args.composition) { + std::vector &layers = args.composition->layers(); + std::vector &comp_planes = args.composition + ->composition_planes(); + + for (DrmCompositionPlane &comp_plane : comp_planes) { + DrmPlane *plane = comp_plane.plane(); + std::vector &source_layers = comp_plane.source_layers(); + + if (comp_plane.type() != DrmCompositionPlane::Type::kDisable) { + if (source_layers.size() > 1) { + ALOGE("Can't handle more than one source layer sz=%zu type=%d", + source_layers.size(), comp_plane.type()); + continue; + } + + if (source_layers.empty() || source_layers.front() >= layers.size()) { + ALOGE("Source layer index %zu out of bounds %zu type=%d", + source_layers.front(), layers.size(), comp_plane.type()); + return -EINVAL; + } + DrmHwcLayer &layer = layers[source_layers.front()]; + + if (plane->AtomicSetState(*pset, layer, source_layers.front(), + crtc->id()) != 0) { + return -EINVAL; + } + } else { + if (plane->AtomicDisablePlane(*pset) != 0) { + return -EINVAL; + } } - } else { - if (plane->AtomicDisablePlane(*pset) != 0) { + } + } + + if (args.clear_active_composition && active_kms_data.composition) { + auto &comp_planes = active_kms_data.composition->composition_planes(); + for (auto &comp_plane : comp_planes) { + if (comp_plane.plane()->AtomicDisablePlane(*pset) != 0) { return -EINVAL; } } } - if (!ret) { - uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET; - if (test_only) - flags |= DRM_MODE_ATOMIC_TEST_ONLY; + uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET; + if (args.test_only) + flags |= DRM_MODE_ATOMIC_TEST_ONLY; - ret = drmModeAtomicCommit(drm->fd(), pset.get(), flags, drm); - if (ret) { - if (!test_only) - ALOGE("Failed to commit pset ret=%d\n", ret); - return ret; - } + int err = drmModeAtomicCommit(drm->fd(), pset.get(), flags, drm); + if (err) { + if (!args.test_only) + ALOGE("Failed to commit pset ret=%d\n", err); + return err; } - if (!test_only) { - if (mode_.blob) { - connector->set_active_mode(mode_.mode); - mode_.old_blob = std::move(mode_.blob); + if (!args.test_only) { + if (args.display_mode) { + connector->set_active_mode(*args.display_mode); + active_kms_data.mode_blob = std::move(mode_blob); } - active_changed_ = false; - if (crtc->out_fence_ptr_property()) { - display_comp->out_fence_ = UniqueFd((int)out_fences[crtc->pipe()]); + if (args.clear_active_composition) { + active_kms_data.composition.reset(); } - } - - return ret; -} - -void DrmDisplayCompositor::ClearDisplay() { - if (!active_composition_) - return; - - if (DisablePlanes(active_composition_.get())) - return; - active_composition_.reset(nullptr); -} - -int DrmDisplayCompositor::ApplyComposition( - std::unique_ptr composition) { - int ret = CommitFrame(composition.get(), false); + if (args.composition) { + active_kms_data.composition = args.composition; + } - if (ret) { - ALOGE("Composite failed for display %d", display_); - // Disable the hw used by the last active composition. This allows us to - // signal the release fences from that composition to avoid hanging. - ClearDisplay(); - return ret; - } + if (args.active) { + active_kms_data.active_state = *args.active; + } - if (composition) { - active_composition_.swap(composition); + if (crtc->out_fence_ptr_property()) { + args.out_fence = UniqueFd((int)out_fence); + } } - return ret; + return 0; } -int DrmDisplayCompositor::TestComposition(DrmDisplayComposition *composition) { - return CommitFrame(composition, true); -} +auto DrmDisplayCompositor::ExecuteAtomicCommit(AtomicCommitArgs &args) -> int { + int err = CommitFrame(args); + + if (!args.test_only) { + if (err) { + ALOGE("Composite failed for display %d", display_); + // Disable the hw used by the last active composition. This allows us to + // signal the release fences from that composition to avoid hanging. + AtomicCommitArgs cl_args = {.clear_active_composition = true}; + if (CommitFrame(cl_args)) { + ALOGE("Failed to clean-up active composition for display %d", display_); + } + return err; + } + } -auto DrmDisplayCompositor::SetDisplayMode(const DrmMode &display_mode) -> bool { - mode_.mode = display_mode; - mode_.blob = mode_.mode.CreateModeBlob(*resource_manager_->GetDrmDevice(display_)); - return !!mode_.blob; -} + return err; +} // namespace android } // namespace android diff --git a/compositor/DrmDisplayCompositor.h b/compositor/DrmDisplayCompositor.h index 434668f..55fe77d 100644 --- a/compositor/DrmDisplayCompositor.h +++ b/compositor/DrmDisplayCompositor.h @@ -34,53 +34,49 @@ namespace android { +struct AtomicCommitArgs { + /* inputs. All fields are optional, but at least one has to be specified */ + bool test_only = false; + std::optional display_mode; + std::optional active; + std::shared_ptr composition; + /* 'clear' should never be used together with 'composition' */ + bool clear_active_composition = false; + + /* out */ + UniqueFd out_fence; + + /* helpers */ + auto HasInputs() -> bool { + return display_mode || active || composition || clear_active_composition; + } +}; + class DrmDisplayCompositor { public: - DrmDisplayCompositor(); - ~DrmDisplayCompositor(); - + DrmDisplayCompositor() = default; + ~DrmDisplayCompositor() = default; auto Init(ResourceManager *resource_manager, int display) -> int; std::unique_ptr CreateInitializedComposition() const; - int ApplyComposition(std::unique_ptr composition); - int TestComposition(DrmDisplayComposition *composition); - void ClearDisplay(); - auto SetDisplayMode(const DrmMode &display_mode) -> bool; - auto SetDisplayActive(bool state) -> void { - active_ = state; - active_changed_ = true; - } - UniqueFd TakeOutFence() { - if (!active_composition_) { - return UniqueFd(); - } - return std::move(active_composition_->out_fence_); - } + auto ExecuteAtomicCommit(AtomicCommitArgs &args) -> int; private: - struct ModeState { - DrmMode mode; - DrmModeUserPropertyBlobUnique blob; - DrmModeUserPropertyBlobUnique old_blob; - }; - DrmDisplayCompositor(const DrmDisplayCompositor &) = delete; - int CommitFrame(DrmDisplayComposition *display_comp, bool test_only); - int DisablePlanes(DrmDisplayComposition *display_comp); - - ResourceManager *resource_manager_; - int display_; - - std::unique_ptr active_composition_; - - bool initialized_; - bool active_{true}, active_changed_{true}; + auto CommitFrame(AtomicCommitArgs &args) -> int; - ModeState mode_; + struct { + std::shared_ptr composition; + DrmModeUserPropertyBlobUnique mode_blob; + bool active_state{}; + } active_kms_data; + ResourceManager *resource_manager_ = nullptr; std::unique_ptr planner_; + bool initialized_{}; + int display_ = -1; }; } // namespace android -- cgit v1.2.3 From 2a1f1ae02d31dcb0ca39de2da103539cb7e4aae7 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Sat, 23 Oct 2021 17:47:35 +0300 Subject: drm_hwcomposer: Route release fence directly Current release_fence merging logic doesn't make much sence, cleanup it. Signed-off-by: Roman Stratiienko --- DrmHwcTwo.cpp | 33 +++++++-------------------------- DrmHwcTwo.h | 5 +---- backend/Backend.cpp | 4 +++- 3 files changed, 11 insertions(+), 31 deletions(-) diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index dada168..1f1e594 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -660,20 +660,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::GetReleaseFences(uint32_t *num_elements, return HWC2::Error::None; } -void DrmHwcTwo::HwcDisplay::AddFenceToPresentFence(UniqueFd fd) { - if (!fd) { - return; - } - - if (present_fence_) { - present_fence_ = UniqueFd( - sync_merge("dc_present", present_fence_.Get(), fd.Get())); - } else { - present_fence_ = std::move(fd); - } -} - -HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) { +HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(AtomicCommitArgs &a_args) { // order the layers by z-order bool use_client_layer = false; uint32_t client_z_order = UINT32_MAX; @@ -741,21 +728,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) { i = overlay_planes.erase(i); } - AtomicCommitArgs a_args = { - .test_only = test, - .composition = composition, - }; - + a_args.composition = composition; ret = compositor_.ExecuteAtomicCommit(a_args); if (ret) { - if (!test) + if (!a_args.test_only) ALOGE("Failed to apply the frame composition ret=%d", ret); return HWC2::Error::BadParameter; - } else { - if (!test) { - AddFenceToPresentFence(std::move(a_args.out_fence)); - } } return HWC2::Error::None; } @@ -769,7 +748,9 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *present_fence) { ++total_stats_.total_frames_; - ret = CreateComposition(false); + AtomicCommitArgs a_args{}; + ret = CreateComposition(a_args); + if (ret != HWC2::Error::None) ++total_stats_.failed_kms_present_; @@ -781,7 +762,7 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *present_fence) { if (ret != HWC2::Error::None) return ret; - *present_fence = present_fence_.Release(); + *present_fence = a_args.out_fence.Release(); ++frame_no_; return HWC2::Error::None; diff --git a/DrmHwcTwo.h b/DrmHwcTwo.h index 0d213fd..974c0a8 100644 --- a/DrmHwcTwo.h +++ b/DrmHwcTwo.h @@ -149,7 +149,7 @@ class DrmHwcTwo : public hwc2_device_t { HwcDisplay(const HwcDisplay &) = delete; HWC2::Error Init(std::vector *planes); - HWC2::Error CreateComposition(bool test); + HWC2::Error CreateComposition(AtomicCommitArgs &a_args); std::vector GetOrderLayersByZPos(); void ClearDisplay(); @@ -327,8 +327,6 @@ class DrmHwcTwo : public hwc2_device_t { std::atomic_int flattenning_state_{ClientFlattenningState::NotRequired}; VSyncWorker flattening_vsync_worker_; - void AddFenceToPresentFence(UniqueFd fd); - constexpr static size_t MATRIX_SIZE = 16; DrmHwcTwo *hwc2_; @@ -351,7 +349,6 @@ class DrmHwcTwo : public hwc2_device_t { uint32_t layer_idx_ = 0; std::map layers_; HwcLayer client_layer_; - UniqueFd present_fence_; int32_t color_mode_{}; std::array color_transform_matrix_{}; android_color_transform_t color_transform_hint_; diff --git a/backend/Backend.cpp b/backend/Backend.cpp index d7eb240..bd1855f 100644 --- a/backend/Backend.cpp +++ b/backend/Backend.cpp @@ -46,8 +46,10 @@ HWC2::Error Backend::ValidateDisplay(DrmHwcTwo::HwcDisplay *display, bool testing_needed = !(client_start == 0 && client_size == layers.size()); + AtomicCommitArgs a_args = {.test_only = true}; + if (testing_needed && - display->CreateComposition(true) != HWC2::Error::None) { + display->CreateComposition(a_args) != HWC2::Error::None) { ++display->total_stats().failed_kms_validate_; client_start = 0; client_size = layers.size(); -- cgit v1.2.3 From 1e053b4e1913beeddc74b423cbd3a753192822c0 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Mon, 25 Oct 2021 22:54:20 +0300 Subject: drm_hwcomposer: Make uevent listener standalone 1. DRM event listener doesn't work in this conditions, uevent blocks the thread and non-blocking select() doesn't make any sense. Remove DRM event handling for now. 2. UEvent listeren is common for all DrmDevices, therefore put it into ResourceManager class. Signed-off-by: Roman Stratiienko --- .ci/.common.sh | 2 +- Android.bp | 2 +- DrmHwcTwo.cpp | 52 +++++----- DrmHwcTwo.h | 14 +-- bufferinfo/legacy/BufferInfoImagination.cpp | 2 + bufferinfo/legacy/BufferInfoLibdrm.cpp | 2 + bufferinfo/legacy/BufferInfoMinigbm.cpp | 2 + drm/DrmDevice.cpp | 16 +-- drm/DrmDevice.h | 8 +- drm/DrmEventListener.cpp | 145 ---------------------------- drm/DrmEventListener.h | 65 ------------- drm/ResourceManager.cpp | 6 ++ drm/ResourceManager.h | 11 +++ drm/UEventListener.cpp | 95 ++++++++++++++++++ drm/UEventListener.h | 48 +++++++++ 15 files changed, 198 insertions(+), 272 deletions(-) delete mode 100644 drm/DrmEventListener.cpp delete mode 100644 drm/DrmEventListener.h create mode 100644 drm/UEventListener.cpp create mode 100644 drm/UEventListener.h diff --git a/.ci/.common.sh b/.ci/.common.sh index 21c2b28..d9b5b3c 100644 --- a/.ci/.common.sh +++ b/.ci/.common.sh @@ -27,13 +27,13 @@ drm/DrmConnector.cpp drm/DrmCrtc.cpp drm/DrmDevice.cpp drm/DrmEncoder.cpp -drm/DrmEventListener.cpp drm/DrmFbImporter.cpp drm/DrmMode.cpp drm/DrmPlane.cpp drm/DrmProperty.cpp DrmHwcTwo.cpp drm/ResourceManager.cpp +drm/UEventListener.cpp drm/VSyncWorker.cpp tests/worker_test.cpp utils/autolock.cpp diff --git a/Android.bp b/Android.bp index ddf66fb..eba92f8 100644 --- a/Android.bp +++ b/Android.bp @@ -95,12 +95,12 @@ cc_library_static { "drm/DrmCrtc.cpp", "drm/DrmDevice.cpp", "drm/DrmEncoder.cpp", - "drm/DrmEventListener.cpp", "drm/DrmFbImporter.cpp", "drm/DrmMode.cpp", "drm/DrmPlane.cpp", "drm/DrmProperty.cpp", "drm/ResourceManager.cpp", + "drm/UEventListener.cpp", "drm/VSyncWorker.cpp", "utils/autolock.cpp", diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index 1f1e594..8b8068c 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -87,11 +87,9 @@ HWC2::Error DrmHwcTwo::Init() { } } - const auto &drm_devices = resource_manager_.getDrmDevices(); - for (const auto &device : drm_devices) { - // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) - device->RegisterHotplugHandler(new DrmHotplugHandler(this, device.get())); - } + resource_manager_.GetUEventListener()->RegisterHotplugHandler( + [this] { HandleHotplugUEvent(); }); + return ret; } @@ -1263,30 +1261,32 @@ void DrmHwcTwo::HandleInitialHotplugState(DrmDevice *drmDevice) { } } -void DrmHwcTwo::DrmHotplugHandler::HandleEvent(uint64_t timestamp_us) { - for (const auto &conn : drm_->connectors()) { - drmModeConnection old_state = conn->state(); - drmModeConnection cur_state = conn->UpdateModes() - ? DRM_MODE_UNKNOWNCONNECTION - : conn->state(); +void DrmHwcTwo::HandleHotplugUEvent() { + for (const auto &drm : resource_manager_.getDrmDevices()) { + for (const auto &conn : drm->connectors()) { + drmModeConnection old_state = conn->state(); + drmModeConnection cur_state = conn->UpdateModes() + ? DRM_MODE_UNKNOWNCONNECTION + : conn->state(); - if (cur_state == old_state) - continue; + if (cur_state == old_state) + continue; - ALOGI("%s event @%" PRIu64 " for connector %u on display %d", - cur_state == DRM_MODE_CONNECTED ? "Plug" : "Unplug", timestamp_us, - conn->id(), conn->display()); - - int display_id = conn->display(); - if (cur_state == DRM_MODE_CONNECTED) { - auto &display = hwc2_->displays_.at(display_id); - display.ChosePreferredConfig(); - } else { - auto &display = hwc2_->displays_.at(display_id); - display.ClearDisplay(); - } + ALOGI("%s event for connector %u on display %d", + cur_state == DRM_MODE_CONNECTED ? "Plug" : "Unplug", conn->id(), + conn->display()); + + int display_id = conn->display(); + if (cur_state == DRM_MODE_CONNECTED) { + auto &display = displays_.at(display_id); + display.ChosePreferredConfig(); + } else { + auto &display = displays_.at(display_id); + display.ClearDisplay(); + } - hwc2_->HandleDisplayHotplug(display_id, cur_state); + HandleDisplayHotplug(display_id, cur_state); + } } } diff --git a/DrmHwcTwo.h b/DrmHwcTwo.h index 974c0a8..ed547c0 100644 --- a/DrmHwcTwo.h +++ b/DrmHwcTwo.h @@ -359,18 +359,6 @@ class DrmHwcTwo : public hwc2_device_t { std::string DumpDelta(DrmHwcTwo::HwcDisplay::Stats delta); }; - class DrmHotplugHandler : public DrmEventHandler { - public: - DrmHotplugHandler(DrmHwcTwo *hwc2, DrmDevice *drm) - : hwc2_(hwc2), drm_(drm) { - } - void HandleEvent(uint64_t timestamp_us); - - private: - DrmHwcTwo *hwc2_; - DrmDevice *drm_; - }; - private: static DrmHwcTwo *toDrmHwcTwo(hwc2_device_t *dev) { return static_cast(dev); @@ -439,6 +427,8 @@ class DrmHwcTwo : public hwc2_device_t { void HandleDisplayHotplug(hwc2_display_t displayid, int state); void HandleInitialHotplugState(DrmDevice *drmDevice); + void HandleHotplugUEvent(); + ResourceManager resource_manager_; std::map displays_; diff --git a/bufferinfo/legacy/BufferInfoImagination.cpp b/bufferinfo/legacy/BufferInfoImagination.cpp index d646072..691dd14 100644 --- a/bufferinfo/legacy/BufferInfoImagination.cpp +++ b/bufferinfo/legacy/BufferInfoImagination.cpp @@ -20,6 +20,8 @@ #include +#include + #include "img_gralloc1_public.h" #include "utils/log.h" diff --git a/bufferinfo/legacy/BufferInfoLibdrm.cpp b/bufferinfo/legacy/BufferInfoLibdrm.cpp index da89eb5..6243d8d 100644 --- a/bufferinfo/legacy/BufferInfoLibdrm.cpp +++ b/bufferinfo/legacy/BufferInfoLibdrm.cpp @@ -23,6 +23,8 @@ #include #include +#include + #include "utils/log.h" #include "utils/properties.h" diff --git a/bufferinfo/legacy/BufferInfoMinigbm.cpp b/bufferinfo/legacy/BufferInfoMinigbm.cpp index d030dff..1657ea6 100644 --- a/bufferinfo/legacy/BufferInfoMinigbm.cpp +++ b/bufferinfo/legacy/BufferInfoMinigbm.cpp @@ -21,6 +21,8 @@ #include #include +#include + #include "cros_gralloc_handle.h" #include "utils/log.h" diff --git a/drm/DrmDevice.cpp b/drm/DrmDevice.cpp index 1753ddc..0f5f581 100644 --- a/drm/DrmDevice.cpp +++ b/drm/DrmDevice.cpp @@ -111,15 +111,11 @@ static std::vector make_primary_display_candidates( return primary_candidates; } -DrmDevice::DrmDevice() : event_listener_(this) { +DrmDevice::DrmDevice() { self.reset(this); mDrmFbImporter = std::make_unique(self); } -DrmDevice::~DrmDevice() { - event_listener_.Exit(); -} - std::tuple DrmDevice::Init(const char *path, int num_displays) { /* TODO: Use drmOpenControl here instead */ fd_ = UniqueFd(open(path, O_RDWR | O_CLOEXEC)); @@ -326,12 +322,6 @@ std::tuple DrmDevice::Init(const char *path, int num_displays) { if (ret) return std::make_tuple(ret, 0); - ret = event_listener_.Init(); - if (ret) { - ALOGE("Can't initialize event listener %d", ret); - return std::make_tuple(ret, 0); - } - for (auto &conn : connectors_) { ret = CreateDisplayPipe(conn.get()); if (ret) { @@ -526,10 +516,6 @@ auto DrmDevice::RegisterUserPropertyBlob(void *data, size_t length) const }); } -DrmEventListener *DrmDevice::event_listener() { - return &event_listener_; -} - int DrmDevice::GetProperty(uint32_t obj_id, uint32_t obj_type, const char *prop_name, DrmProperty *property) const { drmModeObjectPropertiesPtr props = nullptr; diff --git a/drm/DrmDevice.h b/drm/DrmDevice.h index 81c60cd..c08c766 100644 --- a/drm/DrmDevice.h +++ b/drm/DrmDevice.h @@ -25,7 +25,6 @@ #include "DrmConnector.h" #include "DrmCrtc.h" #include "DrmEncoder.h" -#include "DrmEventListener.h" #include "DrmFbImporter.h" #include "DrmPlane.h" #include "utils/UniqueFd.h" @@ -38,7 +37,7 @@ class DrmPlane; class DrmDevice { public: DrmDevice(); - ~DrmDevice(); + ~DrmDevice() = default; std::tuple Init(const char *path, int num_displays); @@ -67,7 +66,6 @@ class DrmDevice { DrmConnector *AvailableWritebackConnector(int display) const; DrmCrtc *GetCrtcForDisplay(int display) const; DrmPlane *GetPlane(uint32_t id) const; - DrmEventListener *event_listener(); int GetCrtcProperty(const DrmCrtc &crtc, const char *prop_name, DrmProperty *property) const; @@ -83,9 +81,6 @@ class DrmDevice { -> DrmModeUserPropertyBlobUnique; bool HandlesDisplay(int display) const; - void RegisterHotplugHandler(DrmEventHandler *handler) { - event_listener_.RegisterHotplugHandler(handler); - } bool HasAddFb2ModifiersSupport() const { return HasAddFb2ModifiersSupport_; @@ -114,7 +109,6 @@ class DrmDevice { std::vector> encoders_; std::vector> crtcs_; std::vector> planes_; - DrmEventListener event_listener_; std::pair min_resolution_; std::pair max_resolution_; diff --git a/drm/DrmEventListener.cpp b/drm/DrmEventListener.cpp deleted file mode 100644 index 53e7032..0000000 --- a/drm/DrmEventListener.cpp +++ /dev/null @@ -1,145 +0,0 @@ -/* - * Copyright (C) 2016 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#define LOG_TAG "hwc-drm-event-listener" - -#include "DrmEventListener.h" - -#include -#include -#include - -#include -#include -#include - -#include "DrmDevice.h" -#include "utils/log.h" - -/* Originally defined in system/core/libsystem/include/system/graphics.h */ -#define HAL_PRIORITY_URGENT_DISPLAY (-8) - -namespace android { - -DrmEventListener::DrmEventListener(DrmDevice *drm) - : Worker("drm-event-listener", HAL_PRIORITY_URGENT_DISPLAY), drm_(drm) { -} - -int DrmEventListener::Init() { - uevent_fd_ = UniqueFd( - socket(PF_NETLINK, SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_KOBJECT_UEVENT)); - if (!uevent_fd_) { - ALOGE("Failed to open uevent socket: %s", strerror(errno)); - return -errno; - } - - struct sockaddr_nl addr {}; - addr.nl_family = AF_NETLINK; - addr.nl_pid = 0; - addr.nl_groups = 0xFFFFFFFF; - - int ret = bind(uevent_fd_.Get(), (struct sockaddr *)&addr, sizeof(addr)); - if (ret) { - ALOGE("Failed to bind uevent socket: %s", strerror(errno)); - return -errno; - } - - // NOLINTNEXTLINE(readability-isolate-declaration) - FD_ZERO(&fds_); - FD_SET(drm_->fd(), &fds_); - FD_SET(uevent_fd_.Get(), &fds_); - max_fd_ = std::max(drm_->fd(), uevent_fd_.Get()); - - return InitWorker(); -} - -void DrmEventListener::RegisterHotplugHandler(DrmEventHandler *handler) { - assert(!hotplug_handler_); - hotplug_handler_.reset(handler); -} - -void DrmEventListener::FlipHandler(int /* fd */, unsigned int /* sequence */, - unsigned int tv_sec, unsigned int tv_usec, - void *user_data) { - auto *handler = (DrmEventHandler *)user_data; - if (!handler) - return; - - handler->HandleEvent((uint64_t)tv_sec * 1000 * 1000 + tv_usec); - delete handler; // NOLINT(cppcoreguidelines-owning-memory) -} - -void DrmEventListener::UEventHandler() { - char buffer[1024]; - ssize_t ret = 0; - - struct timespec ts {}; - - uint64_t timestamp = 0; - ret = clock_gettime(CLOCK_MONOTONIC, &ts); - if (!ret) - timestamp = ts.tv_sec * 1000 * 1000 * 1000 + ts.tv_nsec; - else - ALOGE("Failed to get monotonic clock on hotplug %zd", ret); - - while (true) { - ret = read(uevent_fd_.Get(), &buffer, sizeof(buffer)); - if (ret == 0) - return; - - if (ret < 0) { - ALOGE("Got error reading uevent %zd", ret); - return; - } - - if (!hotplug_handler_) - continue; - - bool drm_event = false; - bool hotplug_event = false; - for (uint32_t i = 0; i < ret;) { - char *event = buffer + i; - if (strcmp(event, "DEVTYPE=drm_minor") != 0) - drm_event = true; - else if (strcmp(event, "HOTPLUG=1") != 0) - hotplug_event = true; - - i += strlen(event) + 1; - } - - if (drm_event && hotplug_event) - hotplug_handler_->HandleEvent(timestamp); - } -} - -void DrmEventListener::Routine() { - int ret = 0; - do { - ret = select(max_fd_ + 1, &fds_, nullptr, nullptr, nullptr); - } while (ret == -1 && errno == EINTR); - - if (FD_ISSET(drm_->fd(), &fds_)) { - drmEventContext event_context = - {.version = 2, - .vblank_handler = nullptr, - .page_flip_handler = DrmEventListener::FlipHandler}; - drmHandleEvent(drm_->fd(), &event_context); - } - - if (FD_ISSET(uevent_fd_.Get(), &fds_)) - UEventHandler(); -} -} // namespace android diff --git a/drm/DrmEventListener.h b/drm/DrmEventListener.h deleted file mode 100644 index f1f7310..0000000 --- a/drm/DrmEventListener.h +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (C) 2016 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef ANDROID_DRM_EVENT_LISTENER_H_ -#define ANDROID_DRM_EVENT_LISTENER_H_ - -#include "utils/UniqueFd.h" -#include "utils/Worker.h" - -namespace android { - -class DrmDevice; - -class DrmEventHandler { - public: - DrmEventHandler() { - } - virtual ~DrmEventHandler() { - } - - virtual void HandleEvent(uint64_t timestamp_us) = 0; -}; - -class DrmEventListener : public Worker { - public: - DrmEventListener(DrmDevice *drm); - virtual ~DrmEventListener() { - } - - int Init(); - - void RegisterHotplugHandler(DrmEventHandler *handler); - - static void FlipHandler(int fd, unsigned int sequence, unsigned int tv_sec, - unsigned int tv_usec, void *user_data); - - protected: - virtual void Routine(); - - private: - void UEventHandler(); - - fd_set fds_{}; - UniqueFd uevent_fd_; - int max_fd_ = -1; - - DrmDevice *drm_; - std::unique_ptr hotplug_handler_; -}; -} // namespace android - -#endif diff --git a/drm/ResourceManager.cpp b/drm/ResourceManager.cpp index c55d6b8..8baa4cb 100644 --- a/drm/ResourceManager.cpp +++ b/drm/ResourceManager.cpp @@ -70,6 +70,12 @@ int ResourceManager::Init() { return -EINVAL; } + ret = uevent_listener_.Init(); + if (ret) { + ALOGE("Can't initialize event listener %d", ret); + return ret; + } + return 0; } diff --git a/drm/ResourceManager.h b/drm/ResourceManager.h index d9e0712..5ffe92c 100644 --- a/drm/ResourceManager.h +++ b/drm/ResourceManager.h @@ -20,6 +20,7 @@ #include #include "DrmDevice.h" +#include "UEventListener.h" #include "DrmFbImporter.h" namespace android { @@ -29,6 +30,10 @@ class ResourceManager { ResourceManager(); ResourceManager(const ResourceManager &) = delete; ResourceManager &operator=(const ResourceManager &) = delete; + ~ResourceManager() { + uevent_listener_.Exit(); + } + int Init(); DrmDevice *GetDrmDevice(int display); const std::vector> &getDrmDevices() const { @@ -41,6 +46,10 @@ class ResourceManager { return scale_with_gpu_; } + UEventListener *GetUEventListener() { + return &uevent_listener_; + } + private: int AddDrmDevice(std::string const &path); @@ -48,6 +57,8 @@ class ResourceManager { std::vector> drms_; bool scale_with_gpu_{}; + + UEventListener uevent_listener_; }; } // namespace android diff --git a/drm/UEventListener.cpp b/drm/UEventListener.cpp new file mode 100644 index 0000000..eae0608 --- /dev/null +++ b/drm/UEventListener.cpp @@ -0,0 +1,95 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#define LOG_TAG "hwc-uevent-listener" + +#include "UEventListener.h" + +#include +#include +#include + +#include +#include +#include + +#include "utils/log.h" + +/* Originally defined in system/core/libsystem/include/system/graphics.h */ +#define HAL_PRIORITY_URGENT_DISPLAY (-8) + +namespace android { + +UEventListener::UEventListener() + : Worker("uevent-listener", HAL_PRIORITY_URGENT_DISPLAY){}; + +int UEventListener::Init() { + uevent_fd_ = UniqueFd( + socket(PF_NETLINK, SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_KOBJECT_UEVENT)); + if (!uevent_fd_) { + ALOGE("Failed to open uevent socket: %s", strerror(errno)); + return -errno; + } + + struct sockaddr_nl addr {}; + addr.nl_family = AF_NETLINK; + addr.nl_pid = 0; + addr.nl_groups = 0xFFFFFFFF; + + int ret = bind(uevent_fd_.Get(), (struct sockaddr *)&addr, sizeof(addr)); + if (ret) { + ALOGE("Failed to bind uevent socket: %s", strerror(errno)); + return -errno; + } + + return InitWorker(); +} + +void UEventListener::Routine() { + char buffer[1024]; + ssize_t ret = 0; + + while (true) { + ret = read(uevent_fd_.Get(), &buffer, sizeof(buffer)); + if (ret == 0) + return; + + if (ret < 0) { + ALOGE("Got error reading uevent %zd", ret); + return; + } + + if (!hotplug_handler_) + continue; + + bool drm_event = false; + bool hotplug_event = false; + for (uint32_t i = 0; i < ret;) { + char *event = buffer + i; + if (strcmp(event, "DEVTYPE=drm_minor") != 0) + drm_event = true; + else if (strcmp(event, "HOTPLUG=1") != 0) + hotplug_event = true; + + i += strlen(event) + 1; + } + + if (drm_event && hotplug_event && hotplug_handler_) { + hotplug_handler_(); + } + } +} +} // namespace android diff --git a/drm/UEventListener.h b/drm/UEventListener.h new file mode 100644 index 0000000..048eb40 --- /dev/null +++ b/drm/UEventListener.h @@ -0,0 +1,48 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef ANDROID_UEVENT_LISTENER_H_ +#define ANDROID_UEVENT_LISTENER_H_ + +#include + +#include "utils/UniqueFd.h" +#include "utils/Worker.h" + +namespace android { + +class UEventListener : public Worker { + public: + UEventListener(); + virtual ~UEventListener() = default; + + int Init(); + + void RegisterHotplugHandler(std::function hotplug_handler) { + hotplug_handler_ = hotplug_handler; + } + + protected: + virtual void Routine(); + + private: + UniqueFd uevent_fd_; + + std::function hotplug_handler_; +}; +} // namespace android + +#endif -- cgit v1.2.3 From 0ee8f58b93a037c5a54bb0a05c12228845a70a76 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Thu, 28 Oct 2021 15:52:38 +0300 Subject: drm_hwcomposer: Don't use Mapper@4 metadata API for legacy getters As it turned out Mapper@4 metadata API calls are slow. Allow using legacy getters again. Closes: https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/-/issues/56 Signed-off-by: Roman Stratiienko --- Android.bp | 25 ++++++++++++++++++------- bufferinfo/BufferInfoGetter.cpp | 10 +++++----- tests/Android.bp | 2 +- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/Android.bp b/Android.bp index eba92f8..87e2efd 100644 --- a/Android.bp +++ b/Android.bp @@ -78,9 +78,8 @@ cc_defaults { vendor: true, } -cc_library_static { - name: "drm_hwcomposer", - defaults: ["hwcomposer.drm_defaults"], +filegroup { + name: "drm_hwcomposer_common", srcs: [ "DrmHwcTwo.cpp", @@ -113,18 +112,30 @@ cc_library_static { ], } +// Kept only for compatibility with older Android version. Please do not use! +cc_library_static { + name: "drm_hwcomposer", + defaults: ["hwcomposer.drm_defaults"], + srcs: [":drm_hwcomposer_common"], +} + cc_library_shared { name: "hwcomposer.drm", defaults: ["hwcomposer.drm_defaults"], - whole_static_libs: ["drm_hwcomposer"], - srcs: ["bufferinfo/legacy/BufferInfoLibdrm.cpp"], + srcs: [ + ":drm_hwcomposer_common", + "bufferinfo/legacy/BufferInfoLibdrm.cpp", + ], + cflags: ["-DUSE_IMAPPER4_METADATA_API"], } cc_library_shared { name: "hwcomposer.drm_minigbm", defaults: ["hwcomposer.drm_defaults"], - whole_static_libs: ["drm_hwcomposer"], - srcs: ["bufferinfo/legacy/BufferInfoMinigbm.cpp"], + srcs: [ + ":drm_hwcomposer_common", + "bufferinfo/legacy/BufferInfoMinigbm.cpp", + ], include_dirs: ["external/minigbm/cros_gralloc"], } diff --git a/bufferinfo/BufferInfoGetter.cpp b/bufferinfo/BufferInfoGetter.cpp index 7f7f8ae..c284365 100644 --- a/bufferinfo/BufferInfoGetter.cpp +++ b/bufferinfo/BufferInfoGetter.cpp @@ -32,17 +32,17 @@ namespace android { BufferInfoGetter *BufferInfoGetter::GetInstance() { static std::unique_ptr inst; - if (inst == nullptr) { -#if PLATFORM_SDK_VERSION >= 30 + if (!inst) { +#if PLATFORM_SDK_VERSION >= 30 && defined(USE_IMAPPER4_METADATA_API) inst.reset(BufferInfoMapperMetadata::CreateInstance()); - if (inst == nullptr) { + if (!inst) { ALOGW( "Generic buffer getter is not available. Falling back to legacy..."); + } #endif + if (!inst) { inst = LegacyBufferInfoGetter::CreateInstance(); -#if PLATFORM_SDK_VERSION >= 30 } -#endif } return inst.get(); diff --git a/tests/Android.bp b/tests/Android.bp index e30898c..56f8c4f 100644 --- a/tests/Android.bp +++ b/tests/Android.bp @@ -1,9 +1,9 @@ cc_library_shared { name: "hwcomposer.filegroups_build_test", defaults: ["hwcomposer.drm_defaults"], - whole_static_libs: ["drm_hwcomposer"], srcs: [ + ":drm_hwcomposer_common", ":drm_hwcomposer_platformhisi", ":drm_hwcomposer_platformimagination", ":drm_hwcomposer_platformmediatek", -- cgit v1.2.3 From a148f21336adb103964fea1d2cacc3f408715c4b Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Tue, 16 Nov 2021 18:23:18 +0200 Subject: drm_hwcomposer: Rework display modes handling Android likes to adapt display output frequency to match window context frequency. Unfortunately platform code has some limitations, therefore hwcomposer HAL should be careful with reporting supported display modes. Known platform limitations: 1: Framework doesn't distinguish between interlaced/progressive modes. 2. Framework will not switch display frequency in case margin in FPS rate is very small (<1FPS or so). Not a big issue, but that is causing some CTS tests to fail. In addition to that VRR technology (or seamless mode switching) require hwcomposer to group modes which tells the framework that seamless mode configuration change is supported within a group of display modes. By this commit do the following: 1. Group modes by the resolution: E.g. Group 1: 1024x768i@60 1024x768i@90 1024x768@50 1024x768@50.1 Group 2: 1920x1080@60 1920x1080@24.3 1920x1080i@60 1920x1080i@120 2. Disable modes in a way that each group keeps only interlaced or proressive modes enabled. In case KMS reported preferred mode is interlaced - prefer interlaced for the whole group, otherwise prefer progressive. 3. Disable mode in case different mode in the same group has similar frequency with delta less than 1FPS. 4. Report only modes which remain enabled to the framework. Test: atest CtsGraphicsTestCases Signed-off-by: Roman Stratiienko --- DrmHwcTwo.cpp | 273 ++++++++++++++++++++++++++++++++------------------- DrmHwcTwo.h | 18 +++- drm/DrmConnector.cpp | 26 +---- drm/DrmConnector.h | 6 -- drm/DrmMode.cpp | 10 +- drm/DrmMode.h | 8 +- 6 files changed, 195 insertions(+), 146 deletions(-) diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index 8b8068c..6faec1c 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -349,9 +349,9 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ChosePreferredConfig() { uint32_t num_configs = 0; HWC2::Error err = GetDisplayConfigs(&num_configs, nullptr); if (err != HWC2::Error::None || !num_configs) - return err; + return HWC2::Error::BadDisplay; - return SetActiveConfig(connector_->get_preferred_mode_id()); + return SetActiveConfig(preferred_config_id_); } HWC2::Error DrmHwcTwo::HwcDisplay::AcceptDisplayChanges() { @@ -378,13 +378,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::DestroyLayer(hwc2_layer_t layer) { return HWC2::Error::None; } -HWC2::Error DrmHwcTwo::HwcDisplay::GetActiveConfig(hwc2_config_t *config) { +HWC2::Error DrmHwcTwo::HwcDisplay::GetActiveConfig( + hwc2_config_t *config) const { supported(__func__); - DrmMode const &mode = connector_->active_mode(); - if (mode.id() == 0) + if (hwc_configs_.count(active_config_id_) == 0) return HWC2::Error::BadConfig; - *config = mode.id(); + *config = active_config_id_; return HWC2::Error::None; } @@ -443,46 +443,47 @@ HWC2::Error DrmHwcTwo::HwcDisplay::GetDisplayAttribute(hwc2_config_t config, int32_t attribute_in, int32_t *value) { supported(__func__); - auto mode = std::find_if(connector_->modes().begin(), - connector_->modes().end(), - [config](DrmMode const &m) { - return m.id() == config; - }); - if (mode == connector_->modes().end()) { - ALOGE("Could not find active mode for %d", config); + int conf = static_cast(config); + + if (hwc_configs_.count(conf) == 0) { + ALOGE("Could not find active mode for %d", conf); return HWC2::Error::BadConfig; } + auto &hwc_config = hwc_configs_[conf]; + static const int32_t kUmPerInch = 25400; uint32_t mm_width = connector_->mm_width(); uint32_t mm_height = connector_->mm_height(); auto attribute = static_cast(attribute_in); switch (attribute) { case HWC2::Attribute::Width: - *value = static_cast(mode->h_display()); + *value = static_cast(hwc_config.mode.h_display()); break; case HWC2::Attribute::Height: - *value = static_cast(mode->v_display()); + *value = static_cast(hwc_config.mode.v_display()); break; case HWC2::Attribute::VsyncPeriod: // in nanoseconds - *value = static_cast(1E9 / mode->v_refresh()); + *value = static_cast(1E9 / hwc_config.mode.v_refresh()); break; case HWC2::Attribute::DpiX: // Dots per 1000 inches - *value = mm_width - ? static_cast(mode->h_display() * kUmPerInch / mm_width) - : -1; + *value = mm_width ? static_cast(hwc_config.mode.h_display() * + kUmPerInch / mm_width) + : -1; break; case HWC2::Attribute::DpiY: // Dots per 1000 inches - *value = mm_height ? static_cast(mode->v_display() * kUmPerInch / - mm_height) + *value = mm_height ? static_cast(hwc_config.mode.v_display() * + kUmPerInch / mm_height) : -1; break; #if PLATFORM_SDK_VERSION > 29 case HWC2::Attribute::ConfigGroup: - *value = 0; /* TODO: Add support for config groups */ + /* Dispite ConfigGroup is a part of HWC2.4 API, framework + * able to request it even if service @2.1 is used */ + *value = hwc_config.group_id; break; #endif default: @@ -506,79 +507,149 @@ HWC2::Error DrmHwcTwo::HwcDisplay::GetDisplayConfigs(uint32_t *num_configs, ALOGE("Failed to update display modes %d", ret); return HWC2::Error::BadDisplay; } - } - // Since the upper layers only look at vactive/hactive/refresh, height and - // width, it doesn't differentiate interlaced from progressive and other - // similar modes. Depending on the order of modes we return to SF, it could - // end up choosing a suboptimal configuration and dropping the preferred - // mode. To workaround this, don't offer interlaced modes to SF if there is - // at least one non-interlaced alternative and only offer a single WxH@R - // mode with at least the prefered mode from in DrmConnector::UpdateModes() - - // TODO(nobody): Remove the following block of code until AOSP handles all - // modes - std::vector sel_modes; - - // Add the preferred mode first to be sure it's not dropped - auto mode = std::find_if(connector_->modes().begin(), - connector_->modes().end(), [&](DrmMode const &m) { - return m.id() == - connector_->get_preferred_mode_id(); - }); - if (mode != connector_->modes().end()) - sel_modes.push_back(*mode); - - // Add the active mode if different from preferred mode - if (connector_->active_mode().id() != connector_->get_preferred_mode_id()) - sel_modes.push_back(connector_->active_mode()); - - // Cycle over the modes and filter out "similar" modes, keeping only the - // first ones in the order given by DRM (from CEA ids and timings order) - for (const DrmMode &mode : connector_->modes()) { - // TODO(nobody): Remove this when 3D Attributes are in AOSP - if (mode.flags() & DRM_MODE_FLAG_3D_MASK) - continue; + hwc_configs_.clear(); + preferred_config_id_ = 0; + int preferred_config_group_id_ = 0; + + if (connector_->modes().empty()) { + ALOGE("No modes reported by KMS"); + return HWC2::Error::BadDisplay; + } - // TODO(nobody): Remove this when the Interlaced attribute is in AOSP - if (mode.flags() & DRM_MODE_FLAG_INTERLACE) { - auto m = std::find_if(connector_->modes().begin(), - connector_->modes().end(), - [&mode](DrmMode const &m) { - return !(m.flags() & DRM_MODE_FLAG_INTERLACE) && - m.h_display() == mode.h_display() && - m.v_display() == mode.v_display(); - }); - if (m == connector_->modes().end()) - sel_modes.push_back(mode); + int last_config_id = 1; + int last_group_id = 1; + + /* Group modes */ + for (const auto &mode : connector_->modes()) { + /* Find group for the new mode or create new group */ + int group_found = 0; + for (auto &hwc_config : hwc_configs_) { + if (mode.h_display() == hwc_config.second.mode.h_display() && + mode.v_display() == hwc_config.second.mode.v_display()) { + group_found = hwc_config.second.group_id; + } + } + if (group_found == 0) { + group_found = last_group_id++; + } - continue; + bool disabled = false; + if (mode.flags() & DRM_MODE_FLAG_3D_MASK) { + ALOGI("Disabling display mode %s (Modes with 3D flag aren't supported)", + mode.name().c_str()); + disabled = true; + } + + /* Add config */ + hwc_configs_[last_config_id] = { + .id = last_config_id, + .group_id = group_found, + .mode = mode, + .disabled = disabled, + }; + + /* Chwck if the mode is preferred */ + if ((mode.type() & DRM_MODE_TYPE_PREFERRED) != 0 && + preferred_config_id_ == 0) { + preferred_config_id_ = last_config_id; + preferred_config_group_id_ = group_found; + } + + last_config_id++; } - // Search for a similar WxH@R mode in the filtered list and drop it if - // another mode with the same WxH@R has already been selected - // TODO(nobody): Remove this when AOSP handles duplicates modes - auto m = std::find_if(sel_modes.begin(), sel_modes.end(), - [&mode](DrmMode const &m) { - return m.h_display() == mode.h_display() && - m.v_display() == mode.v_display() && - m.v_refresh() == mode.v_refresh(); - }); - if (m == sel_modes.end()) - sel_modes.push_back(mode); - } + /* We must have preferred mode. Set first mode as preferred + * in case KMS haven't reported anything. */ + if (preferred_config_id_ == 0) { + preferred_config_id_ = 1; + preferred_config_group_id_ = 1; + } - auto num_modes = static_cast(sel_modes.size()); - if (!configs) { - *num_configs = num_modes; - return HWC2::Error::None; + for (int group = 1; group < last_group_id; group++) { + bool has_interlaced = false; + bool has_progressive = false; + for (auto &hwc_config : hwc_configs_) { + if (hwc_config.second.group_id != group || hwc_config.second.disabled) { + continue; + } + + if (hwc_config.second.IsInterlaced()) { + has_interlaced = true; + } else { + has_progressive = true; + } + } + + bool has_both = has_interlaced && has_progressive; + if (!has_both) { + continue; + } + + bool group_contains_preferred_interlaced = false; + if (group == preferred_config_group_id_ && + hwc_configs_[preferred_config_id_].IsInterlaced()) { + group_contains_preferred_interlaced = true; + } + + for (auto &hwc_config : hwc_configs_) { + if (hwc_config.second.group_id != group || hwc_config.second.disabled) { + continue; + } + + bool disable = group_contains_preferred_interlaced + ? !hwc_config.second.IsInterlaced() + : hwc_config.second.IsInterlaced(); + + if (disable) { + ALOGI( + "Group %i: Disabling display mode %s (This group should consist " + "of %s modes)", + group, hwc_config.second.mode.name().c_str(), + group_contains_preferred_interlaced ? "interlaced" + : "progressive"); + + hwc_config.second.disabled = true; + } + } + } + + /* Group should not contain 2 modes with FPS delta less than ~1HZ + * otherwise android.graphics.cts.SetFrameRateTest CTS will fail + */ + for (int m1 = 1; m1 < last_config_id; m1++) { + for (int m2 = 1; m2 < last_config_id; m2++) { + if (m1 != m2 && + hwc_configs_[m1].group_id == hwc_configs_[m2].group_id && + !hwc_configs_[m1].disabled && !hwc_configs_[m2].disabled && + fabsf(hwc_configs_[m1].mode.v_refresh() - + hwc_configs_[m2].mode.v_refresh()) < 1.0) { + ALOGI( + "Group %i: Disabling display mode %s (Refresh rate value is " + "too close to existing mode %s)", + hwc_configs_[m2].group_id, hwc_configs_[m2].mode.name().c_str(), + hwc_configs_[m1].mode.name().c_str()); + + hwc_configs_[m2].disabled = true; + } + } + } } uint32_t idx = 0; - for (const DrmMode &mode : sel_modes) { - if (idx >= *num_configs) - break; - configs[idx++] = mode.id(); + for (auto &hwc_config : hwc_configs_) { + if (hwc_config.second.disabled) { + continue; + } + + if (configs != nullptr) { + if (idx >= *num_configs) { + break; + } + configs[idx] = hwc_config.second.id; + } + + idx++; } *num_configs = idx; return HWC2::Error::None; @@ -768,18 +839,18 @@ HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *present_fence) { HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) { supported(__func__); - auto mode = std::find_if(connector_->modes().begin(), - connector_->modes().end(), - [config](DrmMode const &m) { - return m.id() == config; - }); - if (mode == connector_->modes().end()) { - ALOGE("Could not find active mode for %d", config); + + int conf = static_cast(config); + + if (hwc_configs_.count(conf) == 0) { + ALOGE("Could not find active mode for %d", conf); return HWC2::Error::BadConfig; } + auto &mode = hwc_configs_[conf].mode; + AtomicCommitArgs a_args = { - .display_mode = *mode, + .display_mode = mode, .clear_active_composition = true, }; @@ -789,13 +860,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfig(hwc2_config_t config) { return HWC2::Error::BadConfig; } - connector_->set_active_mode(*mode); + active_config_id_ = conf; // Setup the client layer's dimensions hwc_rect_t display_frame = {.left = 0, .top = 0, - .right = static_cast(mode->h_display()), - .bottom = static_cast(mode->v_display())}; + .right = static_cast(mode.h_display()), + .bottom = static_cast(mode.v_display())}; client_layer_.SetLayerDisplayFrame(display_frame); return HWC2::Error::None; @@ -938,12 +1009,8 @@ HWC2::Error DrmHwcTwo::HwcDisplay::GetDisplayConnectionType(uint32_t *outType) { HWC2::Error DrmHwcTwo::HwcDisplay::GetDisplayVsyncPeriod( hwc2_vsync_period_t *outVsyncPeriod /* ns */) { supported(__func__); - DrmMode const &mode = connector_->active_mode(); - if (mode.id() == 0) - return HWC2::Error::BadConfig; - - *outVsyncPeriod = static_cast(1E9 / mode.v_refresh()); - return HWC2::Error::None; + return GetDisplayAttribute(active_config_id_, HWC2_ATTRIBUTE_VSYNC_PERIOD, + (int32_t *)(outVsyncPeriod)); } HWC2::Error DrmHwcTwo::HwcDisplay::SetActiveConfigWithConstraints( diff --git a/DrmHwcTwo.h b/DrmHwcTwo.h index ed547c0..7fd4fbd 100644 --- a/DrmHwcTwo.h +++ b/DrmHwcTwo.h @@ -160,7 +160,7 @@ class DrmHwcTwo : public hwc2_device_t { HWC2::Error AcceptDisplayChanges(); HWC2::Error CreateLayer(hwc2_layer_t *layer); HWC2::Error DestroyLayer(hwc2_layer_t layer); - HWC2::Error GetActiveConfig(hwc2_config_t *config); + HWC2::Error GetActiveConfig(hwc2_config_t *config) const; HWC2::Error GetChangedCompositionTypes(uint32_t *num_elements, hwc2_layer_t *layers, int32_t *types); @@ -250,6 +250,22 @@ class DrmHwcTwo : public hwc2_device_t { uint32_t frames_flattened_ = 0; }; + struct HwcDisplayConfig { + int id{}; + int group_id{}; + DrmMode mode; + bool disabled{}; + + bool IsInterlaced() { + return (mode.flags() & DRM_MODE_FLAG_INTERLACE) != 0; + } + }; + + std::map hwc_configs_; + + int active_config_id_ = 0; + int preferred_config_id_ = 0; + const Backend *backend() const { return backend_.get(); } diff --git a/drm/DrmConnector.cpp b/drm/DrmConnector.cpp index b3179c7..5b3c697 100644 --- a/drm/DrmConnector.cpp +++ b/drm/DrmConnector.cpp @@ -159,43 +159,27 @@ std::string DrmConnector::name() const { } int DrmConnector::UpdateModes() { - int fd = drm_->fd(); - - drmModeConnectorPtr c = drmModeGetConnector(fd, id_); + drmModeConnectorPtr c = drmModeGetConnector(drm_->fd(), id_); if (!c) { ALOGE("Failed to get connector %d", id_); return -ENODEV; } - state_ = c->connection; - - bool preferred_mode_found = false; - std::vector new_modes; + modes_.clear(); for (int i = 0; i < c->count_modes; ++i) { bool exists = false; for (const DrmMode &mode : modes_) { if (mode == c->modes[i]) { - new_modes.push_back(mode); exists = true; break; } } + if (!exists) { - DrmMode m(&c->modes[i]); - m.set_id(drm_->next_mode_id()); - new_modes.push_back(m); + modes_.emplace_back(DrmMode(&c->modes[i])); } - // Use only the first DRM_MODE_TYPE_PREFERRED mode found - if (!preferred_mode_found && - (new_modes.back().type() & DRM_MODE_TYPE_PREFERRED)) { - preferred_mode_id_ = new_modes.back().id(); - preferred_mode_found = true; - } - } - modes_.swap(new_modes); - if (!preferred_mode_found && !modes_.empty()) { - preferred_mode_id_ = modes_[0].id(); } + return 0; } diff --git a/drm/DrmConnector.h b/drm/DrmConnector.h index e2789ea..f2305aa 100644 --- a/drm/DrmConnector.h +++ b/drm/DrmConnector.h @@ -82,10 +82,6 @@ class DrmConnector { uint32_t mm_width() const; uint32_t mm_height() const; - uint32_t get_preferred_mode_id() const { - return preferred_mode_id_; - } - private: DrmDevice *drm_; @@ -111,8 +107,6 @@ class DrmConnector { DrmProperty writeback_out_fence_; std::vector possible_encoders_; - - uint32_t preferred_mode_id_{}; }; } // namespace android diff --git a/drm/DrmMode.cpp b/drm/DrmMode.cpp index 971c327..1c8bd0f 100644 --- a/drm/DrmMode.cpp +++ b/drm/DrmMode.cpp @@ -49,14 +49,6 @@ bool DrmMode::operator==(const drmModeModeInfo &m) const { v_scan_ == m.vscan && flags_ == m.flags && type_ == m.type; } -uint32_t DrmMode::id() const { - return id_; -} - -void DrmMode::set_id(uint32_t id) { - id_ = id; -} - uint32_t DrmMode::clock() const { return clock_; } @@ -115,7 +107,7 @@ uint32_t DrmMode::type() const { } std::string DrmMode::name() const { - return name_; + return name_ + "@" + std::to_string(v_refresh()); } auto DrmMode::CreateModeBlob(const DrmDevice &drm) diff --git a/drm/DrmMode.h b/drm/DrmMode.h index 0974b5a..41b9e15 100644 --- a/drm/DrmMode.h +++ b/drm/DrmMode.h @@ -17,9 +17,10 @@ #ifndef ANDROID_DRM_MODE_H_ #define ANDROID_DRM_MODE_H_ -#include +#include #include +#include #include #include "DrmUnique.h" @@ -35,9 +36,6 @@ class DrmMode { bool operator==(const drmModeModeInfo &m) const; - uint32_t id() const; - void set_id(uint32_t id); - uint32_t clock() const; uint16_t h_display() const; @@ -61,8 +59,6 @@ class DrmMode { auto CreateModeBlob(const DrmDevice &drm) -> DrmModeUserPropertyBlobUnique; private: - uint32_t id_ = 0; - uint32_t clock_ = 0; uint16_t h_display_ = 0; -- cgit v1.2.3 From 39d8f7e1934b40dc84e784f2ab2f637f1a6202bb Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Tue, 30 Nov 2021 17:06:44 +0200 Subject: drm_hwcomposer: Validate gralloc0 name for minigbm and libdrm getters Using of incorrect gralloc0 results in a runtime issues, with logs like "Cannot convert hal format to drm format " or other. Validate gralloc name and exit gracefully in case it doesnt't match the one we are expecting. Signed-off-by: Roman Stratiienko --- bufferinfo/BufferInfoGetter.h | 14 +++++++++++--- bufferinfo/legacy/BufferInfoLibdrm.cpp | 16 ++++++++++++++++ bufferinfo/legacy/BufferInfoLibdrm.h | 1 + bufferinfo/legacy/BufferInfoMinigbm.cpp | 13 +++++++++++++ bufferinfo/legacy/BufferInfoMinigbm.h | 1 + 5 files changed, 42 insertions(+), 3 deletions(-) diff --git a/bufferinfo/BufferInfoGetter.h b/bufferinfo/BufferInfoGetter.h index 60ca985..7b088df 100644 --- a/bufferinfo/BufferInfoGetter.h +++ b/bufferinfo/BufferInfoGetter.h @@ -49,6 +49,10 @@ class LegacyBufferInfoGetter : public BufferInfoGetter { int Init(); + virtual int ValidateGralloc() { + return 0; + } + int ConvertBoInfo(buffer_handle_t handle, hwc_drm_bo_t *bo) override = 0; static std::unique_ptr CreateInstance(); @@ -65,9 +69,13 @@ class LegacyBufferInfoGetter : public BufferInfoGetter { LegacyBufferInfoGetter::CreateInstance() { \ auto instance = std::make_unique(); \ if (instance) { \ - int ret = instance->Init(); \ - if (ret) { \ - ALOGE("Failed to initialize the " #getter_ " getter %d", ret); \ + int err = instance->Init(); \ + if (err) { \ + ALOGE("Failed to initialize the " #getter_ " getter %d", err); \ + instance.reset(); \ + } \ + err = instance->ValidateGralloc(); \ + if (err) { \ instance.reset(); \ } \ } \ diff --git a/bufferinfo/legacy/BufferInfoLibdrm.cpp b/bufferinfo/legacy/BufferInfoLibdrm.cpp index 6243d8d..e70536b 100644 --- a/bufferinfo/legacy/BufferInfoLibdrm.cpp +++ b/bufferinfo/legacy/BufferInfoLibdrm.cpp @@ -204,4 +204,20 @@ int BufferInfoLibdrm::ConvertBoInfo(buffer_handle_t handle, hwc_drm_bo_t *bo) { return 0; } +constexpr char gbm_gralloc_module_name[] = "GBM Memory Allocator"; +constexpr char drm_gralloc_module_name[] = "DRM Memory Allocator"; + +int BufferInfoLibdrm::ValidateGralloc() { + if (strcmp(gralloc_->common.name, drm_gralloc_module_name) != 0 && + strcmp(gralloc_->common.name, gbm_gralloc_module_name) != 0) { + ALOGE( + "Gralloc name isn't valid: Expected: \"%s\" or \"%s\", Actual: \"%s\"", + gbm_gralloc_module_name, drm_gralloc_module_name, + gralloc_->common.name); + return -EINVAL; + } + + return 0; +} + } // namespace android diff --git a/bufferinfo/legacy/BufferInfoLibdrm.h b/bufferinfo/legacy/BufferInfoLibdrm.h index 4d37d00..cad8add 100644 --- a/bufferinfo/legacy/BufferInfoLibdrm.h +++ b/bufferinfo/legacy/BufferInfoLibdrm.h @@ -27,6 +27,7 @@ class BufferInfoLibdrm : public LegacyBufferInfoGetter { public: using LegacyBufferInfoGetter::LegacyBufferInfoGetter; int ConvertBoInfo(buffer_handle_t handle, hwc_drm_bo_t *bo) override; + int ValidateGralloc() override; private: bool GetYuvPlaneInfo(int num_fds, buffer_handle_t handle, hwc_drm_bo_t *bo); diff --git a/bufferinfo/legacy/BufferInfoMinigbm.cpp b/bufferinfo/legacy/BufferInfoMinigbm.cpp index 1657ea6..93b9e98 100644 --- a/bufferinfo/legacy/BufferInfoMinigbm.cpp +++ b/bufferinfo/legacy/BufferInfoMinigbm.cpp @@ -22,6 +22,7 @@ #include #include +#include #include "cros_gralloc_handle.h" #include "utils/log.h" @@ -57,4 +58,16 @@ int BufferInfoMinigbm::ConvertBoInfo(buffer_handle_t handle, hwc_drm_bo_t *bo) { return 0; } +constexpr char cros_gralloc_module_name[] = "CrOS Gralloc"; + +int BufferInfoMinigbm::ValidateGralloc() { + if (strcmp(gralloc_->common.name, cros_gralloc_module_name) != 0) { + ALOGE("Gralloc name isn't valid: Expected: \"%s\", Actual: \"%s\"", + cros_gralloc_module_name, gralloc_->common.name); + return -EINVAL; + } + + return 0; +} + } // namespace android diff --git a/bufferinfo/legacy/BufferInfoMinigbm.h b/bufferinfo/legacy/BufferInfoMinigbm.h index bff9d74..04cc2ae 100644 --- a/bufferinfo/legacy/BufferInfoMinigbm.h +++ b/bufferinfo/legacy/BufferInfoMinigbm.h @@ -27,6 +27,7 @@ class BufferInfoMinigbm : public LegacyBufferInfoGetter { public: using LegacyBufferInfoGetter::LegacyBufferInfoGetter; int ConvertBoInfo(buffer_handle_t handle, hwc_drm_bo_t *bo) override; + int ValidateGralloc() override; }; } // namespace android -- cgit v1.2.3 From 5621f5fd4c718ce06b14e4a33dfc68f42b6d10b2 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Tue, 30 Nov 2021 17:48:16 +0200 Subject: drm_hwcomposer: Use gralloc0::perform API by minigbm bufferinfo getter Using of internals of cros_gralloc_handle isn't recommended, since it can be changed at any time. Meanwhile minigbm provides another API to access buffer information based on gralloc0 perform() call. ChromiumOS are using this API by mesa3d and other related projects. Signed-off-by: Roman Stratiienko --- Android.bp | 1 - bufferinfo/legacy/BufferInfoMinigbm.cpp | 88 +++++++++++++++++++++++++------- tests/test_include/cros_gralloc_handle.h | 53 ------------------- 3 files changed, 70 insertions(+), 72 deletions(-) delete mode 100644 tests/test_include/cros_gralloc_handle.h diff --git a/Android.bp b/Android.bp index 87e2efd..4df2b07 100644 --- a/Android.bp +++ b/Android.bp @@ -136,7 +136,6 @@ cc_library_shared { ":drm_hwcomposer_common", "bufferinfo/legacy/BufferInfoMinigbm.cpp", ], - include_dirs: ["external/minigbm/cros_gralloc"], } // Used by hwcomposer.drm_imagination diff --git a/bufferinfo/legacy/BufferInfoMinigbm.cpp b/bufferinfo/legacy/BufferInfoMinigbm.cpp index 93b9e98..777c2b7 100644 --- a/bufferinfo/legacy/BufferInfoMinigbm.cpp +++ b/bufferinfo/legacy/BufferInfoMinigbm.cpp @@ -24,35 +24,80 @@ #include #include -#include "cros_gralloc_handle.h" #include "utils/log.h" - -#define DRM_FORMAT_YVU420_ANDROID fourcc_code('9', '9', '9', '7') - namespace android { LEGACY_BUFFER_INFO_GETTER(BufferInfoMinigbm); +constexpr int CROS_GRALLOC_DRM_GET_FORMAT = 1; +constexpr int CROS_GRALLOC_DRM_GET_DIMENSIONS = 2; +constexpr int CROS_GRALLOC_DRM_GET_BUFFER_INFO = 4; +constexpr int CROS_GRALLOC_DRM_GET_USAGE = 5; + +struct cros_gralloc0_buffer_info { + uint32_t drm_fourcc; + int num_fds; + int fds[4]; + uint64_t modifier; + int offset[4]; + int stride[4]; +}; + int BufferInfoMinigbm::ConvertBoInfo(buffer_handle_t handle, hwc_drm_bo_t *bo) { - auto *gr_handle = (cros_gralloc_handle *)handle; - if (!gr_handle) + if (handle == nullptr) { + return -EINVAL; + } + + uint32_t width{}; + uint32_t height{}; + if (gralloc_->perform(gralloc_, CROS_GRALLOC_DRM_GET_DIMENSIONS, handle, + &width, &height) != 0) { + ALOGE( + "CROS_GRALLOC_DRM_GET_DIMENSIONS operation has failed. " + "Please ensure you are using the latest minigbm."); + return -EINVAL; + } + + int32_t droid_format{}; + if (gralloc_->perform(gralloc_, CROS_GRALLOC_DRM_GET_FORMAT, handle, + &droid_format) != 0) { + ALOGE( + "CROS_GRALLOC_DRM_GET_FORMAT operation has failed. " + "Please ensure you are using the latest minigbm."); return -EINVAL; + } + + uint32_t usage{}; + if (gralloc_->perform(gralloc_, CROS_GRALLOC_DRM_GET_USAGE, handle, &usage) != + 0) { + ALOGE( + "CROS_GRALLOC_DRM_GET_USAGE operation has failed. " + "Please ensure you are using the latest minigbm."); + return -EINVAL; + } + + struct cros_gralloc0_buffer_info info {}; + if (gralloc_->perform(gralloc_, CROS_GRALLOC_DRM_GET_BUFFER_INFO, handle, + &info) != 0) { + ALOGE( + "CROS_GRALLOC_DRM_GET_BUFFER_INFO operation has failed. " + "Please ensure you are using the latest minigbm."); + return -EINVAL; + } - bo->width = gr_handle->width; - bo->height = gr_handle->height; - bo->hal_format = gr_handle->droid_format; + bo->width = width; + bo->height = height; - bo->format = gr_handle->format; - if (bo->format == DRM_FORMAT_YVU420_ANDROID) - bo->format = DRM_FORMAT_YVU420; + bo->hal_format = droid_format; - bo->usage = gr_handle->usage; + bo->format = info.drm_fourcc; + bo->usage = usage; - for (int i = 0; i < gr_handle->num_planes; i++) { - bo->modifiers[i] = gr_handle->format_modifier; - bo->prime_fds[i] = gr_handle->fds[i]; - bo->pitches[i] = gr_handle->strides[i]; - bo->offsets[i] = gr_handle->offsets[i]; + for (int i = 0; i < info.num_fds; i++) { + bo->modifiers[i] = info.modifier; + bo->prime_fds[i] = info.fds[i]; + bo->pitches[i] = info.stride[i]; + bo->offsets[i] = info.offset[i]; } return 0; @@ -67,6 +112,13 @@ int BufferInfoMinigbm::ValidateGralloc() { return -EINVAL; } + if (gralloc_->perform == nullptr) { + ALOGE( + "CrOS gralloc has no perform call implemented. Please upgrade your " + "minigbm."); + return -EINVAL; + } + return 0; } diff --git a/tests/test_include/cros_gralloc_handle.h b/tests/test_include/cros_gralloc_handle.h deleted file mode 100644 index d77d777..0000000 --- a/tests/test_include/cros_gralloc_handle.h +++ /dev/null @@ -1,53 +0,0 @@ -// clang-format off -/* - * Copyright 2016 The Chromium OS Authors. All rights reserved. - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#ifndef CROS_GRALLOC_HANDLE_H -#define CROS_GRALLOC_HANDLE_H - -#include -#include - -#define DRV_MAX_PLANES 4 -#define DRV_MAX_FDS (DRV_MAX_PLANES + 1) - -struct cros_gralloc_handle : public native_handle_t { - /* - * File descriptors must immediately follow the native_handle_t base and used file - * descriptors must be packed at the beginning of this array to work with - * native_handle_clone(). - * - * This field contains 'num_planes' plane file descriptors followed by an optional metadata - * reserved region file descriptor if 'reserved_region_size' is greater than zero. - */ - int32_t fds[DRV_MAX_FDS]; - uint32_t strides[DRV_MAX_PLANES]; - uint32_t offsets[DRV_MAX_PLANES]; - uint32_t sizes[DRV_MAX_PLANES]; - uint32_t id; - uint32_t width; - uint32_t height; - uint32_t format; /* DRM format */ - uint32_t tiling; - uint64_t format_modifier; - uint64_t use_flags; /* Buffer creation flags */ - uint32_t magic; - uint32_t pixel_stride; - int32_t droid_format; - int32_t usage; /* Android usage. */ - uint32_t num_planes; - uint64_t reserved_region_size; - uint64_t total_size; /* Total allocation size */ - /* - * Name is a null terminated char array located at handle->base.data[handle->name_offset]. - */ - uint32_t name_offset; -} __attribute__((packed)); - -typedef const struct cros_gralloc_handle *cros_gralloc_handle_t; - -#endif -// clang-format on -- cgit v1.2.3 From deb77352b12e723f101cc2bf948faf93b4c603dd Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Mon, 6 Dec 2021 12:59:26 +0200 Subject: drm_hwcomposer: Fix all cases which triggers an error on -Wsign-compare Android-9 has -Wsign-compare enabled by default and it causes build issues. Enable -Wsign-compare option in CI, so we won't introduce such issues anymore. Signed-off-by: Roman Stratiienko --- .ci/.common.sh | 2 +- backend/Backend.cpp | 12 ++++++------ bufferinfo/legacy/BufferInfoLibdrm.cpp | 8 ++++---- drm/DrmFbImporter.cpp | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/.ci/.common.sh b/.ci/.common.sh index d9b5b3c..48cc594 100644 --- a/.ci/.common.sh +++ b/.ci/.common.sh @@ -4,7 +4,7 @@ CLANG="clang++-12" CLANG_TIDY="clang-tidy-12" CXXARGS="-fPIC -Wall -Werror -DPLATFORM_SDK_VERSION=30 -D__ANDROID_API__=30 -Wsign-promo -Wimplicit-fallthrough" -CXXARGS+=" -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS -Wno-gnu-include-next " +CXXARGS+=" -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS -Wno-gnu-include-next -Wsign-compare" CXXARGS+=" -fvisibility-inlines-hidden -std=gnu++17 -DHWC2_USE_CPP11 -DHWC2_INCLUDE_STRINGIFICATION -fno-rtti" BUILD_FILES=( diff --git a/backend/Backend.cpp b/backend/Backend.cpp index bd1855f..ce606dd 100644 --- a/backend/Backend.cpp +++ b/backend/Backend.cpp @@ -72,7 +72,7 @@ std::tuple Backend::GetClientLayers( int client_start = -1; size_t client_size = 0; - for (int z_order = 0; z_order < layers.size(); ++z_order) { + for (size_t z_order = 0; z_order < layers.size(); ++z_order) { if (IsClientLayer(display, layers[z_order])) { if (client_start < 0) client_start = (int)z_order; @@ -100,7 +100,7 @@ bool Backend::HardwareSupportsLayerType(HWC2::Composition comp_type) { uint32_t Backend::CalcPixOps(const std::vector &layers, size_t first_z, size_t size) { uint32_t pixops = 0; - for (int z_order = 0; z_order < layers.size(); ++z_order) { + for (size_t z_order = 0; z_order < layers.size(); ++z_order) { if (z_order >= first_z && z_order < first_z + size) { hwc_rect_t df = layers[z_order]->display_frame(); pixops += (df.right - df.left) * (df.bottom - df.top); @@ -111,7 +111,7 @@ uint32_t Backend::CalcPixOps(const std::vector &layers, void Backend::MarkValidated(std::vector &layers, size_t client_first_z, size_t client_size) { - for (int z_order = 0; z_order < layers.size(); ++z_order) { + for (size_t z_order = 0; z_order < layers.size(); ++z_order) { if (z_order >= client_first_z && z_order < client_first_z + client_size) layers[z_order]->set_validated_type(HWC2::Composition::Client); else @@ -152,12 +152,12 @@ std::tuple Backend::GetExtraClientRange( steps = 1 + layers.size() - extra_client; } - uint32_t gpu_pixops = INT_MAX; - for (int i = 0; i < steps; i++) { + uint32_t gpu_pixops = UINT32_MAX; + for (size_t i = 0; i < steps; i++) { uint32_t po = CalcPixOps(layers, start + i, client_size); if (po < gpu_pixops) { gpu_pixops = po; - client_start = start + i; + client_start = start + int(i); } } } diff --git a/bufferinfo/legacy/BufferInfoLibdrm.cpp b/bufferinfo/legacy/BufferInfoLibdrm.cpp index e70536b..47481b2 100644 --- a/bufferinfo/legacy/BufferInfoLibdrm.cpp +++ b/bufferinfo/legacy/BufferInfoLibdrm.cpp @@ -66,15 +66,15 @@ static const struct DroidYuvFormat kDroidYuvFormats[] = { #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) -static int get_fourcc_yuv(uint32_t native, enum chroma_order chroma_order, - size_t chroma_step) { +static uint32_t get_fourcc_yuv(uint32_t native, enum chroma_order chroma_order, + size_t chroma_step) { for (auto droid_yuv_format : kDroidYuvFormats) if (droid_yuv_format.native == native && droid_yuv_format.chroma_order == chroma_order && droid_yuv_format.chroma_step == chroma_step) return droid_yuv_format.fourcc; - return -1; + return UINT32_MAX; } static bool is_yuv(uint32_t native) { @@ -131,7 +131,7 @@ bool BufferInfoLibdrm::GetYuvPlaneInfo(int num_fds, buffer_handle_t handle, /* .chroma_step is the byte distance between the same chroma channel * values of subsequent pixels, assumed to be the same for Cb and Cr. */ bo->format = get_fourcc_yuv(bo->hal_format, chroma_order, ycbcr.chroma_step); - if (bo->format == -1) { + if (bo->format == UINT32_MAX) { ALOGW( "unsupported YUV format, native = %x, chroma_order = %s, chroma_step = " "%d", diff --git a/drm/DrmFbImporter.cpp b/drm/DrmFbImporter.cpp index 8440093..25f32b7 100644 --- a/drm/DrmFbImporter.cpp +++ b/drm/DrmFbImporter.cpp @@ -41,7 +41,7 @@ auto DrmFbIdHandle::CreateInstance(hwc_drm_bo_t *bo, GemHandle first_gem_handle, int32_t err = 0; /* Framebuffer object creation require gem handle for every used plane */ - for (int i = 1; i < local->gem_handles_.size(); i++) { + for (size_t i = 1; i < local->gem_handles_.size(); i++) { if (bo->prime_fds[i] > 0) { if (bo->prime_fds[i] != bo->prime_fds[0]) { err = drmPrimeFDToHandle(drm->fd(), bo->prime_fds[i], @@ -101,7 +101,7 @@ DrmFbIdHandle::~DrmFbIdHandle() { * request via system properties) */ struct drm_gem_close gem_close {}; - for (int i = 0; i < gem_handles_.size(); i++) { + for (size_t i = 0; i < gem_handles_.size(); i++) { /* Don't close invalid handle. Close handle only once in cases * where several YUV planes located in the single buffer. */ if (gem_handles_[i] == 0 || -- cgit v1.2.3 From 720f65224fb0ceaaee8581c4db1e55a79669c494 Mon Sep 17 00:00:00 2001 From: Roman Stratiienko Date: Mon, 6 Dec 2021 14:56:14 +0200 Subject: drm_hwcomposer: Assume premultiplied alpha for CLIENT layer On db845c, we have seen an odd behavior with modal dialogs in AOSP, where the background which is normally darkened, was rendering as black, though it would occasionally flicker to the right thing. This cropped up after alpha support on planes landed in 5.15, so we assumed it was an edge case issue with the dpu1 driver. But while working on a separate issue seen on x86 hardware: https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/-/issues/46 Roman noticed drm_hwcomposer always sets the blending type to NONE for the client layer. Which had gone un-noticed becase he didn't have any devices that exposed the NONE type. Thus Roman implemented this patch, which sets up the client blend type to PREMULTIPLIED. While it did not resolve the x86 issue above, it does resolve the incorrect rendering seen on db845c with alpha support enabled. Tested-by: Martin Juecker martin.juecker@gmail.com #Exynos 4412 Signed-off-by: Roman Stratiienko Signed-off-by: John Stultz Change-Id: I95601c680ca1af0dc9d3b3f102f79f77af081b75 --- DrmHwcTwo.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp index 6faec1c..9bf1327 100644 --- a/DrmHwcTwo.cpp +++ b/DrmHwcTwo.cpp @@ -341,6 +341,8 @@ HWC2::Error DrmHwcTwo::HwcDisplay::Init(std::vector *planes) { return HWC2::Error::BadDisplay; } + client_layer_.SetLayerBlendMode(HWC2_BLEND_MODE_PREMULTIPLIED); + return ChosePreferredConfig(); } -- cgit v1.2.3