[PATCH hwc v2 00/18] Add scene flattening support

[PATCH hwc v2 00/18] Add scene flattening support

[Alexandru Gheorghe]

Flattening a scene in order to reduce memory consumption it's an idea
which had been floating around on irc and mailing list several times,
this patchset adds support for flattening a scene using a writeback
connector, the latest version of the kernel patches could be found
here [1].

v1 for this patch series could be found here [2].

Changes since v1:
  - Add support to use either the same crtc or a dedicated one, this
    had been discussed on irc here [3].
  - Add support for parsing more than one drm node, this is triggered
    by the fact the on Mali DP each device has it's own driver, so
    it's needed in order to use a dedicate crtc for flattening.
  - Parse encoder possible_clones to detect if writeback could
    function simultaneously with the display connector.
  - Fixes for some bugs discovered while working on this.
  - Split changes in multiple patches.

The patches could be classified in:

[1-3]: Bug fixes that could be independently merged
  drm_hwcomposer: vsyncworker: Fix uninitialized enabled_ field
  drm_hwcomposer: vsyncworker: Fix deadlock on exit path
  drm_hwcomposer: drmeventlistener: Set nl_pid to 0

[4-5]: Support for multiple drm devices:
  drm_hwcomposer: Add resource manager class
  drm_hwcomposer: Enable resource manager support

[6-18]: Scene flattening support
  drm_hwcomposer: Add writeback connector support
  drm_hwcomposer: Add display field to Drmencoder
  drm_hwcomposer: Parse and store possible_clones information
  drm_hwcomposer: Handle writeback connectors
  drm_hwcomposer: hwcutils: Add function for cloning a DrmHwcLayer
  drm_hwcomposer: Add utility functions to copy displaycomposition
    internals
  drm_hwcomposer: Add utility function to create an initialized
    composition
  drm_hwcomposer: Pass buffer sizes to Prepareframebuffer
  drm_hwcomposer: Fix race in ApplyFrame
  drm_hwcomposer: Add worker to trigger scene flattenning
  drm_hwcomposer: Find writeback connector for scene flattening
  drm_hwcomposer: Flatten scene synchronously
  drm_hwcomposer: Flatten scene asynchronously

Tested both code paths on Mali DP with some hacks that disable
the GLCompositor.

[1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
[2] https://www.spinics.net/lists/dri-devel/msg169046.html
[3] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2018-03-21



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 01/18] drm_hwcomposer: vsyncworker: Fix uninitialized enabled_ field

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 vsyncworker.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vsyncworker.cpp b/vsyncworker.cpp
index 3ad16fe..3bfe4be 100644
--- a/vsyncworker.cpp
+++ b/vsyncworker.cpp
@@ -35,6 +35,7 @@ VSyncWorker::VSyncWorker()
     : Worker("vsync", HAL_PRIORITY_URGENT_DISPLAY),
       drm_(NULL),
       display_(-1),
+      enabled_(false),
       last_timestamp_(-1) {
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Alexandru Gheorghe]

vsyncworker::Routine assumes that when -EINTR is returned by
WaitForSignalOrExitLocked the lock as been released, which is not
true, so it hangs if a vsyncworker is never enabled and Exit is
called.

There are two code paths in WaitForSignalOrExitLocked that return
-EINTR, one releases the lock the other doesn't.
Looking at the clients of WaitForSignalOrExitLocked all assume the lock
is still held, except vsyncworker::Routine.
So, the proper fix needs two changes:
- Make WaitForSignalOrExitLocked consistent and always hold the lock
  when exiting.
- Release lock in vsynworker::Routine on all code paths.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 vsyncworker.cpp | 1 +
 worker.cpp      | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/vsyncworker.cpp b/vsyncworker.cpp
index 3bfe4be..7c0c741 100644
--- a/vsyncworker.cpp
+++ b/vsyncworker.cpp
@@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
   if (!enabled_) {
     ret = WaitForSignalOrExitLocked();
     if (ret == -EINTR) {
+      Unlock();
       return;
     }
   }
diff --git a/worker.cpp b/worker.cpp
index da6c580..5b351e0 100644
--- a/worker.cpp
+++ b/worker.cpp
@@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t max_nanoseconds) {
     ret = -ETIMEDOUT;
   }
 
+  // release leaves mutex locked when going out of scope
+  lk.release();
+
   // exit takes precedence on timeout
   if (should_exit())
     ret = -EINTR;
 
-  // release leaves mutex locked when going out of scope
-  lk.release();
-
   return ret;
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 03/18] drm_hwcomposer: drmeventlistener: Set nl_pid to 0

[Alexandru Gheorghe]

By setting nl_pid to 0, we let the kernel to assign a port for us.
In the current implementation there is no way we could create more
than one instance for drmeventlistener.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmeventlistener.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drmeventlistener.cpp b/drmeventlistener.cpp
index 984d1dd..5534182 100644
--- a/drmeventlistener.cpp
+++ b/drmeventlistener.cpp
@@ -46,7 +46,7 @@ int DrmEventListener::Init() {
   struct sockaddr_nl addr;
   memset(&addr, 0, sizeof(addr));
   addr.nl_family = AF_NETLINK;
-  addr.nl_pid = getpid();
+  addr.nl_pid = 0;
   addr.nl_groups = 0xFFFFFFFF;
 
   int ret = bind(uevent_fd_.get(), (struct sockaddr *)&addr, sizeof(addr));
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class

[Alexandru Gheorghe]

Add a resource manager object that is responsible for detecting all
kms devices and allocates unique display numbers for every detected
display.

This is controlled by the value of hwc.drm.device property, if it ends
with a %, it will try to open minor devices until and error is detected.
E.g: /dev/dri/card%

Additionally, this will be used for finding an available writeback
connector that will be used for the flattening of the currently
displayed scene.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 Android.mk          |  1 +
 resourcemanager.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 resourcemanager.h   | 29 ++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 resourcemanager.cpp
 create mode 100644 resourcemanager.h

diff --git a/Android.mk b/Android.mk
index 1add286..736fe24 100644
--- a/Android.mk
+++ b/Android.mk
@@ -52,6 +52,7 @@ LOCAL_C_INCLUDES := \
 
 LOCAL_SRC_FILES := \
 	autolock.cpp \
+	resourcemanager.cpp \
 	drmresources.cpp \
 	drmconnector.cpp \
 	drmcrtc.cpp \
diff --git a/resourcemanager.cpp b/resourcemanager.cpp
new file mode 100644
index 0000000..e7b654e
--- /dev/null
+++ b/resourcemanager.cpp
@@ -0,0 +1,71 @@
+#include "resourcemanager.h"
+#include <cutils/log.h>
+#include <cutils/properties.h>
+
+namespace android {
+
+ResourceManager::ResourceManager() : gralloc_(NULL) {
+}
+
+int ResourceManager::Init() {
+  char path_pattern[PROPERTY_VALUE_MAX];
+  property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
+
+  uint8_t minor = 0;
+  int last_display_index = 0;
+  int last_char = strlen(path_pattern) - 1;
+  do {
+    char path[PROPERTY_VALUE_MAX];
+    if (path_pattern[last_char] == '%') {
+      path_pattern[last_char] = '\0';
+      snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor);
+      path_pattern[last_char] = '%';
+    } else {
+      snprintf(path, PROPERTY_VALUE_MAX, "%s", path_pattern);
+    }
+    std::unique_ptr<DrmResources> drm = std::make_unique<DrmResources>();
+    last_display_index = drm->Init(this, path, last_display_index);
+    if (last_display_index < 0) {
+      break;
+    }
+    std::shared_ptr<Importer> importer;
+    importer.reset(Importer::CreateInstance(drm.get()));
+    if (!importer) {
+      ALOGE("Failed to create importer instance");
+      break;
+    }
+    importers_.push_back(importer);
+    drms_.push_back(std::move(drm));
+    minor++;
+    last_display_index++;
+  } while (path_pattern[last_char] == '%');
+
+  if (!drms_.size()) {
+    ALOGE("Failed to find any working drm device");
+    return -EINVAL;
+  }
+
+  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
+                       (const hw_module_t **)&gralloc_);
+}
+
+DrmResources *ResourceManager::GetDrmResources(int display) {
+  for (uint32_t i = 0; i < drms_.size(); i++) {
+    if (drms_[i]->HandlesDisplay(display))
+      return drms_[i].get();
+  }
+  return NULL;
+}
+
+std::shared_ptr<Importer> ResourceManager::GetImporter(int display) {
+  for (uint32_t i = 0; i < drms_.size(); i++) {
+    if (drms_[i]->HandlesDisplay(display))
+      return importers_[i];
+  }
+  return NULL;
+}
+
+const gralloc_module_t *ResourceManager::GetGralloc() {
+  return gralloc_;
+}
+}
diff --git a/resourcemanager.h b/resourcemanager.h
new file mode 100644
index 0000000..b8caa9a
--- /dev/null
+++ b/resourcemanager.h
@@ -0,0 +1,29 @@
+#ifndef RESOURCEMANAGER_H
+#define RESOURCEMANAGER_H
+
+#include "drmresources.h"
+#include "platform.h"
+
+namespace android {
+
+class DrmResources;
+class Importer;
+
+class ResourceManager {
+ public:
+  ResourceManager();
+  ResourceManager(const ResourceManager &) = delete;
+  ResourceManager &operator=(const ResourceManager &) = delete;
+  int Init();
+  DrmResources *GetDrmResources(int display);
+  std::shared_ptr<Importer> GetImporter(int display);
+  const gralloc_module_t *GetGralloc();
+
+ private:
+  std::vector<std::unique_ptr<DrmResources>> drms_;
+  std::vector<std::shared_ptr<Importer>> importers_;
+  const gralloc_module_t *gralloc_;
+};
+}
+
+#endif  // RESOURCEMANAGER_H
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alexandru Gheorghe]

Use the newly added ResourceManager for creating and detecting all the
drm devices instead of assuming that there is only one device.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmhwctwo.cpp    | 34 +++++++++++++---------------------
 drmhwctwo.h      |  4 +---
 drmresources.cpp | 25 ++++++++++++++++++-------
 drmresources.h   | 14 +++++++++++---
 4 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index dfca1a6..cddd5da 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -58,40 +58,32 @@ DrmHwcTwo::DrmHwcTwo() {
 }
 
 HWC2::Error DrmHwcTwo::Init() {
-  int ret = drm_.Init();
+  int ret = resource_manager_.Init();
   if (ret) {
-    ALOGE("Can't initialize drm object %d", ret);
+    ALOGE("Can't initialize the resource manager %d", ret);
     return HWC2::Error::NoResources;
   }
 
-  importer_.reset(Importer::CreateInstance(&drm_));
-  if (!importer_) {
-    ALOGE("Failed to create importer instance");
+  DrmResources *drm = resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
+  std::shared_ptr<Importer> importer =
+      resource_manager_.GetImporter(HWC_DISPLAY_PRIMARY);
+  if (!drm || !importer) {
+    ALOGE("Failed to get a valid drmresource and importer");
     return HWC2::Error::NoResources;
   }
+  displays_.emplace(
+      std::piecewise_construct, std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
+      std::forward_as_tuple(drm, importer, resource_manager_.GetGralloc(),
+                            HWC_DISPLAY_PRIMARY, HWC2::DisplayType::Physical));
 
-  ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
-                      (const hw_module_t **)&gralloc_);
-  if (ret) {
-    ALOGE("Failed to open gralloc module %d", ret);
-    return HWC2::Error::NoResources;
-  }
-
-  displays_.emplace(std::piecewise_construct,
-                    std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
-                    std::forward_as_tuple(&drm_, importer_, gralloc_,
-                                          HWC_DISPLAY_PRIMARY,
-                                          HWC2::DisplayType::Physical));
-
-  DrmCrtc *crtc = drm_.GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
+  DrmCrtc *crtc = drm->GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
   if (!crtc) {
     ALOGE("Failed to get crtc for display %d",
           static_cast<int>(HWC_DISPLAY_PRIMARY));
     return HWC2::Error::BadDisplay;
   }
-
   std::vector<DrmPlane *> display_planes;
-  for (auto &plane : drm_.planes()) {
+  for (auto &plane : drm->planes()) {
     if (plane->GetCrtcSupported(*crtc))
       display_planes.push_back(plane.get());
   }
diff --git a/drmhwctwo.h b/drmhwctwo.h
index 0490e2a..beb5d2d 100644
--- a/drmhwctwo.h
+++ b/drmhwctwo.h
@@ -262,9 +262,7 @@ class DrmHwcTwo : public hwc2_device_t {
   HWC2::Error RegisterCallback(int32_t descriptor, hwc2_callback_data_t data,
                                hwc2_function_pointer_t function);
 
-  DrmResources drm_;
-  std::shared_ptr<Importer> importer_;  // Shared with HwcDisplay
-  const gralloc_module_t *gralloc_;
+  ResourceManager resource_manager_;
   std::map<hwc2_display_t, HwcDisplay> displays_;
   std::map<HWC2::Callback, HwcCallback> callbacks_;
 };
diff --git a/drmresources.cpp b/drmresources.cpp
index 32dd376..a5ddda0 100644
--- a/drmresources.cpp
+++ b/drmresources.cpp
@@ -42,10 +42,9 @@ DrmResources::~DrmResources() {
   event_listener_.Exit();
 }
 
-int DrmResources::Init() {
-  char path[PROPERTY_VALUE_MAX];
-  property_get("hwc.drm.device", path, "/dev/dri/card0");
-
+int DrmResources::Init(ResourceManager *resource_manager, char *path,
+                       int start_display_index) {
+  resource_manager_ = resource_manager;
   /* TODO: Use drmOpenControl here instead */
   fd_.Set(open(path, O_RDWR));
   if (fd() < 0) {
@@ -76,8 +75,8 @@ int DrmResources::Init() {
   max_resolution_ =
       std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
 
-  bool found_primary = false;
-  int display_num = 1;
+  bool found_primary = start_display_index != 0;
+  int display_num = found_primary ? start_display_index : 1;
 
   for (int i = 0; !ret && i < res->count_crtcs; ++i) {
     drmModeCrtcPtr c = drmModeGetCrtc(fd(), res->crtcs[i]);
@@ -161,9 +160,11 @@ int DrmResources::Init() {
   for (auto &conn : connectors_) {
     if (conn->internal() && !found_primary) {
       conn->set_display(0);
+      displays_[0] = 0;
       found_primary = true;
     } else {
       conn->set_display(display_num);
+      displays_[display_num] = display_num;
       ++display_num;
     }
   }
@@ -171,7 +172,9 @@ int DrmResources::Init() {
   // Then look for primary amongst external connectors
   for (auto &conn : connectors_) {
     if (conn->external() && !found_primary) {
+      displays_.erase(conn->display());
       conn->set_display(0);
+      displays_[0] = 0;
       found_primary = true;
     }
   }
@@ -226,7 +229,11 @@ int DrmResources::Init() {
       return ret;
     }
   }
-  return 0;
+  return displays_.size() ? displays_.rbegin()->first : -EINVAL;
+}
+
+bool DrmResources::HandlesDisplay(int display) const {
+  return displays_.find(display) != displays_.end();
 }
 
 DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
@@ -349,6 +356,10 @@ DrmEventListener *DrmResources::event_listener() {
   return &event_listener_;
 }
 
+ResourceManager *DrmResources::resource_manager() {
+  return resource_manager_;
+}
+
 int DrmResources::GetProperty(uint32_t obj_id, uint32_t obj_type,
                               const char *prop_name, DrmProperty *property) {
   drmModeObjectPropertiesPtr props;
diff --git a/drmresources.h b/drmresources.h
index 4cca48c..4cdcd87 100644
--- a/drmresources.h
+++ b/drmresources.h
@@ -17,22 +17,26 @@
 #ifndef ANDROID_DRM_H_
 #define ANDROID_DRM_H_
 
+#include <stdint.h>
 #include "drmconnector.h"
 #include "drmcrtc.h"
 #include "drmencoder.h"
 #include "drmeventlistener.h"
 #include "drmplane.h"
-
-#include <stdint.h>
+#include "platform.h"
+#include "resourcemanager.h"
 
 namespace android {
 
+class ResourceManager;
+
 class DrmResources {
  public:
   DrmResources();
   ~DrmResources();
 
-  int Init();
+  int Init(ResourceManager *resource_manager, char *path,
+           int start_display_index);
 
   int fd() const {
     return fd_.get();
@@ -58,6 +62,7 @@ class DrmResources {
   DrmCrtc *GetCrtcForDisplay(int display) const;
   DrmPlane *GetPlane(uint32_t id) const;
   DrmEventListener *event_listener();
+  ResourceManager *resource_manager();
 
   int GetPlaneProperty(const DrmPlane &plane, const char *prop_name,
                        DrmProperty *property);
@@ -71,6 +76,7 @@ class DrmResources {
 
   int CreatePropertyBlob(void *data, size_t length, uint32_t *blob_id);
   int DestroyPropertyBlob(uint32_t blob_id);
+  bool HandlesDisplay(int display) const;
 
  private:
   int TryEncoderForDisplay(int display, DrmEncoder *enc);
@@ -90,6 +96,8 @@ class DrmResources {
 
   std::pair<uint32_t, uint32_t> min_resolution_;
   std::pair<uint32_t, uint32_t> max_resolution_;
+  std::map<int, int> displays_;
+  ResourceManager *resource_manager_;
 };
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 06/18] drm_hwcomposer: Add writeback connector support

[Alexandru Gheorghe]

Writeback connector is a special case of connector, which can be
linked to a CRTC in order to get the result of the composition back to
a memory buffer. This had not been merged to the mainline kernel yet,
latest version of the kernel patches could be found here [1].

[1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmconnector.cpp | 42 +++++++++++++++++++++++++++++++++++++++++-
 drmconnector.h   |  7 +++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drmconnector.cpp b/drmconnector.cpp
index 145518f..e482832 100644
--- a/drmconnector.cpp
+++ b/drmconnector.cpp
@@ -52,6 +52,26 @@ int DrmConnector::Init() {
     ALOGE("Could not get CRTC_ID property\n");
     return ret;
   }
+  if (writeback()) {
+    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS",
+                                     &writeback_pixel_formats_);
+    if (ret) {
+      ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
+      return ret;
+    }
+    ret =
+        drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", &writeback_fb_id_);
+    if (ret) {
+      ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
+      return ret;
+    }
+    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR",
+                                     &writeback_out_fence_);
+    if (ret) {
+      ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
+      return ret;
+    }
+  }
   return 0;
 }
 
@@ -78,8 +98,16 @@ bool DrmConnector::external() const {
          type_ == DRM_MODE_CONNECTOR_VGA;
 }
 
+bool DrmConnector::writeback() const {
+#ifdef DRM_MODE_CONNECTOR_WRITEBACK
+  return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
+#else
+  return false;
+#endif
+}
+
 bool DrmConnector::valid_type() const {
-  return internal() || external();
+  return internal() || external() || writeback();
 }
 
 int DrmConnector::UpdateModes() {
@@ -130,6 +158,18 @@ const DrmProperty &DrmConnector::crtc_id_property() const {
   return crtc_id_property_;
 }
 
+const DrmProperty &DrmConnector::writeback_pixel_formats() const {
+  return writeback_pixel_formats_;
+}
+
+const DrmProperty &DrmConnector::writeback_fb_id() const {
+  return writeback_fb_id_;
+}
+
+const DrmProperty &DrmConnector::writeback_out_fence() const {
+  return writeback_out_fence_;
+}
+
 DrmEncoder *DrmConnector::encoder() const {
   return encoder_;
 }
diff --git a/drmconnector.h b/drmconnector.h
index 5601e06..e139730 100644
--- a/drmconnector.h
+++ b/drmconnector.h
@@ -46,6 +46,7 @@ class DrmConnector {
 
   bool internal() const;
   bool external() const;
+  bool writeback() const;
   bool valid_type() const;
 
   int UpdateModes();
@@ -58,6 +59,9 @@ class DrmConnector {
 
   const DrmProperty &dpms_property() const;
   const DrmProperty &crtc_id_property() const;
+  const DrmProperty &writeback_pixel_formats() const;
+  const DrmProperty &writeback_fb_id() const;
+  const DrmProperty &writeback_out_fence() const;
 
   const std::vector<DrmEncoder *> &possible_encoders() const {
     return possible_encoders_;
@@ -88,6 +92,9 @@ class DrmConnector {
 
   DrmProperty dpms_property_;
   DrmProperty crtc_id_property_;
+  DrmProperty writeback_pixel_formats_;
+  DrmProperty writeback_fb_id_;
+  DrmProperty writeback_out_fence_;
 
   std::vector<DrmEncoder *> possible_encoders_;
 };
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 07/18] drm_hwcomposer: Add display field to Drmencoder

[Alexandru Gheorghe]

In the current implementation TryEncoderForDisplay just looks
at the crtc linked to the display, if that's not assigned to
a display it means the encoder could be used, otherwise iterate
to the list of possible_crtcs and find one which is not used.

This logic works fine when you have just one encoder connected to a
crtc but with two or more, like is the case when we attach a writeback
connector, we need to know if we already assigned the encoder to a
display.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmencoder.cpp | 14 ++++++++++++++
 drmencoder.h   |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/drmencoder.cpp b/drmencoder.cpp
index 3d762f3..1da7ec3 100644
--- a/drmencoder.cpp
+++ b/drmencoder.cpp
@@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
                        const std::vector<DrmCrtc *> &possible_crtcs)
     : id_(e->encoder_id),
       crtc_(current_crtc),
+      display_(-1),
       possible_crtcs_(possible_crtcs) {
 }
 
@@ -40,5 +41,18 @@ DrmCrtc *DrmEncoder::crtc() const {
 
 void DrmEncoder::set_crtc(DrmCrtc *crtc) {
   crtc_ = crtc;
+  set_display(crtc->display());
+}
+
+int DrmEncoder::display() const {
+  return display_;
+}
+
+void DrmEncoder::set_display(int display) {
+  display_ = display;
+}
+
+bool DrmEncoder::can_bind(int display) const {
+  return display_ == -1 || display_ == display;
 }
 }
diff --git a/drmencoder.h b/drmencoder.h
index 58ccbfb..7e06691 100644
--- a/drmencoder.h
+++ b/drmencoder.h
@@ -36,6 +36,9 @@ class DrmEncoder {
 
   DrmCrtc *crtc() const;
   void set_crtc(DrmCrtc *crtc);
+  bool can_bind(int display) const;
+  void set_display(int display);
+  int display() const;
 
   const std::vector<DrmCrtc *> &possible_crtcs() const {
     return possible_crtcs_;
@@ -44,6 +47,7 @@ class DrmEncoder {
  private:
   uint32_t id_;
   DrmCrtc *crtc_;
+  int display_;
 
   std::vector<DrmCrtc *> possible_crtcs_;
 };
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 08/18] drm_hwcomposer: Parse and store possible_clones information

[Alexandru Gheorghe]

drmModeEncoder has a field called possible_clones. It's a bit mask
which tells if the encoder could be simultaneously connected, to the
same CRTC, with the encoders specified in the possible_clones mask.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmencoder.cpp   | 8 ++++++++
 drmencoder.h     | 4 ++++
 drmresources.cpp | 9 ++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drmencoder.cpp b/drmencoder.cpp
index 1da7ec3..ff675f5 100644
--- a/drmencoder.cpp
+++ b/drmencoder.cpp
@@ -39,6 +39,14 @@ DrmCrtc *DrmEncoder::crtc() const {
   return crtc_;
 }
 
+bool DrmEncoder::can_clone(DrmEncoder *encoder) {
+  return possible_clones_.find(encoder) != possible_clones_.end();
+}
+
+void DrmEncoder::add_possible_clone(DrmEncoder *possible_clone) {
+  possible_clones_[possible_clone] = true;
+}
+
 void DrmEncoder::set_crtc(DrmCrtc *crtc) {
   crtc_ = crtc;
   set_display(crtc->display());
diff --git a/drmencoder.h b/drmencoder.h
index 7e06691..5e7c010 100644
--- a/drmencoder.h
+++ b/drmencoder.h
@@ -21,6 +21,7 @@
 
 #include <stdint.h>
 #include <vector>
+#include <map>
 #include <xf86drmMode.h>
 
 namespace android {
@@ -43,6 +44,8 @@ class DrmEncoder {
   const std::vector<DrmCrtc *> &possible_crtcs() const {
     return possible_crtcs_;
   }
+  bool can_clone(DrmEncoder *encoder);
+  void add_possible_clone(DrmEncoder *possible_clone);
 
  private:
   uint32_t id_;
@@ -50,6 +53,7 @@ class DrmEncoder {
   int display_;
 
   std::vector<DrmCrtc *> possible_crtcs_;
+  std::map<DrmEncoder *, bool> possible_clones_;
 };
 }
 
diff --git a/drmresources.cpp b/drmresources.cpp
index a5ddda0..39f50be 100644
--- a/drmresources.cpp
+++ b/drmresources.cpp
@@ -97,6 +97,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
     crtcs_.emplace_back(std::move(crtc));
   }
 
+  std::vector<int> possible_clones;
   for (int i = 0; !ret && i < res->count_encoders; ++i) {
     drmModeEncoderPtr e = drmModeGetEncoder(fd(), res->encoders[i]);
     if (!e) {
@@ -117,12 +118,18 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
 
     std::unique_ptr<DrmEncoder> enc(
         new DrmEncoder(e, current_crtc, possible_crtcs));
-
+    possible_clones.push_back(e->possible_clones);
     drmModeFreeEncoder(e);
 
     encoders_.emplace_back(std::move(enc));
   }
 
+  for (uint32_t i = 0; i < encoders_.size(); i++) {
+    for (uint32_t j = 0; j < encoders_.size(); j++)
+      if (possible_clones[i] & (1 << j))
+        encoders_[i]->add_possible_clone(encoders_[j].get());
+  }
+
   for (int i = 0; !ret && i < res->count_connectors; ++i) {
     drmModeConnectorPtr c = drmModeGetConnector(fd(), res->connectors[i]);
     if (!c) {
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 09/18] drm_hwcomposer: Handle writeback connectors

[Alexandru Gheorghe]

When writeback connectors are available assign them to displays, in
order to be able to use them for flattening of the current displayed
scene. The pipeline for each display will look like this:

CRTC ---- encoder ------------ display connector.
 |------- writeback enc ------ writeback connector.

However, the writeback connector will be later used/enabled only if
one of the following conditions are met:
 - Could be a clone of the display connector, as pointed by the
   possible_clones property.
 - The display_connector is disconnected, so we are safe to use it for
   flattening the scene that's already presented on another display.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmresources.cpp | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 drmresources.h   |  3 +++
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drmresources.cpp b/drmresources.cpp
index 39f50be..fef6835 100644
--- a/drmresources.cpp
+++ b/drmresources.cpp
@@ -64,6 +64,14 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
     return ret;
   }
 
+#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
+  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
+  if (ret) {
+    ALOGI("Failed to set writeback cap %d", ret);
+    ret = 0;
+  }
+#endif
+
   drmModeResPtr res = drmModeGetResources(fd());
   if (!res) {
     ALOGE("Failed to get DrmResources resources");
@@ -169,7 +177,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
       conn->set_display(0);
       displays_[0] = 0;
       found_primary = true;
-    } else {
+    } else if (conn->external()) {
       conn->set_display(display_num);
       displays_[display_num] = display_num;
       ++display_num;
@@ -230,6 +238,8 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
   }
 
   for (auto &conn : connectors_) {
+    if (conn->writeback())
+      continue;
     ret = CreateDisplayPipe(conn.get());
     if (ret) {
       ALOGE("Failed CreateDisplayPipe %d with %d", conn->id(), ret);
@@ -245,7 +255,15 @@ bool DrmResources::HandlesDisplay(int display) const {
 
 DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
   for (auto &conn : connectors_) {
-    if (conn->display() == display)
+    if (conn->display() == display && !conn->writeback())
+      return conn.get();
+  }
+  return NULL;
+}
+
+DrmConnector *DrmResources::GetWritebackConnectorForDisplay(int display) const {
+  for (auto &conn : connectors_) {
+    if (conn->display() == display && conn->writeback())
       return conn.get();
   }
   return NULL;
@@ -280,6 +298,7 @@ int DrmResources::TryEncoderForDisplay(int display, DrmEncoder *enc) {
   DrmCrtc *crtc = enc->crtc();
   if (crtc && crtc->can_bind(display)) {
     crtc->set_display(display);
+    enc->set_display(display);
     return 0;
   }
 
@@ -306,6 +325,7 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
   if (connector->encoder()) {
     int ret = TryEncoderForDisplay(display, connector->encoder());
     if (!ret) {
+      AttachWriteback(connector);
       return 0;
     } else if (ret != -EAGAIN) {
       ALOGE("Could not set mode %d/%d", display, ret);
@@ -317,6 +337,7 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
     int ret = TryEncoderForDisplay(display, enc);
     if (!ret) {
       connector->set_encoder(enc);
+      AttachWriteback(connector);
       return 0;
     } else if (ret != -EAGAIN) {
       ALOGE("Could not set mode %d/%d", display, ret);
@@ -328,6 +349,43 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
   return -ENODEV;
 }
 
+/*
+ * Attach writeback connector to the CRTC linked to the display_conn
+ *
+ */
+int DrmResources::AttachWriteback(DrmConnector *display_conn) {
+  int ret = -EINVAL;
+  if (display_conn->writeback())
+    return -EINVAL;
+  DrmEncoder *display_enc = display_conn->encoder();
+  if (!display_enc)
+    return -EINVAL;
+  DrmCrtc *display_crtc = display_enc->crtc();
+  if (!display_crtc)
+    return -EINVAL;
+  if (GetWritebackConnectorForDisplay(display_crtc->display()) != NULL)
+    return -EINVAL;
+  for (auto &writeback_conn : connectors_) {
+    if (writeback_conn->display() >= 0 || !writeback_conn->writeback())
+      continue;
+    for (DrmEncoder *writeback_enc : writeback_conn->possible_encoders()) {
+      for (DrmCrtc *possible_crtc : writeback_enc->possible_crtcs()) {
+        if (possible_crtc != display_crtc)
+          continue;
+        // Use just encoders which had not been bound already
+        if (writeback_enc->can_bind(display_crtc->display())) {
+          writeback_enc->set_crtc(display_crtc);
+          writeback_conn->set_encoder(writeback_enc);
+          writeback_conn->set_display(display_crtc->display());
+          writeback_conn->UpdateModes();
+          return 0;
+        }
+      }
+    }
+  }
+  return ret;
+}
+
 int DrmResources::CreatePropertyBlob(void *data, size_t length,
                                      uint32_t *blob_id) {
   struct drm_mode_create_blob create_blob;
diff --git a/drmresources.h b/drmresources.h
index 4cdcd87..4fb17fc 100644
--- a/drmresources.h
+++ b/drmresources.h
@@ -59,6 +59,8 @@ class DrmResources {
   }
 
   DrmConnector *GetConnectorForDisplay(int display) const;
+  DrmConnector *GetWritebackConnectorForDisplay(int display) const;
+  DrmConnector *FindWritebackConnector(int display) const;
   DrmCrtc *GetCrtcForDisplay(int display) const;
   DrmPlane *GetPlane(uint32_t id) const;
   DrmEventListener *event_listener();
@@ -84,6 +86,7 @@ class DrmResources {
                   DrmProperty *property);
 
   int CreateDisplayPipe(DrmConnector *connector);
+  int AttachWriteback(DrmConnector *display_conn);
 
   UniqueFd fd_;
   uint32_t mode_id_ = 0;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 10/18] drm_hwcomposer: hwcutils: Add function for cloning a DrmHwcLayer

[Alexandru Gheorghe]

When doing flattening of a composition on a different CRTC we need to be
able to clone a layer in order to import it and then pass it to another CRTC.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmhwcomposer.h |  1 +
 hwcutils.cpp    | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drmhwcomposer.h b/drmhwcomposer.h
index f8440fb..b256caf 100644
--- a/drmhwcomposer.h
+++ b/drmhwcomposer.h
@@ -150,6 +150,7 @@ struct DrmHwcLayer {
 
   int InitFromHwcLayer(hwc_layer_1_t *sf_layer, Importer *importer,
                        const gralloc_module_t *gralloc);
+  int PopulateFromDrmHwcLayer(DrmHwcLayer *layer);
   int ImportBuffer(Importer *importer, const gralloc_module_t *gralloc);
 
   void SetTransform(int32_t sf_transform);
diff --git a/hwcutils.cpp b/hwcutils.cpp
index 53a7d82..ff37c3b 100644
--- a/hwcutils.cpp
+++ b/hwcutils.cpp
@@ -149,6 +149,17 @@ int DrmHwcLayer::InitFromHwcLayer(hwc_layer_1_t *sf_layer, Importer *importer,
   return ImportBuffer(importer, gralloc);
 }
 
+int DrmHwcLayer::PopulateFromDrmHwcLayer(DrmHwcLayer *src_layer) {
+  blending = src_layer->blending;
+  sf_handle = src_layer->sf_handle;
+  acquire_fence = dup(src_layer->acquire_fence.get());
+  display_frame = src_layer->display_frame;
+  alpha = src_layer->alpha;
+  source_crop = src_layer->source_crop;
+  transform = src_layer->transform;
+  return 0;
+}
+
 int DrmHwcLayer::ImportBuffer(Importer *importer,
                               const gralloc_module_t *gralloc) {
   int ret = buffer.ImportBuffer(sf_handle, importer);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 11/18] drm_hwcomposer: Add utility functions to copy displaycomposition internals

[Alexandru Gheorghe]

Add utility functions to copy the DrmHwcLayer and DrmCompositionPlanes
from another DrmDisplayComposition.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycomposition.cpp | 29 +++++++++++++++++++++++++++++
 drmdisplaycomposition.h   |  3 +++
 2 files changed, 32 insertions(+)

diff --git a/drmdisplaycomposition.cpp b/drmdisplaycomposition.cpp
index 66e67a4..dd64f46 100644
--- a/drmdisplaycomposition.cpp
+++ b/drmdisplaycomposition.cpp
@@ -99,6 +99,35 @@ int DrmDisplayComposition::SetLayers(DrmHwcLayer *layers, size_t num_layers,
   return 0;
 }
 
+int DrmDisplayComposition::CopyLayers(DrmDisplayComposition *src) {
+  geometry_changed_ = true;
+  type_ = DRM_COMPOSITION_TYPE_FRAME;
+  std::shared_ptr<Importer> importer =
+      drm_->resource_manager()->GetImporter(crtc()->display());
+  if (!importer) {
+    ALOGE("Failed to find a valid importer");
+    return -EINVAL;
+  }
+  for (DrmHwcLayer &src_layer : src->layers()) {
+    DrmHwcLayer copy;
+    copy.PopulateFromDrmHwcLayer(&src_layer);
+    int ret = copy.ImportBuffer(importer.get(),
+                                drm_->resource_manager()->GetGralloc());
+    if (ret) {
+      ALOGE("Failed to import buffer ret = %d", ret);
+      return -EINVAL;
+    }
+    layers_.emplace_back(std::move(copy));
+  }
+  return 0;
+}
+
+void DrmDisplayComposition::CopyCompPlanes(DrmDisplayComposition *src) {
+  for (auto comp_plane : src->composition_planes()) {
+    composition_planes_.push_back(comp_plane);
+  }
+}
+
 int DrmDisplayComposition::SetDpmsMode(uint32_t dpms_mode) {
   if (!validate_composition_type(DRM_COMPOSITION_TYPE_DPMS))
     return -EINVAL;
diff --git a/drmdisplaycomposition.h b/drmdisplaycomposition.h
index 9183925..c646420 100644
--- a/drmdisplaycomposition.h
+++ b/drmdisplaycomposition.h
@@ -68,6 +68,7 @@ class DrmCompositionPlane {
 
   DrmCompositionPlane() = default;
   DrmCompositionPlane(DrmCompositionPlane &&rhs) = default;
+  DrmCompositionPlane(const DrmCompositionPlane &rhs) = default;
   DrmCompositionPlane &operator=(DrmCompositionPlane &&other) = default;
   DrmCompositionPlane(Type type, DrmPlane *plane, DrmCrtc *crtc)
       : type_(type), plane_(plane), crtc_(crtc) {
@@ -120,6 +121,8 @@ class DrmDisplayComposition {
            Planner *planner, uint64_t frame_no);
 
   int SetLayers(DrmHwcLayer *layers, size_t num_layers, bool geometry_changed);
+  int CopyLayers(DrmDisplayComposition *src);
+  void CopyCompPlanes(DrmDisplayComposition *src);
   int AddPlaneComposition(DrmCompositionPlane plane);
   int AddPlaneDisable(DrmPlane *plane);
   int SetDpmsMode(uint32_t dpms_mode);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 12/18] drm_hwcomposer: Add utility function to create an initialized composition

[Alexandru Gheorghe]

There is a lot of boilerplate for creating an initialized
drmdisplaycomposition. This patch gathers that in a separate method.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 23 +++++++++++++++++++++++
 drmdisplaycompositor.h   |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index e556e86..6e5be24 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -221,6 +221,7 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
     ALOGE("Failed to initialize drm compositor lock %d\n", ret);
     return ret;
   }
+  planner_ = Planner::CreateInstance(drm);
 
   initialized_ = true;
   return 0;
@@ -231,6 +232,28 @@ std::unique_ptr<DrmDisplayComposition> DrmDisplayCompositor::CreateComposition()
   return std::unique_ptr<DrmDisplayComposition>(new DrmDisplayComposition());
 }
 
+std::unique_ptr<DrmDisplayComposition>
+DrmDisplayCompositor::CreateInitializedComposition() const {
+  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
+  if (!crtc) {
+    ALOGE("Failed to find crtc for display = %d", display_);
+    return std::unique_ptr<DrmDisplayComposition>();
+  }
+  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
+  std::shared_ptr<Importer> importer =
+      drm_->resource_manager()->GetImporter(display_);
+  if (!importer) {
+    ALOGE("Failed to find resources for display = %d", display_);
+    return std::unique_ptr<DrmDisplayComposition>();
+  }
+  int ret = comp->Init(drm_, crtc, importer.get(), planner_.get(), 0);
+  if (ret) {
+    ALOGE("Failed to init composition for display = %d", display_);
+    return std::unique_ptr<DrmDisplayComposition>();
+  }
+  return comp;
+}
+
 std::tuple<uint32_t, uint32_t, int>
 DrmDisplayCompositor::GetActiveModeResolution() {
   DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index f1965fb..ccaffb4 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -87,6 +87,7 @@ class DrmDisplayCompositor {
   int Init(DrmResources *drm, int display);
 
   std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
+  std::unique_ptr<DrmDisplayComposition> CreateInitializedComposition() const;
   int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
   int Composite();
   int SquashAll();
@@ -155,6 +156,7 @@ class DrmDisplayCompositor {
   // we need to reset them on every Dump() call.
   mutable uint64_t dump_frames_composited_;
   mutable uint64_t dump_last_timestamp_ns_;
+  std::unique_ptr<Planner> planner_;
 };
 }
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 13/18] drm_hwcomposer: Pass buffer sizes to Prepareframebuffer

[Alexandru Gheorghe]

Currently Prepareframebuffer uses the mode of the connected connector
to decide how big the buffer should be, however when using the
drmdisplaycompositor just for flattening, the mode had not been set
yet, so we need a way to pass the desired buffer sizes.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 7 ++++---
 drmdisplaycompositor.h   | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index 6e5be24..afd3b05 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -268,14 +268,15 @@ DrmDisplayCompositor::GetActiveModeResolution() {
 }
 
 int DrmDisplayCompositor::PrepareFramebuffer(
-    DrmFramebuffer &fb, DrmDisplayComposition *display_comp) {
+    DrmFramebuffer &fb, DrmDisplayComposition *display_comp, uint32_t width,
+    uint32_t height) {
   int ret = fb.WaitReleased(-1);
   if (ret) {
     ALOGE("Failed to wait for framebuffer release %d", ret);
     return ret;
   }
-  uint32_t width, height;
-  std::tie(width, height, ret) = GetActiveModeResolution();
+  if (width == 0 || height == 0)
+    std::tie(width, height, ret) = GetActiveModeResolution();
   if (ret) {
     ALOGE(
         "Failed to allocate framebuffer because the display resolution could "
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index ccaffb4..0f8daad 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -115,7 +115,8 @@ class DrmDisplayCompositor {
   static const int kAcquireWaitTimeoutMs = 100;
 
   int PrepareFramebuffer(DrmFramebuffer &fb,
-                         DrmDisplayComposition *display_comp);
+                         DrmDisplayComposition *display_comp,
+                         uint32_t width = 0, uint32_t height = 0);
   int ApplySquash(DrmDisplayComposition *display_comp);
   int ApplyPreComposite(DrmDisplayComposition *display_comp);
   int PrepareFrame(DrmDisplayComposition *display_comp);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 14/18] drm_hwcomposer: Fix race in ApplyFrame

[Alexandru Gheorghe]

ApplyFrame holds the lock just when it swaps the value of
active_composition_, in a multithread context we could end up in a
situation where something is shown on the screen, but something else
is set in active_composition_. Fix it by holding the lock during
CommitFrame.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 40 +++++++++++++++++-----------------------
 drmdisplaycompositor.h   |  2 +-
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index afd3b05..576539b 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -791,11 +791,6 @@ std::tuple<int, uint32_t> DrmDisplayCompositor::CreateModeBlob(
 }
 
 void DrmDisplayCompositor::ClearDisplay() {
-  AutoLock lock(&lock_, "compositor");
-  int ret = lock.Lock();
-  if (ret)
-    return;
-
   if (!active_composition_)
     return;
 
@@ -808,11 +803,25 @@ void DrmDisplayCompositor::ClearDisplay() {
 }
 
 void DrmDisplayCompositor::ApplyFrame(
-    std::unique_ptr<DrmDisplayComposition> composition, int status) {
+    std::unique_ptr<DrmDisplayComposition> composition, int status,
+    bool writeback) {
+  AutoLock lock(&lock_, __FUNCTION__);
+  if (lock.Lock())
+    return;
   int ret = status;
-
-  if (!ret)
+  if (!ret) {
+    if (writeback && !CountdownExpired()) {
+      ALOGE("Abort playing back scene");
+      return;
+    }
     ret = CommitFrame(composition.get(), false);
+    if (!ret) {
+      ++dump_frames_composited_;
+      if (active_composition_)
+        active_composition_->SignalCompositionDone();
+      active_composition_.swap(composition);
+    }
+  }
 
   if (ret) {
     ALOGE("Composite failed for display %d", display_);
@@ -821,21 +830,6 @@ void DrmDisplayCompositor::ApplyFrame(
     ClearDisplay();
     return;
   }
-  ++dump_frames_composited_;
-
-  if (active_composition_)
-    active_composition_->SignalCompositionDone();
-
-  ret = pthread_mutex_lock(&lock_);
-  if (ret)
-    ALOGE("Failed to acquire lock for active_composition swap");
-
-  active_composition_.swap(composition);
-
-  if (!ret)
-    ret = pthread_mutex_unlock(&lock_);
-  if (ret)
-    ALOGE("Failed to release lock for active_composition swap");
 }
 
 int DrmDisplayCompositor::ApplyComposition(
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index 0f8daad..b35ef70 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -127,7 +127,7 @@ class DrmDisplayCompositor {
 
   void ClearDisplay();
   void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
-                  int status);
+                  int status, bool writeback = false);
 
   std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 15/18] drm_hwcomposer: Add worker to trigger scene flattenning

[Alexandru Gheorghe]

Add a vsync worker that calls back into the DrmDisplayCompositor,
for now at every 60 vsyncs if the scene does not change we trigger
the flattening of the scene using the writeback connector.
Other, more complex and proper heuristics could be implemented later
on.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 45 ++++++++++++++++++++++++++++++++++++++++++---
 drmdisplaycompositor.h   | 12 +++++++++++-
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index 576539b..e535e8a 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -39,6 +39,20 @@
 
 namespace android {
 
+class CompositorVsyncCallback : public VsyncCallback {
+ public:
+  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
+      : compositor_(compositor) {
+  }
+
+  void Callback(int display, int64_t timestamp) {
+    compositor_->Vsync(display, timestamp);
+  }
+
+ private:
+  DrmDisplayCompositor *compositor_;
+};
+
 void SquashState::Init(DrmHwcLayer *layers, size_t num_layers) {
   generation_number_++;
   valid_history_ = 0;
@@ -183,7 +197,8 @@ DrmDisplayCompositor::DrmDisplayCompositor()
       framebuffer_index_(0),
       squash_framebuffer_index_(0),
       dump_frames_composited_(0),
-      dump_last_timestamp_ns_(0) {
+      dump_last_timestamp_ns_(0),
+      flatten_countdown_(FLATTEN_COUNTDOWN_INIT) {
   struct timespec ts;
   if (clock_gettime(CLOCK_MONOTONIC, &ts))
     return;
@@ -193,7 +208,7 @@ DrmDisplayCompositor::DrmDisplayCompositor()
 DrmDisplayCompositor::~DrmDisplayCompositor() {
   if (!initialized_)
     return;
-
+  vsync_worker_.Exit();
   int ret = pthread_mutex_lock(&lock_);
   if (ret)
     ALOGE("Failed to acquire compositor lock %d", ret);
@@ -222,7 +237,9 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
     return ret;
   }
   planner_ = Planner::CreateInstance(drm);
-
+  vsync_worker_.Init(drm_, display_);
+  auto callback = std::make_shared<CompositorVsyncCallback>(this);
+  vsync_worker_.RegisterCallback(callback);
   initialized_ = true;
   return 0;
 }
@@ -896,6 +913,10 @@ int DrmDisplayCompositor::ApplyComposition(
   return ret;
 }
 
+int DrmDisplayCompositor::FlattenScene() {
+  return -EINVAL;
+}
+
 int DrmDisplayCompositor::SquashAll() {
   AutoLock lock(&lock_, "compositor");
   int ret = lock.Lock();
@@ -1044,6 +1065,24 @@ move_layers_back:
   return ret;
 }
 
+bool DrmDisplayCompositor::CountdownExpired() const {
+  return flatten_countdown_ <= 0;
+}
+
+void DrmDisplayCompositor::Vsync(int display, int64_t timestamp) {
+  AutoLock lock(&lock_, __FUNCTION__);
+  if (lock.Lock())
+    return;
+  flatten_countdown_--;
+  if (CountdownExpired()) {
+    lock.Unlock();
+    int ret = FlattenScene();
+    ALOGI("scene flattening triggered for display %d at timestamp %" PRIu64
+          " result = %d \n",
+          display, timestamp, ret);
+  }
+}
+
 void DrmDisplayCompositor::Dump(std::ostringstream *out) const {
   int ret = pthread_mutex_lock(&lock_);
   if (ret)
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index b35ef70..26201b9 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -29,11 +29,16 @@
 
 #include <hardware/hardware.h>
 #include <hardware/hwcomposer.h>
+#include <vsyncworker.h>
 
 // One for the front, one for the back, and one for cases where we need to
 // squash a frame that the hw can't display with hw overlays.
 #define DRM_DISPLAY_BUFFERS 3
 
+// If a scene is still for this number of vblanks flatten it to reduce power
+// consumption.
+#define FLATTEN_COUNTDOWN_INIT 60
+
 namespace android {
 
 class GLWorkerCompositor;
@@ -92,7 +97,7 @@ class DrmDisplayCompositor {
   int Composite();
   int SquashAll();
   void Dump(std::ostringstream *out) const;
-
+  void Vsync(int display, int64_t timestamp);
   std::tuple<uint32_t, uint32_t, int> GetActiveModeResolution();
 
   SquashState *squash_state() {
@@ -128,6 +133,9 @@ class DrmDisplayCompositor {
   void ClearDisplay();
   void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
                   int status, bool writeback = false);
+  int FlattenScene();
+
+  bool CountdownExpired() const;
 
   std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
 
@@ -157,6 +165,8 @@ class DrmDisplayCompositor {
   // we need to reset them on every Dump() call.
   mutable uint64_t dump_frames_composited_;
   mutable uint64_t dump_last_timestamp_ns_;
+  VSyncWorker vsync_worker_;
+  int64_t flatten_countdown_;
   std::unique_ptr<Planner> planner_;
 };
 }
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 16/18] drm_hwcomposer: Find writeback connector for scene flattening

[Alexandru Gheorghe]

Add logic for finding a suitable writeback connector, there are two
possibilities for finding an usable writeback connector:

1) Attached to the same CRTC as the display and can function
   concurrently with the display connector.

2) On a different CRTC and the display connector is not used (state !=
DRM_MODE_CONNECTED). What's not handled here and should be handle is
what happens if connector changes state while flattening, but since
hotplug is not wired yet, it's not something we should worry about.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmresources.cpp    | 25 +++++++++++++++++++++++++
 drmresources.h      |  2 +-
 resourcemanager.cpp | 24 ++++++++++++++++++++++++
 resourcemanager.h   |  1 +
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drmresources.cpp b/drmresources.cpp
index fef6835..70126a4 100644
--- a/drmresources.cpp
+++ b/drmresources.cpp
@@ -269,6 +269,31 @@ DrmConnector *DrmResources::GetWritebackConnectorForDisplay(int display) const {
   return NULL;
 }
 
+// TODO what happens when hotplugging
+DrmConnector *DrmResources::AvailableWritebackConnector(int display) const {
+  DrmConnector *writeback_conn = GetWritebackConnectorForDisplay(display);
+  DrmConnector *display_conn = GetConnectorForDisplay(display);
+  // If we have a writeback already attached to the same CRTC, just use that, if
+  // possible
+  if (display_conn && writeback_conn &&
+      writeback_conn->encoder()->can_clone(display_conn->encoder()))
+    return writeback_conn;
+
+  // Use another CRTC if available and doesn't have any connector
+  for (auto &crtc : crtcs_) {
+    if (crtc->display() == display)
+      continue;
+    display_conn = GetConnectorForDisplay(crtc->display());
+    // If we have a display connected don't use it for writeback
+    if (display_conn && display_conn->state() == DRM_MODE_CONNECTED)
+      continue;
+    writeback_conn = GetWritebackConnectorForDisplay(crtc->display());
+    if (writeback_conn)
+      return writeback_conn;
+  }
+  return NULL;
+}
+
 DrmCrtc *DrmResources::GetCrtcForDisplay(int display) const {
   for (auto &crtc : crtcs_) {
     if (crtc->display() == display)
diff --git a/drmresources.h b/drmresources.h
index 4fb17fc..9176b8e 100644
--- a/drmresources.h
+++ b/drmresources.h
@@ -60,7 +60,7 @@ class DrmResources {
 
   DrmConnector *GetConnectorForDisplay(int display) const;
   DrmConnector *GetWritebackConnectorForDisplay(int display) const;
-  DrmConnector *FindWritebackConnector(int display) const;
+  DrmConnector *AvailableWritebackConnector(int display) const;
   DrmCrtc *GetCrtcForDisplay(int display) const;
   DrmPlane *GetPlane(uint32_t id) const;
   DrmEventListener *event_listener();
diff --git a/resourcemanager.cpp b/resourcemanager.cpp
index e7b654e..b2a4458 100644
--- a/resourcemanager.cpp
+++ b/resourcemanager.cpp
@@ -49,6 +49,30 @@ int ResourceManager::Init() {
                        (const hw_module_t **)&gralloc_);
 }
 
+DrmConnector *ResourceManager::AvailableWritebackConnector(int display) {
+  DrmResources *drm_resource = GetDrmResources(display);
+  DrmConnector *writeback_conn = NULL;
+  if (drm_resource) {
+    writeback_conn = drm_resource->AvailableWritebackConnector(display);
+    if (writeback_conn) {
+      ALOGI("Use writeback connected to display %d\n",
+            writeback_conn->display());
+      return writeback_conn;
+    }
+  }
+  for (auto &drm : drms_) {
+    if (drm.get() == drm_resource)
+      continue;
+    writeback_conn = drm->AvailableWritebackConnector(display);
+    if (writeback_conn) {
+      ALOGI("Use writeback connected to display %d\n",
+            writeback_conn->display());
+      return writeback_conn;
+    }
+  }
+  return writeback_conn;
+}
+
 DrmResources *ResourceManager::GetDrmResources(int display) {
   for (uint32_t i = 0; i < drms_.size(); i++) {
     if (drms_[i]->HandlesDisplay(display))
diff --git a/resourcemanager.h b/resourcemanager.h
index b8caa9a..57f7a2a 100644
--- a/resourcemanager.h
+++ b/resourcemanager.h
@@ -18,6 +18,7 @@ class ResourceManager {
   DrmResources *GetDrmResources(int display);
   std::shared_ptr<Importer> GetImporter(int display);
   const gralloc_module_t *GetGralloc();
+  DrmConnector *AvailableWritebackConnector(int display);
 
  private:
   std::vector<std::unique_ptr<DrmResources>> drms_;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 17/18] drm_hwcomposer: Flatten scene synchronously

[Alexandru Gheorghe]

Flatten scene on the same CRTC as the one driving the display.
The active composition is played back to the display with a buffer
attached to the writeback connector.
Then we build a composition that has only one plane enabled and that
uses the result of the writeback as the input.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++--
 drmdisplaycompositor.h   |   7 +-
 2 files changed, 204 insertions(+), 6 deletions(-)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index e535e8a..cb670e6 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -36,6 +36,7 @@
 #include "drmplane.h"
 #include "drmresources.h"
 #include "glworker.h"
+static const uint32_t kWaitWritebackFence = 100;  // ms
 
 namespace android {
 
@@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
 }
 
 int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
-                                      bool test_only) {
+                                      bool test_only,
+                                      DrmDisplayComposition *writeback_comp,
+                                      DrmConnector *writeback_conn) {
   ATRACE_CALL();
 
   int ret = 0;
@@ -532,6 +535,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
   std::vector<DrmCompositionPlane> &comp_planes =
       display_comp->composition_planes();
   uint64_t out_fences[drm_->crtcs().size()];
+  int writeback_fence = -1;
 
   DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
   if (!connector) {
@@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     return -ENOMEM;
   }
 
+  if (writeback_comp != NULL) {
+    if (writeback_conn == NULL)
+      return -EINVAL;
+    if (writeback_conn->writeback_fb_id().id() == 0 ||
+        writeback_conn->writeback_out_fence().id() == 0) {
+      ALOGE("Writeback properties don't exit");
+      return -EINVAL;
+    }
+    if (writeback_comp->layers().size() != 1) {
+      ALOGE("Invalid number of layers for writeback composition");
+      return -EINVAL;
+    }
+    ret = drmModeAtomicAddProperty(
+        pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
+        writeback_comp->layers().back().buffer->fb_id);
+    if (ret < 0) {
+      ALOGE("Failed to add writeback_fb_id");
+      return ret;
+    }
+    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
+                                   writeback_conn->writeback_out_fence().id(),
+                                   (uint64_t)&writeback_fence);
+    if (ret < 0) {
+      ALOGE("Failed to add writeback_out_fence");
+      return ret;
+    }
+  }
   if (crtc->out_fence_ptr_property().id() != 0) {
-    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
-                                   (uint64_t) &out_fences[crtc->pipe()]);
+    ret = drmModeAtomicAddProperty(pset, crtc->id(),
+                                   crtc->out_fence_ptr_property().id(),
+                                   (uint64_t)&out_fences[crtc->pipe()]);
     if (ret < 0) {
       ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
       drmModeAtomicFree(pset);
@@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     }
   }
 
+  if (writeback_conn != NULL) {
+    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
+                                   writeback_conn->crtc_id_property().id(),
+                                   crtc->id());
+    if (ret < 0) {
+      ALOGE("Failed to  attach writeback");
+    }
+  }
+
   for (DrmCompositionPlane &comp_plane : comp_planes) {
     DrmPlane *plane = comp_plane.plane();
     DrmCrtc *crtc = comp_plane.crtc();
@@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
 
   if (!ret) {
     uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET;
-    if (test_only)
+    if (test_only) {
       flags |= DRM_MODE_ATOMIC_TEST_ONLY;
+    } else {
+      if (writeback_comp != NULL) {
+        if (!CountdownExpired() && active_composition_) {
+          ALOGE("Writeback composition not needed, abort commit");
+          drmModeAtomicFree(pset);
+          return -EINVAL;
+        };
+        flags |= DRM_MODE_ATOMIC_NONBLOCK;
+      }
+    }
 
     ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
     if (ret) {
@@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
   if (crtc->out_fence_ptr_property().id()) {
     display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
   }
+  if (writeback_fence >= 0) {
+    if (writeback_comp->layers().size() != 1) {
+      ALOGE("Invalid numbers of layer for writeback_comp");
+      return -EINVAL;
+    }
+    writeback_comp->layers()[0].acquire_fence.Set(writeback_fence);
+  }
 
   return ret;
 }
@@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame(
       if (active_composition_)
         active_composition_->SignalCompositionDone();
       active_composition_.swap(composition);
+      flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
+      vsync_worker_.VSyncControl(!writeback);
     }
   }
 
@@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition(
   return ret;
 }
 
+int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src,
+                                             DrmDisplayComposition *dst,
+                                             DrmConnector *writeback_conn) {
+  int ret = 0;
+  if (src == NULL || dst == NULL)
+    return -EINVAL;
+  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
+  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
+                                    src->crtc());
+  for (DrmCompositionPlane &comp_plane : src_planes) {
+    if (comp_plane.plane() == NULL) {
+      ALOGE("Skipping squash all because of NULL plane");
+      ret = -EINVAL;
+    }
+    if (!squashed_comp.plane() &&
+        comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
+      squashed_comp.set_plane(comp_plane.plane());
+    else
+      dst->AddPlaneDisable(comp_plane.plane());
+  }
+
+  DrmFramebuffer *writeback_fb = NULL;
+  AutoLock lock(&lock_, __FUNCTION__);
+  if ((ret = lock.Lock()))
+    return ret;
+  writeback_fb = &framebuffers_[framebuffer_index_];
+  framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
+  ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(),
+                           mode_.mode.v_display());
+  if (ret) {
+    ALOGE("Failed to prepare destination buffer");
+    return ret;
+  }
+  lock.Unlock();
+  ret = CommitFrame(src, true, dst, writeback_conn);
+  if (ret) {
+    ALOGE("Atomic check failed");
+    return ret;
+  }
+  if ((ret = lock.Lock()))
+    return ret;
+  if (!CountdownExpired() && active_composition_) {
+    ALOGE("Writeback composition not needed abort");
+    return -EINVAL;
+  }
+  ret = CommitFrame(src, false, dst, writeback_conn);
+  lock.Unlock();
+  if (ret || dst->layers().size() != 1) {
+    ALOGE("Failed to flatten scene using writeback");
+    return -EINVAL;
+  }
+  squashed_comp.source_layers().push_back(0);
+  ret =
+      sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence);
+  if (ret) {
+    ALOGE("Failed to wait on writeback fence");
+    return ret;
+  }
+  ret = dst->AddPlaneComposition(std::move(squashed_comp));
+  if (ret) {
+    ALOGE("Failed to add flatten scene");
+    return ret;
+  }
+  ret = dst->FinalizeComposition();
+  if (ret) {
+    ALOGE("Failed to finalize composition");
+    return ret;
+  }
+  return 0;
+}
+
+int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) {
+  if (writeback_conn->display() != display_) {
+    ALOGE("Cannot flatten synchronously on different display");
+    return -EINVAL;
+  }
+  ALOGI("FlattenSynchronously using the same display");
+  int ret = 0;
+  /* Flattened composition with only one layer that is built
+   * using the writeback connector
+   */
+  std::unique_ptr<DrmDisplayComposition> writeback_comp =
+      CreateInitializedComposition();
+  /* Copy of the active_composition_, we need a copy because
+   * if we use the active composition we have to hold the lock
+   * for the entire sequence of flattening.
+   */
+  std::unique_ptr<DrmDisplayComposition> copy_comp =
+      CreateInitializedComposition();
+
+  if (!copy_comp || !writeback_comp)
+    return -EINVAL;
+  AutoLock lock(&lock_, __FUNCTION__);
+  if ((ret = lock.Lock()))
+    return ret;
+  if (CountdownExpired()) {
+    ret = copy_comp->CopyLayers(active_composition_.get());
+    if (ret)
+      return ret;
+    copy_comp->CopyCompPlanes(active_composition_.get());
+  } else {
+    return -EINVAL;
+  }
+  lock.Unlock();
+  ret =
+      WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn);
+  if (ret) {
+    ALOGE("Failed to prepare writebackScene");
+    return ret;
+  }
+
+  ApplyFrame(std::move(writeback_comp), 0, true);
+  return 0;
+}
+
 int DrmDisplayCompositor::FlattenScene() {
-  return -EINVAL;
+  DrmConnector *writeback_conn =
+      drm_->resource_manager()->AvailableWritebackConnector(display_);
+  if (!active_composition_ || !writeback_conn)
+    return -EINVAL;
+  std::vector<DrmCompositionPlane> &src_planes =
+      active_composition_->composition_planes();
+  size_t src_planes_with_layer = std::count_if(
+      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
+        return p.type() != DrmCompositionPlane::Type::kDisable;
+      });
+
+  if (src_planes_with_layer <= 1)
+    return -EALREADY;
+
+  if (writeback_conn->display() == display_) {
+    return FlattenSynchronously(writeback_conn);
+  }
+
+  return 0;
 }
 
 int DrmDisplayCompositor::SquashAll() {
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index 26201b9..4cc4a5e 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -125,7 +125,9 @@ class DrmDisplayCompositor {
   int ApplySquash(DrmDisplayComposition *display_comp);
   int ApplyPreComposite(DrmDisplayComposition *display_comp);
   int PrepareFrame(DrmDisplayComposition *display_comp);
-  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
+  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only,
+                  DrmDisplayComposition *writeback_comp = NULL,
+                  DrmConnector *writeback_conn = NULL);
   int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
   int ApplyDpms(DrmDisplayComposition *display_comp);
   int DisablePlanes(DrmDisplayComposition *display_comp);
@@ -134,7 +136,10 @@ class DrmDisplayCompositor {
   void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
                   int status, bool writeback = false);
   int FlattenScene();
+  int FlattenSynchronously(DrmConnector *writeback_conn);
 
+  int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst,
+                         DrmConnector *writeback_conn);
   bool CountdownExpired() const;
 
   std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH hwc v2 18/18] drm_hwcomposer: Flatten scene asynchronously

[Alexandru Gheorghe]

The steps for flattening a scene on a dedicated crtc are:

1. Find an available and unused crtc(the display connector is
disconnected).
2. Copy layers from active composition.
3. Plan layers of copy on the unused crtc. This is realized by using a
newly created DrmDisplayCompositor object.
4. Commit copy to the unsed crtc and get the result as a
writeback_comp.
5. Copy the writeback comp and commit(if needed) to the display. The
copying of the writeback comp is needed because the crtc might not be
on the same dri node so the buffers will have to be re-imported.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drmdisplaycompositor.cpp | 173 ++++++++++++++++++++++++++++++++++++++++++++++-
 drmdisplaycompositor.h   |   5 ++
 2 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index cb670e6..72d0226 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -973,6 +973,84 @@ int DrmDisplayCompositor::ApplyComposition(
   return ret;
 }
 
+int DrmDisplayCompositor::FlattenOnDisplay(
+    std::unique_ptr<DrmDisplayComposition> &src,
+    std::unique_ptr<DrmDisplayComposition> &writeback,
+    DrmConnector *writeback_conn, DrmMode &src_mode) {
+  int ret = 0;
+  ret = writeback_conn->UpdateModes();
+  if (ret) {
+    ALOGE("Failed to update modes %d", ret);
+    return ret;
+  }
+  for (const DrmMode &mode : writeback_conn->modes()) {
+    if (mode.h_display() == src_mode.h_display() &&
+        mode.v_display() == src_mode.v_display()) {
+      writeback->SetDisplayMode(mode);
+      mode_.mode = mode;
+      if (mode_.blob_id)
+        drm_->DestroyPropertyBlob(mode_.blob_id);
+      std::tie(ret, mode_.blob_id) = CreateModeBlob(mode_.mode);
+      if (ret) {
+        ALOGE("Failed to create mode blob for display %d", display_);
+        return ret;
+      }
+      mode_.needs_modeset = true;
+      break;
+    }
+  }
+  if (mode_.blob_id <= 0) {
+    ALOGE("Failed to find similar mode");
+    return -EINVAL;
+  }
+
+  std::vector<DrmPlane *> primary_planes;
+  std::vector<DrmPlane *> overlay_planes;
+  SquashState squash_state;
+  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
+  if (!crtc) {
+    ALOGE("Failed to find crtc for display %d", display_);
+    return -EINVAL;
+  }
+
+  // TODO what happens if planes could go to both CRTCs, don't think it's
+  // handled anywhere
+  for (auto &plane : drm_->planes()) {
+    if (!plane->GetCrtcSupported(*crtc))
+      continue;
+    if (plane->type() == DRM_PLANE_TYPE_PRIMARY)
+      primary_planes.push_back(plane.get());
+    else if (plane->type() == DRM_PLANE_TYPE_OVERLAY)
+      overlay_planes.push_back(plane.get());
+  }
+  ret = src->Plan(&squash_state, &primary_planes, &overlay_planes);
+  if (ret) {
+    ALOGE("Failed to plan the composition ret = %d", ret);
+    return ret;
+  }
+  if (src->squash_regions().size() > 0) {
+    ALOGE("Abandon we don't want to fire up the GPU");
+    return -EINVAL;
+  }
+
+  // Disable the planes we're not using
+  for (auto i = primary_planes.begin(); i != primary_planes.end();) {
+    src->AddPlaneDisable(*i);
+    i = primary_planes.erase(i);
+  }
+  for (auto i = overlay_planes.begin(); i != overlay_planes.end();) {
+    src->AddPlaneDisable(*i);
+    i = overlay_planes.erase(i);
+  }
+
+  ret = WritebackComposite(src.get(), writeback.get(), writeback_conn);
+  if (ret) {
+    ALOGE("Failed to writeback ret=%d", ret);
+    return ret;
+  }
+  return 0;
+}
+
 int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src,
                                              DrmDisplayComposition *dst,
                                              DrmConnector *writeback_conn) {
@@ -1088,6 +1166,97 @@ int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) {
   return 0;
 }
 
+int DrmDisplayCompositor::FlattenAsynchronously(
+    DrmConnector *writeback_conn) {
+  if (writeback_conn->display() == display_) {
+    ALOGE("Cannot flatten asynchronously on the same display");
+    return -EINVAL;
+  }
+  ALOGI("FlattenAsynchronously on a different display");
+  int ret = 0;
+  ResourceManager *resource_manager = drm_->resource_manager();
+  DrmResources *drm_resource =
+      resource_manager->GetDrmResources(writeback_conn->display());
+  if (!drm_resource) {
+    ALOGE("Failed to find resources for display = %d",
+          writeback_conn->display());
+    return -EINVAL;
+  }
+  DrmDisplayCompositor drmdisplaycompositor;
+  ret = drmdisplaycompositor.Init(drm_resource, writeback_conn->display());
+  if (ret) {
+    ALOGE("Failed to init  drmdisplaycompositor = %d", ret);
+  }
+  /* Copy of the active_composition, needed because of two things:
+   * 1) Not to hold the lock for the whole time we are accessing
+   *    active_composition
+   * 2) Will be committed on a crtc that might not be on the same
+   *    dri node, so buffers need to be imported on the right node.
+   */
+  std::unique_ptr<DrmDisplayComposition> copy_comp =
+      drmdisplaycompositor.CreateInitializedComposition();
+  /* Flattened composition with only one layer that is built
+   * using the writeback connector
+   */
+  std::unique_ptr<DrmDisplayComposition> writeback_comp =
+      drmdisplaycompositor.CreateInitializedComposition();
+  /* Copy of the writeback composition that will be committed to the display,
+   *  needed because of 2)from above
+   */
+  std::unique_ptr<DrmDisplayComposition> next_comp =
+      CreateInitializedComposition();
+
+  DrmCrtc *crtc = NULL;
+  if (!copy_comp || !next_comp || !writeback_comp)
+    return -EINVAL;
+  AutoLock lock(&lock_, __FUNCTION__);
+  if ((ret = lock.Lock()))
+    return ret;
+  if (CountdownExpired()) {
+    crtc = active_composition_->crtc();
+    ret = copy_comp->CopyLayers(active_composition_.get());
+    if (ret)
+      return ret;
+  } else {
+    return -EINVAL;
+  }
+  lock.Unlock();
+  ret = drmdisplaycompositor.FlattenOnDisplay(copy_comp, writeback_comp,
+                                                   writeback_conn, mode_.mode);
+  if (ret) {
+    ALOGE("Failed to plan ret = %d", ret);
+    return ret;
+  }
+
+  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
+                                    crtc);
+  for (auto &plane : drm_->planes()) {
+    if (plane->type() == DRM_PLANE_TYPE_PRIMARY)
+      squashed_comp.set_plane(plane.get());
+    else
+      next_comp->AddPlaneDisable(plane.get());
+  }
+  next_comp->layers().emplace_back();
+  DrmHwcLayer &src_layer = writeback_comp->layers().back();
+  DrmHwcLayer &next_layer = next_comp->layers().back();
+  next_layer.PopulateFromDrmHwcLayer(&src_layer);
+  ret = next_layer.buffer.ImportBuffer(
+      next_layer.sf_handle, resource_manager->GetImporter(display_).get());
+  if (ret) {
+    ALOGE("Failed to import framebuffer for display %d", ret);
+    return ret;
+  }
+  squashed_comp.source_layers().push_back(0);
+  ret = next_comp->AddPlaneComposition(std::move(squashed_comp));
+  if (ret) {
+    ALOGE("Failed to add plane composition %d", ret);
+    return ret;
+  }
+  ApplyFrame(std::move(next_comp), 0, true);
+  ALOGE("Apply frame end scope");
+  return ret;
+}
+
 int DrmDisplayCompositor::FlattenScene() {
   DrmConnector *writeback_conn =
       drm_->resource_manager()->AvailableWritebackConnector(display_);
@@ -1103,7 +1272,9 @@ int DrmDisplayCompositor::FlattenScene() {
   if (src_planes_with_layer <= 1)
     return -EALREADY;
 
-  if (writeback_conn->display() == display_) {
+  if (writeback_conn->display() != display_) {
+    return FlattenAsynchronously(writeback_conn);
+  } else {
     return FlattenSynchronously(writeback_conn);
   }
 
diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
index 4cc4a5e..a218bb7 100644
--- a/drmdisplaycompositor.h
+++ b/drmdisplaycompositor.h
@@ -137,6 +137,11 @@ class DrmDisplayCompositor {
                   int status, bool writeback = false);
   int FlattenScene();
   int FlattenSynchronously(DrmConnector *writeback_conn);
+  int FlattenAsynchronously(DrmConnector *writeback_conn);
+  int FlattenOnDisplay(std::unique_ptr<DrmDisplayComposition> &src,
+                         std::unique_ptr<DrmDisplayComposition> &writeback,
+                         DrmConnector *writeback_conn,
+                         DrmMode &src_mode);
 
   int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst,
                          DrmConnector *writeback_conn);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 00/18] Add scene flattening support

[John Stultz]

On Wed, Apr 11, 2018 at 8:22 AM, Alexandru Gheorghe
<alexandru-cosmin.gheorghe@arm.com> wrote:
> Flattening a scene in order to reduce memory consumption it's an idea
> which had been floating around on irc and mailing list several times,
> this patchset adds support for flattening a scene using a writeback
> connector, the latest version of the kernel patches could be found
> here [1].
>
> v1 for this patch series could be found here [2].
>
> Changes since v1:
>   - Add support to use either the same crtc or a dedicated one, this
>     had been discussed on irc here [3].
>   - Add support for parsing more than one drm node, this is triggered
>     by the fact the on Mali DP each device has it's own driver, so
>     it's needed in order to use a dedicate crtc for flattening.
>   - Parse encoder possible_clones to detect if writeback could
>     function simultaneously with the display connector.
>   - Fixes for some bugs discovered while working on this.
>   - Split changes in multiple patches.

Just for testing w/ other devices currently using drm_hwcomposer, do
you happen to have the patchset in a public git tree somewhere?

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 00/18] Add scene flattening support

[Alexandru-Cosmin Gheorghe]

Hi John,

On Thu, Apr 12, 2018 at 04:18:54PM -0700, John Stultz wrote:
> On Wed, Apr 11, 2018 at 8:22 AM, Alexandru Gheorghe
> <alexandru-cosmin.gheorghe@arm.com> wrote:
> > Flattening a scene in order to reduce memory consumption it's an idea
> > which had been floating around on irc and mailing list several times,
> > this patchset adds support for flattening a scene using a writeback
> > connector, the latest version of the kernel patches could be found
> > here [1].
> >
> > v1 for this patch series could be found here [2].
> >
> > Changes since v1:
> >   - Add support to use either the same crtc or a dedicated one, this
> >     had been discussed on irc here [3].
> >   - Add support for parsing more than one drm node, this is triggered
> >     by the fact the on Mali DP each device has it's own driver, so
> >     it's needed in order to use a dedicate crtc for flattening.
> >   - Parse encoder possible_clones to detect if writeback could
> >     function simultaneously with the display connector.
> >   - Fixes for some bugs discovered while working on this.
> >   - Split changes in multiple patches.
> 
> Just for testing w/ other devices currently using drm_hwcomposer, do
> you happen to have the patchset in a public git tree somewhere?
> 

Sorry no, but they should apply without conflict on master branch.
> thanks
> -john

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

Re: [PATCH hwc v2 00/18] Add scene flattening support

[Alexandru-Cosmin Gheorghe]

On second thought, I pushed this patchset here:
https://github.com/ARM-software/drm-hwcomposer/tree/scene_flattening_support

On Fri, Apr 13, 2018 at 10:52:55AM +0100, Alexandru-Cosmin Gheorghe wrote:
> Hi John,
> 
> On Thu, Apr 12, 2018 at 04:18:54PM -0700, John Stultz wrote:
> > On Wed, Apr 11, 2018 at 8:22 AM, Alexandru Gheorghe
> > <alexandru-cosmin.gheorghe@arm.com> wrote:
> > > Flattening a scene in order to reduce memory consumption it's an idea
> > > which had been floating around on irc and mailing list several times,
> > > this patchset adds support for flattening a scene using a writeback
> > > connector, the latest version of the kernel patches could be found
> > > here [1].
> > >
> > > v1 for this patch series could be found here [2].
> > >
> > > Changes since v1:
> > >   - Add support to use either the same crtc or a dedicated one, this
> > >     had been discussed on irc here [3].
> > >   - Add support for parsing more than one drm node, this is triggered
> > >     by the fact the on Mali DP each device has it's own driver, so
> > >     it's needed in order to use a dedicate crtc for flattening.
> > >   - Parse encoder possible_clones to detect if writeback could
> > >     function simultaneously with the display connector.
> > >   - Fixes for some bugs discovered while working on this.
> > >   - Split changes in multiple patches.
> > 
> > Just for testing w/ other devices currently using drm_hwcomposer, do
> > you happen to have the patchset in a public git tree somewhere?
> > 
> 
> Sorry no, but they should apply without conflict on master branch.
> > thanks
> > -john
> 
> -- 
> Cheers,
> Alex G
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH hwc v2 01/18] drm_hwcomposer: vsyncworker: Fix uninitialized enabled_ field

[Robert Foss]

Hey Alexandru,

Feel free to add:
Signed-off-by: Robert Foss <robert.foss@collabora.com>


Rob.

On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>   vsyncworker.cpp | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> index 3ad16fe..3bfe4be 100644
> --- a/vsyncworker.cpp
> +++ b/vsyncworker.cpp
> @@ -35,6 +35,7 @@ VSyncWorker::VSyncWorker()
>       : Worker("vsync", HAL_PRIORITY_URGENT_DISPLAY),
>         drm_(NULL),
>         display_(-1),
> +      enabled_(false),
>         last_timestamp_(-1) {
>   }
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Robert Foss]

Hey Alexandru,

Feel free my SoB on this patch:
Signed-off-by: Robert Foss <robert.foss@collabora.com>


Rob.


On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:
> vsyncworker::Routine assumes that when -EINTR is returned by
> WaitForSignalOrExitLocked the lock as been released, which is not
> true, so it hangs if a vsyncworker is never enabled and Exit is
> called.
> 
> There are two code paths in WaitForSignalOrExitLocked that return
> -EINTR, one releases the lock the other doesn't.
> Looking at the clients of WaitForSignalOrExitLocked all assume the lock
> is still held, except vsyncworker::Routine.
> So, the proper fix needs two changes:
> - Make WaitForSignalOrExitLocked consistent and always hold the lock
>    when exiting.
> - Release lock in vsynworker::Routine on all code paths.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>   vsyncworker.cpp | 1 +
>   worker.cpp      | 6 +++---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> index 3bfe4be..7c0c741 100644
> --- a/vsyncworker.cpp
> +++ b/vsyncworker.cpp
> @@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
>     if (!enabled_) {
>       ret = WaitForSignalOrExitLocked();
>       if (ret == -EINTR) {
> +      Unlock();
>         return;
>       }
>     }
> diff --git a/worker.cpp b/worker.cpp
> index da6c580..5b351e0 100644
> --- a/worker.cpp
> +++ b/worker.cpp
> @@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t max_nanoseconds) {
>       ret = -ETIMEDOUT;
>     }
>   
> +  // release leaves mutex locked when going out of scope
> +  lk.release();
> +
>     // exit takes precedence on timeout
>     if (should_exit())
>       ret = -EINTR;
>   
> -  // release leaves mutex locked when going out of scope
> -  lk.release();
> -
>     return ret;
>   }
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 03/18] drm_hwcomposer: drmeventlistener: Set nl_pid to 0

[Robert Foss]

Hey Alexandru,

Feel free to add my SoB to this patch:
Signed-off-by: Robert Foss <robert.foss@collabora.com>


Rob.

On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:
> By setting nl_pid to 0, we let the kernel to assign a port for us.
> In the current implementation there is no way we could create more
> than one instance for drmeventlistener.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>   drmeventlistener.cpp | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drmeventlistener.cpp b/drmeventlistener.cpp
> index 984d1dd..5534182 100644
> --- a/drmeventlistener.cpp
> +++ b/drmeventlistener.cpp
> @@ -46,7 +46,7 @@ int DrmEventListener::Init() {
>     struct sockaddr_nl addr;
>     memset(&addr, 0, sizeof(addr));
>     addr.nl_family = AF_NETLINK;
> -  addr.nl_pid = getpid();
> +  addr.nl_pid = 0;
>     addr.nl_groups = 0xFFFFFFFF;
>   
>     int ret = bind(uevent_fd_.get(), (struct sockaddr *)&addr, sizeof(addr));
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 01/18] drm_hwcomposer: vsyncworker: Fix uninitialized enabled_ field

[Alexandru-Cosmin Gheorghe]

On Mon, Apr 16, 2018 at 12:30:13PM +0200, Robert Foss wrote:

Hi Rob,

Thanks for the review.

> Hey Alexandru,
> 
> Feel free to add:
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>

Should I re-send this 3 patches or could you just pushed them to
master and adding your SoB in the process?
 
> 
> Rob.
> 
> On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:
> >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> >---
> >  vsyncworker.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> >index 3ad16fe..3bfe4be 100644
> >--- a/vsyncworker.cpp
> >+++ b/vsyncworker.cpp
> >@@ -35,6 +35,7 @@ VSyncWorker::VSyncWorker()
> >      : Worker("vsync", HAL_PRIORITY_URGENT_DISPLAY),
> >        drm_(NULL),
> >        display_(-1),
> >+      enabled_(false),
> >        last_timestamp_(-1) {
> >  }
> >

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

Re: [PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:13PM +0100, Alexandru Gheorghe wrote:
> vsyncworker::Routine assumes that when -EINTR is returned by
> WaitForSignalOrExitLocked the lock as been released, which is not
> true, so it hangs if a vsyncworker is never enabled and Exit is
> called.
> 
> There are two code paths in WaitForSignalOrExitLocked that return
> -EINTR, one releases the lock the other doesn't.
> Looking at the clients of WaitForSignalOrExitLocked all assume the lock
> is still held, except vsyncworker::Routine.
> So, the proper fix needs two changes:
> - Make WaitForSignalOrExitLocked consistent and always hold the lock
>   when exiting.
> - Release lock in vsynworker::Routine on all code paths.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  vsyncworker.cpp | 1 +
>  worker.cpp      | 6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> index 3bfe4be..7c0c741 100644
> --- a/vsyncworker.cpp
> +++ b/vsyncworker.cpp
> @@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
>    if (!enabled_) {
>      ret = WaitForSignalOrExitLocked();
>      if (ret == -EINTR) {
> +      Unlock();
>        return;
>      }
>    }
> diff --git a/worker.cpp b/worker.cpp
> index da6c580..5b351e0 100644
> --- a/worker.cpp
> +++ b/worker.cpp
> @@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t max_nanoseconds) {
>      ret = -ETIMEDOUT;
>    }
>  
> +  // release leaves mutex locked when going out of scope
> +  lk.release();
> +
>    // exit takes precedence on timeout
>    if (should_exit())
>      ret = -EINTR;
>  
> -  // release leaves mutex locked when going out of scope
> -  lk.release();
> -

I'm not sure why this chunk makes a difference? If the above was
"return -EINTR;" it would, but it's just assigning ret.

Sean

>    return ret;
>  }
>  
> -- 
> 2.7.4
> 

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

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

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:16PM +0100, Alexandru Gheorghe wrote:
> Use the newly added ResourceManager for creating and detecting all the
> drm devices instead of assuming that there is only one device.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmhwctwo.cpp    | 34 +++++++++++++---------------------
>  drmhwctwo.h      |  4 +---
>  drmresources.cpp | 25 ++++++++++++++++++-------
>  drmresources.h   | 14 +++++++++++---
>  4 files changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index dfca1a6..cddd5da 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -58,40 +58,32 @@ DrmHwcTwo::DrmHwcTwo() {
>  }
>  
>  HWC2::Error DrmHwcTwo::Init() {
> -  int ret = drm_.Init();
> +  int ret = resource_manager_.Init();
>    if (ret) {
> -    ALOGE("Can't initialize drm object %d", ret);
> +    ALOGE("Can't initialize the resource manager %d", ret);
>      return HWC2::Error::NoResources;
>    }
>  
> -  importer_.reset(Importer::CreateInstance(&drm_));
> -  if (!importer_) {
> -    ALOGE("Failed to create importer instance");
> +  DrmResources *drm = resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
> +  std::shared_ptr<Importer> importer =
> +      resource_manager_.GetImporter(HWC_DISPLAY_PRIMARY);
> +  if (!drm || !importer) {
> +    ALOGE("Failed to get a valid drmresource and importer");
>      return HWC2::Error::NoResources;
>    }
> +  displays_.emplace(
> +      std::piecewise_construct, std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
> +      std::forward_as_tuple(drm, importer, resource_manager_.GetGralloc(),
> +                            HWC_DISPLAY_PRIMARY, HWC2::DisplayType::Physical));
>  
> -  ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> -                      (const hw_module_t **)&gralloc_);
> -  if (ret) {
> -    ALOGE("Failed to open gralloc module %d", ret);
> -    return HWC2::Error::NoResources;
> -  }
> -
> -  displays_.emplace(std::piecewise_construct,
> -                    std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
> -                    std::forward_as_tuple(&drm_, importer_, gralloc_,
> -                                          HWC_DISPLAY_PRIMARY,
> -                                          HWC2::DisplayType::Physical));
> -
> -  DrmCrtc *crtc = drm_.GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
> +  DrmCrtc *crtc = drm->GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
>    if (!crtc) {
>      ALOGE("Failed to get crtc for display %d",
>            static_cast<int>(HWC_DISPLAY_PRIMARY));
>      return HWC2::Error::BadDisplay;
>    }
> -
>    std::vector<DrmPlane *> display_planes;
> -  for (auto &plane : drm_.planes()) {
> +  for (auto &plane : drm->planes()) {
>      if (plane->GetCrtcSupported(*crtc))
>        display_planes.push_back(plane.get());
>    }
> diff --git a/drmhwctwo.h b/drmhwctwo.h
> index 0490e2a..beb5d2d 100644
> --- a/drmhwctwo.h
> +++ b/drmhwctwo.h
> @@ -262,9 +262,7 @@ class DrmHwcTwo : public hwc2_device_t {
>    HWC2::Error RegisterCallback(int32_t descriptor, hwc2_callback_data_t data,
>                                 hwc2_function_pointer_t function);
>  
> -  DrmResources drm_;
> -  std::shared_ptr<Importer> importer_;  // Shared with HwcDisplay
> -  const gralloc_module_t *gralloc_;
> +  ResourceManager resource_manager_;
>    std::map<hwc2_display_t, HwcDisplay> displays_;
>    std::map<HWC2::Callback, HwcCallback> callbacks_;
>  };
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 32dd376..a5ddda0 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -42,10 +42,9 @@ DrmResources::~DrmResources() {
>    event_listener_.Exit();
>  }
>  
> -int DrmResources::Init() {
> -  char path[PROPERTY_VALUE_MAX];
> -  property_get("hwc.drm.device", path, "/dev/dri/card0");
> -
> +int DrmResources::Init(ResourceManager *resource_manager, char *path,
> +                       int start_display_index) {
> +  resource_manager_ = resource_manager;

You can avoid the backpointer if you just pass the RM to the right places (looks
like compositor + composition). Bonus points if you can remove drm_ from those
objects once you've done that.

>    /* TODO: Use drmOpenControl here instead */
>    fd_.Set(open(path, O_RDWR));
>    if (fd() < 0) {
> @@ -76,8 +75,8 @@ int DrmResources::Init() {
>    max_resolution_ =
>        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
>  
> -  bool found_primary = false;
> -  int display_num = 1;
> +  bool found_primary = start_display_index != 0;
> +  int display_num = found_primary ? start_display_index : 1;

This could use a comment. AFAICT, you're assuming the primary display will
always be in the first drm device, which is fine, but should be explained
_somewhere_.

>  
>    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
>      drmModeCrtcPtr c = drmModeGetCrtc(fd(), res->crtcs[i]);
> @@ -161,9 +160,11 @@ int DrmResources::Init() {
>    for (auto &conn : connectors_) {
>      if (conn->internal() && !found_primary) {
>        conn->set_display(0);
> +      displays_[0] = 0;
>        found_primary = true;
>      } else {
>        conn->set_display(display_num);
> +      displays_[display_num] = display_num;
>        ++display_num;
>      }
>    }
> @@ -171,7 +172,9 @@ int DrmResources::Init() {
>    // Then look for primary amongst external connectors
>    for (auto &conn : connectors_) {
>      if (conn->external() && !found_primary) {
> +      displays_.erase(conn->display());
>        conn->set_display(0);
> +      displays_[0] = 0;
>        found_primary = true;
>      }
>    }
> @@ -226,7 +229,11 @@ int DrmResources::Init() {
>        return ret;
>      }
>    }
> -  return 0;
> +  return displays_.size() ? displays_.rbegin()->first : -EINVAL;

I'd rather not change the meaning of the return value (especially without a
comment somewhere to let readers know this function doesn't follow the 0 ||
-ERRNO convention). Consider returning a pair of ret,display.

> +}
> +
> +bool DrmResources::HandlesDisplay(int display) const {
> +  return displays_.find(display) != displays_.end();
>  }
>  
>  DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
> @@ -349,6 +356,10 @@ DrmEventListener *DrmResources::event_listener() {
>    return &event_listener_;
>  }
>  
> +ResourceManager *DrmResources::resource_manager() {
> +  return resource_manager_;
> +}
> +
>  int DrmResources::GetProperty(uint32_t obj_id, uint32_t obj_type,
>                                const char *prop_name, DrmProperty *property) {
>    drmModeObjectPropertiesPtr props;
> diff --git a/drmresources.h b/drmresources.h
> index 4cca48c..4cdcd87 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -17,22 +17,26 @@
>  #ifndef ANDROID_DRM_H_
>  #define ANDROID_DRM_H_
>  
> +#include <stdint.h>
>  #include "drmconnector.h"
>  #include "drmcrtc.h"
>  #include "drmencoder.h"
>  #include "drmeventlistener.h"
>  #include "drmplane.h"
> -
> -#include <stdint.h>

Why this change?

> +#include "platform.h"
> +#include "resourcemanager.h"
>  
>  namespace android {
>  
> +class ResourceManager;
> +
>  class DrmResources {
>   public:
>    DrmResources();
>    ~DrmResources();
>  
> -  int Init();
> +  int Init(ResourceManager *resource_manager, char *path,
> +           int start_display_index);
>  
>    int fd() const {
>      return fd_.get();
> @@ -58,6 +62,7 @@ class DrmResources {
>    DrmCrtc *GetCrtcForDisplay(int display) const;
>    DrmPlane *GetPlane(uint32_t id) const;
>    DrmEventListener *event_listener();
> +  ResourceManager *resource_manager();
>  
>    int GetPlaneProperty(const DrmPlane &plane, const char *prop_name,
>                         DrmProperty *property);
> @@ -71,6 +76,7 @@ class DrmResources {
>  
>    int CreatePropertyBlob(void *data, size_t length, uint32_t *blob_id);
>    int DestroyPropertyBlob(uint32_t blob_id);
> +  bool HandlesDisplay(int display) const;
>  
>   private:
>    int TryEncoderForDisplay(int display, DrmEncoder *enc);
> @@ -90,6 +96,8 @@ class DrmResources {
>  
>    std::pair<uint32_t, uint32_t> min_resolution_;
>    std::pair<uint32_t, uint32_t> max_resolution_;
> +  std::map<int, int> displays_;
> +  ResourceManager *resource_manager_;
>  };
>  }
>  
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 06/18] drm_hwcomposer: Add writeback connector support

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:17PM +0100, Alexandru Gheorghe wrote:
> Writeback connector is a special case of connector, which can be
> linked to a CRTC in order to get the result of the composition back to
> a memory buffer. This had not been merged to the mainline kernel yet,
> latest version of the kernel patches could be found here [1].
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmconnector.cpp | 42 +++++++++++++++++++++++++++++++++++++++++-
>  drmconnector.h   |  7 +++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drmconnector.cpp b/drmconnector.cpp
> index 145518f..e482832 100644
> --- a/drmconnector.cpp
> +++ b/drmconnector.cpp
> @@ -52,6 +52,26 @@ int DrmConnector::Init() {
>      ALOGE("Could not get CRTC_ID property\n");
>      return ret;
>    }
> +  if (writeback()) {
> +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS",
> +                                     &writeback_pixel_formats_);
> +    if (ret) {
> +      ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
> +      return ret;
> +    }
> +    ret =
> +        drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", &writeback_fb_id_);

Please break on the function arguments instead of the =


> +    if (ret) {
> +      ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
> +      return ret;
> +    }
> +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR",
> +                                     &writeback_out_fence_);

Like this :)

With that,

Reviewed-by: Sean Paul <seanpaul@chromium.org>




> +    if (ret) {
> +      ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
> +      return ret;
> +    }
> +  }
>    return 0;
>  }
>  
> @@ -78,8 +98,16 @@ bool DrmConnector::external() const {
>           type_ == DRM_MODE_CONNECTOR_VGA;
>  }
>  
> +bool DrmConnector::writeback() const {
> +#ifdef DRM_MODE_CONNECTOR_WRITEBACK
> +  return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
> +#else
> +  return false;
> +#endif
> +}
> +
>  bool DrmConnector::valid_type() const {
> -  return internal() || external();
> +  return internal() || external() || writeback();
>  }
>  
>  int DrmConnector::UpdateModes() {
> @@ -130,6 +158,18 @@ const DrmProperty &DrmConnector::crtc_id_property() const {
>    return crtc_id_property_;
>  }
>  
> +const DrmProperty &DrmConnector::writeback_pixel_formats() const {
> +  return writeback_pixel_formats_;
> +}
> +
> +const DrmProperty &DrmConnector::writeback_fb_id() const {
> +  return writeback_fb_id_;
> +}
> +
> +const DrmProperty &DrmConnector::writeback_out_fence() const {
> +  return writeback_out_fence_;
> +}
> +
>  DrmEncoder *DrmConnector::encoder() const {
>    return encoder_;
>  }
> diff --git a/drmconnector.h b/drmconnector.h
> index 5601e06..e139730 100644
> --- a/drmconnector.h
> +++ b/drmconnector.h
> @@ -46,6 +46,7 @@ class DrmConnector {
>  
>    bool internal() const;
>    bool external() const;
> +  bool writeback() const;
>    bool valid_type() const;
>  
>    int UpdateModes();
> @@ -58,6 +59,9 @@ class DrmConnector {
>  
>    const DrmProperty &dpms_property() const;
>    const DrmProperty &crtc_id_property() const;
> +  const DrmProperty &writeback_pixel_formats() const;
> +  const DrmProperty &writeback_fb_id() const;
> +  const DrmProperty &writeback_out_fence() const;
>  
>    const std::vector<DrmEncoder *> &possible_encoders() const {
>      return possible_encoders_;
> @@ -88,6 +92,9 @@ class DrmConnector {
>  
>    DrmProperty dpms_property_;
>    DrmProperty crtc_id_property_;
> +  DrmProperty writeback_pixel_formats_;
> +  DrmProperty writeback_fb_id_;
> +  DrmProperty writeback_out_fence_;
>  
>    std::vector<DrmEncoder *> possible_encoders_;
>  };
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 07/18] drm_hwcomposer: Add display field to Drmencoder

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:18PM +0100, Alexandru Gheorghe wrote:
> In the current implementation TryEncoderForDisplay just looks
> at the crtc linked to the display, if that's not assigned to
> a display it means the encoder could be used, otherwise iterate
> to the list of possible_crtcs and find one which is not used.
> 
> This logic works fine when you have just one encoder connected to a
> crtc but with two or more, like is the case when we attach a writeback
> connector, we need to know if we already assigned the encoder to a
> display.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmencoder.cpp | 14 ++++++++++++++
>  drmencoder.h   |  4 ++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drmencoder.cpp b/drmencoder.cpp
> index 3d762f3..1da7ec3 100644
> --- a/drmencoder.cpp
> +++ b/drmencoder.cpp
> @@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
>                         const std::vector<DrmCrtc *> &possible_crtcs)
>      : id_(e->encoder_id),
>        crtc_(current_crtc),
> +      display_(-1),
>        possible_crtcs_(possible_crtcs) {
>  }
>  
> @@ -40,5 +41,18 @@ DrmCrtc *DrmEncoder::crtc() const {
>  
>  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
>    crtc_ = crtc;
> +  set_display(crtc->display());
> +}
> +
> +int DrmEncoder::display() const {
> +  return display_;
> +}
> +
> +void DrmEncoder::set_display(int display) {
> +  display_ = display;
> +}

Instead of adding this, just call set_crtc() in TryEncoderForDisplay() for the
already-bound case. That way we only have one entry point for this.

> +
> +bool DrmEncoder::can_bind(int display) const {
> +  return display_ == -1 || display_ == display;
>  }
>  }
> diff --git a/drmencoder.h b/drmencoder.h
> index 58ccbfb..7e06691 100644
> --- a/drmencoder.h
> +++ b/drmencoder.h
> @@ -36,6 +36,9 @@ class DrmEncoder {
>  
>    DrmCrtc *crtc() const;
>    void set_crtc(DrmCrtc *crtc);
> +  bool can_bind(int display) const;
> +  void set_display(int display);
> +  int display() const;
>  
>    const std::vector<DrmCrtc *> &possible_crtcs() const {
>      return possible_crtcs_;
> @@ -44,6 +47,7 @@ class DrmEncoder {
>   private:
>    uint32_t id_;
>    DrmCrtc *crtc_;
> +  int display_;
>  
>    std::vector<DrmCrtc *> possible_crtcs_;
>  };
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 08/18] drm_hwcomposer: Parse and store possible_clones information

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:19PM +0100, Alexandru Gheorghe wrote:
> drmModeEncoder has a field called possible_clones. It's a bit mask
> which tells if the encoder could be simultaneously connected, to the
> same CRTC, with the encoders specified in the possible_clones mask.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmencoder.cpp   | 8 ++++++++
>  drmencoder.h     | 4 ++++
>  drmresources.cpp | 9 ++++++++-
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drmencoder.cpp b/drmencoder.cpp
> index 1da7ec3..ff675f5 100644
> --- a/drmencoder.cpp
> +++ b/drmencoder.cpp
> @@ -39,6 +39,14 @@ DrmCrtc *DrmEncoder::crtc() const {
>    return crtc_;
>  }
>  
> +bool DrmEncoder::can_clone(DrmEncoder *encoder) {
> +  return possible_clones_.find(encoder) != possible_clones_.end();
> +}

The find() call is probably enough to justify CamelCase for this function. FTR,
I _hate_ this part of the style guide and wish I had just picked one or the
other.

To improve readability, can you also change the name of "encoder" to
"possible_clone" like below so it's super obvious what this does?

> +
> +void DrmEncoder::add_possible_clone(DrmEncoder *possible_clone) {
> +  possible_clones_[possible_clone] = true;
> +}
> +
>  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
>    crtc_ = crtc;
>    set_display(crtc->display());
> diff --git a/drmencoder.h b/drmencoder.h
> index 7e06691..5e7c010 100644
> --- a/drmencoder.h
> +++ b/drmencoder.h
> @@ -21,6 +21,7 @@
>  
>  #include <stdint.h>
>  #include <vector>
> +#include <map>

Alphabetical

>  #include <xf86drmMode.h>
>  
>  namespace android {
> @@ -43,6 +44,8 @@ class DrmEncoder {
>    const std::vector<DrmCrtc *> &possible_crtcs() const {
>      return possible_crtcs_;
>    }
> +  bool can_clone(DrmEncoder *encoder);
> +  void add_possible_clone(DrmEncoder *possible_clone);
>  
>   private:
>    uint32_t id_;
> @@ -50,6 +53,7 @@ class DrmEncoder {
>    int display_;
>  
>    std::vector<DrmCrtc *> possible_crtcs_;
> +  std::map<DrmEncoder *, bool> possible_clones_;
>  };
>  }
>  
> diff --git a/drmresources.cpp b/drmresources.cpp
> index a5ddda0..39f50be 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -97,6 +97,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>      crtcs_.emplace_back(std::move(crtc));
>    }
>  
> +  std::vector<int> possible_clones;
>    for (int i = 0; !ret && i < res->count_encoders; ++i) {
>      drmModeEncoderPtr e = drmModeGetEncoder(fd(), res->encoders[i]);
>      if (!e) {
> @@ -117,12 +118,18 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>  
>      std::unique_ptr<DrmEncoder> enc(
>          new DrmEncoder(e, current_crtc, possible_crtcs));
> -
> +    possible_clones.push_back(e->possible_clones);
>      drmModeFreeEncoder(e);
>  
>      encoders_.emplace_back(std::move(enc));
>    }
>  
> +  for (uint32_t i = 0; i < encoders_.size(); i++) {
> +    for (uint32_t j = 0; j < encoders_.size(); j++)

for (auto &enc: encoders_) {
  for (auto &clone: encoders_) {

Or something similarly C++'y, looping through indices is sooo last decade :-)


> +      if (possible_clones[i] & (1 << j))
> +        encoders_[i]->add_possible_clone(encoders_[j].get());
> +  }
> +
>    for (int i = 0; !ret && i < res->count_connectors; ++i) {
>      drmModeConnectorPtr c = drmModeGetConnector(fd(), res->connectors[i]);
>      if (!c) {
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 02/18] drm_hwcomposer: vsyncworker: Fix deadlock on exit path

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 03:25:42PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:13PM +0100, Alexandru Gheorghe wrote:
> > vsyncworker::Routine assumes that when -EINTR is returned by
> > WaitForSignalOrExitLocked the lock as been released, which is not
> > true, so it hangs if a vsyncworker is never enabled and Exit is
> > called.
> > 
> > There are two code paths in WaitForSignalOrExitLocked that return
> > -EINTR, one releases the lock the other doesn't.
> > Looking at the clients of WaitForSignalOrExitLocked all assume the lock
> > is still held, except vsyncworker::Routine.
> > So, the proper fix needs two changes:
> > - Make WaitForSignalOrExitLocked consistent and always hold the lock
> >   when exiting.
> > - Release lock in vsynworker::Routine on all code paths.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  vsyncworker.cpp | 1 +
> >  worker.cpp      | 6 +++---
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> > index 3bfe4be..7c0c741 100644
> > --- a/vsyncworker.cpp
> > +++ b/vsyncworker.cpp
> > @@ -120,6 +120,7 @@ void VSyncWorker::Routine() {
> >    if (!enabled_) {
> >      ret = WaitForSignalOrExitLocked();
> >      if (ret == -EINTR) {
> > +      Unlock();
> >        return;
> >      }
> >    }
> > diff --git a/worker.cpp b/worker.cpp
> > index da6c580..5b351e0 100644
> > --- a/worker.cpp
> > +++ b/worker.cpp
> > @@ -66,13 +66,13 @@ int Worker::WaitForSignalOrExitLocked(int64_t max_nanoseconds) {
> >      ret = -ETIMEDOUT;
> >    }
> >  
> > +  // release leaves mutex locked when going out of scope
> > +  lk.release();
> > +
> >    // exit takes precedence on timeout
> >    if (should_exit())
> >      ret = -EINTR;
> >  
> > -  // release leaves mutex locked when going out of scope
> > -  lk.release();
> > -
> 
> I'm not sure why this chunk makes a difference? If the above was
> "return -EINTR;" it would, but it's just assigning ret.
> 

You are certainly right, just dyslexia on my side. 
I will update the patch.

> Sean
> 
> >    return ret;
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

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

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 03:54:02PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:16PM +0100, Alexandru Gheorghe wrote:
> > Use the newly added ResourceManager for creating and detecting all the
> > drm devices instead of assuming that there is only one device.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmhwctwo.cpp    | 34 +++++++++++++---------------------
> >  drmhwctwo.h      |  4 +---
> >  drmresources.cpp | 25 ++++++++++++++++++-------
> >  drmresources.h   | 14 +++++++++++---
> >  4 files changed, 43 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> > index dfca1a6..cddd5da 100644
> > --- a/drmhwctwo.cpp
> > +++ b/drmhwctwo.cpp
> > @@ -58,40 +58,32 @@ DrmHwcTwo::DrmHwcTwo() {
> >  }
> >  
> >  HWC2::Error DrmHwcTwo::Init() {
> > -  int ret = drm_.Init();
> > +  int ret = resource_manager_.Init();
> >    if (ret) {
> > -    ALOGE("Can't initialize drm object %d", ret);
> > +    ALOGE("Can't initialize the resource manager %d", ret);
> >      return HWC2::Error::NoResources;
> >    }
> >  
> > -  importer_.reset(Importer::CreateInstance(&drm_));
> > -  if (!importer_) {
> > -    ALOGE("Failed to create importer instance");
> > +  DrmResources *drm = resource_manager_.GetDrmResources(HWC_DISPLAY_PRIMARY);
> > +  std::shared_ptr<Importer> importer =
> > +      resource_manager_.GetImporter(HWC_DISPLAY_PRIMARY);
> > +  if (!drm || !importer) {
> > +    ALOGE("Failed to get a valid drmresource and importer");
> >      return HWC2::Error::NoResources;
> >    }
> > +  displays_.emplace(
> > +      std::piecewise_construct, std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
> > +      std::forward_as_tuple(drm, importer, resource_manager_.GetGralloc(),
> > +                            HWC_DISPLAY_PRIMARY, HWC2::DisplayType::Physical));
> >  
> > -  ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> > -                      (const hw_module_t **)&gralloc_);
> > -  if (ret) {
> > -    ALOGE("Failed to open gralloc module %d", ret);
> > -    return HWC2::Error::NoResources;
> > -  }
> > -
> > -  displays_.emplace(std::piecewise_construct,
> > -                    std::forward_as_tuple(HWC_DISPLAY_PRIMARY),
> > -                    std::forward_as_tuple(&drm_, importer_, gralloc_,
> > -                                          HWC_DISPLAY_PRIMARY,
> > -                                          HWC2::DisplayType::Physical));
> > -
> > -  DrmCrtc *crtc = drm_.GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
> > +  DrmCrtc *crtc = drm->GetCrtcForDisplay(static_cast<int>(HWC_DISPLAY_PRIMARY));
> >    if (!crtc) {
> >      ALOGE("Failed to get crtc for display %d",
> >            static_cast<int>(HWC_DISPLAY_PRIMARY));
> >      return HWC2::Error::BadDisplay;
> >    }
> > -
> >    std::vector<DrmPlane *> display_planes;
> > -  for (auto &plane : drm_.planes()) {
> > +  for (auto &plane : drm->planes()) {
> >      if (plane->GetCrtcSupported(*crtc))
> >        display_planes.push_back(plane.get());
> >    }
> > diff --git a/drmhwctwo.h b/drmhwctwo.h
> > index 0490e2a..beb5d2d 100644
> > --- a/drmhwctwo.h
> > +++ b/drmhwctwo.h
> > @@ -262,9 +262,7 @@ class DrmHwcTwo : public hwc2_device_t {
> >    HWC2::Error RegisterCallback(int32_t descriptor, hwc2_callback_data_t data,
> >                                 hwc2_function_pointer_t function);
> >  
> > -  DrmResources drm_;
> > -  std::shared_ptr<Importer> importer_;  // Shared with HwcDisplay
> > -  const gralloc_module_t *gralloc_;
> > +  ResourceManager resource_manager_;
> >    std::map<hwc2_display_t, HwcDisplay> displays_;
> >    std::map<HWC2::Callback, HwcCallback> callbacks_;
> >  };
> > diff --git a/drmresources.cpp b/drmresources.cpp
> > index 32dd376..a5ddda0 100644
> > --- a/drmresources.cpp
> > +++ b/drmresources.cpp
> > @@ -42,10 +42,9 @@ DrmResources::~DrmResources() {
> >    event_listener_.Exit();
> >  }
> >  
> > -int DrmResources::Init() {
> > -  char path[PROPERTY_VALUE_MAX];
> > -  property_get("hwc.drm.device", path, "/dev/dri/card0");
> > -
> > +int DrmResources::Init(ResourceManager *resource_manager, char *path,
> > +                       int start_display_index) {
> > +  resource_manager_ = resource_manager;
> 
> You can avoid the backpointer if you just pass the RM to the right places (looks
> like compositor + composition). Bonus points if you can remove drm_ from those
> objects once you've done that.

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

> 
> >    /* TODO: Use drmOpenControl here instead */
> >    fd_.Set(open(path, O_RDWR));
> >    if (fd() < 0) {
> > @@ -76,8 +75,8 @@ int DrmResources::Init() {
> >    max_resolution_ =
> >        std::pair<uint32_t, uint32_t>(res->max_width, res->max_height);
> >  
> > -  bool found_primary = false;
> > -  int display_num = 1;
> > +  bool found_primary = start_display_index != 0;
> > +  int display_num = found_primary ? start_display_index : 1;
> 
> This could use a comment. AFAICT, you're assuming the primary display will
> always be in the first drm device, which is fine, but should be explained
> _somewhere_.

Will do.

> 
> >  
> >    for (int i = 0; !ret && i < res->count_crtcs; ++i) {
> >      drmModeCrtcPtr c = drmModeGetCrtc(fd(), res->crtcs[i]);
> > @@ -161,9 +160,11 @@ int DrmResources::Init() {
> >    for (auto &conn : connectors_) {
> >      if (conn->internal() && !found_primary) {
> >        conn->set_display(0);
> > +      displays_[0] = 0;
> >        found_primary = true;
> >      } else {
> >        conn->set_display(display_num);
> > +      displays_[display_num] = display_num;
> >        ++display_num;
> >      }
> >    }
> > @@ -171,7 +172,9 @@ int DrmResources::Init() {
> >    // Then look for primary amongst external connectors
> >    for (auto &conn : connectors_) {
> >      if (conn->external() && !found_primary) {
> > +      displays_.erase(conn->display());
> >        conn->set_display(0);
> > +      displays_[0] = 0;
> >        found_primary = true;
> >      }
> >    }
> > @@ -226,7 +229,11 @@ int DrmResources::Init() {
> >        return ret;
> >      }
> >    }
> > -  return 0;
> > +  return displays_.size() ? displays_.rbegin()->first : -EINVAL;
> 
> I'd rather not change the meaning of the return value (especially without a
> comment somewhere to let readers know this function doesn't follow the 0 ||
> -ERRNO convention). Consider returning a pair of ret,display.

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

> 
> > +}
> > +
> > +bool DrmResources::HandlesDisplay(int display) const {
> > +  return displays_.find(display) != displays_.end();
> >  }
> >  
> >  DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
> > @@ -349,6 +356,10 @@ DrmEventListener *DrmResources::event_listener() {
> >    return &event_listener_;
> >  }
> >  
> > +ResourceManager *DrmResources::resource_manager() {
> > +  return resource_manager_;
> > +}
> > +
> >  int DrmResources::GetProperty(uint32_t obj_id, uint32_t obj_type,
> >                                const char *prop_name, DrmProperty *property) {
> >    drmModeObjectPropertiesPtr props;
> > diff --git a/drmresources.h b/drmresources.h
> > index 4cca48c..4cdcd87 100644
> > --- a/drmresources.h
> > +++ b/drmresources.h
> > @@ -17,22 +17,26 @@
> >  #ifndef ANDROID_DRM_H_
> >  #define ANDROID_DRM_H_
> >  
> > +#include <stdint.h>
> >  #include "drmconnector.h"
> >  #include "drmcrtc.h"
> >  #include "drmencoder.h"
> >  #include "drmeventlistener.h"
> >  #include "drmplane.h"
> > -
> > -#include <stdint.h>
> 
> Why this change?

I blame clang-format-diff-3.8 -i. I suppose it should be in a different
commit.
> 
> > +#include "platform.h"
> > +#include "resourcemanager.h"
> >  
> >  namespace android {
> >  
> > +class ResourceManager;
> > +
> >  class DrmResources {
> >   public:
> >    DrmResources();
> >    ~DrmResources();
> >  
> > -  int Init();
> > +  int Init(ResourceManager *resource_manager, char *path,
> > +           int start_display_index);
> >  
> >    int fd() const {
> >      return fd_.get();
> > @@ -58,6 +62,7 @@ class DrmResources {
> >    DrmCrtc *GetCrtcForDisplay(int display) const;
> >    DrmPlane *GetPlane(uint32_t id) const;
> >    DrmEventListener *event_listener();
> > +  ResourceManager *resource_manager();
> >  
> >    int GetPlaneProperty(const DrmPlane &plane, const char *prop_name,
> >                         DrmProperty *property);
> > @@ -71,6 +76,7 @@ class DrmResources {
> >  
> >    int CreatePropertyBlob(void *data, size_t length, uint32_t *blob_id);
> >    int DestroyPropertyBlob(uint32_t blob_id);
> > +  bool HandlesDisplay(int display) const;
> >  
> >   private:
> >    int TryEncoderForDisplay(int display, DrmEncoder *enc);
> > @@ -90,6 +96,8 @@ class DrmResources {
> >  
> >    std::pair<uint32_t, uint32_t> min_resolution_;
> >    std::pair<uint32_t, uint32_t> max_resolution_;
> > +  std::map<int, int> displays_;
> > +  ResourceManager *resource_manager_;
> >  };
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 01/18] drm_hwcomposer: vsyncworker: Fix uninitialized enabled_ field

[Sean Paul]

On Mon, Apr 16, 2018 at 01:18:53PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Mon, Apr 16, 2018 at 12:30:13PM +0200, Robert Foss wrote:
> 
> Hi Rob,
> 
> Thanks for the review.
> 
> > Hey Alexandru,
> > 
> > Feel free to add:
> > Signed-off-by: Robert Foss <robert.foss@collabora.com>
> >
> 
> Should I re-send this 3 patches or could you just pushed them to
> master and adding your SoB in the process?

I've pushed patches 1 & 3 with Robert's Ack. I'll review the rest this morning
at which time you can send a v2 with the unapplied patches. Alternatively, if
you want to try out the new gitlab flow, you can send v2 via a merge request
there.

Sean

>  
> > 
> > Rob.
> > 
> > On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:
> > >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > >---
> > >  vsyncworker.cpp | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > >diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> > >index 3ad16fe..3bfe4be 100644
> > >--- a/vsyncworker.cpp
> > >+++ b/vsyncworker.cpp
> > >@@ -35,6 +35,7 @@ VSyncWorker::VSyncWorker()
> > >      : Worker("vsync", HAL_PRIORITY_URGENT_DISPLAY),
> > >        drm_(NULL),
> > >        display_(-1),
> > >+      enabled_(false),
> > >        last_timestamp_(-1) {
> > >  }
> > >
> 
> -- 
> Cheers,
> Alex G

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

Re: [PATCH hwc v2 06/18] drm_hwcomposer: Add writeback connector support

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 03:59:07PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:17PM +0100, Alexandru Gheorghe wrote:
> > Writeback connector is a special case of connector, which can be
> > linked to a CRTC in order to get the result of the composition back to
> > a memory buffer. This had not been merged to the mainline kernel yet,
> > latest version of the kernel patches could be found here [1].
> > 
> > [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/167703.html
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmconnector.cpp | 42 +++++++++++++++++++++++++++++++++++++++++-
> >  drmconnector.h   |  7 +++++++
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drmconnector.cpp b/drmconnector.cpp
> > index 145518f..e482832 100644
> > --- a/drmconnector.cpp
> > +++ b/drmconnector.cpp
> > @@ -52,6 +52,26 @@ int DrmConnector::Init() {
> >      ALOGE("Could not get CRTC_ID property\n");
> >      return ret;
> >    }
> > +  if (writeback()) {
> > +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_PIXEL_FORMATS",
> > +                                     &writeback_pixel_formats_);
> > +    if (ret) {
> > +      ALOGE("Could not get WRITEBACK_PIXEL_FORMATS connector_id = %d\n", id_);
> > +      return ret;
> > +    }
> > +    ret =
> > +        drm_->GetConnectorProperty(*this, "WRITEBACK_FB_ID", &writeback_fb_id_);
> 
> Please break on the function arguments instead of the =

It's clang-format-diff-3.8 fault again.
Will do.

> 
> 
> > +    if (ret) {
> > +      ALOGE("Could not get WRITEBACK_FB_ID connector_id = %d\n", id_);
> > +      return ret;
> > +    }
> > +    ret = drm_->GetConnectorProperty(*this, "WRITEBACK_OUT_FENCE_PTR",
> > +                                     &writeback_out_fence_);
> 
> Like this :)

That's me :).

> 
> With that,
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
> 
> 
> 
> > +    if (ret) {
> > +      ALOGE("Could not get WRITEBACK_OUT_FENCE_PTR connector_id = %d\n", id_);
> > +      return ret;
> > +    }
> > +  }
> >    return 0;
> >  }
> >  
> > @@ -78,8 +98,16 @@ bool DrmConnector::external() const {
> >           type_ == DRM_MODE_CONNECTOR_VGA;
> >  }
> >  
> > +bool DrmConnector::writeback() const {
> > +#ifdef DRM_MODE_CONNECTOR_WRITEBACK
> > +  return type_ == DRM_MODE_CONNECTOR_WRITEBACK;
> > +#else
> > +  return false;
> > +#endif
> > +}
> > +
> >  bool DrmConnector::valid_type() const {
> > -  return internal() || external();
> > +  return internal() || external() || writeback();
> >  }
> >  
> >  int DrmConnector::UpdateModes() {
> > @@ -130,6 +158,18 @@ const DrmProperty &DrmConnector::crtc_id_property() const {
> >    return crtc_id_property_;
> >  }
> >  
> > +const DrmProperty &DrmConnector::writeback_pixel_formats() const {
> > +  return writeback_pixel_formats_;
> > +}
> > +
> > +const DrmProperty &DrmConnector::writeback_fb_id() const {
> > +  return writeback_fb_id_;
> > +}
> > +
> > +const DrmProperty &DrmConnector::writeback_out_fence() const {
> > +  return writeback_out_fence_;
> > +}
> > +
> >  DrmEncoder *DrmConnector::encoder() const {
> >    return encoder_;
> >  }
> > diff --git a/drmconnector.h b/drmconnector.h
> > index 5601e06..e139730 100644
> > --- a/drmconnector.h
> > +++ b/drmconnector.h
> > @@ -46,6 +46,7 @@ class DrmConnector {
> >  
> >    bool internal() const;
> >    bool external() const;
> > +  bool writeback() const;
> >    bool valid_type() const;
> >  
> >    int UpdateModes();
> > @@ -58,6 +59,9 @@ class DrmConnector {
> >  
> >    const DrmProperty &dpms_property() const;
> >    const DrmProperty &crtc_id_property() const;
> > +  const DrmProperty &writeback_pixel_formats() const;
> > +  const DrmProperty &writeback_fb_id() const;
> > +  const DrmProperty &writeback_out_fence() const;
> >  
> >    const std::vector<DrmEncoder *> &possible_encoders() const {
> >      return possible_encoders_;
> > @@ -88,6 +92,9 @@ class DrmConnector {
> >  
> >    DrmProperty dpms_property_;
> >    DrmProperty crtc_id_property_;
> > +  DrmProperty writeback_pixel_formats_;
> > +  DrmProperty writeback_fb_id_;
> > +  DrmProperty writeback_out_fence_;
> >  
> >    std::vector<DrmEncoder *> possible_encoders_;
> >  };
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 07/18] drm_hwcomposer: Add display field to Drmencoder

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 04:02:07PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:18PM +0100, Alexandru Gheorghe wrote:
> > In the current implementation TryEncoderForDisplay just looks
> > at the crtc linked to the display, if that's not assigned to
> > a display it means the encoder could be used, otherwise iterate
> > to the list of possible_crtcs and find one which is not used.
> > 
> > This logic works fine when you have just one encoder connected to a
> > crtc but with two or more, like is the case when we attach a writeback
> > connector, we need to know if we already assigned the encoder to a
> > display.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmencoder.cpp | 14 ++++++++++++++
> >  drmencoder.h   |  4 ++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/drmencoder.cpp b/drmencoder.cpp
> > index 3d762f3..1da7ec3 100644
> > --- a/drmencoder.cpp
> > +++ b/drmencoder.cpp
> > @@ -27,6 +27,7 @@ DrmEncoder::DrmEncoder(drmModeEncoderPtr e, DrmCrtc *current_crtc,
> >                         const std::vector<DrmCrtc *> &possible_crtcs)
> >      : id_(e->encoder_id),
> >        crtc_(current_crtc),
> > +      display_(-1),
> >        possible_crtcs_(possible_crtcs) {
> >  }
> >  
> > @@ -40,5 +41,18 @@ DrmCrtc *DrmEncoder::crtc() const {
> >  
> >  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
> >    crtc_ = crtc;
> > +  set_display(crtc->display());
> > +}
> > +
> > +int DrmEncoder::display() const {
> > +  return display_;
> > +}
> > +
> > +void DrmEncoder::set_display(int display) {
> > +  display_ = display;
> > +}
> 
> Instead of adding this, just call set_crtc() in TryEncoderForDisplay() for the
> already-bound case. That way we only have one entry point for this.

Fair enough, I will remove set_display.

> 
> > +
> > +bool DrmEncoder::can_bind(int display) const {
> > +  return display_ == -1 || display_ == display;
> >  }
> >  }
> > diff --git a/drmencoder.h b/drmencoder.h
> > index 58ccbfb..7e06691 100644
> > --- a/drmencoder.h
> > +++ b/drmencoder.h
> > @@ -36,6 +36,9 @@ class DrmEncoder {
> >  
> >    DrmCrtc *crtc() const;
> >    void set_crtc(DrmCrtc *crtc);
> > +  bool can_bind(int display) const;
> > +  void set_display(int display);
> > +  int display() const;
> >  
> >    const std::vector<DrmCrtc *> &possible_crtcs() const {
> >      return possible_crtcs_;
> > @@ -44,6 +47,7 @@ class DrmEncoder {
> >   private:
> >    uint32_t id_;
> >    DrmCrtc *crtc_;
> > +  int display_;
> >  
> >    std::vector<DrmCrtc *> possible_crtcs_;
> >  };
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 08/18] drm_hwcomposer: Parse and store possible_clones information

[Alexandru-Cosmin Gheorghe]

Hi Sean,

On Mon, Apr 16, 2018 at 04:19:14PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:19PM +0100, Alexandru Gheorghe wrote:
> > drmModeEncoder has a field called possible_clones. It's a bit mask
> > which tells if the encoder could be simultaneously connected, to the
> > same CRTC, with the encoders specified in the possible_clones mask.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmencoder.cpp   | 8 ++++++++
> >  drmencoder.h     | 4 ++++
> >  drmresources.cpp | 9 ++++++++-
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drmencoder.cpp b/drmencoder.cpp
> > index 1da7ec3..ff675f5 100644
> > --- a/drmencoder.cpp
> > +++ b/drmencoder.cpp
> > @@ -39,6 +39,14 @@ DrmCrtc *DrmEncoder::crtc() const {
> >    return crtc_;
> >  }
> >  
> > +bool DrmEncoder::can_clone(DrmEncoder *encoder) {
> > +  return possible_clones_.find(encoder) != possible_clones_.end();
> > +}
> 
> The find() call is probably enough to justify CamelCase for this function. FTR,
> I _hate_ this part of the style guide and wish I had just picked one or the
> other.
> 
> To improve readability, can you also change the name of "encoder" to
> "possible_clone" like below so it's super obvious what this does?
> 

Sure, will do.

> > +
> > +void DrmEncoder::add_possible_clone(DrmEncoder *possible_clone) {
> > +  possible_clones_[possible_clone] = true;
> > +}
> > +
> >  void DrmEncoder::set_crtc(DrmCrtc *crtc) {
> >    crtc_ = crtc;
> >    set_display(crtc->display());
> > diff --git a/drmencoder.h b/drmencoder.h
> > index 7e06691..5e7c010 100644
> > --- a/drmencoder.h
> > +++ b/drmencoder.h
> > @@ -21,6 +21,7 @@
> >  
> >  #include <stdint.h>
> >  #include <vector>
> > +#include <map>
> 
> Alphabetical

Sure, wil do.

> 
> >  #include <xf86drmMode.h>
> >  
> >  namespace android {
> > @@ -43,6 +44,8 @@ class DrmEncoder {
> >    const std::vector<DrmCrtc *> &possible_crtcs() const {
> >      return possible_crtcs_;
> >    }
> > +  bool can_clone(DrmEncoder *encoder);
> > +  void add_possible_clone(DrmEncoder *possible_clone);
> >  
> >   private:
> >    uint32_t id_;
> > @@ -50,6 +53,7 @@ class DrmEncoder {
> >    int display_;
> >  
> >    std::vector<DrmCrtc *> possible_crtcs_;
> > +  std::map<DrmEncoder *, bool> possible_clones_;
> >  };
> >  }
> >  
> > diff --git a/drmresources.cpp b/drmresources.cpp
> > index a5ddda0..39f50be 100644
> > --- a/drmresources.cpp
> > +++ b/drmresources.cpp
> > @@ -97,6 +97,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
> >      crtcs_.emplace_back(std::move(crtc));
> >    }
> >  
> > +  std::vector<int> possible_clones;
> >    for (int i = 0; !ret && i < res->count_encoders; ++i) {
> >      drmModeEncoderPtr e = drmModeGetEncoder(fd(), res->encoders[i]);
> >      if (!e) {
> > @@ -117,12 +118,18 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
> >  
> >      std::unique_ptr<DrmEncoder> enc(
> >          new DrmEncoder(e, current_crtc, possible_crtcs));
> > -
> > +    possible_clones.push_back(e->possible_clones);
> >      drmModeFreeEncoder(e);
> >  
> >      encoders_.emplace_back(std::move(enc));
> >    }
> >  
> > +  for (uint32_t i = 0; i < encoders_.size(); i++) {
> > +    for (uint32_t j = 0; j < encoders_.size(); j++)
> 
> for (auto &enc: encoders_) {
>   for (auto &clone: encoders_) {
> 
> Or something similarly C++'y, looping through indices is sooo last decade :-)

Oldie but goldie. 
I do need the index in order to check the possible clones mask.
I will try to find something more millennial/Generation z :).

> 
> 
> > +      if (possible_clones[i] & (1 << j))
> > +        encoders_[i]->add_possible_clone(encoders_[j].get());
> > +  }
> > +
> >    for (int i = 0; !ret && i < res->count_connectors; ++i) {
> >      drmModeConnectorPtr c = drmModeGetConnector(fd(), res->connectors[i]);
> >      if (!c) {
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 01/18] drm_hwcomposer: vsyncworker: Fix uninitialized enabled_ field

[Alexandru-Cosmin Gheorghe]

On Tue, Apr 17, 2018 at 09:45:06AM -0400, Sean Paul wrote:
> On Mon, Apr 16, 2018 at 01:18:53PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > On Mon, Apr 16, 2018 at 12:30:13PM +0200, Robert Foss wrote:
> > 
> > Hi Rob,
> > 
> > Thanks for the review.
> > 
> > > Hey Alexandru,
> > > 
> > > Feel free to add:
> > > Signed-off-by: Robert Foss <robert.foss@collabora.com>
> > >
> > 
> > Should I re-send this 3 patches or could you just pushed them to
> > master and adding your SoB in the process?
> 
> I've pushed patches 1 & 3 with Robert's Ack. I'll review the rest this morning
> at which time you can send a v2 with the unapplied patches. Alternatively, if
> you want to try out the new gitlab flow, you can send v2 via a merge request
> there.

Thanks.

I'm all for trying new things. So, I will give gitlab flow a chance.

> 
> Sean
> 
> >  
> > > 
> > > Rob.
> > > 
> > > On 04/11/2018 05:22 PM, Alexandru Gheorghe wrote:
> > > >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > >---
> > > >  vsyncworker.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > >diff --git a/vsyncworker.cpp b/vsyncworker.cpp
> > > >index 3ad16fe..3bfe4be 100644
> > > >--- a/vsyncworker.cpp
> > > >+++ b/vsyncworker.cpp
> > > >@@ -35,6 +35,7 @@ VSyncWorker::VSyncWorker()
> > > >      : Worker("vsync", HAL_PRIORITY_URGENT_DISPLAY),
> > > >        drm_(NULL),
> > > >        display_(-1),
> > > >+      enabled_(false),
> > > >        last_timestamp_(-1) {
> > > >  }
> > > >
> > 
> > -- 
> > Cheers,
> > Alex G
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

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

[Sean Paul]

On Tue, Apr 17, 2018 at 02:43:17PM +0100, Alexandru-Cosmin Gheorghe wrote:
> Hi Sean,
> 
> On Mon, Apr 16, 2018 at 03:54:02PM -0400, Sean Paul wrote:
> > On Wed, Apr 11, 2018 at 04:22:16PM +0100, Alexandru Gheorghe wrote:
> > > Use the newly added ResourceManager for creating and detecting all the
> > > drm devices instead of assuming that there is only one device.
> > > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > ---

<snip />

> > > diff --git a/drmresources.cpp b/drmresources.cpp
> > > index 32dd376..a5ddda0 100644
> > > --- a/drmresources.cpp
> > > +++ b/drmresources.cpp
> > > @@ -42,10 +42,9 @@ DrmResources::~DrmResources() {
> > >    event_listener_.Exit();
> > >  }
> > >  
> > > -int DrmResources::Init() {
> > > -  char path[PROPERTY_VALUE_MAX];
> > > -  property_get("hwc.drm.device", path, "/dev/dri/card0");
> > > -
> > > +int DrmResources::Init(ResourceManager *resource_manager, char *path,
> > > +                       int start_display_index) {
> > > +  resource_manager_ = resource_manager;
> > 
> > You can avoid the backpointer if you just pass the RM to the right places (looks
> > like compositor + composition). Bonus points if you can remove drm_ from those
> > objects once you've done that.
> 
> That's the thing Compositor/Composition already had drm_, hence the
> need of the backpointer. Didn't want to touch that as well. I suppose
> there is no strong reason why both Compositor & Composition shouldn't
> have just a ResourceManager object.

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

<snip />

> > > diff --git a/drmresources.h b/drmresources.h
> > > index 4cca48c..4cdcd87 100644
> > > --- a/drmresources.h
> > > +++ b/drmresources.h
> > > @@ -17,22 +17,26 @@
> > >  #ifndef ANDROID_DRM_H_
> > >  #define ANDROID_DRM_H_
> > >  
> > > +#include <stdint.h>
> > >  #include "drmconnector.h"
> > >  #include "drmcrtc.h"
> > >  #include "drmencoder.h"
> > >  #include "drmeventlistener.h"
> > >  #include "drmplane.h"
> > > -
> > > -#include <stdint.h>
> > 
> > Why this change?
> 
> I blame clang-format-diff-3.8 -i. I suppose it should be in a different
> commit.

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

Sean

> > 
> > > +#include "platform.h"
> > > +#include "resourcemanager.h"
> > >  
> > >  namespace android {
> > >  
> > > +class ResourceManager;
> > > +
> > >  class DrmResources {
> > >   public:
> > >    DrmResources();
> > >    ~DrmResources();
> > >  
> > > -  int Init();
> > > +  int Init(ResourceManager *resource_manager, char *path,
> > > +           int start_display_index);
> > >  
> > >    int fd() const {
> > >      return fd_.get();
> > > @@ -58,6 +62,7 @@ class DrmResources {
> > >    DrmCrtc *GetCrtcForDisplay(int display) const;
> > >    DrmPlane *GetPlane(uint32_t id) const;
> > >    DrmEventListener *event_listener();
> > > +  ResourceManager *resource_manager();
> > >  
> > >    int GetPlaneProperty(const DrmPlane &plane, const char *prop_name,
> > >                         DrmProperty *property);
> > > @@ -71,6 +76,7 @@ class DrmResources {
> > >  
> > >    int CreatePropertyBlob(void *data, size_t length, uint32_t *blob_id);
> > >    int DestroyPropertyBlob(uint32_t blob_id);
> > > +  bool HandlesDisplay(int display) const;
> > >  
> > >   private:
> > >    int TryEncoderForDisplay(int display, DrmEncoder *enc);
> > > @@ -90,6 +96,8 @@ class DrmResources {
> > >  
> > >    std::pair<uint32_t, uint32_t> min_resolution_;
> > >    std::pair<uint32_t, uint32_t> max_resolution_;
> > > +  std::map<int, int> displays_;
> > > +  ResourceManager *resource_manager_;
> > >  };
> > >  }
> > >  
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

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

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

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:16PM +0100, Alexandru Gheorghe wrote:
> Use the newly added ResourceManager for creating and detecting all the
> drm devices instead of assuming that there is only one device.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---

<snip />

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

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

Sean

>   public:
>    DrmResources();
>    ~DrmResources();
>  
> -  int Init();
> +  int Init(ResourceManager *resource_manager, char *path,
> +           int start_display_index);
>  
>    int fd() const {
>      return fd_.get();
> @@ -58,6 +62,7 @@ class DrmResources {
>    DrmCrtc *GetCrtcForDisplay(int display) const;
>    DrmPlane *GetPlane(uint32_t id) const;
>    DrmEventListener *event_listener();
> +  ResourceManager *resource_manager();
>  
>    int GetPlaneProperty(const DrmPlane &plane, const char *prop_name,
>                         DrmProperty *property);
> @@ -71,6 +76,7 @@ class DrmResources {
>  
>    int CreatePropertyBlob(void *data, size_t length, uint32_t *blob_id);
>    int DestroyPropertyBlob(uint32_t blob_id);
> +  bool HandlesDisplay(int display) const;
>  
>   private:
>    int TryEncoderForDisplay(int display, DrmEncoder *enc);
> @@ -90,6 +96,8 @@ class DrmResources {
>  
>    std::pair<uint32_t, uint32_t> min_resolution_;
>    std::pair<uint32_t, uint32_t> max_resolution_;
> +  std::map<int, int> displays_;
> +  ResourceManager *resource_manager_;
>  };
>  }
>  
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:15PM +0100, Alexandru Gheorghe wrote:
> Add a resource manager object that is responsible for detecting all
> kms devices and allocates unique display numbers for every detected
> display.
> 
> This is controlled by the value of hwc.drm.device property, if it ends
> with a %, it will try to open minor devices until and error is detected.
> E.g: /dev/dri/card%

I'm a bit on the fence as to whether to use the %, or add a new
hwc.drm.scan_devices property. I guess since we need to convey the path anyways
this is fine, but it really needs to be better documented. 

> 
> Additionally, this will be used for finding an available writeback
> connector that will be used for the flattening of the currently
> displayed scene.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  Android.mk          |  1 +
>  resourcemanager.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  resourcemanager.h   | 29 ++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 resourcemanager.cpp
>  create mode 100644 resourcemanager.h
> 
> diff --git a/Android.mk b/Android.mk
> index 1add286..736fe24 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -52,6 +52,7 @@ LOCAL_C_INCLUDES := \
>  
>  LOCAL_SRC_FILES := \
>  	autolock.cpp \
> +	resourcemanager.cpp \
>  	drmresources.cpp \
>  	drmconnector.cpp \
>  	drmcrtc.cpp \
> diff --git a/resourcemanager.cpp b/resourcemanager.cpp
> new file mode 100644
> index 0000000..e7b654e
> --- /dev/null
> +++ b/resourcemanager.cpp
> @@ -0,0 +1,71 @@
> +#include "resourcemanager.h"
> +#include <cutils/log.h>
> +#include <cutils/properties.h>
> +
> +namespace android {
> +
> +ResourceManager::ResourceManager() : gralloc_(NULL) {
> +}
> +
> +int ResourceManager::Init() {
> +  char path_pattern[PROPERTY_VALUE_MAX];
> +  property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");

We probably also don't want to default to scanning, since that might change
behavior in existing boards.

> +
> +  uint8_t minor = 0;

Please use unsigned/int instead of fixed-size types. Unless the number of bits
is relevant to use of the variable, let the compiler handle it.

> +  int last_display_index = 0;

Could we just call this num_displays? It was kind of hard for me to reason
through this, especially when it's call "start_display_index" when you jump into
drm::Init(). I also think drm->Init's return pair should return 
<ret, displays_added> instead of <ret, display_index>, so each time you call
Init(), you're adding to num_displays.

> +  int last_char = strlen(path_pattern) - 1;
> +  do {
> +    char path[PROPERTY_VALUE_MAX];

Please use string

> +    if (path_pattern[last_char] == '%') {
> +      path_pattern[last_char] = '\0';
> +      snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor);
> +      path_pattern[last_char] = '%';

This doesn't work for minor > 10.

> +    } else {
> +      snprintf(path, PROPERTY_VALUE_MAX, "%s", path_pattern);
> +    }
> +    std::unique_ptr<DrmResources> drm = std::make_unique<DrmResources>();
> +    last_display_index = drm->Init(this, path, last_display_index);
> +    if (last_display_index < 0) {
> +      break;
> +    }
> +    std::shared_ptr<Importer> importer;
> +    importer.reset(Importer::CreateInstance(drm.get()));
> +    if (!importer) {
> +      ALOGE("Failed to create importer instance");
> +      break;
> +    }
> +    importers_.push_back(importer);
> +    drms_.push_back(std::move(drm));
> +    minor++;
> +    last_display_index++;
> +  } while (path_pattern[last_char] == '%');
> +
> +  if (!drms_.size()) {
> +    ALOGE("Failed to find any working drm device");
> +    return -EINVAL;
> +  }
> +
> +  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> +                       (const hw_module_t **)&gralloc_);
> +}

Consider the following:

int ResourceManager::AddDrmDevice(std::string path) {
  std::unique_ptr<DrmDevice> drm = std::make_unique<DrmDevice>();
  int displays_added, ret;
  std::tie(displays_added, ret) = drm->Init(path.c_str(), num_displays_);
  if (ret)
    return ret;

  std::shared_ptr<Importer> importer;
  importer.reset(Importer::CreateInstance(drm.get()));
  if (!importer) {
    ALOGE("Failed to create importer instance");
    return -ENODEV;
  }

  importers_.push_back(importer);
  drms_.push_back(std::move(drm));
  num_displays_ += displays_added;
  return 0;
}

int ResourceManager::Init() {
  char path_pattern[PROPERTY_VALUE_MAX];
  int path_len = property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");

  if (path_pattern[path_len - 1] != '%')
    return AddDrmDevice(std::string(path_pattern);

  path_pattern[path_len - 1] = '\0';
  for (int ret = 0, idx = 0; !ret; ++idx) {
    ostringstream path;
    path << path_pattern << idx;
    ret = AddDrmDevice(path.str());
  }
  if (!num_displays_) {
    ALOGE("Failed to initialize any displays");
    return -EINVAL;
  }

  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
                       (const hw_module_t **)&gralloc_);
}

I think resolves the issues from the original patches and incorporates the
suggestions of drm->Init() returning the tuple of added displays, as well as
eliminating the backpointer.


> +
> +DrmResources *ResourceManager::GetDrmResources(int display) {
> +  for (uint32_t i = 0; i < drms_.size(); i++) {

for (auto &drm_: drms_) {

> +    if (drms_[i]->HandlesDisplay(display))
> +      return drms_[i].get();
> +  }
> +  return NULL;
> +}
> +
> +std::shared_ptr<Importer> ResourceManager::GetImporter(int display) {
> +  for (uint32_t i = 0; i < drms_.size(); i++) {

Same here 

> +    if (drms_[i]->HandlesDisplay(display))
> +      return importers_[i];
> +  }
> +  return NULL;
> +}
> +
> +const gralloc_module_t *ResourceManager::GetGralloc() {

I think this should be called gralloc()

> +  return gralloc_;
> +}
> +}
> diff --git a/resourcemanager.h b/resourcemanager.h
> new file mode 100644
> index 0000000..b8caa9a
> --- /dev/null
> +++ b/resourcemanager.h
> @@ -0,0 +1,29 @@
> +#ifndef RESOURCEMANAGER_H
> +#define RESOURCEMANAGER_H
> +
> +#include "drmresources.h"
> +#include "platform.h"
> +
> +namespace android {
> +
> +class DrmResources;
> +class Importer;

I think you need either the forward declarations or the includes, but not both?

> +
> +class ResourceManager {
> + public:
> +  ResourceManager();
> +  ResourceManager(const ResourceManager &) = delete;
> +  ResourceManager &operator=(const ResourceManager &) = delete;
> +  int Init();
> +  DrmResources *GetDrmResources(int display);
> +  std::shared_ptr<Importer> GetImporter(int display);
> +  const gralloc_module_t *GetGralloc();
> +
> + private:
> +  std::vector<std::unique_ptr<DrmResources>> drms_;
> +  std::vector<std::shared_ptr<Importer>> importers_;
> +  const gralloc_module_t *gralloc_;
> +};
> +}
> +
> +#endif  // RESOURCEMANAGER_H
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 09/18] drm_hwcomposer: Handle writeback connectors

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:20PM +0100, Alexandru Gheorghe wrote:
> When writeback connectors are available assign them to displays, in
> order to be able to use them for flattening of the current displayed
> scene. The pipeline for each display will look like this:
> 
> CRTC ---- encoder ------------ display connector.
>  |------- writeback enc ------ writeback connector.
> 
> However, the writeback connector will be later used/enabled only if
> one of the following conditions are met:
>  - Could be a clone of the display connector, as pointed by the
>    possible_clones property.
>  - The display_connector is disconnected, so we are safe to use it for
>    flattening the scene that's already presented on another display.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmresources.cpp | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  drmresources.h   |  3 +++
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/drmresources.cpp b/drmresources.cpp
> index 39f50be..fef6835 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -64,6 +64,14 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>      return ret;
>    }
>  
> +#ifdef DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> +  ret = drmSetClientCap(fd(), DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1);
> +  if (ret) {
> +    ALOGI("Failed to set writeback cap %d", ret);
> +    ret = 0;
> +  }
> +#endif
> +
>    drmModeResPtr res = drmModeGetResources(fd());
>    if (!res) {
>      ALOGE("Failed to get DrmResources resources");
> @@ -169,7 +177,7 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>        conn->set_display(0);
>        displays_[0] = 0;
>        found_primary = true;
> -    } else {
> +    } else if (conn->external()) {
>        conn->set_display(display_num);
>        displays_[display_num] = display_num;
>        ++display_num;
> @@ -230,6 +238,8 @@ int DrmResources::Init(ResourceManager *resource_manager, char *path,
>    }
>  
>    for (auto &conn : connectors_) {
> +    if (conn->writeback())
> +      continue;
>      ret = CreateDisplayPipe(conn.get());
>      if (ret) {
>        ALOGE("Failed CreateDisplayPipe %d with %d", conn->id(), ret);
> @@ -245,7 +255,15 @@ bool DrmResources::HandlesDisplay(int display) const {
>  
>  DrmConnector *DrmResources::GetConnectorForDisplay(int display) const {
>    for (auto &conn : connectors_) {
> -    if (conn->display() == display)
> +    if (conn->display() == display && !conn->writeback())
> +      return conn.get();
> +  }
> +  return NULL;
> +}
> +
> +DrmConnector *DrmResources::GetWritebackConnectorForDisplay(int display) const {
> +  for (auto &conn : connectors_) {
> +    if (conn->display() == display && conn->writeback())
>        return conn.get();
>    }
>    return NULL;
> @@ -280,6 +298,7 @@ int DrmResources::TryEncoderForDisplay(int display, DrmEncoder *enc) {
>    DrmCrtc *crtc = enc->crtc();
>    if (crtc && crtc->can_bind(display)) {
>      crtc->set_display(display);
> +    enc->set_display(display);
>      return 0;
>    }
>  
> @@ -306,6 +325,7 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>    if (connector->encoder()) {
>      int ret = TryEncoderForDisplay(display, connector->encoder());
>      if (!ret) {
> +      AttachWriteback(connector);

AttachWriteback returns int, but you throw it away here. Additionally,
AttachWriteback always follows a successful TryEncoderForDisplay, so it makes
sense to just call it from there.

>        return 0;
>      } else if (ret != -EAGAIN) {
>        ALOGE("Could not set mode %d/%d", display, ret);
> @@ -317,6 +337,7 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>      int ret = TryEncoderForDisplay(display, enc);
>      if (!ret) {
>        connector->set_encoder(enc);
> +      AttachWriteback(connector);
>        return 0;
>      } else if (ret != -EAGAIN) {
>        ALOGE("Could not set mode %d/%d", display, ret);
> @@ -328,6 +349,43 @@ int DrmResources::CreateDisplayPipe(DrmConnector *connector) {
>    return -ENODEV;
>  }
>  
> +/*
> + * Attach writeback connector to the CRTC linked to the display_conn
> + *
> + */
> +int DrmResources::AttachWriteback(DrmConnector *display_conn) {
> +  int ret = -EINVAL;

This isn't really used, just return the error code directly at the bottom.

> +  if (display_conn->writeback())
> +    return -EINVAL;

This condition would benefit from a log

> +  DrmEncoder *display_enc = display_conn->encoder();
> +  if (!display_enc)
> +    return -EINVAL;
> +  DrmCrtc *display_crtc = display_enc->crtc();
> +  if (!display_crtc)
> +    return -EINVAL;

Are these possible given this is only called after a successful
TryEncoderForDisplay()?

> +  if (GetWritebackConnectorForDisplay(display_crtc->display()) != NULL)
> +    return -EINVAL;

Again, logging would be useful.

> +  for (auto &writeback_conn : connectors_) {
> +    if (writeback_conn->display() >= 0 || !writeback_conn->writeback())

There doesn't seem to be any situation where you iterate through connectors_ and
you don't have some type of writeback() check. So it seems like it'd make sense
to track these in different vectors.

> +      continue;
> +    for (DrmEncoder *writeback_enc : writeback_conn->possible_encoders()) {
> +      for (DrmCrtc *possible_crtc : writeback_enc->possible_crtcs()) {
> +        if (possible_crtc != display_crtc)
> +          continue;
> +        // Use just encoders which had not been bound already
> +        if (writeback_enc->can_bind(display_crtc->display())) {
> +          writeback_enc->set_crtc(display_crtc);
> +          writeback_conn->set_encoder(writeback_enc);
> +          writeback_conn->set_display(display_crtc->display());
> +          writeback_conn->UpdateModes();
> +          return 0;
> +        }
> +      }
> +    }
> +  }
> +  return ret;
> +}
> +
>  int DrmResources::CreatePropertyBlob(void *data, size_t length,
>                                       uint32_t *blob_id) {
>    struct drm_mode_create_blob create_blob;
> diff --git a/drmresources.h b/drmresources.h
> index 4cdcd87..4fb17fc 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -59,6 +59,8 @@ class DrmResources {
>    }
>  
>    DrmConnector *GetConnectorForDisplay(int display) const;
> +  DrmConnector *GetWritebackConnectorForDisplay(int display) const;
> +  DrmConnector *FindWritebackConnector(int display) const;
>    DrmCrtc *GetCrtcForDisplay(int display) const;
>    DrmPlane *GetPlane(uint32_t id) const;
>    DrmEventListener *event_listener();
> @@ -84,6 +86,7 @@ class DrmResources {
>                    DrmProperty *property);
>  
>    int CreateDisplayPipe(DrmConnector *connector);
> +  int AttachWriteback(DrmConnector *display_conn);
>  
>    UniqueFd fd_;
>    uint32_t mode_id_ = 0;
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class

[Robert Foss]

Hey,

On 04/17/2018 05:33 PM, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:15PM +0100, Alexandru Gheorghe wrote:
>> Add a resource manager object that is responsible for detecting all
>> kms devices and allocates unique display numbers for every detected
>> display.
>>
>> This is controlled by the value of hwc.drm.device property, if it ends
>> with a %, it will try to open minor devices until and error is detected.
>> E.g: /dev/dri/card%
> 
> I'm a bit on the fence as to whether to use the %, or add a new
> hwc.drm.scan_devices property. I guess since we need to convey the path anyways
> this is fine, but it really needs to be better documented.

I'm looking at this stuff in another series about DRM Node probing[1],
and I'll look into using properties to define what we are looking for.

But those properties won't be paths, but rather PCI IDs and driver vendor names.

As for what to do in the series, I don't have much of an opinion. But I'm likely 
to try to change it in the not too distant future.


[1] https://www.spinics.net/lists/dri-devel/msg172496.html

> 
>>
>> Additionally, this will be used for finding an available writeback
>> connector that will be used for the flattening of the currently
>> displayed scene.
>>
>> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
>> ---
>>   Android.mk          |  1 +
>>   resourcemanager.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   resourcemanager.h   | 29 ++++++++++++++++++++++
>>   3 files changed, 101 insertions(+)
>>   create mode 100644 resourcemanager.cpp
>>   create mode 100644 resourcemanager.h
>>
>> diff --git a/Android.mk b/Android.mk
>> index 1add286..736fe24 100644
>> --- a/Android.mk
>> +++ b/Android.mk
>> @@ -52,6 +52,7 @@ LOCAL_C_INCLUDES := \
>>   
>>   LOCAL_SRC_FILES := \
>>   	autolock.cpp \
>> +	resourcemanager.cpp \
>>   	drmresources.cpp \
>>   	drmconnector.cpp \
>>   	drmcrtc.cpp \
>> diff --git a/resourcemanager.cpp b/resourcemanager.cpp
>> new file mode 100644
>> index 0000000..e7b654e
>> --- /dev/null
>> +++ b/resourcemanager.cpp
>> @@ -0,0 +1,71 @@
>> +#include "resourcemanager.h"
>> +#include <cutils/log.h>
>> +#include <cutils/properties.h>
>> +
>> +namespace android {
>> +
>> +ResourceManager::ResourceManager() : gralloc_(NULL) {
>> +}
>> +
>> +int ResourceManager::Init() {
>> +  char path_pattern[PROPERTY_VALUE_MAX];
>> +  property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
> 
> We probably also don't want to default to scanning, since that might change
> behavior in existing boards.
> 
>> +
>> +  uint8_t minor = 0;
> 
> Please use unsigned/int instead of fixed-size types. Unless the number of bits
> is relevant to use of the variable, let the compiler handle it.
> 
>> +  int last_display_index = 0;
> 
> Could we just call this num_displays? It was kind of hard for me to reason
> through this, especially when it's call "start_display_index" when you jump into
> drm::Init(). I also think drm->Init's return pair should return
> <ret, displays_added> instead of <ret, display_index>, so each time you call
> Init(), you're adding to num_displays.
> 
>> +  int last_char = strlen(path_pattern) - 1;
>> +  do {
>> +    char path[PROPERTY_VALUE_MAX];
> 
> Please use string
> 
>> +    if (path_pattern[last_char] == '%') {
>> +      path_pattern[last_char] = '\0';
>> +      snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor);
>> +      path_pattern[last_char] = '%';
> 
> This doesn't work for minor > 10.
> 
>> +    } else {
>> +      snprintf(path, PROPERTY_VALUE_MAX, "%s", path_pattern);
>> +    }
>> +    std::unique_ptr<DrmResources> drm = std::make_unique<DrmResources>();
>> +    last_display_index = drm->Init(this, path, last_display_index);
>> +    if (last_display_index < 0) {
>> +      break;
>> +    }
>> +    std::shared_ptr<Importer> importer;
>> +    importer.reset(Importer::CreateInstance(drm.get()));
>> +    if (!importer) {
>> +      ALOGE("Failed to create importer instance");
>> +      break;
>> +    }
>> +    importers_.push_back(importer);
>> +    drms_.push_back(std::move(drm));
>> +    minor++;
>> +    last_display_index++;
>> +  } while (path_pattern[last_char] == '%');
>> +
>> +  if (!drms_.size()) {
>> +    ALOGE("Failed to find any working drm device");
>> +    return -EINVAL;
>> +  }
>> +
>> +  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
>> +                       (const hw_module_t **)&gralloc_);
>> +}
> 
> Consider the following:
> 
> int ResourceManager::AddDrmDevice(std::string path) {
>    std::unique_ptr<DrmDevice> drm = std::make_unique<DrmDevice>();
>    int displays_added, ret;
>    std::tie(displays_added, ret) = drm->Init(path.c_str(), num_displays_);
>    if (ret)
>      return ret;
> 
>    std::shared_ptr<Importer> importer;
>    importer.reset(Importer::CreateInstance(drm.get()));
>    if (!importer) {
>      ALOGE("Failed to create importer instance");
>      return -ENODEV;
>    }
> 
>    importers_.push_back(importer);
>    drms_.push_back(std::move(drm));
>    num_displays_ += displays_added;
>    return 0;
> }
> 
> int ResourceManager::Init() {
>    char path_pattern[PROPERTY_VALUE_MAX];
>    int path_len = property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
> 
>    if (path_pattern[path_len - 1] != '%')
>      return AddDrmDevice(std::string(path_pattern);
> 
>    path_pattern[path_len - 1] = '\0';
>    for (int ret = 0, idx = 0; !ret; ++idx) {
>      ostringstream path;
>      path << path_pattern << idx;
>      ret = AddDrmDevice(path.str());
>    }
>    if (!num_displays_) {
>      ALOGE("Failed to initialize any displays");
>      return -EINVAL;
>    }
> 
>    return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
>                         (const hw_module_t **)&gralloc_);
> }
> 
> I think resolves the issues from the original patches and incorporates the
> suggestions of drm->Init() returning the tuple of added displays, as well as
> eliminating the backpointer.
> 
> 
>> +
>> +DrmResources *ResourceManager::GetDrmResources(int display) {
>> +  for (uint32_t i = 0; i < drms_.size(); i++) {
> 
> for (auto &drm_: drms_) {
> 
>> +    if (drms_[i]->HandlesDisplay(display))
>> +      return drms_[i].get();
>> +  }
>> +  return NULL;
>> +}
>> +
>> +std::shared_ptr<Importer> ResourceManager::GetImporter(int display) {
>> +  for (uint32_t i = 0; i < drms_.size(); i++) {
> 
> Same here
> 
>> +    if (drms_[i]->HandlesDisplay(display))
>> +      return importers_[i];
>> +  }
>> +  return NULL;
>> +}
>> +
>> +const gralloc_module_t *ResourceManager::GetGralloc() {
> 
> I think this should be called gralloc()
> 
>> +  return gralloc_;
>> +}
>> +}
>> diff --git a/resourcemanager.h b/resourcemanager.h
>> new file mode 100644
>> index 0000000..b8caa9a
>> --- /dev/null
>> +++ b/resourcemanager.h
>> @@ -0,0 +1,29 @@
>> +#ifndef RESOURCEMANAGER_H
>> +#define RESOURCEMANAGER_H
>> +
>> +#include "drmresources.h"
>> +#include "platform.h"
>> +
>> +namespace android {
>> +
>> +class DrmResources;
>> +class Importer;
> 
> I think you need either the forward declarations or the includes, but not both?
> 
>> +
>> +class ResourceManager {
>> + public:
>> +  ResourceManager();
>> +  ResourceManager(const ResourceManager &) = delete;
>> +  ResourceManager &operator=(const ResourceManager &) = delete;
>> +  int Init();
>> +  DrmResources *GetDrmResources(int display);
>> +  std::shared_ptr<Importer> GetImporter(int display);
>> +  const gralloc_module_t *GetGralloc();
>> +
>> + private:
>> +  std::vector<std::unique_ptr<DrmResources>> drms_;
>> +  std::vector<std::shared_ptr<Importer>> importers_;
>> +  const gralloc_module_t *gralloc_;
>> +};
>> +}
>> +
>> +#endif  // RESOURCEMANAGER_H
>> -- 
>> 2.7.4
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 10/18] drm_hwcomposer: hwcutils: Add function for cloning a DrmHwcLayer

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:21PM +0100, Alexandru Gheorghe wrote:
> When doing flattening of a composition on a different CRTC we need to be
> able to clone a layer in order to import it and then pass it to another CRTC.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmhwcomposer.h |  1 +
>  hwcutils.cpp    | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drmhwcomposer.h b/drmhwcomposer.h
> index f8440fb..b256caf 100644
> --- a/drmhwcomposer.h
> +++ b/drmhwcomposer.h
> @@ -150,6 +150,7 @@ struct DrmHwcLayer {
>  
>    int InitFromHwcLayer(hwc_layer_1_t *sf_layer, Importer *importer,
>                         const gralloc_module_t *gralloc);
> +  int PopulateFromDrmHwcLayer(DrmHwcLayer *layer);
>    int ImportBuffer(Importer *importer, const gralloc_module_t *gralloc);
>  
>    void SetTransform(int32_t sf_transform);
> diff --git a/hwcutils.cpp b/hwcutils.cpp
> index 53a7d82..ff37c3b 100644
> --- a/hwcutils.cpp
> +++ b/hwcutils.cpp
> @@ -149,6 +149,17 @@ int DrmHwcLayer::InitFromHwcLayer(hwc_layer_1_t *sf_layer, Importer *importer,
>    return ImportBuffer(importer, gralloc);
>  }
>  
> +int DrmHwcLayer::PopulateFromDrmHwcLayer(DrmHwcLayer *src_layer) {
> +  blending = src_layer->blending;
> +  sf_handle = src_layer->sf_handle;
> +  acquire_fence = dup(src_layer->acquire_fence.get());

Hmm, I think this is the only place where we duplicate a UniqueFd. I _think_
this will be Ok, but do you need to use this? Could you instead defer trying to
flatten if the original acquire_fence hasn't fired? Also, you don't check the
return value.

> +  display_frame = src_layer->display_frame;
> +  alpha = src_layer->alpha;
> +  source_crop = src_layer->source_crop;
> +  transform = src_layer->transform;

It also doesn't seem like you're populating all of the fields so the function
name is misleading.


> +  return 0;
> +}
> +
>  int DrmHwcLayer::ImportBuffer(Importer *importer,
>                                const gralloc_module_t *gralloc) {
>    int ret = buffer.ImportBuffer(sf_handle, importer);
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 11/18] drm_hwcomposer: Add utility functions to copy displaycomposition internals

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:22PM +0100, Alexandru Gheorghe wrote:
> Add utility functions to copy the DrmHwcLayer and DrmCompositionPlanes
> from another DrmDisplayComposition.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycomposition.cpp | 29 +++++++++++++++++++++++++++++
>  drmdisplaycomposition.h   |  3 +++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drmdisplaycomposition.cpp b/drmdisplaycomposition.cpp
> index 66e67a4..dd64f46 100644
> --- a/drmdisplaycomposition.cpp
> +++ b/drmdisplaycomposition.cpp
> @@ -99,6 +99,35 @@ int DrmDisplayComposition::SetLayers(DrmHwcLayer *layers, size_t num_layers,
>    return 0;
>  }
>  
> +int DrmDisplayComposition::CopyLayers(DrmDisplayComposition *src) {
> +  geometry_changed_ = true;
> +  type_ = DRM_COMPOSITION_TYPE_FRAME;
> +  std::shared_ptr<Importer> importer =
> +      drm_->resource_manager()->GetImporter(crtc()->display());
> +  if (!importer) {
> +    ALOGE("Failed to find a valid importer");
> +    return -EINVAL;
> +  }
> +  for (DrmHwcLayer &src_layer : src->layers()) {
> +    DrmHwcLayer copy;
> +    copy.PopulateFromDrmHwcLayer(&src_layer);
> +    int ret = copy.ImportBuffer(importer.get(),
> +                                drm_->resource_manager()->GetGralloc());
> +    if (ret) {
> +      ALOGE("Failed to import buffer ret = %d", ret);
> +      return -EINVAL;
> +    }
> +    layers_.emplace_back(std::move(copy));
> +  }
> +  return 0;
> +}

This seems to do more than just CopyLayers, and it seems quite specialized to
your purpose. Can you do something similar to SquashFrame(), where the new
composition is crafted in context? It might be a little less awkward than
sprinkling in these seemingly generic copy functions.

Sean

> +
> +void DrmDisplayComposition::CopyCompPlanes(DrmDisplayComposition *src) {
> +  for (auto comp_plane : src->composition_planes()) {
> +    composition_planes_.push_back(comp_plane);
> +  }
> +}
> +
>  int DrmDisplayComposition::SetDpmsMode(uint32_t dpms_mode) {
>    if (!validate_composition_type(DRM_COMPOSITION_TYPE_DPMS))
>      return -EINVAL;
> diff --git a/drmdisplaycomposition.h b/drmdisplaycomposition.h
> index 9183925..c646420 100644
> --- a/drmdisplaycomposition.h
> +++ b/drmdisplaycomposition.h
> @@ -68,6 +68,7 @@ class DrmCompositionPlane {
>  
>    DrmCompositionPlane() = default;
>    DrmCompositionPlane(DrmCompositionPlane &&rhs) = default;
> +  DrmCompositionPlane(const DrmCompositionPlane &rhs) = default;
>    DrmCompositionPlane &operator=(DrmCompositionPlane &&other) = default;
>    DrmCompositionPlane(Type type, DrmPlane *plane, DrmCrtc *crtc)
>        : type_(type), plane_(plane), crtc_(crtc) {
> @@ -120,6 +121,8 @@ class DrmDisplayComposition {
>             Planner *planner, uint64_t frame_no);
>  
>    int SetLayers(DrmHwcLayer *layers, size_t num_layers, bool geometry_changed);
> +  int CopyLayers(DrmDisplayComposition *src);
> +  void CopyCompPlanes(DrmDisplayComposition *src);
>    int AddPlaneComposition(DrmCompositionPlane plane);
>    int AddPlaneDisable(DrmPlane *plane);
>    int SetDpmsMode(uint32_t dpms_mode);
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 12/18] drm_hwcomposer: Add utility function to create an initialized composition

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:23PM +0100, Alexandru Gheorghe wrote:
> There is a lot of boilerplate for creating an initialized
> drmdisplaycomposition. This patch gathers that in a separate method.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycompositor.cpp | 23 +++++++++++++++++++++++
>  drmdisplaycompositor.h   |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index e556e86..6e5be24 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -221,6 +221,7 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
>      ALOGE("Failed to initialize drm compositor lock %d\n", ret);
>      return ret;
>    }
> +  planner_ = Planner::CreateInstance(drm);

What's this?

>  
>    initialized_ = true;
>    return 0;
> @@ -231,6 +232,28 @@ std::unique_ptr<DrmDisplayComposition> DrmDisplayCompositor::CreateComposition()
>    return std::unique_ptr<DrmDisplayComposition>(new DrmDisplayComposition());
>  }
>  
> +std::unique_ptr<DrmDisplayComposition>
> +DrmDisplayCompositor::CreateInitializedComposition() const {
> +  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
> +  if (!crtc) {
> +    ALOGE("Failed to find crtc for display = %d", display_);
> +    return std::unique_ptr<DrmDisplayComposition>();
> +  }
> +  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
> +  std::shared_ptr<Importer> importer =
> +      drm_->resource_manager()->GetImporter(display_);
> +  if (!importer) {
> +    ALOGE("Failed to find resources for display = %d", display_);
> +    return std::unique_ptr<DrmDisplayComposition>();
> +  }
> +  int ret = comp->Init(drm_, crtc, importer.get(), planner_.get(), 0);
> +  if (ret) {
> +    ALOGE("Failed to init composition for display = %d", display_);
> +    return std::unique_ptr<DrmDisplayComposition>();
> +  }
> +  return comp;
> +}
> +

This seems sufficiently small that you can squash it into the patch that uses
it. The same can be said for some of the other "Add function to do X" which
don't use the function.

>  std::tuple<uint32_t, uint32_t, int>
>  DrmDisplayCompositor::GetActiveModeResolution() {
>    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index f1965fb..ccaffb4 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -87,6 +87,7 @@ class DrmDisplayCompositor {
>    int Init(DrmResources *drm, int display);
>  
>    std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
> +  std::unique_ptr<DrmDisplayComposition> CreateInitializedComposition() const;
>    int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
>    int Composite();
>    int SquashAll();
> @@ -155,6 +156,7 @@ class DrmDisplayCompositor {
>    // we need to reset them on every Dump() call.
>    mutable uint64_t dump_frames_composited_;
>    mutable uint64_t dump_last_timestamp_ns_;
> +  std::unique_ptr<Planner> planner_;
>  };
>  }
>  
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 13/18] drm_hwcomposer: Pass buffer sizes to Prepareframebuffer

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:24PM +0100, Alexandru Gheorghe wrote:
> Currently Prepareframebuffer uses the mode of the connected connector
> to decide how big the buffer should be, however when using the
> drmdisplaycompositor just for flattening, the mode had not been set
> yet, so we need a way to pass the desired buffer sizes.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycompositor.cpp | 7 ++++---
>  drmdisplaycompositor.h   | 3 ++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index 6e5be24..afd3b05 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -268,14 +268,15 @@ DrmDisplayCompositor::GetActiveModeResolution() {
>  }
>  
>  int DrmDisplayCompositor::PrepareFramebuffer(
> -    DrmFramebuffer &fb, DrmDisplayComposition *display_comp) {
> +    DrmFramebuffer &fb, DrmDisplayComposition *display_comp, uint32_t width,
> +    uint32_t height) {
>    int ret = fb.WaitReleased(-1);
>    if (ret) {
>      ALOGE("Failed to wait for framebuffer release %d", ret);
>      return ret;
>    }
> -  uint32_t width, height;
> -  std::tie(width, height, ret) = GetActiveModeResolution();
> +  if (width == 0 || height == 0)
> +    std::tie(width, height, ret) = GetActiveModeResolution();

Just plumb it through at the other callsites.

>    if (ret) {
>      ALOGE(
>          "Failed to allocate framebuffer because the display resolution could "
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index ccaffb4..0f8daad 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -115,7 +115,8 @@ class DrmDisplayCompositor {
>    static const int kAcquireWaitTimeoutMs = 100;
>  
>    int PrepareFramebuffer(DrmFramebuffer &fb,
> -                         DrmDisplayComposition *display_comp);
> +                         DrmDisplayComposition *display_comp,
> +                         uint32_t width = 0, uint32_t height = 0);
>    int ApplySquash(DrmDisplayComposition *display_comp);
>    int ApplyPreComposite(DrmDisplayComposition *display_comp);
>    int PrepareFrame(DrmDisplayComposition *display_comp);
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 14/18] drm_hwcomposer: Fix race in ApplyFrame

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:25PM +0100, Alexandru Gheorghe wrote:
> ApplyFrame holds the lock just when it swaps the value of
> active_composition_, in a multithread context we could end up in a
> situation where something is shown on the screen, but something else
> is set in active_composition_. Fix it by holding the lock during
> CommitFrame.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycompositor.cpp | 40 +++++++++++++++++-----------------------
>  drmdisplaycompositor.h   |  2 +-
>  2 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index afd3b05..576539b 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -791,11 +791,6 @@ std::tuple<int, uint32_t> DrmDisplayCompositor::CreateModeBlob(
>  }
>  
>  void DrmDisplayCompositor::ClearDisplay() {
> -  AutoLock lock(&lock_, "compositor");
> -  int ret = lock.Lock();
> -  if (ret)
> -    return;
> -
>    if (!active_composition_)
>      return;
>  
> @@ -808,11 +803,25 @@ void DrmDisplayCompositor::ClearDisplay() {
>  }
>  
>  void DrmDisplayCompositor::ApplyFrame(
> -    std::unique_ptr<DrmDisplayComposition> composition, int status) {
> +    std::unique_ptr<DrmDisplayComposition> composition, int status,
> +    bool writeback) {

The writeback argument addition seems unrelated to this change.

> +  AutoLock lock(&lock_, __FUNCTION__);
> +  if (lock.Lock())
> +    return;
>    int ret = status;
> -
> -  if (!ret)
> +  if (!ret) {
> +    if (writeback && !CountdownExpired()) {
> +      ALOGE("Abort playing back scene");
> +      return;
> +    }
>      ret = CommitFrame(composition.get(), false);
> +    if (!ret) {
> +      ++dump_frames_composited_;
> +      if (active_composition_)
> +        active_composition_->SignalCompositionDone();
> +      active_composition_.swap(composition);

Why move this stuff?

> +    }
> +  }
>  
>    if (ret) {
>      ALOGE("Composite failed for display %d", display_);
> @@ -821,21 +830,6 @@ void DrmDisplayCompositor::ApplyFrame(
>      ClearDisplay();
>      return;
>    }
> -  ++dump_frames_composited_;
> -
> -  if (active_composition_)
> -    active_composition_->SignalCompositionDone();
> -
> -  ret = pthread_mutex_lock(&lock_);
> -  if (ret)
> -    ALOGE("Failed to acquire lock for active_composition swap");
> -
> -  active_composition_.swap(composition);
> -
> -  if (!ret)
> -    ret = pthread_mutex_unlock(&lock_);
> -  if (ret)
> -    ALOGE("Failed to release lock for active_composition swap");
>  }
>  
>  int DrmDisplayCompositor::ApplyComposition(
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index 0f8daad..b35ef70 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -127,7 +127,7 @@ class DrmDisplayCompositor {
>  
>    void ClearDisplay();
>    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> -                  int status);
> +                  int status, bool writeback = false);
>  
>    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
>  
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 15/18] drm_hwcomposer: Add worker to trigger scene flattenning

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:26PM +0100, Alexandru Gheorghe wrote:
> Add a vsync worker that calls back into the DrmDisplayCompositor,
> for now at every 60 vsyncs if the scene does not change we trigger
> the flattening of the scene using the writeback connector.
> Other, more complex and proper heuristics could be implemented later
> on.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycompositor.cpp | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  drmdisplaycompositor.h   | 12 +++++++++++-
>  2 files changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index 576539b..e535e8a 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -39,6 +39,20 @@
>  
>  namespace android {
>  
> +class CompositorVsyncCallback : public VsyncCallback {
> + public:
> +  CompositorVsyncCallback(DrmDisplayCompositor *compositor)
> +      : compositor_(compositor) {
> +  }
> +
> +  void Callback(int display, int64_t timestamp) {
> +    compositor_->Vsync(display, timestamp);
> +  }
> +
> + private:
> +  DrmDisplayCompositor *compositor_;
> +};
> +
>  void SquashState::Init(DrmHwcLayer *layers, size_t num_layers) {
>    generation_number_++;
>    valid_history_ = 0;
> @@ -183,7 +197,8 @@ DrmDisplayCompositor::DrmDisplayCompositor()
>        framebuffer_index_(0),
>        squash_framebuffer_index_(0),
>        dump_frames_composited_(0),
> -      dump_last_timestamp_ns_(0) {
> +      dump_last_timestamp_ns_(0),
> +      flatten_countdown_(FLATTEN_COUNTDOWN_INIT) {
>    struct timespec ts;
>    if (clock_gettime(CLOCK_MONOTONIC, &ts))
>      return;
> @@ -193,7 +208,7 @@ DrmDisplayCompositor::DrmDisplayCompositor()
>  DrmDisplayCompositor::~DrmDisplayCompositor() {
>    if (!initialized_)
>      return;
> -
> +  vsync_worker_.Exit();
>    int ret = pthread_mutex_lock(&lock_);
>    if (ret)
>      ALOGE("Failed to acquire compositor lock %d", ret);
> @@ -222,7 +237,9 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
>      return ret;
>    }
>    planner_ = Planner::CreateInstance(drm);
> -
> +  vsync_worker_.Init(drm_, display_);
> +  auto callback = std::make_shared<CompositorVsyncCallback>(this);
> +  vsync_worker_.RegisterCallback(callback);
>    initialized_ = true;
>    return 0;
>  }
> @@ -896,6 +913,10 @@ int DrmDisplayCompositor::ApplyComposition(
>    return ret;
>  }
>  
> +int DrmDisplayCompositor::FlattenScene() {
> +  return -EINVAL;
> +}

Hmm... not sure this is a useful inclusion.

> +
>  int DrmDisplayCompositor::SquashAll() {
>    AutoLock lock(&lock_, "compositor");
>    int ret = lock.Lock();
> @@ -1044,6 +1065,24 @@ move_layers_back:
>    return ret;
>  }
>  
> +bool DrmDisplayCompositor::CountdownExpired() const {
> +  return flatten_countdown_ <= 0;
> +}
> +
> +void DrmDisplayCompositor::Vsync(int display, int64_t timestamp) {
> +  AutoLock lock(&lock_, __FUNCTION__);

We use __func__ elsewhere, we should probably be consistent (same goes for other
instances).

> +  if (lock.Lock())
> +    return;
> +  flatten_countdown_--;
> +  if (CountdownExpired()) {

Doing:

if (--flatten_countdown_ > 0)
  return;

Allows you to remove the CountdownExpired() function and save a level of
indentation for the rest of the function.


> +    lock.Unlock();
> +    int ret = FlattenScene();
> +    ALOGI("scene flattening triggered for display %d at timestamp %" PRIu64
> +          " result = %d \n",
> +          display, timestamp, ret);
> +  }
> +}
> +
>  void DrmDisplayCompositor::Dump(std::ostringstream *out) const {
>    int ret = pthread_mutex_lock(&lock_);
>    if (ret)
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index b35ef70..26201b9 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -29,11 +29,16 @@
>  
>  #include <hardware/hardware.h>
>  #include <hardware/hwcomposer.h>
> +#include <vsyncworker.h>

"vsyncworker.h"

>  
>  // One for the front, one for the back, and one for cases where we need to
>  // squash a frame that the hw can't display with hw overlays.
>  #define DRM_DISPLAY_BUFFERS 3
>  
> +// If a scene is still for this number of vblanks flatten it to reduce power
> +// consumption.
> +#define FLATTEN_COUNTDOWN_INIT 60
> +
>  namespace android {
>  
>  class GLWorkerCompositor;
> @@ -92,7 +97,7 @@ class DrmDisplayCompositor {
>    int Composite();
>    int SquashAll();
>    void Dump(std::ostringstream *out) const;
> -
> +  void Vsync(int display, int64_t timestamp);
>    std::tuple<uint32_t, uint32_t, int> GetActiveModeResolution();
>  
>    SquashState *squash_state() {
> @@ -128,6 +133,9 @@ class DrmDisplayCompositor {
>    void ClearDisplay();
>    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
>                    int status, bool writeback = false);
> +  int FlattenScene();
> +
> +  bool CountdownExpired() const;
>  
>    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
>  
> @@ -157,6 +165,8 @@ class DrmDisplayCompositor {
>    // we need to reset them on every Dump() call.
>    mutable uint64_t dump_frames_composited_;
>    mutable uint64_t dump_last_timestamp_ns_;
> +  VSyncWorker vsync_worker_;
> +  int64_t flatten_countdown_;
>    std::unique_ptr<Planner> planner_;
>  };
>  }
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 16/18] drm_hwcomposer: Find writeback connector for scene flattening

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:27PM +0100, Alexandru Gheorghe wrote:
> Add logic for finding a suitable writeback connector, there are two
> possibilities for finding an usable writeback connector:
> 
> 1) Attached to the same CRTC as the display and can function
>    concurrently with the display connector.
> 
> 2) On a different CRTC and the display connector is not used (state !=
> DRM_MODE_CONNECTED). What's not handled here and should be handle is
> what happens if connector changes state while flattening, but since
> hotplug is not wired yet, it's not something we should worry about.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmresources.cpp    | 25 +++++++++++++++++++++++++
>  drmresources.h      |  2 +-
>  resourcemanager.cpp | 24 ++++++++++++++++++++++++
>  resourcemanager.h   |  1 +
>  4 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drmresources.cpp b/drmresources.cpp
> index fef6835..70126a4 100644
> --- a/drmresources.cpp
> +++ b/drmresources.cpp
> @@ -269,6 +269,31 @@ DrmConnector *DrmResources::GetWritebackConnectorForDisplay(int display) const {
>    return NULL;
>  }
>  
> +// TODO what happens when hotplugging
> +DrmConnector *DrmResources::AvailableWritebackConnector(int display) const {
> +  DrmConnector *writeback_conn = GetWritebackConnectorForDisplay(display);
> +  DrmConnector *display_conn = GetConnectorForDisplay(display);
> +  // If we have a writeback already attached to the same CRTC, just use that, if
> +  // possible
> +  if (display_conn && writeback_conn &&
> +      writeback_conn->encoder()->can_clone(display_conn->encoder()))
> +    return writeback_conn;
> +
> +  // Use another CRTC if available and doesn't have any connector
> +  for (auto &crtc : crtcs_) {
> +    if (crtc->display() == display)
> +      continue;
> +    display_conn = GetConnectorForDisplay(crtc->display());
> +    // If we have a display connected don't use it for writeback
> +    if (display_conn && display_conn->state() == DRM_MODE_CONNECTED)
> +      continue;
> +    writeback_conn = GetWritebackConnectorForDisplay(crtc->display());
> +    if (writeback_conn)
> +      return writeback_conn;
> +  }
> +  return NULL;
> +}
> +
>  DrmCrtc *DrmResources::GetCrtcForDisplay(int display) const {
>    for (auto &crtc : crtcs_) {
>      if (crtc->display() == display)
> diff --git a/drmresources.h b/drmresources.h
> index 4fb17fc..9176b8e 100644
> --- a/drmresources.h
> +++ b/drmresources.h
> @@ -60,7 +60,7 @@ class DrmResources {
>  
>    DrmConnector *GetConnectorForDisplay(int display) const;
>    DrmConnector *GetWritebackConnectorForDisplay(int display) const;
> -  DrmConnector *FindWritebackConnector(int display) const;

What's up with this?

> +  DrmConnector *AvailableWritebackConnector(int display) const;
>    DrmCrtc *GetCrtcForDisplay(int display) const;
>    DrmPlane *GetPlane(uint32_t id) const;
>    DrmEventListener *event_listener();
> diff --git a/resourcemanager.cpp b/resourcemanager.cpp
> index e7b654e..b2a4458 100644
> --- a/resourcemanager.cpp
> +++ b/resourcemanager.cpp
> @@ -49,6 +49,30 @@ int ResourceManager::Init() {
>                         (const hw_module_t **)&gralloc_);
>  }
>  
> +DrmConnector *ResourceManager::AvailableWritebackConnector(int display) {
> +  DrmResources *drm_resource = GetDrmResources(display);
> +  DrmConnector *writeback_conn = NULL;
> +  if (drm_resource) {
> +    writeback_conn = drm_resource->AvailableWritebackConnector(display);
> +    if (writeback_conn) {
> +      ALOGI("Use writeback connected to display %d\n",
> +            writeback_conn->display());

This seems a little chatty, it's written every flatten, yeah?

> +      return writeback_conn;
> +    }
> +  }
> +  for (auto &drm : drms_) {
> +    if (drm.get() == drm_resource)
> +      continue;
> +    writeback_conn = drm->AvailableWritebackConnector(display);
> +    if (writeback_conn) {
> +      ALOGI("Use writeback connected to display %d\n",
> +            writeback_conn->display());
> +      return writeback_conn;
> +    }
> +  }
> +  return writeback_conn;
> +}
> +
>  DrmResources *ResourceManager::GetDrmResources(int display) {
>    for (uint32_t i = 0; i < drms_.size(); i++) {
>      if (drms_[i]->HandlesDisplay(display))
> diff --git a/resourcemanager.h b/resourcemanager.h
> index b8caa9a..57f7a2a 100644
> --- a/resourcemanager.h
> +++ b/resourcemanager.h
> @@ -18,6 +18,7 @@ class ResourceManager {
>    DrmResources *GetDrmResources(int display);
>    std::shared_ptr<Importer> GetImporter(int display);
>    const gralloc_module_t *GetGralloc();
> +  DrmConnector *AvailableWritebackConnector(int display);
>  
>   private:
>    std::vector<std::unique_ptr<DrmResources>> drms_;
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 17/18] drm_hwcomposer: Flatten scene synchronously

[Sean Paul]

On Wed, Apr 11, 2018 at 04:22:28PM +0100, Alexandru Gheorghe wrote:
> Flatten scene on the same CRTC as the one driving the display.
> The active composition is played back to the display with a buffer
> attached to the writeback connector.
> Then we build a composition that has only one plane enabled and that
> uses the result of the writeback as the input.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++--
>  drmdisplaycompositor.h   |   7 +-
>  2 files changed, 204 insertions(+), 6 deletions(-)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index e535e8a..cb670e6 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -36,6 +36,7 @@
>  #include "drmplane.h"
>  #include "drmresources.h"
>  #include "glworker.h"
> +static const uint32_t kWaitWritebackFence = 100;  // ms
>  
>  namespace android {
>  
> @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
>  }
>  
>  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> -                                      bool test_only) {
> +                                      bool test_only,
> +                                      DrmDisplayComposition *writeback_comp,
> +                                      DrmConnector *writeback_conn) {
>    ATRACE_CALL();
>  
>    int ret = 0;
> @@ -532,6 +535,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>    std::vector<DrmCompositionPlane> &comp_planes =
>        display_comp->composition_planes();
>    uint64_t out_fences[drm_->crtcs().size()];
> +  int writeback_fence = -1;
>  
>    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
>    if (!connector) {
> @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>      return -ENOMEM;
>    }
>  
> +  if (writeback_comp != NULL) {
> +    if (writeback_conn == NULL)
> +      return -EINVAL;
> +    if (writeback_conn->writeback_fb_id().id() == 0 ||
> +        writeback_conn->writeback_out_fence().id() == 0) {
> +      ALOGE("Writeback properties don't exit");
> +      return -EINVAL;
> +    }
> +    if (writeback_comp->layers().size() != 1) {
> +      ALOGE("Invalid number of layers for writeback composition");
> +      return -EINVAL;
> +    }
> +    ret = drmModeAtomicAddProperty(
> +        pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
> +        writeback_comp->layers().back().buffer->fb_id);
> +    if (ret < 0) {
> +      ALOGE("Failed to add writeback_fb_id");
> +      return ret;
> +    }
> +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
> +                                   writeback_conn->writeback_out_fence().id(),
> +                                   (uint64_t)&writeback_fence);

Upcasting int to u64 isn't a great idea, please go the other way (as we do with
out_fences).

> +    if (ret < 0) {
> +      ALOGE("Failed to add writeback_out_fence");
> +      return ret;
> +    }
> +  }

This would be more readable if it was split off into a function.

>    if (crtc->out_fence_ptr_property().id() != 0) {
> -    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
> -                                   (uint64_t) &out_fences[crtc->pipe()]);
> +    ret = drmModeAtomicAddProperty(pset, crtc->id(),
> +                                   crtc->out_fence_ptr_property().id(),
> +                                   (uint64_t)&out_fences[crtc->pipe()]);
>      if (ret < 0) {
>        ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
>        drmModeAtomicFree(pset);
> @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>      }
>    }
>  
> +  if (writeback_conn != NULL) {
> +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
> +                                   writeback_conn->crtc_id_property().id(),
> +                                   crtc->id());
> +    if (ret < 0) {
> +      ALOGE("Failed to  attach writeback");
> +    }
> +  }

Can you do this above with the rest of the writeback properties?

> +
>    for (DrmCompositionPlane &comp_plane : comp_planes) {
>      DrmPlane *plane = comp_plane.plane();
>      DrmCrtc *crtc = comp_plane.crtc();
> @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>  
>    if (!ret) {
>      uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET;
> -    if (test_only)
> +    if (test_only) {
>        flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> +    } else {
> +      if (writeback_comp != NULL) {
> +        if (!CountdownExpired() && active_composition_) {

Given that we're holding the lock throughout this function, can't you just
abort at the start?

> +          ALOGE("Writeback composition not needed, abort commit");
> +          drmModeAtomicFree(pset);
> +          return -EINVAL;
> +        };
> +        flags |= DRM_MODE_ATOMIC_NONBLOCK;

I'm guessing this is the cause of the active_composition race you're fixing
earlier in the series? Could you please pull this out and squash it into that
patch as a "Use non-blocking commits" standalone? It might be useful for
testing, and this is something that's substantial enough to warrant the
additional visibility of its own patch.

> +      }
> +    }
>  
>      ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
>      if (ret) {
> @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>    if (crtc->out_fence_ptr_property().id()) {
>      display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
>    }
> +  if (writeback_fence >= 0) {
> +    if (writeback_comp->layers().size() != 1) {
> +      ALOGE("Invalid numbers of layer for writeback_comp");
> +      return -EINVAL;
> +    }

You already test this above?

> +    writeback_comp->layers()[0].acquire_fence.Set(writeback_fence);
> +  }
>  
>    return ret;
>  }
> @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame(
>        if (active_composition_)
>          active_composition_->SignalCompositionDone();
>        active_composition_.swap(composition);
> +      flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> +      vsync_worker_.VSyncControl(!writeback);
>      }
>    }
>  
> @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition(
>    return ret;
>  }
>  
> +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src,
> +                                             DrmDisplayComposition *dst,
> +                                             DrmConnector *writeback_conn) {
> +  int ret = 0;
> +  if (src == NULL || dst == NULL)
> +    return -EINVAL;
> +  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
> +  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
> +                                    src->crtc());
> +  for (DrmCompositionPlane &comp_plane : src_planes) {
> +    if (comp_plane.plane() == NULL) {
> +      ALOGE("Skipping squash all because of NULL plane");
> +      ret = -EINVAL;
> +    }
> +    if (!squashed_comp.plane() &&
> +        comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
> +      squashed_comp.set_plane(comp_plane.plane());
> +    else
> +      dst->AddPlaneDisable(comp_plane.plane());
> +  }
> +
> +  DrmFramebuffer *writeback_fb = NULL;
> +  AutoLock lock(&lock_, __FUNCTION__);
> +  if ((ret = lock.Lock()))

Same comments regarding assignment in a conditional.

> +    return ret;
> +  writeback_fb = &framebuffers_[framebuffer_index_];
> +  framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
> +  ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(),
> +                           mode_.mode.v_display());
> +  if (ret) {
> +    ALOGE("Failed to prepare destination buffer");
> +    return ret;
> +  }
> +  lock.Unlock();
> +  ret = CommitFrame(src, true, dst, writeback_conn);
> +  if (ret) {
> +    ALOGE("Atomic check failed");
> +    return ret;
> +  }
> +  if ((ret = lock.Lock()))

All of these locks and unlocks are going to cause races, as mentioned below,
please re-evaluate.

> +    return ret;
> +  if (!CountdownExpired() && active_composition_) {
> +    ALOGE("Writeback composition not needed abort");
> +    return -EINVAL;
> +  }
> +  ret = CommitFrame(src, false, dst, writeback_conn);
> +  lock.Unlock();
> +  if (ret || dst->layers().size() != 1) {
> +    ALOGE("Failed to flatten scene using writeback");
> +    return -EINVAL;
> +  }
> +  squashed_comp.source_layers().push_back(0);
> +  ret =
> +      sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence);

line break

> +  if (ret) {
> +    ALOGE("Failed to wait on writeback fence");
> +    return ret;
> +  }
> +  ret = dst->AddPlaneComposition(std::move(squashed_comp));
> +  if (ret) {
> +    ALOGE("Failed to add flatten scene");
> +    return ret;
> +  }
> +  ret = dst->FinalizeComposition();
> +  if (ret) {
> +    ALOGE("Failed to finalize composition");
> +    return ret;
> +  }
> +  return 0;
> +}
> +
> +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) {
> +  if (writeback_conn->display() != display_) {
> +    ALOGE("Cannot flatten synchronously on different display");
> +    return -EINVAL;
> +  }

You check this right before calling, so this isn't needed.

> +  ALOGI("FlattenSynchronously using the same display");

I think you should downgrade this log level.

> +  int ret = 0;
> +  /* Flattened composition with only one layer that is built
> +   * using the writeback connector
> +   */
> +  std::unique_ptr<DrmDisplayComposition> writeback_comp =
> +      CreateInitializedComposition();
> +  /* Copy of the active_composition_, we need a copy because
> +   * if we use the active composition we have to hold the lock
> +   * for the entire sequence of flattening.
> +   */
> +  std::unique_ptr<DrmDisplayComposition> copy_comp =
> +      CreateInitializedComposition();
> +
> +  if (!copy_comp || !writeback_comp)
> +    return -EINVAL;
> +  AutoLock lock(&lock_, __FUNCTION__);
> +  if ((ret = lock.Lock()))

Assignments in if statements make me nervous. Since you don't need the
declaration above, just do:

  int ret = lock.Lock();
  if (ret)

> +    return ret;
> +  if (CountdownExpired()) {
> +    ret = copy_comp->CopyLayers(active_composition_.get());
> +    if (ret)
> +      return ret;
> +    copy_comp->CopyCompPlanes(active_composition_.get());
> +  } else {
> +    return -EINVAL;
> +  }

  if (!CountdownExpired())
    return -EINVAL;

  ret = copy_comp->CopyLayers(active_composition_.get());
  if (ret)
    return ret;
  copy_comp->CopyCompPlanes(active_composition_.get());


> +  lock.Unlock();

Just hold the lock through WritebackComposite(), and consider creating a
ApplyFrameLocked() to hold it through ApplyFrame(). You should be able to reduce
the number of times you have to check CountdownExpired() (or remove it)
significantly by just consistently locking things.

> +  ret =
> +      WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn);
> +  if (ret) {
> +    ALOGE("Failed to prepare writebackScene");
> +    return ret;
> +  }
> +
> +  ApplyFrame(std::move(writeback_comp), 0, true);
> +  return 0;
> +}
> +
>  int DrmDisplayCompositor::FlattenScene() {
> -  return -EINVAL;
> +  DrmConnector *writeback_conn =
> +      drm_->resource_manager()->AvailableWritebackConnector(display_);
> +  if (!active_composition_ || !writeback_conn)
> +    return -EINVAL;
> +  std::vector<DrmCompositionPlane> &src_planes =
> +      active_composition_->composition_planes();
> +  size_t src_planes_with_layer = std::count_if(
> +      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
> +        return p.type() != DrmCompositionPlane::Type::kDisable;
> +      });
> +
> +  if (src_planes_with_layer <= 1)
> +    return -EALREADY;
> +
> +  if (writeback_conn->display() == display_) {
> +    return FlattenSynchronously(writeback_conn);
> +  }
> +
> +  return 0;
>  }
>  
>  int DrmDisplayCompositor::SquashAll() {
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index 26201b9..4cc4a5e 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -125,7 +125,9 @@ class DrmDisplayCompositor {
>    int ApplySquash(DrmDisplayComposition *display_comp);
>    int ApplyPreComposite(DrmDisplayComposition *display_comp);
>    int PrepareFrame(DrmDisplayComposition *display_comp);
> -  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
> +  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only,
> +                  DrmDisplayComposition *writeback_comp = NULL,
> +                  DrmConnector *writeback_conn = NULL);
>    int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
>    int ApplyDpms(DrmDisplayComposition *display_comp);
>    int DisablePlanes(DrmDisplayComposition *display_comp);
> @@ -134,7 +136,10 @@ class DrmDisplayCompositor {
>    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
>                    int status, bool writeback = false);
>    int FlattenScene();
> +  int FlattenSynchronously(DrmConnector *writeback_conn);

Can we come up with a better name than Synchonrous/Asynchronous? Or at the very
least comment on what that means?

>  
> +  int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst,
> +                         DrmConnector *writeback_conn);
>    bool CountdownExpired() const;
>  
>    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> -- 
> 2.7.4
> 

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

Re: [PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class

[Alexandru-Cosmin Gheorghe]

On Tue, Apr 17, 2018 at 06:08:06PM +0200, Robert Foss wrote:
> Hey,
> 
> On 04/17/2018 05:33 PM, Sean Paul wrote:
> >On Wed, Apr 11, 2018 at 04:22:15PM +0100, Alexandru Gheorghe wrote:
> >>Add a resource manager object that is responsible for detecting all
> >>kms devices and allocates unique display numbers for every detected
> >>display.
> >>
> >>This is controlled by the value of hwc.drm.device property, if it ends
> >>with a %, it will try to open minor devices until and error is detected.
> >>E.g: /dev/dri/card%
> >
> >I'm a bit on the fence as to whether to use the %, or add a new
> >hwc.drm.scan_devices property. I guess since we need to convey the path anyways
> >this is fine, but it really needs to be better documented.
> 
> I'm looking at this stuff in another series about DRM Node probing[1],
> and I'll look into using properties to define what we are looking for.
> 
> But those properties won't be paths, but rather PCI IDs and driver vendor names.
> 
> As for what to do in the series, I don't have much of an opinion. But I'm
> likely to try to change it in the not too distant future.
> 
> 
> [1] https://www.spinics.net/lists/dri-devel/msg172496.html

Aren't this two complementary? 
This series try to go through all available nodes and yours provides a
mechanism to check if file descriptor match some expectations.
You still have to open them somehow.

> 
> >
> >>
> >>Additionally, this will be used for finding an available writeback
> >>connector that will be used for the flattening of the currently
> >>displayed scene.
> >>
> >>Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> >>---
> >>  Android.mk          |  1 +
> >>  resourcemanager.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  resourcemanager.h   | 29 ++++++++++++++++++++++
> >>  3 files changed, 101 insertions(+)
> >>  create mode 100644 resourcemanager.cpp
> >>  create mode 100644 resourcemanager.h
> >>
> >>diff --git a/Android.mk b/Android.mk
> >>index 1add286..736fe24 100644
> >>--- a/Android.mk
> >>+++ b/Android.mk
> >>@@ -52,6 +52,7 @@ LOCAL_C_INCLUDES := \
> >>  LOCAL_SRC_FILES := \
> >>  	autolock.cpp \
> >>+	resourcemanager.cpp \
> >>  	drmresources.cpp \
> >>  	drmconnector.cpp \
> >>  	drmcrtc.cpp \
> >>diff --git a/resourcemanager.cpp b/resourcemanager.cpp
> >>new file mode 100644
> >>index 0000000..e7b654e
> >>--- /dev/null
> >>+++ b/resourcemanager.cpp
> >>@@ -0,0 +1,71 @@
> >>+#include "resourcemanager.h"
> >>+#include <cutils/log.h>
> >>+#include <cutils/properties.h>
> >>+
> >>+namespace android {
> >>+
> >>+ResourceManager::ResourceManager() : gralloc_(NULL) {
> >>+}
> >>+
> >>+int ResourceManager::Init() {
> >>+  char path_pattern[PROPERTY_VALUE_MAX];
> >>+  property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
> >
> >We probably also don't want to default to scanning, since that might change
> >behavior in existing boards.
> >
> >>+
> >>+  uint8_t minor = 0;
> >
> >Please use unsigned/int instead of fixed-size types. Unless the number of bits
> >is relevant to use of the variable, let the compiler handle it.
> >
> >>+  int last_display_index = 0;
> >
> >Could we just call this num_displays? It was kind of hard for me to reason
> >through this, especially when it's call "start_display_index" when you jump into
> >drm::Init(). I also think drm->Init's return pair should return
> ><ret, displays_added> instead of <ret, display_index>, so each time you call
> >Init(), you're adding to num_displays.
> >
> >>+  int last_char = strlen(path_pattern) - 1;
> >>+  do {
> >>+    char path[PROPERTY_VALUE_MAX];
> >
> >Please use string
> >
> >>+    if (path_pattern[last_char] == '%') {
> >>+      path_pattern[last_char] = '\0';
> >>+      snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor);
> >>+      path_pattern[last_char] = '%';
> >
> >This doesn't work for minor > 10.
> >
> >>+    } else {
> >>+      snprintf(path, PROPERTY_VALUE_MAX, "%s", path_pattern);
> >>+    }
> >>+    std::unique_ptr<DrmResources> drm = std::make_unique<DrmResources>();
> >>+    last_display_index = drm->Init(this, path, last_display_index);
> >>+    if (last_display_index < 0) {
> >>+      break;
> >>+    }
> >>+    std::shared_ptr<Importer> importer;
> >>+    importer.reset(Importer::CreateInstance(drm.get()));
> >>+    if (!importer) {
> >>+      ALOGE("Failed to create importer instance");
> >>+      break;
> >>+    }
> >>+    importers_.push_back(importer);
> >>+    drms_.push_back(std::move(drm));
> >>+    minor++;
> >>+    last_display_index++;
> >>+  } while (path_pattern[last_char] == '%');
> >>+
> >>+  if (!drms_.size()) {
> >>+    ALOGE("Failed to find any working drm device");
> >>+    return -EINVAL;
> >>+  }
> >>+
> >>+  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> >>+                       (const hw_module_t **)&gralloc_);
> >>+}
> >
> >Consider the following:
> >
> >int ResourceManager::AddDrmDevice(std::string path) {
> >   std::unique_ptr<DrmDevice> drm = std::make_unique<DrmDevice>();
> >   int displays_added, ret;
> >   std::tie(displays_added, ret) = drm->Init(path.c_str(), num_displays_);
> >   if (ret)
> >     return ret;
> >
> >   std::shared_ptr<Importer> importer;
> >   importer.reset(Importer::CreateInstance(drm.get()));
> >   if (!importer) {
> >     ALOGE("Failed to create importer instance");
> >     return -ENODEV;
> >   }
> >
> >   importers_.push_back(importer);
> >   drms_.push_back(std::move(drm));
> >   num_displays_ += displays_added;
> >   return 0;
> >}
> >
> >int ResourceManager::Init() {
> >   char path_pattern[PROPERTY_VALUE_MAX];
> >   int path_len = property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
> >
> >   if (path_pattern[path_len - 1] != '%')
> >     return AddDrmDevice(std::string(path_pattern);
> >
> >   path_pattern[path_len - 1] = '\0';
> >   for (int ret = 0, idx = 0; !ret; ++idx) {
> >     ostringstream path;
> >     path << path_pattern << idx;
> >     ret = AddDrmDevice(path.str());
> >   }
> >   if (!num_displays_) {
> >     ALOGE("Failed to initialize any displays");
> >     return -EINVAL;
> >   }
> >
> >   return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> >                        (const hw_module_t **)&gralloc_);
> >}
> >
> >I think resolves the issues from the original patches and incorporates the
> >suggestions of drm->Init() returning the tuple of added displays, as well as
> >eliminating the backpointer.
> >
> >
> >>+
> >>+DrmResources *ResourceManager::GetDrmResources(int display) {
> >>+  for (uint32_t i = 0; i < drms_.size(); i++) {
> >
> >for (auto &drm_: drms_) {
> >
> >>+    if (drms_[i]->HandlesDisplay(display))
> >>+      return drms_[i].get();
> >>+  }
> >>+  return NULL;
> >>+}
> >>+
> >>+std::shared_ptr<Importer> ResourceManager::GetImporter(int display) {
> >>+  for (uint32_t i = 0; i < drms_.size(); i++) {
> >
> >Same here
> >
> >>+    if (drms_[i]->HandlesDisplay(display))
> >>+      return importers_[i];
> >>+  }
> >>+  return NULL;
> >>+}
> >>+
> >>+const gralloc_module_t *ResourceManager::GetGralloc() {
> >
> >I think this should be called gralloc()
> >
> >>+  return gralloc_;
> >>+}
> >>+}
> >>diff --git a/resourcemanager.h b/resourcemanager.h
> >>new file mode 100644
> >>index 0000000..b8caa9a
> >>--- /dev/null
> >>+++ b/resourcemanager.h
> >>@@ -0,0 +1,29 @@
> >>+#ifndef RESOURCEMANAGER_H
> >>+#define RESOURCEMANAGER_H
> >>+
> >>+#include "drmresources.h"
> >>+#include "platform.h"
> >>+
> >>+namespace android {
> >>+
> >>+class DrmResources;
> >>+class Importer;
> >
> >I think you need either the forward declarations or the includes, but not both?
> >
> >>+
> >>+class ResourceManager {
> >>+ public:
> >>+  ResourceManager();
> >>+  ResourceManager(const ResourceManager &) = delete;
> >>+  ResourceManager &operator=(const ResourceManager &) = delete;
> >>+  int Init();
> >>+  DrmResources *GetDrmResources(int display);
> >>+  std::shared_ptr<Importer> GetImporter(int display);
> >>+  const gralloc_module_t *GetGralloc();
> >>+
> >>+ private:
> >>+  std::vector<std::unique_ptr<DrmResources>> drms_;
> >>+  std::vector<std::shared_ptr<Importer>> importers_;
> >>+  const gralloc_module_t *gralloc_;
> >>+};
> >>+}
> >>+
> >>+#endif  // RESOURCEMANAGER_H
> >>-- 
> >>2.7.4
> >>
> >

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

Re: [PATCH hwc v2 04/18] drm_hwcomposer: Add resource manager class

[Robert Foss]

On 04/18/2018 12:12 PM, Alexandru-Cosmin Gheorghe wrote:
> On Tue, Apr 17, 2018 at 06:08:06PM +0200, Robert Foss wrote:
>> Hey,
>>
>> On 04/17/2018 05:33 PM, Sean Paul wrote:
>>> On Wed, Apr 11, 2018 at 04:22:15PM +0100, Alexandru Gheorghe wrote:
>>>> Add a resource manager object that is responsible for detecting all
>>>> kms devices and allocates unique display numbers for every detected
>>>> display.
>>>>
>>>> This is controlled by the value of hwc.drm.device property, if it ends
>>>> with a %, it will try to open minor devices until and error is detected.
>>>> E.g: /dev/dri/card%
>>>
>>> I'm a bit on the fence as to whether to use the %, or add a new
>>> hwc.drm.scan_devices property. I guess since we need to convey the path anyways
>>> this is fine, but it really needs to be better documented.
>>
>> I'm looking at this stuff in another series about DRM Node probing[1],
>> and I'll look into using properties to define what we are looking for.
>>
>> But those properties won't be paths, but rather PCI IDs and driver vendor names.
>>
>> As for what to do in the series, I don't have much of an opinion. But I'm
>> likely to try to change it in the not too distant future.
>>
>>
>> [1] https://www.spinics.net/lists/dri-devel/msg172496.html
> 
> Aren't this two complementary?
> This series try to go through all available nodes and yours provides a
> mechanism to check if file descriptor match some expectations.
> You still have to open them somehow.

Yes, they are, I'm just noting that I'll prod around this area and change it in 
not too long, whatever decision is made here I'm likely to tweak it :)


Rob.

> 
>>
>>>
>>>>
>>>> Additionally, this will be used for finding an available writeback
>>>> connector that will be used for the flattening of the currently
>>>> displayed scene.
>>>>
>>>> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
>>>> ---
>>>>   Android.mk          |  1 +
>>>>   resourcemanager.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   resourcemanager.h   | 29 ++++++++++++++++++++++
>>>>   3 files changed, 101 insertions(+)
>>>>   create mode 100644 resourcemanager.cpp
>>>>   create mode 100644 resourcemanager.h
>>>>
>>>> diff --git a/Android.mk b/Android.mk
>>>> index 1add286..736fe24 100644
>>>> --- a/Android.mk
>>>> +++ b/Android.mk
>>>> @@ -52,6 +52,7 @@ LOCAL_C_INCLUDES := \
>>>>   LOCAL_SRC_FILES := \
>>>>   	autolock.cpp \
>>>> +	resourcemanager.cpp \
>>>>   	drmresources.cpp \
>>>>   	drmconnector.cpp \
>>>>   	drmcrtc.cpp \
>>>> diff --git a/resourcemanager.cpp b/resourcemanager.cpp
>>>> new file mode 100644
>>>> index 0000000..e7b654e
>>>> --- /dev/null
>>>> +++ b/resourcemanager.cpp
>>>> @@ -0,0 +1,71 @@
>>>> +#include "resourcemanager.h"
>>>> +#include <cutils/log.h>
>>>> +#include <cutils/properties.h>
>>>> +
>>>> +namespace android {
>>>> +
>>>> +ResourceManager::ResourceManager() : gralloc_(NULL) {
>>>> +}
>>>> +
>>>> +int ResourceManager::Init() {
>>>> +  char path_pattern[PROPERTY_VALUE_MAX];
>>>> +  property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
>>>
>>> We probably also don't want to default to scanning, since that might change
>>> behavior in existing boards.
>>>
>>>> +
>>>> +  uint8_t minor = 0;
>>>
>>> Please use unsigned/int instead of fixed-size types. Unless the number of bits
>>> is relevant to use of the variable, let the compiler handle it.
>>>
>>>> +  int last_display_index = 0;
>>>
>>> Could we just call this num_displays? It was kind of hard for me to reason
>>> through this, especially when it's call "start_display_index" when you jump into
>>> drm::Init(). I also think drm->Init's return pair should return
>>> <ret, displays_added> instead of <ret, display_index>, so each time you call
>>> Init(), you're adding to num_displays.
>>>
>>>> +  int last_char = strlen(path_pattern) - 1;
>>>> +  do {
>>>> +    char path[PROPERTY_VALUE_MAX];
>>>
>>> Please use string
>>>
>>>> +    if (path_pattern[last_char] == '%') {
>>>> +      path_pattern[last_char] = '\0';
>>>> +      snprintf(path, PROPERTY_VALUE_MAX, "%s%d", path_pattern, minor);
>>>> +      path_pattern[last_char] = '%';
>>>
>>> This doesn't work for minor > 10.
>>>
>>>> +    } else {
>>>> +      snprintf(path, PROPERTY_VALUE_MAX, "%s", path_pattern);
>>>> +    }
>>>> +    std::unique_ptr<DrmResources> drm = std::make_unique<DrmResources>();
>>>> +    last_display_index = drm->Init(this, path, last_display_index);
>>>> +    if (last_display_index < 0) {
>>>> +      break;
>>>> +    }
>>>> +    std::shared_ptr<Importer> importer;
>>>> +    importer.reset(Importer::CreateInstance(drm.get()));
>>>> +    if (!importer) {
>>>> +      ALOGE("Failed to create importer instance");
>>>> +      break;
>>>> +    }
>>>> +    importers_.push_back(importer);
>>>> +    drms_.push_back(std::move(drm));
>>>> +    minor++;
>>>> +    last_display_index++;
>>>> +  } while (path_pattern[last_char] == '%');
>>>> +
>>>> +  if (!drms_.size()) {
>>>> +    ALOGE("Failed to find any working drm device");
>>>> +    return -EINVAL;
>>>> +  }
>>>> +
>>>> +  return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
>>>> +                       (const hw_module_t **)&gralloc_);
>>>> +}
>>>
>>> Consider the following:
>>>
>>> int ResourceManager::AddDrmDevice(std::string path) {
>>>    std::unique_ptr<DrmDevice> drm = std::make_unique<DrmDevice>();
>>>    int displays_added, ret;
>>>    std::tie(displays_added, ret) = drm->Init(path.c_str(), num_displays_);
>>>    if (ret)
>>>      return ret;
>>>
>>>    std::shared_ptr<Importer> importer;
>>>    importer.reset(Importer::CreateInstance(drm.get()));
>>>    if (!importer) {
>>>      ALOGE("Failed to create importer instance");
>>>      return -ENODEV;
>>>    }
>>>
>>>    importers_.push_back(importer);
>>>    drms_.push_back(std::move(drm));
>>>    num_displays_ += displays_added;
>>>    return 0;
>>> }
>>>
>>> int ResourceManager::Init() {
>>>    char path_pattern[PROPERTY_VALUE_MAX];
>>>    int path_len = property_get("hwc.drm.device", path_pattern, "/dev/dri/card%");
>>>
>>>    if (path_pattern[path_len - 1] != '%')
>>>      return AddDrmDevice(std::string(path_pattern);
>>>
>>>    path_pattern[path_len - 1] = '\0';
>>>    for (int ret = 0, idx = 0; !ret; ++idx) {
>>>      ostringstream path;
>>>      path << path_pattern << idx;
>>>      ret = AddDrmDevice(path.str());
>>>    }
>>>    if (!num_displays_) {
>>>      ALOGE("Failed to initialize any displays");
>>>      return -EINVAL;
>>>    }
>>>
>>>    return hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
>>>                         (const hw_module_t **)&gralloc_);
>>> }
>>>
>>> I think resolves the issues from the original patches and incorporates the
>>> suggestions of drm->Init() returning the tuple of added displays, as well as
>>> eliminating the backpointer.
>>>
>>>
>>>> +
>>>> +DrmResources *ResourceManager::GetDrmResources(int display) {
>>>> +  for (uint32_t i = 0; i < drms_.size(); i++) {
>>>
>>> for (auto &drm_: drms_) {
>>>
>>>> +    if (drms_[i]->HandlesDisplay(display))
>>>> +      return drms_[i].get();
>>>> +  }
>>>> +  return NULL;
>>>> +}
>>>> +
>>>> +std::shared_ptr<Importer> ResourceManager::GetImporter(int display) {
>>>> +  for (uint32_t i = 0; i < drms_.size(); i++) {
>>>
>>> Same here
>>>
>>>> +    if (drms_[i]->HandlesDisplay(display))
>>>> +      return importers_[i];
>>>> +  }
>>>> +  return NULL;
>>>> +}
>>>> +
>>>> +const gralloc_module_t *ResourceManager::GetGralloc() {
>>>
>>> I think this should be called gralloc()
>>>
>>>> +  return gralloc_;
>>>> +}
>>>> +}
>>>> diff --git a/resourcemanager.h b/resourcemanager.h
>>>> new file mode 100644
>>>> index 0000000..b8caa9a
>>>> --- /dev/null
>>>> +++ b/resourcemanager.h
>>>> @@ -0,0 +1,29 @@
>>>> +#ifndef RESOURCEMANAGER_H
>>>> +#define RESOURCEMANAGER_H
>>>> +
>>>> +#include "drmresources.h"
>>>> +#include "platform.h"
>>>> +
>>>> +namespace android {
>>>> +
>>>> +class DrmResources;
>>>> +class Importer;
>>>
>>> I think you need either the forward declarations or the includes, but not both?
>>>
>>>> +
>>>> +class ResourceManager {
>>>> + public:
>>>> +  ResourceManager();
>>>> +  ResourceManager(const ResourceManager &) = delete;
>>>> +  ResourceManager &operator=(const ResourceManager &) = delete;
>>>> +  int Init();
>>>> +  DrmResources *GetDrmResources(int display);
>>>> +  std::shared_ptr<Importer> GetImporter(int display);
>>>> +  const gralloc_module_t *GetGralloc();
>>>> +
>>>> + private:
>>>> +  std::vector<std::unique_ptr<DrmResources>> drms_;
>>>> +  std::vector<std::shared_ptr<Importer>> importers_;
>>>> +  const gralloc_module_t *gralloc_;
>>>> +};
>>>> +}
>>>> +
>>>> +#endif  // RESOURCEMANAGER_H
>>>> -- 
>>>> 2.7.4
>>>>
>>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hwc v2 10/18] drm_hwcomposer: hwcutils: Add function for cloning a DrmHwcLayer

[Alexandru-Cosmin Gheorghe]

On Tue, Apr 17, 2018 at 12:14:14PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:21PM +0100, Alexandru Gheorghe wrote:
> > When doing flattening of a composition on a different CRTC we need to be
> > able to clone a layer in order to import it and then pass it to another CRTC.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmhwcomposer.h |  1 +
> >  hwcutils.cpp    | 11 +++++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/drmhwcomposer.h b/drmhwcomposer.h
> > index f8440fb..b256caf 100644
> > --- a/drmhwcomposer.h
> > +++ b/drmhwcomposer.h
> > @@ -150,6 +150,7 @@ struct DrmHwcLayer {
> >  
> >    int InitFromHwcLayer(hwc_layer_1_t *sf_layer, Importer *importer,
> >                         const gralloc_module_t *gralloc);
> > +  int PopulateFromDrmHwcLayer(DrmHwcLayer *layer);
> >    int ImportBuffer(Importer *importer, const gralloc_module_t *gralloc);
> >  
> >    void SetTransform(int32_t sf_transform);
> > diff --git a/hwcutils.cpp b/hwcutils.cpp
> > index 53a7d82..ff37c3b 100644
> > --- a/hwcutils.cpp
> > +++ b/hwcutils.cpp
> > @@ -149,6 +149,17 @@ int DrmHwcLayer::InitFromHwcLayer(hwc_layer_1_t *sf_layer, Importer *importer,
> >    return ImportBuffer(importer, gralloc);
> >  }
> >  
> > +int DrmHwcLayer::PopulateFromDrmHwcLayer(DrmHwcLayer *src_layer) {
> > +  blending = src_layer->blending;
> > +  sf_handle = src_layer->sf_handle;
> > +  acquire_fence = dup(src_layer->acquire_fence.get());
> 
> Hmm, I think this is the only place where we duplicate a UniqueFd. I _think_
> this will be Ok, but do you need to use this? Could you instead defer trying to
> flatten if the original acquire_fence hasn't fired? Also, you don't check the
> return value.

Thinking about it, I don't think there is any way we could try
flattening, without acquire_fence being already fired.

> 
> > +  display_frame = src_layer->display_frame;
> > +  alpha = src_layer->alpha;
> > +  source_crop = src_layer->source_crop;
> > +  transform = src_layer->transform;
> 
> It also doesn't seem like you're populating all of the fields so the function
> name is misleading.
> 

Which one are you referring to? This follows the properties copied by
HwcLayer::PopulateDrmLayer which I assume was correct.

> 
> > +  return 0;
> > +}
> > +
> >  int DrmHwcLayer::ImportBuffer(Importer *importer,
> >                                const gralloc_module_t *gralloc) {
> >    int ret = buffer.ImportBuffer(sf_handle, importer);
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 12/18] drm_hwcomposer: Add utility function to create an initialized composition

[Alexandru-Cosmin Gheorghe]

On Tue, Apr 17, 2018 at 12:37:15PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:23PM +0100, Alexandru Gheorghe wrote:
> > There is a lot of boilerplate for creating an initialized
> > drmdisplaycomposition. This patch gathers that in a separate method.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmdisplaycompositor.cpp | 23 +++++++++++++++++++++++
> >  drmdisplaycompositor.h   |  2 ++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index e556e86..6e5be24 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -221,6 +221,7 @@ int DrmDisplayCompositor::Init(DrmResources *drm, int display) {
> >      ALOGE("Failed to initialize drm compositor lock %d\n", ret);
> >      return ret;
> >    }
> > +  planner_ = Planner::CreateInstance(drm);
> 
> What's this?

We need a planner for the function bellow, I could re-write this to be
an argument of CreateComposition if you think that helps.

> 
> >  
> >    initialized_ = true;
> >    return 0;
> > @@ -231,6 +232,28 @@ std::unique_ptr<DrmDisplayComposition> DrmDisplayCompositor::CreateComposition()
> >    return std::unique_ptr<DrmDisplayComposition>(new DrmDisplayComposition());
> >  }
> >  
> > +std::unique_ptr<DrmDisplayComposition>
> > +DrmDisplayCompositor::CreateInitializedComposition() const {
> > +  DrmCrtc *crtc = drm_->GetCrtcForDisplay(display_);
> > +  if (!crtc) {
> > +    ALOGE("Failed to find crtc for display = %d", display_);
> > +    return std::unique_ptr<DrmDisplayComposition>();
> > +  }
> > +  std::unique_ptr<DrmDisplayComposition> comp = CreateComposition();
> > +  std::shared_ptr<Importer> importer =
> > +      drm_->resource_manager()->GetImporter(display_);
> > +  if (!importer) {
> > +    ALOGE("Failed to find resources for display = %d", display_);
> > +    return std::unique_ptr<DrmDisplayComposition>();
> > +  }
> > +  int ret = comp->Init(drm_, crtc, importer.get(), planner_.get(), 0);
> > +  if (ret) {
> > +    ALOGE("Failed to init composition for display = %d", display_);
> > +    return std::unique_ptr<DrmDisplayComposition>();
> > +  }
> > +  return comp;
> > +}
> > +
> 
> This seems sufficiently small that you can squash it into the patch that uses
> it. The same can be said for some of the other "Add function to do X" which
> don't use the function.
> 
> >  std::tuple<uint32_t, uint32_t, int>
> >  DrmDisplayCompositor::GetActiveModeResolution() {
> >    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > index f1965fb..ccaffb4 100644
> > --- a/drmdisplaycompositor.h
> > +++ b/drmdisplaycompositor.h
> > @@ -87,6 +87,7 @@ class DrmDisplayCompositor {
> >    int Init(DrmResources *drm, int display);
> >  
> >    std::unique_ptr<DrmDisplayComposition> CreateComposition() const;
> > +  std::unique_ptr<DrmDisplayComposition> CreateInitializedComposition() const;
> >    int ApplyComposition(std::unique_ptr<DrmDisplayComposition> composition);
> >    int Composite();
> >    int SquashAll();
> > @@ -155,6 +156,7 @@ class DrmDisplayCompositor {
> >    // we need to reset them on every Dump() call.
> >    mutable uint64_t dump_frames_composited_;
> >    mutable uint64_t dump_last_timestamp_ns_;
> > +  std::unique_ptr<Planner> planner_;
> >  };
> >  }
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 14/18] drm_hwcomposer: Fix race in ApplyFrame

[Alexandru-Cosmin Gheorghe]

On Tue, Apr 17, 2018 at 01:02:18PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:25PM +0100, Alexandru Gheorghe wrote:
> > ApplyFrame holds the lock just when it swaps the value of
> > active_composition_, in a multithread context we could end up in a
> > situation where something is shown on the screen, but something else
> > is set in active_composition_. Fix it by holding the lock during
> > CommitFrame.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmdisplaycompositor.cpp | 40 +++++++++++++++++-----------------------
> >  drmdisplaycompositor.h   |  2 +-
> >  2 files changed, 18 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index afd3b05..576539b 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -791,11 +791,6 @@ std::tuple<int, uint32_t> DrmDisplayCompositor::CreateModeBlob(
> >  }
> >  
> >  void DrmDisplayCompositor::ClearDisplay() {
> > -  AutoLock lock(&lock_, "compositor");
> > -  int ret = lock.Lock();
> > -  if (ret)
> > -    return;
> > -
> >    if (!active_composition_)
> >      return;
> >  
> > @@ -808,11 +803,25 @@ void DrmDisplayCompositor::ClearDisplay() {
> >  }
> >  
> >  void DrmDisplayCompositor::ApplyFrame(
> > -    std::unique_ptr<DrmDisplayComposition> composition, int status) {
> > +    std::unique_ptr<DrmDisplayComposition> composition, int status,
> > +    bool writeback) {
> 
> The writeback argument addition seems unrelated to this change.

Agree.

> 
> > +  AutoLock lock(&lock_, __FUNCTION__);
> > +  if (lock.Lock())
> > +    return;
> >    int ret = status;
> > -
> > -  if (!ret)
> > +  if (!ret) {
> > +    if (writeback && !CountdownExpired()) {
> > +      ALOGE("Abort playing back scene");
> > +      return;
> > +    }
> >      ret = CommitFrame(composition.get(), false);
> > +    if (!ret) {
> > +      ++dump_frames_composited_;
> > +      if (active_composition_)
> > +        active_composition_->SignalCompositionDone();
> > +      active_composition_.swap(composition);
> 
> Why move this stuff?

Because both CommitFrame and swap need the lock, the code in between
don't need that.

> 
> > +    }
> > +  }
> >  
> >    if (ret) {
> >      ALOGE("Composite failed for display %d", display_);
> > @@ -821,21 +830,6 @@ void DrmDisplayCompositor::ApplyFrame(
> >      ClearDisplay();
> >      return;
> >    }
> > -  ++dump_frames_composited_;
> > -
> > -  if (active_composition_)
> > -    active_composition_->SignalCompositionDone();
> > -
> > -  ret = pthread_mutex_lock(&lock_);
> > -  if (ret)
> > -    ALOGE("Failed to acquire lock for active_composition swap");
> > -
> > -  active_composition_.swap(composition);
> > -
> > -  if (!ret)
> > -    ret = pthread_mutex_unlock(&lock_);
> > -  if (ret)
> > -    ALOGE("Failed to release lock for active_composition swap");
> >  }
> >  
> >  int DrmDisplayCompositor::ApplyComposition(
> > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > index 0f8daad..b35ef70 100644
> > --- a/drmdisplaycompositor.h
> > +++ b/drmdisplaycompositor.h
> > @@ -127,7 +127,7 @@ class DrmDisplayCompositor {
> >  
> >    void ClearDisplay();
> >    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> > -                  int status);
> > +                  int status, bool writeback = false);
> >  
> >    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 17/18] drm_hwcomposer: Flatten scene synchronously

[Alexandru-Cosmin Gheorghe]

On Tue, Apr 17, 2018 at 01:47:46PM -0400, Sean Paul wrote:
> On Wed, Apr 11, 2018 at 04:22:28PM +0100, Alexandru Gheorghe wrote:
> > Flatten scene on the same CRTC as the one driving the display.
> > The active composition is played back to the display with a buffer
> > attached to the writeback connector.
> > Then we build a composition that has only one plane enabled and that
> > uses the result of the writeback as the input.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++--
> >  drmdisplaycompositor.h   |   7 +-
> >  2 files changed, 204 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > index e535e8a..cb670e6 100644
> > --- a/drmdisplaycompositor.cpp
> > +++ b/drmdisplaycompositor.cpp
> > @@ -36,6 +36,7 @@
> >  #include "drmplane.h"
> >  #include "drmresources.h"
> >  #include "glworker.h"
> > +static const uint32_t kWaitWritebackFence = 100;  // ms
> >  
> >  namespace android {
> >  
> > @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
> >  }
> >  
> >  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > -                                      bool test_only) {
> > +                                      bool test_only,
> > +                                      DrmDisplayComposition *writeback_comp,
> > +                                      DrmConnector *writeback_conn) {
> >    ATRACE_CALL();
> >  
> >    int ret = 0;
> > @@ -532,6 +535,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >    std::vector<DrmCompositionPlane> &comp_planes =
> >        display_comp->composition_planes();
> >    uint64_t out_fences[drm_->crtcs().size()];
> > +  int writeback_fence = -1;
> >  
> >    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> >    if (!connector) {
> > @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >      return -ENOMEM;
> >    }
> >  
> > +  if (writeback_comp != NULL) {
> > +    if (writeback_conn == NULL)
> > +      return -EINVAL;
> > +    if (writeback_conn->writeback_fb_id().id() == 0 ||
> > +        writeback_conn->writeback_out_fence().id() == 0) {
> > +      ALOGE("Writeback properties don't exit");
> > +      return -EINVAL;
> > +    }
> > +    if (writeback_comp->layers().size() != 1) {
> > +      ALOGE("Invalid number of layers for writeback composition");
> > +      return -EINVAL;
> > +    }
> > +    ret = drmModeAtomicAddProperty(
> > +        pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
> > +        writeback_comp->layers().back().buffer->fb_id);
> > +    if (ret < 0) {
> > +      ALOGE("Failed to add writeback_fb_id");
> > +      return ret;
> > +    }
> > +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
> > +                                   writeback_conn->writeback_out_fence().id(),
> > +                                   (uint64_t)&writeback_fence);
> 
> Upcasting int to u64 isn't a great idea, please go the other way (as we do with
> out_fences).

Genuinely curious about this, why does it make a difference I'm
upcasting the address not the int itself.
More, the kernel does this s32 __user *fence_ptr = u64_to_user_ptr(val);
To be completly fair it should have been int32_t.


> 
> > +    if (ret < 0) {
> > +      ALOGE("Failed to add writeback_out_fence");
> > +      return ret;
> > +    }
> > +  }
> 
> This would be more readable if it was split off into a function.
> 
> >    if (crtc->out_fence_ptr_property().id() != 0) {
> > -    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
> > -                                   (uint64_t) &out_fences[crtc->pipe()]);
> > +    ret = drmModeAtomicAddProperty(pset, crtc->id(),
> > +                                   crtc->out_fence_ptr_property().id(),
> > +                                   (uint64_t)&out_fences[crtc->pipe()]);
> >      if (ret < 0) {
> >        ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
> >        drmModeAtomicFree(pset);
> > @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >      }
> >    }
> >  
> > +  if (writeback_conn != NULL) {
> > +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
> > +                                   writeback_conn->crtc_id_property().id(),
> > +                                   crtc->id());
> > +    if (ret < 0) {
> > +      ALOGE("Failed to  attach writeback");
> > +    }
> > +  }
> 
> Can you do this above with the rest of the writeback properties?
> 
> > +
> >    for (DrmCompositionPlane &comp_plane : comp_planes) {
> >      DrmPlane *plane = comp_plane.plane();
> >      DrmCrtc *crtc = comp_plane.crtc();
> > @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >  
> >    if (!ret) {
> >      uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET;
> > -    if (test_only)
> > +    if (test_only) {
> >        flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> > +    } else {
> > +      if (writeback_comp != NULL) {
> > +        if (!CountdownExpired() && active_composition_) {
> 
> Given that we're holding the lock throughout this function, can't you just
> abort at the start?

Yes, we could/should.

> 
> > +          ALOGE("Writeback composition not needed, abort commit");
> > +          drmModeAtomicFree(pset);
> > +          return -EINVAL;
> > +        };
> > +        flags |= DRM_MODE_ATOMIC_NONBLOCK;
> 
> I'm guessing this is the cause of the active_composition race you're fixing
> earlier in the series? Could you please pull this out and squash it into that
> patch as a "Use non-blocking commits" standalone? It might be useful for
> testing, and this is something that's substantial enough to warrant the
> additional visibility of its own patch.

Not really, that race could happen  even without non blocking. The
current implementation is:
1. Commit
2. lock.
3. Set active_composition
4. Unlock.

With two threads there is no guarantee active_composition will contain
what it's been comitted.


> 
> > +      }
> > +    }
> >  
> >      ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
> >      if (ret) {
> > @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >    if (crtc->out_fence_ptr_property().id()) {
> >      display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
> >    }
> > +  if (writeback_fence >= 0) {
> > +    if (writeback_comp->layers().size() != 1) {
> > +      ALOGE("Invalid numbers of layer for writeback_comp");
> > +      return -EINVAL;
> > +    }
> 
> You already test this above?
> 
> > +    writeback_comp->layers()[0].acquire_fence.Set(writeback_fence);
> > +  }
> >  
> >    return ret;
> >  }
> > @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame(
> >        if (active_composition_)
> >          active_composition_->SignalCompositionDone();
> >        active_composition_.swap(composition);
> > +      flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> > +      vsync_worker_.VSyncControl(!writeback);
> >      }
> >    }
> >  
> > @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition(
> >    return ret;
> >  }
> >  
> > +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src,
> > +                                             DrmDisplayComposition *dst,
> > +                                             DrmConnector *writeback_conn) {
> > +  int ret = 0;
> > +  if (src == NULL || dst == NULL)
> > +    return -EINVAL;
> > +  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
> > +  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
> > +                                    src->crtc());
> > +  for (DrmCompositionPlane &comp_plane : src_planes) {
> > +    if (comp_plane.plane() == NULL) {
> > +      ALOGE("Skipping squash all because of NULL plane");
> > +      ret = -EINVAL;
> > +    }
> > +    if (!squashed_comp.plane() &&
> > +        comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
> > +      squashed_comp.set_plane(comp_plane.plane());
> > +    else
> > +      dst->AddPlaneDisable(comp_plane.plane());
> > +  }
> > +
> > +  DrmFramebuffer *writeback_fb = NULL;
> > +  AutoLock lock(&lock_, __FUNCTION__);
> > +  if ((ret = lock.Lock()))
> 
> Same comments regarding assignment in a conditional.
> 
> > +    return ret;
> > +  writeback_fb = &framebuffers_[framebuffer_index_];
> > +  framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
> > +  ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(),
> > +                           mode_.mode.v_display());
> > +  if (ret) {
> > +    ALOGE("Failed to prepare destination buffer");
> > +    return ret;
> > +  }
> > +  lock.Unlock();
> > +  ret = CommitFrame(src, true, dst, writeback_conn);
> > +  if (ret) {
> > +    ALOGE("Atomic check failed");
> > +    return ret;
> > +  }
> > +  if ((ret = lock.Lock()))
> 
> All of these locks and unlocks are going to cause races, as mentioned below,
> please re-evaluate.
> 
> > +    return ret;
> > +  if (!CountdownExpired() && active_composition_) {
> > +    ALOGE("Writeback composition not needed abort");
> > +    return -EINVAL;
> > +  }
> > +  ret = CommitFrame(src, false, dst, writeback_conn);
> > +  lock.Unlock();
> > +  if (ret || dst->layers().size() != 1) {
> > +    ALOGE("Failed to flatten scene using writeback");
> > +    return -EINVAL;
> > +  }
> > +  squashed_comp.source_layers().push_back(0);
> > +  ret =
> > +      sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence);
> 
> line break
> 
> > +  if (ret) {
> > +    ALOGE("Failed to wait on writeback fence");
> > +    return ret;
> > +  }
> > +  ret = dst->AddPlaneComposition(std::move(squashed_comp));
> > +  if (ret) {
> > +    ALOGE("Failed to add flatten scene");
> > +    return ret;
> > +  }
> > +  ret = dst->FinalizeComposition();
> > +  if (ret) {
> > +    ALOGE("Failed to finalize composition");
> > +    return ret;
> > +  }
> > +  return 0;
> > +}
> > +
> > +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) {
> > +  if (writeback_conn->display() != display_) {
> > +    ALOGE("Cannot flatten synchronously on different display");
> > +    return -EINVAL;
> > +  }
> 
> You check this right before calling, so this isn't needed.
> 
> > +  ALOGI("FlattenSynchronously using the same display");
> 
> I think you should downgrade this log level.
> 
> > +  int ret = 0;
> > +  /* Flattened composition with only one layer that is built
> > +   * using the writeback connector
> > +   */
> > +  std::unique_ptr<DrmDisplayComposition> writeback_comp =
> > +      CreateInitializedComposition();
> > +  /* Copy of the active_composition_, we need a copy because
> > +   * if we use the active composition we have to hold the lock
> > +   * for the entire sequence of flattening.
> > +   */
> > +  std::unique_ptr<DrmDisplayComposition> copy_comp =
> > +      CreateInitializedComposition();
> > +
> > +  if (!copy_comp || !writeback_comp)
> > +    return -EINVAL;
> > +  AutoLock lock(&lock_, __FUNCTION__);
> > +  if ((ret = lock.Lock()))
> 
> Assignments in if statements make me nervous. Since you don't need the
> declaration above, just do:
> 
>   int ret = lock.Lock();
>   if (ret)
> 
> > +    return ret;
> > +  if (CountdownExpired()) {
> > +    ret = copy_comp->CopyLayers(active_composition_.get());
> > +    if (ret)
> > +      return ret;
> > +    copy_comp->CopyCompPlanes(active_composition_.get());
> > +  } else {
> > +    return -EINVAL;
> > +  }
> 
>   if (!CountdownExpired())
>     return -EINVAL;
> 
>   ret = copy_comp->CopyLayers(active_composition_.get());
>   if (ret)
>     return ret;
>   copy_comp->CopyCompPlanes(active_composition_.get());
> 
> 
> > +  lock.Unlock();
> 
> Just hold the lock through WritebackComposite(), and consider creating a
> ApplyFrameLocked() to hold it through ApplyFrame(). You should be able to reduce
> the number of times you have to check CountdownExpired() (or remove it)
> significantly by just consistently locking things.
> 
> > +  ret =
> > +      WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn);
> > +  if (ret) {
> > +    ALOGE("Failed to prepare writebackScene");
> > +    return ret;
> > +  }
> > +
> > +  ApplyFrame(std::move(writeback_comp), 0, true);
> > +  return 0;
> > +}
> > +
> >  int DrmDisplayCompositor::FlattenScene() {
> > -  return -EINVAL;
> > +  DrmConnector *writeback_conn =
> > +      drm_->resource_manager()->AvailableWritebackConnector(display_);
> > +  if (!active_composition_ || !writeback_conn)
> > +    return -EINVAL;
> > +  std::vector<DrmCompositionPlane> &src_planes =
> > +      active_composition_->composition_planes();
> > +  size_t src_planes_with_layer = std::count_if(
> > +      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
> > +        return p.type() != DrmCompositionPlane::Type::kDisable;
> > +      });
> > +
> > +  if (src_planes_with_layer <= 1)
> > +    return -EALREADY;
> > +
> > +  if (writeback_conn->display() == display_) {
> > +    return FlattenSynchronously(writeback_conn);
> > +  }
> > +
> > +  return 0;
> >  }
> >  
> >  int DrmDisplayCompositor::SquashAll() {
> > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > index 26201b9..4cc4a5e 100644
> > --- a/drmdisplaycompositor.h
> > +++ b/drmdisplaycompositor.h
> > @@ -125,7 +125,9 @@ class DrmDisplayCompositor {
> >    int ApplySquash(DrmDisplayComposition *display_comp);
> >    int ApplyPreComposite(DrmDisplayComposition *display_comp);
> >    int PrepareFrame(DrmDisplayComposition *display_comp);
> > -  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
> > +  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only,
> > +                  DrmDisplayComposition *writeback_comp = NULL,
> > +                  DrmConnector *writeback_conn = NULL);
> >    int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
> >    int ApplyDpms(DrmDisplayComposition *display_comp);
> >    int DisablePlanes(DrmDisplayComposition *display_comp);
> > @@ -134,7 +136,10 @@ class DrmDisplayCompositor {
> >    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> >                    int status, bool writeback = false);
> >    int FlattenScene();
> > +  int FlattenSynchronously(DrmConnector *writeback_conn);
> 
> Can we come up with a better name than Synchonrous/Asynchronous? Or at the very
> least comment on what that means?
> 
> >  
> > +  int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst,
> > +                         DrmConnector *writeback_conn);
> >    bool CountdownExpired() const;
> >  
> >    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH hwc v2 17/18] drm_hwcomposer: Flatten scene synchronously

[Sean Paul]

On Wed, Apr 18, 2018 at 12:14:03PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Tue, Apr 17, 2018 at 01:47:46PM -0400, Sean Paul wrote:
> > On Wed, Apr 11, 2018 at 04:22:28PM +0100, Alexandru Gheorghe wrote:
> > > Flatten scene on the same CRTC as the one driving the display.
> > > The active composition is played back to the display with a buffer
> > > attached to the writeback connector.
> > > Then we build a composition that has only one plane enabled and that
> > > uses the result of the writeback as the input.
> > > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > ---
> > >  drmdisplaycompositor.cpp | 203 +++++++++++++++++++++++++++++++++++++++++++++--
> > >  drmdisplaycompositor.h   |   7 +-
> > >  2 files changed, 204 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> > > index e535e8a..cb670e6 100644
> > > --- a/drmdisplaycompositor.cpp
> > > +++ b/drmdisplaycompositor.cpp
> > > @@ -36,6 +36,7 @@
> > >  #include "drmplane.h"
> > >  #include "drmresources.h"
> > >  #include "glworker.h"
> > > +static const uint32_t kWaitWritebackFence = 100;  // ms
> > >  
> > >  namespace android {
> > >  
> > > @@ -523,7 +524,9 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
> > >  }
> > >  
> > >  int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > > -                                      bool test_only) {
> > > +                                      bool test_only,
> > > +                                      DrmDisplayComposition *writeback_comp,
> > > +                                      DrmConnector *writeback_conn) {
> > >    ATRACE_CALL();
> > >  
> > >    int ret = 0;
> > > @@ -532,6 +535,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > >    std::vector<DrmCompositionPlane> &comp_planes =
> > >        display_comp->composition_planes();
> > >    uint64_t out_fences[drm_->crtcs().size()];
> > > +  int writeback_fence = -1;
> > >  
> > >    DrmConnector *connector = drm_->GetConnectorForDisplay(display_);
> > >    if (!connector) {
> > > @@ -550,9 +554,37 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > >      return -ENOMEM;
> > >    }
> > >  
> > > +  if (writeback_comp != NULL) {
> > > +    if (writeback_conn == NULL)
> > > +      return -EINVAL;
> > > +    if (writeback_conn->writeback_fb_id().id() == 0 ||
> > > +        writeback_conn->writeback_out_fence().id() == 0) {
> > > +      ALOGE("Writeback properties don't exit");
> > > +      return -EINVAL;
> > > +    }
> > > +    if (writeback_comp->layers().size() != 1) {
> > > +      ALOGE("Invalid number of layers for writeback composition");
> > > +      return -EINVAL;
> > > +    }
> > > +    ret = drmModeAtomicAddProperty(
> > > +        pset, writeback_conn->id(), writeback_conn->writeback_fb_id().id(),
> > > +        writeback_comp->layers().back().buffer->fb_id);
> > > +    if (ret < 0) {
> > > +      ALOGE("Failed to add writeback_fb_id");
> > > +      return ret;
> > > +    }
> > > +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
> > > +                                   writeback_conn->writeback_out_fence().id(),
> > > +                                   (uint64_t)&writeback_fence);
> > 
> > Upcasting int to u64 isn't a great idea, please go the other way (as we do with
> > out_fences).
> 
> Genuinely curious about this, why does it make a difference I'm
> upcasting the address not the int itself.

Right, for some reason I had inserted a * inside the parens with my head. Please
ignore :)

> More, the kernel does this s32 __user *fence_ptr = u64_to_user_ptr(val);
> To be completly fair it should have been int32_t.
> 
> 
> > 
> > > +    if (ret < 0) {
> > > +      ALOGE("Failed to add writeback_out_fence");
> > > +      return ret;
> > > +    }
> > > +  }
> > 
> > This would be more readable if it was split off into a function.
> > 
> > >    if (crtc->out_fence_ptr_property().id() != 0) {
> > > -    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->out_fence_ptr_property().id(),
> > > -                                   (uint64_t) &out_fences[crtc->pipe()]);
> > > +    ret = drmModeAtomicAddProperty(pset, crtc->id(),
> > > +                                   crtc->out_fence_ptr_property().id(),
> > > +                                   (uint64_t)&out_fences[crtc->pipe()]);
> > >      if (ret < 0) {
> > >        ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
> > >        drmModeAtomicFree(pset);
> > > @@ -580,6 +612,15 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > >      }
> > >    }
> > >  
> > > +  if (writeback_conn != NULL) {
> > > +    ret = drmModeAtomicAddProperty(pset, writeback_conn->id(),
> > > +                                   writeback_conn->crtc_id_property().id(),
> > > +                                   crtc->id());
> > > +    if (ret < 0) {
> > > +      ALOGE("Failed to  attach writeback");
> > > +    }
> > > +  }
> > 
> > Can you do this above with the rest of the writeback properties?
> > 
> > > +
> > >    for (DrmCompositionPlane &comp_plane : comp_planes) {
> > >      DrmPlane *plane = comp_plane.plane();
> > >      DrmCrtc *crtc = comp_plane.crtc();
> > > @@ -729,8 +770,18 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > >  
> > >    if (!ret) {
> > >      uint32_t flags = DRM_MODE_ATOMIC_ALLOW_MODESET;
> > > -    if (test_only)
> > > +    if (test_only) {
> > >        flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> > > +    } else {
> > > +      if (writeback_comp != NULL) {
> > > +        if (!CountdownExpired() && active_composition_) {
> > 
> > Given that we're holding the lock throughout this function, can't you just
> > abort at the start?
> 
> Yes, we could/should.
> 
> > 
> > > +          ALOGE("Writeback composition not needed, abort commit");
> > > +          drmModeAtomicFree(pset);
> > > +          return -EINVAL;
> > > +        };
> > > +        flags |= DRM_MODE_ATOMIC_NONBLOCK;
> > 
> > I'm guessing this is the cause of the active_composition race you're fixing
> > earlier in the series? Could you please pull this out and squash it into that
> > patch as a "Use non-blocking commits" standalone? It might be useful for
> > testing, and this is something that's substantial enough to warrant the
> > additional visibility of its own patch.
> 
> Not really, that race could happen  even without non blocking. The
> current implementation is:
> 1. Commit
> 2. lock.
> 3. Set active_composition
> 4. Unlock.
> 
> With two threads there is no guarantee active_composition will contain
> what it's been comitted.
> 

Makes sense, thanks for laying it out.

Sean
> 
> > 
> > > +      }
> > > +    }
> > >  
> > >      ret = drmModeAtomicCommit(drm_->fd(), pset, flags, drm_);
> > >      if (ret) {
> > > @@ -769,6 +820,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> > >    if (crtc->out_fence_ptr_property().id()) {
> > >      display_comp->set_out_fence((int) out_fences[crtc->pipe()]);
> > >    }
> > > +  if (writeback_fence >= 0) {
> > > +    if (writeback_comp->layers().size() != 1) {
> > > +      ALOGE("Invalid numbers of layer for writeback_comp");
> > > +      return -EINVAL;
> > > +    }
> > 
> > You already test this above?
> > 
> > > +    writeback_comp->layers()[0].acquire_fence.Set(writeback_fence);
> > > +  }
> > >  
> > >    return ret;
> > >  }
> > > @@ -837,6 +895,8 @@ void DrmDisplayCompositor::ApplyFrame(
> > >        if (active_composition_)
> > >          active_composition_->SignalCompositionDone();
> > >        active_composition_.swap(composition);
> > > +      flatten_countdown_ = FLATTEN_COUNTDOWN_INIT;
> > > +      vsync_worker_.VSyncControl(!writeback);
> > >      }
> > >    }
> > >  
> > > @@ -913,8 +973,141 @@ int DrmDisplayCompositor::ApplyComposition(
> > >    return ret;
> > >  }
> > >  
> > > +int DrmDisplayCompositor::WritebackComposite(DrmDisplayComposition *src,
> > > +                                             DrmDisplayComposition *dst,
> > > +                                             DrmConnector *writeback_conn) {
> > > +  int ret = 0;
> > > +  if (src == NULL || dst == NULL)
> > > +    return -EINVAL;
> > > +  std::vector<DrmCompositionPlane> &src_planes = src->composition_planes();
> > > +  DrmCompositionPlane squashed_comp(DrmCompositionPlane::Type::kPrecomp, NULL,
> > > +                                    src->crtc());
> > > +  for (DrmCompositionPlane &comp_plane : src_planes) {
> > > +    if (comp_plane.plane() == NULL) {
> > > +      ALOGE("Skipping squash all because of NULL plane");
> > > +      ret = -EINVAL;
> > > +    }
> > > +    if (!squashed_comp.plane() &&
> > > +        comp_plane.plane()->type() == DRM_PLANE_TYPE_PRIMARY)
> > > +      squashed_comp.set_plane(comp_plane.plane());
> > > +    else
> > > +      dst->AddPlaneDisable(comp_plane.plane());
> > > +  }
> > > +
> > > +  DrmFramebuffer *writeback_fb = NULL;
> > > +  AutoLock lock(&lock_, __FUNCTION__);
> > > +  if ((ret = lock.Lock()))
> > 
> > Same comments regarding assignment in a conditional.
> > 
> > > +    return ret;
> > > +  writeback_fb = &framebuffers_[framebuffer_index_];
> > > +  framebuffer_index_ = (framebuffer_index_ + 1) % DRM_DISPLAY_BUFFERS;
> > > +  ret = PrepareFramebuffer(*writeback_fb, dst, mode_.mode.h_display(),
> > > +                           mode_.mode.v_display());
> > > +  if (ret) {
> > > +    ALOGE("Failed to prepare destination buffer");
> > > +    return ret;
> > > +  }
> > > +  lock.Unlock();
> > > +  ret = CommitFrame(src, true, dst, writeback_conn);
> > > +  if (ret) {
> > > +    ALOGE("Atomic check failed");
> > > +    return ret;
> > > +  }
> > > +  if ((ret = lock.Lock()))
> > 
> > All of these locks and unlocks are going to cause races, as mentioned below,
> > please re-evaluate.
> > 
> > > +    return ret;
> > > +  if (!CountdownExpired() && active_composition_) {
> > > +    ALOGE("Writeback composition not needed abort");
> > > +    return -EINVAL;
> > > +  }
> > > +  ret = CommitFrame(src, false, dst, writeback_conn);
> > > +  lock.Unlock();
> > > +  if (ret || dst->layers().size() != 1) {
> > > +    ALOGE("Failed to flatten scene using writeback");
> > > +    return -EINVAL;
> > > +  }
> > > +  squashed_comp.source_layers().push_back(0);
> > > +  ret =
> > > +      sync_wait(dst->layers()[0].acquire_fence.Release(), kWaitWritebackFence);
> > 
> > line break
> > 
> > > +  if (ret) {
> > > +    ALOGE("Failed to wait on writeback fence");
> > > +    return ret;
> > > +  }
> > > +  ret = dst->AddPlaneComposition(std::move(squashed_comp));
> > > +  if (ret) {
> > > +    ALOGE("Failed to add flatten scene");
> > > +    return ret;
> > > +  }
> > > +  ret = dst->FinalizeComposition();
> > > +  if (ret) {
> > > +    ALOGE("Failed to finalize composition");
> > > +    return ret;
> > > +  }
> > > +  return 0;
> > > +}
> > > +
> > > +int DrmDisplayCompositor::FlattenSynchronously(DrmConnector *writeback_conn) {
> > > +  if (writeback_conn->display() != display_) {
> > > +    ALOGE("Cannot flatten synchronously on different display");
> > > +    return -EINVAL;
> > > +  }
> > 
> > You check this right before calling, so this isn't needed.
> > 
> > > +  ALOGI("FlattenSynchronously using the same display");
> > 
> > I think you should downgrade this log level.
> > 
> > > +  int ret = 0;
> > > +  /* Flattened composition with only one layer that is built
> > > +   * using the writeback connector
> > > +   */
> > > +  std::unique_ptr<DrmDisplayComposition> writeback_comp =
> > > +      CreateInitializedComposition();
> > > +  /* Copy of the active_composition_, we need a copy because
> > > +   * if we use the active composition we have to hold the lock
> > > +   * for the entire sequence of flattening.
> > > +   */
> > > +  std::unique_ptr<DrmDisplayComposition> copy_comp =
> > > +      CreateInitializedComposition();
> > > +
> > > +  if (!copy_comp || !writeback_comp)
> > > +    return -EINVAL;
> > > +  AutoLock lock(&lock_, __FUNCTION__);
> > > +  if ((ret = lock.Lock()))
> > 
> > Assignments in if statements make me nervous. Since you don't need the
> > declaration above, just do:
> > 
> >   int ret = lock.Lock();
> >   if (ret)
> > 
> > > +    return ret;
> > > +  if (CountdownExpired()) {
> > > +    ret = copy_comp->CopyLayers(active_composition_.get());
> > > +    if (ret)
> > > +      return ret;
> > > +    copy_comp->CopyCompPlanes(active_composition_.get());
> > > +  } else {
> > > +    return -EINVAL;
> > > +  }
> > 
> >   if (!CountdownExpired())
> >     return -EINVAL;
> > 
> >   ret = copy_comp->CopyLayers(active_composition_.get());
> >   if (ret)
> >     return ret;
> >   copy_comp->CopyCompPlanes(active_composition_.get());
> > 
> > 
> > > +  lock.Unlock();
> > 
> > Just hold the lock through WritebackComposite(), and consider creating a
> > ApplyFrameLocked() to hold it through ApplyFrame(). You should be able to reduce
> > the number of times you have to check CountdownExpired() (or remove it)
> > significantly by just consistently locking things.
> > 
> > > +  ret =
> > > +      WritebackComposite(copy_comp.get(), writeback_comp.get(), writeback_conn);
> > > +  if (ret) {
> > > +    ALOGE("Failed to prepare writebackScene");
> > > +    return ret;
> > > +  }
> > > +
> > > +  ApplyFrame(std::move(writeback_comp), 0, true);
> > > +  return 0;
> > > +}
> > > +
> > >  int DrmDisplayCompositor::FlattenScene() {
> > > -  return -EINVAL;
> > > +  DrmConnector *writeback_conn =
> > > +      drm_->resource_manager()->AvailableWritebackConnector(display_);
> > > +  if (!active_composition_ || !writeback_conn)
> > > +    return -EINVAL;
> > > +  std::vector<DrmCompositionPlane> &src_planes =
> > > +      active_composition_->composition_planes();
> > > +  size_t src_planes_with_layer = std::count_if(
> > > +      src_planes.begin(), src_planes.end(), [](DrmCompositionPlane &p) {
> > > +        return p.type() != DrmCompositionPlane::Type::kDisable;
> > > +      });
> > > +
> > > +  if (src_planes_with_layer <= 1)
> > > +    return -EALREADY;
> > > +
> > > +  if (writeback_conn->display() == display_) {
> > > +    return FlattenSynchronously(writeback_conn);
> > > +  }
> > > +
> > > +  return 0;
> > >  }
> > >  
> > >  int DrmDisplayCompositor::SquashAll() {
> > > diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> > > index 26201b9..4cc4a5e 100644
> > > --- a/drmdisplaycompositor.h
> > > +++ b/drmdisplaycompositor.h
> > > @@ -125,7 +125,9 @@ class DrmDisplayCompositor {
> > >    int ApplySquash(DrmDisplayComposition *display_comp);
> > >    int ApplyPreComposite(DrmDisplayComposition *display_comp);
> > >    int PrepareFrame(DrmDisplayComposition *display_comp);
> > > -  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only);
> > > +  int CommitFrame(DrmDisplayComposition *display_comp, bool test_only,
> > > +                  DrmDisplayComposition *writeback_comp = NULL,
> > > +                  DrmConnector *writeback_conn = NULL);
> > >    int SquashFrame(DrmDisplayComposition *src, DrmDisplayComposition *dst);
> > >    int ApplyDpms(DrmDisplayComposition *display_comp);
> > >    int DisablePlanes(DrmDisplayComposition *display_comp);
> > > @@ -134,7 +136,10 @@ class DrmDisplayCompositor {
> > >    void ApplyFrame(std::unique_ptr<DrmDisplayComposition> composition,
> > >                    int status, bool writeback = false);
> > >    int FlattenScene();
> > > +  int FlattenSynchronously(DrmConnector *writeback_conn);
> > 
> > Can we come up with a better name than Synchonrous/Asynchronous? Or at the very
> > least comment on what that means?
> > 
> > >  
> > > +  int WritebackComposite(DrmDisplayComposition *src, DrmDisplayComposition *dst,
> > > +                         DrmConnector *writeback_conn);
> > >    bool CountdownExpired() const;
> > >  
> > >    std::tuple<int, uint32_t> CreateModeBlob(const DrmMode &mode);
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

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

Re: [PATCH hwc v2 00/18] Add scene flattening support

[John Stultz]

On Fri, Apr 13, 2018 at 5:48 AM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> On second thought, I pushed this patchset here:
> https://github.com/ARM-software/drm-hwcomposer/tree/scene_flattening_support

Thanks for doing this!  For what its worth, I've spun these up on the
HiKey960 (which doesn't have the mali display processor), and they
don't seem to cause any trouble.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel