[RFC][PATCH 0/4 v2] drm_hwcomposer: Changes to support HiKey/HiKey960

[RFC][PATCH 0/4 v2] drm_hwcomposer: Changes to support HiKey/HiKey960

[John Stultz]

Here is a second revision of the early RFC patch set I wanted to
send out to get some review and feedback on.

I've been working to enable the drm_hwcomposer for HiKey and
HiKey960 boards in AOSP, and this patchset contains the required
changes to the drm_hwcomposer code to get things working.

I'm really quite naive when it comes to graphics, and I'm sure I
don't fully understand the drm_hwcomposer code, so forgive me if
I've done anything terribly stupid here.

The first two patches are, I think, mostly straightforward, the
first being a bug fix, and the second adding an importer for
the ARM gralloc implementations.

The next two patches, while largely reworked from v1,  are still
a bit iffy, and I'd appreciate guidance on how to improve them.
The third forcing client composition if there is only one layer
and the fourth providing a glFinish() fallback which fixes
on-screen tearing when the gl compositior fails to initalize.

I also have two additional patches, which I've not included
here - as they still need work, which get the glcompositor to
properly function on HiKey960. But the curious can find them
here:
  https://github.com/johnstultz-work/drm_hwcomposer/commits/drm_hwc

If anyone wants to try the code out on either a HiKey or
HiKey960, you will need just the 4 patches here on top of the
freedesktop/master branch of drm_hwcomposer. You'll also need
the freedesktop/master branch of libdrm. You'll need the
following patches to the device/linaro/hikey project:
 https://github.com/johnstultz-work/android_device_linaro_hikey/commits/drm_hwc

And you'll need to replace the kernel Image-dtb with one built
from the following tree:
 https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/hikey-hwcomposer-deps

Any thoughts or feedback will be very much appreciated!

Many thanks to Rob Herring and Matt Szczesiak for help to get
this working so far! And Sean Paul for his recent help working
out the crtc activiation bug.

Thanks so much for your time!
-john

Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>


John Stultz (4):
  drm_hwcomposer: Make sure we set the active state when doing modesets
  drm_hwcomposer: Add platformhisi buffer importer for hikey and
    hikey960
  drm_hwcomposer: Use client compositing if there is only one plane
  drm_hwcomposer: Try to fallback if GLCompisition fails

 Android.mk               |  15 +++-
 drmdisplaycompositor.cpp |  11 +++
 drmhwctwo.cpp            |   7 ++
 platformhisi.cpp         | 200 +++++++++++++++++++++++++++++++++++++++++++++++
 platformhisi.h           |  50 ++++++++++++
 5 files changed, 282 insertions(+), 1 deletion(-)
 create mode 100644 platformhisi.cpp
 create mode 100644 platformhisi.h

-- 
2.7.4

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

[RFC][PATCH 1/4 v2] drm_hwcomposer: Make sure we set the active state when doing modesets

[John Stultz]

In trying to use drm_hwcomposer with HiKey/HiKey960 boards, I
found that the crtc wouldn't intitalize and the atomic commit
calls were failing.

I initially chased this down to following check in the kernel
drm_atomic_crtc_check() function failing:

 if (state->event && !state->active && !crtc->state->active) {
     DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n",
                      crtc->base.id, crtc->name);
     return -EINVAL;
 }

Where because a fence was submitted state->event was set, but
the crtc state was not active. This results in the atomic commit
to fail and no mode to be set.

After hacking around this in the kernel, Sean Paul helped me
understand that it was the kernel complaining about the crtc
state being provided in the atomic commit which did not have the
active flag set.

Thus, the proper fix is to make sure when we do the modesetting
that we also set the crtc state active flag in property set.

With this change, the kernel no longer rejects the atomic commit
and the crtc initializes properly.

Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drmdisplaycompositor.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index acd13b8..3a20b31 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -520,6 +520,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
   }
 
   if (mode_.needs_modeset) {
+    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->active_property().id(), 1);
+    if (ret < 0) {
+      ALOGE("Failed to add crtc active to pset\n");
+      drmModeAtomicFree(pset);
+      return ret;
+    }
+
     ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->mode_property().id(),
                                    mode_.blob_id) < 0 ||
           drmModeAtomicAddProperty(pset, connector->id(),
-- 
2.7.4

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

[RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[John Stultz]

This allows for importing buffers allocated from the
hikey and hikey960 gralloc implelementations.

Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
* Make platformhisi and the generic importer exclusive in the build
* Fixup vendor check
---
 Android.mk       |  15 ++++-
 platformhisi.cpp | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 platformhisi.h   |  50 ++++++++++++++
 3 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 platformhisi.cpp
 create mode 100644 platformhisi.h

diff --git a/Android.mk b/Android.mk
index 8b11e37..4a91383 100644
--- a/Android.mk
+++ b/Android.mk
@@ -66,7 +66,6 @@ LOCAL_SRC_FILES := \
 	glworker.cpp \
 	hwcutils.cpp \
 	platform.cpp \
-	platformdrmgeneric.cpp \
 	separate_rects.cpp \
 	virtualcompositorworker.cpp \
 	vsyncworker.cpp
@@ -75,7 +74,21 @@ LOCAL_CPPFLAGS += \
 	-DHWC2_USE_CPP11 \
 	-DHWC2_INCLUDE_STRINGIFICATION
 
+
+ifeq ($(TARGET_PRODUCT),hikey960)
+LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER
+LOCAL_C_INCLUDES += device/linaro/hikey/gralloc960/
+LOCAL_SRC_FILES += platformhisi.cpp
+else
+ifeq ($(TARGET_PRODUCT),hikey)
+LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER -DHIKEY
+LOCAL_C_INCLUDES += device/linaro/hikey/gralloc/
+LOCAL_SRC_FILES += platformhisi.cpp
+else
 LOCAL_CPPFLAGS += -DUSE_DRM_GENERIC_IMPORTER
+LOCAL_SRC_FILES += platformdrmgeneric.cpp
+endif
+endif
 
 LOCAL_MODULE := hwcomposer.drm
 LOCAL_MODULE_TAGS := optional
diff --git a/platformhisi.cpp b/platformhisi.cpp
new file mode 100644
index 0000000..b46bf7c
--- /dev/null
+++ b/platformhisi.cpp
@@ -0,0 +1,200 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#define LOG_TAG "hwc-platform-hisi"
+
+#include "drmresources.h"
+#include "platform.h"
+#include "platformhisi.h"
+
+
+#include <drm/drm_fourcc.h>
+#include <cinttypes>
+#include <stdatomic.h>
+#include <xf86drm.h>
+#include <xf86drmMode.h>
+
+#include <cutils/log.h>
+#include <hardware/gralloc.h>
+#include "gralloc_priv.h"
+
+
+namespace android {
+
+#ifdef USE_HISI_IMPORTER
+// static
+Importer *Importer::CreateInstance(DrmResources *drm) {
+  HisiImporter *importer = new HisiImporter(drm);
+  if (!importer)
+    return NULL;
+
+  int ret = importer->Init();
+  if (ret) {
+    ALOGE("Failed to initialize the hisi importer %d", ret);
+    delete importer;
+    return NULL;
+  }
+  return importer;
+}
+#endif
+
+HisiImporter::HisiImporter(DrmResources *drm) : drm_(drm) {
+}
+
+HisiImporter::~HisiImporter() {
+}
+
+int HisiImporter::Init() {
+  int ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
+                          (const hw_module_t **)&gralloc_);
+  if (ret) {
+    ALOGE("Failed to open gralloc module %d", ret);
+    return ret;
+  }
+
+  if (strcasecmp(gralloc_->common.author, "ARM Ltd."))
+    ALOGW("Using non-ARM gralloc module: %s/%s\n", gralloc_->common.name,
+          gralloc_->common.author);
+
+  return 0;
+}
+
+#ifdef HIKEY
+uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
+  switch (hal_format) {
+    case HAL_PIXEL_FORMAT_RGB_888:
+      return DRM_FORMAT_BGR888;
+    case HAL_PIXEL_FORMAT_BGRA_8888:
+      return DRM_FORMAT_ARGB8888;
+    case HAL_PIXEL_FORMAT_RGBX_8888:
+      return DRM_FORMAT_XBGR8888;
+    case HAL_PIXEL_FORMAT_RGBA_8888:
+      return DRM_FORMAT_ABGR8888;
+    case HAL_PIXEL_FORMAT_RGB_565:
+      return DRM_FORMAT_BGR565;
+    case HAL_PIXEL_FORMAT_YV12:
+      return DRM_FORMAT_YVU420;
+    default:
+      ALOGE("Cannot convert hal format to drm format %u", hal_format);
+      return -EINVAL;
+  }
+}
+#else /* HIKEY960 case*/
+uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
+  switch (hal_format) {
+    case HAL_PIXEL_FORMAT_RGB_888:
+      return DRM_FORMAT_BGR888;
+    case HAL_PIXEL_FORMAT_BGRA_8888:
+      return DRM_FORMAT_XBGR8888;
+    case HAL_PIXEL_FORMAT_RGBX_8888:
+      return DRM_FORMAT_XBGR8888;
+    case HAL_PIXEL_FORMAT_RGBA_8888:
+      return DRM_FORMAT_XBGR8888;
+    case HAL_PIXEL_FORMAT_RGB_565:
+      return DRM_FORMAT_BGR565;
+    case HAL_PIXEL_FORMAT_YV12:
+      return DRM_FORMAT_YVU420;
+    default:
+      ALOGE("Cannot convert hal format to drm format %u", hal_format);
+      return -EINVAL;
+  }
+}
+#endif /* HIKEY */
+
+EGLImageKHR HisiImporter::ImportImage(EGLDisplay egl_display, buffer_handle_t handle) {
+  private_handle_t const *hnd = reinterpret_cast < private_handle_t const *>(handle);
+  if (!hnd)
+    return NULL;
+  EGLint attr[] = {
+    EGL_WIDTH, hnd->width,
+    EGL_HEIGHT, hnd->height,
+    EGL_LINUX_DRM_FOURCC_EXT, (EGLint)ConvertHalFormatToDrm(hnd->req_format),
+    EGL_DMA_BUF_PLANE0_FD_EXT, hnd->share_fd,
+    EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
+    EGL_DMA_BUF_PLANE0_PITCH_EXT, hnd->byte_stride,
+    EGL_NONE,
+  };
+  return eglCreateImageKHR(egl_display, EGL_NO_CONTEXT, EGL_LINUX_DMA_BUF_EXT, NULL, attr);
+}
+
+int HisiImporter::ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) {
+  private_handle_t const *hnd = reinterpret_cast < private_handle_t const *>(handle);
+  if (!hnd)
+    return -EINVAL;
+
+  uint32_t gem_handle;
+  int ret = drmPrimeFDToHandle(drm_->fd(), hnd->share_fd, &gem_handle);
+  if (ret) {
+    ALOGE("failed to import prime fd %d ret=%d", hnd->share_fd, ret);
+    return ret;
+  }
+
+  memset(bo, 0, sizeof(hwc_drm_bo_t));
+  bo->width = hnd->width;
+  bo->height = hnd->height;
+  bo->format = ConvertHalFormatToDrm(hnd->req_format);
+  bo->usage = hnd->usage;
+#ifdef HIKEY
+  bo->pitches[0] = hnd->width * 4;
+#else
+  bo->pitches[0] = hnd->byte_stride;
+#endif
+  bo->gem_handles[0] = gem_handle;
+  bo->offsets[0] = 0;
+
+  ret = drmModeAddFB2(drm_->fd(), bo->width, bo->height, bo->format,
+                      bo->gem_handles, bo->pitches, bo->offsets, &bo->fb_id, 0);
+  if (ret) {
+    ALOGE("could not create drm fb %d", ret);
+    return ret;
+  }
+
+  return ret;
+}
+
+int HisiImporter::ReleaseBuffer(hwc_drm_bo_t *bo) {
+  if (bo->fb_id)
+    if (drmModeRmFB(drm_->fd(), bo->fb_id))
+      ALOGE("Failed to rm fb");
+
+  struct drm_gem_close gem_close;
+  memset(&gem_close, 0, sizeof(gem_close));
+  int num_gem_handles = sizeof(bo->gem_handles) / sizeof(bo->gem_handles[0]);
+  for (int i = 0; i < num_gem_handles; i++) {
+    if (!bo->gem_handles[i])
+      continue;
+
+    gem_close.handle = bo->gem_handles[i];
+    int ret = drmIoctl(drm_->fd(), DRM_IOCTL_GEM_CLOSE, &gem_close);
+    if (ret)
+      ALOGE("Failed to close gem handle %d %d", i, ret);
+    else
+      bo->gem_handles[i] = 0;
+  }
+  return 0;
+}
+
+#ifdef USE_HISI_IMPORTER
+std::unique_ptr<Planner> Planner::CreateInstance(DrmResources *) {
+  std::unique_ptr<Planner> planner(new Planner);
+  planner->AddStage<PlanStageGreedy>();
+  return planner;
+}
+#endif
+
+}
+
+
diff --git a/platformhisi.h b/platformhisi.h
new file mode 100644
index 0000000..f7a7d8c
--- /dev/null
+++ b/platformhisi.h
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ANDROID_PLATFORM_HISI_H_
+#define ANDROID_PLATFORM_HISI_H_
+
+#include "drmresources.h"
+#include "platform.h"
+#include "platformdrmgeneric.h"
+
+#include <stdatomic.h>
+
+#include <hardware/gralloc.h>
+
+namespace android {
+
+class HisiImporter : public Importer {
+ public:
+  HisiImporter(DrmResources *drm);
+  ~HisiImporter() override;
+
+  int Init();
+
+  EGLImageKHR ImportImage(EGLDisplay egl_display, buffer_handle_t handle) override;
+  int ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) override;
+  int ReleaseBuffer(hwc_drm_bo_t *bo) override;
+
+ private:
+  uint32_t ConvertHalFormatToDrm(uint32_t hal_format);
+
+  DrmResources *drm_;
+
+  const gralloc_module_t *gralloc_;
+};
+}
+
+#endif
-- 
2.7.4

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

[RFC][PATCH 3/4 v2] drm_hwcomposer: Use client compositing if there is only one plane

[John Stultz]

Originally based on work by Rob Herring, this patch changes
ValidateDisplay() so that if there is only one plane, we modify
Device composited layers to be Client composited.

Without this, on devices with just one plane, nothing gets
displayed on the screen.

Suggestions for alternative solutions here would be greatly
appreciated!

Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
* Rework Rob's change to check planes
---
 drmhwctwo.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index dfca1a6..6d88c5c 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -695,6 +695,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
         layer.set_validated_type(HWC2::Composition::Client);
         ++*num_types;
         break;
+      case HWC2::Composition::Device:
+	/* If we only have one plane, always do Client composition */
+        if (primary_planes_.size() + overlay_planes_.size() == 1) {
+          layer.set_validated_type(HWC2::Composition::Client);
+          ++*num_types;
+          break;
+        }
       default:
         layer.set_validated_type(layer.sf_type());
         break;
-- 
2.7.4

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

[RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

When using drm_hwcomposer with the hikey board, the resulting
display shows lots of tearing.

This seems to be due to EGLcomposition not initializing
properly, potentially due to I'm guessing limitations of what
the  utgard mali driver can do. I've noted that with the
HiKey960 board, this patch is *not* necessary.

Hacking around a bit, I found that since the glworker code
isn't running properly, we never call glFinish(), which
is required to fix the tearing.

Ideas for a better way to implement this would be greatly
appreciated!

Cc: Marissa Wall <marissaw@google.com>
Cc: Sean Paul <seanpaul@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Robert Foss <robert.foss@collabora.com>
Cc: Matt Szczesiak <matt.szczesiak@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Hanna <david.hanna11@gmail.com>
Cc: Rob Herring <rob.herring@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
* Simplified, focusing on the key glFinsh() call
---
 drmdisplaycompositor.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index 3a20b31..eb0b77a 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -439,6 +439,10 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
 
       fb.set_release_fence_fd(ret);
       ret = 0;
+    } else {
+      /*If we're not doing anything, block to avoid tearing */
+      glFinish();
+      return 0;
     }
   }
 
-- 
2.7.4

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

Re: [RFC][PATCH 3/4 v2] drm_hwcomposer: Use client compositing if there is only one plane

[Rob Herring]

On Tue, Jan 23, 2018 at 5:16 PM, John Stultz <john.stultz@linaro.org> wrote:
> Originally based on work by Rob Herring, this patch changes
> ValidateDisplay() so that if there is only one plane, we modify
> Device composited layers to be Client composited.
>
> Without this, on devices with just one plane, nothing gets
> displayed on the screen.
>
> Suggestions for alternative solutions here would be greatly
> appreciated!
>
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Rework Rob's change to check planes
> ---
>  drmhwctwo.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index dfca1a6..6d88c5c 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -695,6 +695,13 @@ HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>          layer.set_validated_type(HWC2::Composition::Client);
>          ++*num_types;
>          break;
> +      case HWC2::Composition::Device:
> +       /* If we only have one plane, always do Client composition */
> +        if (primary_planes_.size() + overlay_planes_.size() == 1) {
> +          layer.set_validated_type(HWC2::Composition::Client);
> +          ++*num_types;
> +          break;
> +        }

This needs to be conditional on GL compositing being disabled (either
thru init failure or a property flag). Also, this can be generalized
to use as many planes as we have. Something like the following patch.

BTW, "gl_enabled" doesn't exist. I left that for you to figure out how
to set and propagate.

diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
index dfca1a6e9d2d..d7a85bbf67dd 100644
--- a/drmhwctwo.cpp
+++ b/drmhwctwo.cpp
@@ -684,6 +684,8 @@ HWC2::Error
DrmHwcTwo::HwcDisplay::SetVsyncEnabled(int32_t enabled) {
 HWC2::Error DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
                                                    uint32_t *num_requests) {
   supported(__func__);
+  int planes = primary_planes_.size() + overlay_planes_.size();
+
   *num_types = 0;
   *num_requests = 0;
   for (std::pair<const hwc2_layer_t, DrmHwcTwo::HwcLayer> &l : layers_) {
@@ -696,7 +698,12 @@ HWC2::Error
DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
         ++*num_types;
         break;
       default:
-        layer.set_validated_type(layer.sf_type());
+        if (--planes > 0 || gl_enabled) {
+          layer.set_validated_type(layer.sf_type());
+        } else {
+          layer.set_validated_type(HWC2::Composition::Client);
+          ++*num_types;
+        }
         break;
     }
   }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[Sean Paul]

On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
> This allows for importing buffers allocated from the
> hikey and hikey960 gralloc implelementations.
> 
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Make platformhisi and the generic importer exclusive in the build

I actually prefer the opposite. If everything is always compiled, we reduce the
chance of breaking boards when the base class is updated. I'm sure there is a
good reason for this, but perhaps there's another way?

Sean

> * Fixup vendor check
> ---
>  Android.mk       |  15 ++++-
>  platformhisi.cpp | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  platformhisi.h   |  50 ++++++++++++++
>  3 files changed, 264 insertions(+), 1 deletion(-)
>  create mode 100644 platformhisi.cpp
>  create mode 100644 platformhisi.h
> 
> diff --git a/Android.mk b/Android.mk
> index 8b11e37..4a91383 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -66,7 +66,6 @@ LOCAL_SRC_FILES := \
>  	glworker.cpp \
>  	hwcutils.cpp \
>  	platform.cpp \
> -	platformdrmgeneric.cpp \
>  	separate_rects.cpp \
>  	virtualcompositorworker.cpp \
>  	vsyncworker.cpp
> @@ -75,7 +74,21 @@ LOCAL_CPPFLAGS += \
>  	-DHWC2_USE_CPP11 \
>  	-DHWC2_INCLUDE_STRINGIFICATION
>  
> +
> +ifeq ($(TARGET_PRODUCT),hikey960)
> +LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER
> +LOCAL_C_INCLUDES += device/linaro/hikey/gralloc960/
> +LOCAL_SRC_FILES += platformhisi.cpp
> +else
> +ifeq ($(TARGET_PRODUCT),hikey)
> +LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER -DHIKEY
> +LOCAL_C_INCLUDES += device/linaro/hikey/gralloc/
> +LOCAL_SRC_FILES += platformhisi.cpp
> +else
>  LOCAL_CPPFLAGS += -DUSE_DRM_GENERIC_IMPORTER
> +LOCAL_SRC_FILES += platformdrmgeneric.cpp
> +endif
> +endif
>  
>  LOCAL_MODULE := hwcomposer.drm
>  LOCAL_MODULE_TAGS := optional
> diff --git a/platformhisi.cpp b/platformhisi.cpp
> new file mode 100644
> index 0000000..b46bf7c
> --- /dev/null
> +++ b/platformhisi.cpp
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright (C) 2015 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#define LOG_TAG "hwc-platform-hisi"
> +
> +#include "drmresources.h"
> +#include "platform.h"
> +#include "platformhisi.h"
> +
> +
> +#include <drm/drm_fourcc.h>
> +#include <cinttypes>
> +#include <stdatomic.h>
> +#include <xf86drm.h>
> +#include <xf86drmMode.h>
> +
> +#include <cutils/log.h>
> +#include <hardware/gralloc.h>
> +#include "gralloc_priv.h"
> +
> +
> +namespace android {
> +
> +#ifdef USE_HISI_IMPORTER
> +// static
> +Importer *Importer::CreateInstance(DrmResources *drm) {
> +  HisiImporter *importer = new HisiImporter(drm);
> +  if (!importer)
> +    return NULL;
> +
> +  int ret = importer->Init();
> +  if (ret) {
> +    ALOGE("Failed to initialize the hisi importer %d", ret);
> +    delete importer;
> +    return NULL;
> +  }
> +  return importer;
> +}
> +#endif
> +
> +HisiImporter::HisiImporter(DrmResources *drm) : drm_(drm) {
> +}
> +
> +HisiImporter::~HisiImporter() {
> +}
> +
> +int HisiImporter::Init() {
> +  int ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> +                          (const hw_module_t **)&gralloc_);
> +  if (ret) {
> +    ALOGE("Failed to open gralloc module %d", ret);
> +    return ret;
> +  }
> +
> +  if (strcasecmp(gralloc_->common.author, "ARM Ltd."))
> +    ALOGW("Using non-ARM gralloc module: %s/%s\n", gralloc_->common.name,
> +          gralloc_->common.author);
> +
> +  return 0;
> +}
> +
> +#ifdef HIKEY
> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
> +  switch (hal_format) {
> +    case HAL_PIXEL_FORMAT_RGB_888:
> +      return DRM_FORMAT_BGR888;
> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> +      return DRM_FORMAT_ARGB8888;
> +    case HAL_PIXEL_FORMAT_RGBX_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGBA_8888:
> +      return DRM_FORMAT_ABGR8888;
> +    case HAL_PIXEL_FORMAT_RGB_565:
> +      return DRM_FORMAT_BGR565;
> +    case HAL_PIXEL_FORMAT_YV12:
> +      return DRM_FORMAT_YVU420;
> +    default:
> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
> +      return -EINVAL;
> +  }
> +}
> +#else /* HIKEY960 case*/
> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
> +  switch (hal_format) {
> +    case HAL_PIXEL_FORMAT_RGB_888:
> +      return DRM_FORMAT_BGR888;
> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGBX_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGBA_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGB_565:
> +      return DRM_FORMAT_BGR565;
> +    case HAL_PIXEL_FORMAT_YV12:
> +      return DRM_FORMAT_YVU420;
> +    default:
> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
> +      return -EINVAL;
> +  }
> +}
> +#endif /* HIKEY */
> +
> +EGLImageKHR HisiImporter::ImportImage(EGLDisplay egl_display, buffer_handle_t handle) {
> +  private_handle_t const *hnd = reinterpret_cast < private_handle_t const *>(handle);
> +  if (!hnd)
> +    return NULL;
> +  EGLint attr[] = {
> +    EGL_WIDTH, hnd->width,
> +    EGL_HEIGHT, hnd->height,
> +    EGL_LINUX_DRM_FOURCC_EXT, (EGLint)ConvertHalFormatToDrm(hnd->req_format),
> +    EGL_DMA_BUF_PLANE0_FD_EXT, hnd->share_fd,
> +    EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
> +    EGL_DMA_BUF_PLANE0_PITCH_EXT, hnd->byte_stride,
> +    EGL_NONE,
> +  };
> +  return eglCreateImageKHR(egl_display, EGL_NO_CONTEXT, EGL_LINUX_DMA_BUF_EXT, NULL, attr);
> +}
> +
> +int HisiImporter::ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) {
> +  private_handle_t const *hnd = reinterpret_cast < private_handle_t const *>(handle);
> +  if (!hnd)
> +    return -EINVAL;
> +
> +  uint32_t gem_handle;
> +  int ret = drmPrimeFDToHandle(drm_->fd(), hnd->share_fd, &gem_handle);
> +  if (ret) {
> +    ALOGE("failed to import prime fd %d ret=%d", hnd->share_fd, ret);
> +    return ret;
> +  }
> +
> +  memset(bo, 0, sizeof(hwc_drm_bo_t));
> +  bo->width = hnd->width;
> +  bo->height = hnd->height;
> +  bo->format = ConvertHalFormatToDrm(hnd->req_format);
> +  bo->usage = hnd->usage;
> +#ifdef HIKEY
> +  bo->pitches[0] = hnd->width * 4;
> +#else
> +  bo->pitches[0] = hnd->byte_stride;
> +#endif
> +  bo->gem_handles[0] = gem_handle;
> +  bo->offsets[0] = 0;
> +
> +  ret = drmModeAddFB2(drm_->fd(), bo->width, bo->height, bo->format,
> +                      bo->gem_handles, bo->pitches, bo->offsets, &bo->fb_id, 0);
> +  if (ret) {
> +    ALOGE("could not create drm fb %d", ret);
> +    return ret;
> +  }
> +
> +  return ret;
> +}
> +
> +int HisiImporter::ReleaseBuffer(hwc_drm_bo_t *bo) {
> +  if (bo->fb_id)
> +    if (drmModeRmFB(drm_->fd(), bo->fb_id))
> +      ALOGE("Failed to rm fb");
> +
> +  struct drm_gem_close gem_close;
> +  memset(&gem_close, 0, sizeof(gem_close));
> +  int num_gem_handles = sizeof(bo->gem_handles) / sizeof(bo->gem_handles[0]);
> +  for (int i = 0; i < num_gem_handles; i++) {
> +    if (!bo->gem_handles[i])
> +      continue;
> +
> +    gem_close.handle = bo->gem_handles[i];
> +    int ret = drmIoctl(drm_->fd(), DRM_IOCTL_GEM_CLOSE, &gem_close);
> +    if (ret)
> +      ALOGE("Failed to close gem handle %d %d", i, ret);
> +    else
> +      bo->gem_handles[i] = 0;
> +  }
> +  return 0;
> +}
> +
> +#ifdef USE_HISI_IMPORTER
> +std::unique_ptr<Planner> Planner::CreateInstance(DrmResources *) {
> +  std::unique_ptr<Planner> planner(new Planner);
> +  planner->AddStage<PlanStageGreedy>();
> +  return planner;
> +}
> +#endif
> +
> +}
> +
> +
> diff --git a/platformhisi.h b/platformhisi.h
> new file mode 100644
> index 0000000..f7a7d8c
> --- /dev/null
> +++ b/platformhisi.h
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (C) 2015 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef ANDROID_PLATFORM_HISI_H_
> +#define ANDROID_PLATFORM_HISI_H_
> +
> +#include "drmresources.h"
> +#include "platform.h"
> +#include "platformdrmgeneric.h"
> +
> +#include <stdatomic.h>
> +
> +#include <hardware/gralloc.h>
> +
> +namespace android {
> +
> +class HisiImporter : public Importer {
> + public:
> +  HisiImporter(DrmResources *drm);
> +  ~HisiImporter() override;
> +
> +  int Init();
> +
> +  EGLImageKHR ImportImage(EGLDisplay egl_display, buffer_handle_t handle) override;
> +  int ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) override;
> +  int ReleaseBuffer(hwc_drm_bo_t *bo) override;
> +
> + private:
> +  uint32_t ConvertHalFormatToDrm(uint32_t hal_format);
> +
> +  DrmResources *drm_;
> +
> +  const gralloc_module_t *gralloc_;
> +};
> +}
> +
> +#endif
> -- 
> 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: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[Sean Paul]

On Tue, Jan 23, 2018 at 03:16:39PM -0800, John Stultz wrote:
> When using drm_hwcomposer with the hikey board, the resulting
> display shows lots of tearing.
> 
> This seems to be due to EGLcomposition not initializing
> properly, potentially due to I'm guessing limitations of what
> the  utgard mali driver can do. I've noted that with the
> HiKey960 board, this patch is *not* necessary.
> 
> Hacking around a bit, I found that since the glworker code
> isn't running properly, we never call glFinish(), which
> is required to fix the tearing.
> 
> Ideas for a better way to implement this would be greatly
> appreciated!

Sounds like you'll have to dig into the gl compositor to fix this. I think
chances are quite good there's a better way than below.

Good luck!

> 
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Simplified, focusing on the key glFinsh() call
> ---
>  drmdisplaycompositor.cpp | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index 3a20b31..eb0b77a 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -439,6 +439,10 @@ int DrmDisplayCompositor::PrepareFrame(DrmDisplayComposition *display_comp) {
>  
>        fb.set_release_fence_fd(ret);
>        ret = 0;
> +    } else {
> +      /*If we're not doing anything, block to avoid tearing */
> +      glFinish();
> +      return 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: [RFC][PATCH 1/4 v2] drm_hwcomposer: Make sure we set the active state when doing modesets

[Sean Paul]

On Tue, Jan 23, 2018 at 03:16:36PM -0800, John Stultz wrote:
> In trying to use drm_hwcomposer with HiKey/HiKey960 boards, I
> found that the crtc wouldn't intitalize and the atomic commit
> calls were failing.
> 
> I initially chased this down to following check in the kernel
> drm_atomic_crtc_check() function failing:
> 
>  if (state->event && !state->active && !crtc->state->active) {
>      DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n",
>                       crtc->base.id, crtc->name);
>      return -EINVAL;
>  }
> 
> Where because a fence was submitted state->event was set, but
> the crtc state was not active. This results in the atomic commit
> to fail and no mode to be set.
> 
> After hacking around this in the kernel, Sean Paul helped me
> understand that it was the kernel complaining about the crtc
> state being provided in the atomic commit which did not have the
> active flag set.
> 
> Thus, the proper fix is to make sure when we do the modesetting
> that we also set the crtc state active flag in property set.
> 
> With this change, the kernel no longer rejects the atomic commit
> and the crtc initializes properly.
> 
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Applied.

Thanks for the patch!

Sean

> ---
>  drmdisplaycompositor.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index acd13b8..3a20b31 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -520,6 +520,13 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>    }
>  
>    if (mode_.needs_modeset) {
> +    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->active_property().id(), 1);
> +    if (ret < 0) {
> +      ALOGE("Failed to add crtc active to pset\n");
> +      drmModeAtomicFree(pset);
> +      return ret;
> +    }
> +
>      ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->mode_property().id(),
>                                     mode_.blob_id) < 0 ||
>            drmModeAtomicAddProperty(pset, connector->id(),
> -- 
> 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: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[Sean Paul]

On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
> This allows for importing buffers allocated from the
> hikey and hikey960 gralloc implelementations.
> 
> Cc: Marissa Wall <marissaw@google.com>
> Cc: Sean Paul <seanpaul@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Robert Foss <robert.foss@collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: David Hanna <david.hanna11@gmail.com>
> Cc: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Make platformhisi and the generic importer exclusive in the build
> * Fixup vendor check

Hi John,
Thanks for your patches, I have a few more comments on the code aside from the
Makefile nit.

> ---
>  Android.mk       |  15 ++++-
>  platformhisi.cpp | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  platformhisi.h   |  50 ++++++++++++++
>  3 files changed, 264 insertions(+), 1 deletion(-)
>  create mode 100644 platformhisi.cpp
>  create mode 100644 platformhisi.h
> 
> diff --git a/Android.mk b/Android.mk
> index 8b11e37..4a91383 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -66,7 +66,6 @@ LOCAL_SRC_FILES := \
>  	glworker.cpp \
>  	hwcutils.cpp \
>  	platform.cpp \
> -	platformdrmgeneric.cpp \
>  	separate_rects.cpp \
>  	virtualcompositorworker.cpp \
>  	vsyncworker.cpp
> @@ -75,7 +74,21 @@ LOCAL_CPPFLAGS += \
>  	-DHWC2_USE_CPP11 \
>  	-DHWC2_INCLUDE_STRINGIFICATION
>  
> +
> +ifeq ($(TARGET_PRODUCT),hikey960)
> +LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER
> +LOCAL_C_INCLUDES += device/linaro/hikey/gralloc960/
> +LOCAL_SRC_FILES += platformhisi.cpp
> +else
> +ifeq ($(TARGET_PRODUCT),hikey)
> +LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER -DHIKEY
> +LOCAL_C_INCLUDES += device/linaro/hikey/gralloc/
> +LOCAL_SRC_FILES += platformhisi.cpp
> +else
>  LOCAL_CPPFLAGS += -DUSE_DRM_GENERIC_IMPORTER
> +LOCAL_SRC_FILES += platformdrmgeneric.cpp
> +endif
> +endif
>  
>  LOCAL_MODULE := hwcomposer.drm
>  LOCAL_MODULE_TAGS := optional
> diff --git a/platformhisi.cpp b/platformhisi.cpp
> new file mode 100644
> index 0000000..b46bf7c
> --- /dev/null
> +++ b/platformhisi.cpp
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright (C) 2015 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#define LOG_TAG "hwc-platform-hisi"
> +
> +#include "drmresources.h"
> +#include "platform.h"
> +#include "platformhisi.h"
> +
> +
> +#include <drm/drm_fourcc.h>
> +#include <cinttypes>
> +#include <stdatomic.h>
> +#include <xf86drm.h>
> +#include <xf86drmMode.h>
> +
> +#include <cutils/log.h>
> +#include <hardware/gralloc.h>
> +#include "gralloc_priv.h"
> +
> +
> +namespace android {
> +
> +#ifdef USE_HISI_IMPORTER
> +// static
> +Importer *Importer::CreateInstance(DrmResources *drm) {
> +  HisiImporter *importer = new HisiImporter(drm);
> +  if (!importer)
> +    return NULL;
> +
> +  int ret = importer->Init();
> +  if (ret) {
> +    ALOGE("Failed to initialize the hisi importer %d", ret);
> +    delete importer;
> +    return NULL;
> +  }
> +  return importer;
> +}
> +#endif
> +
> +HisiImporter::HisiImporter(DrmResources *drm) : drm_(drm) {
> +}
> +
> +HisiImporter::~HisiImporter() {
> +}
> +
> +int HisiImporter::Init() {
> +  int ret = hw_get_module(GRALLOC_HARDWARE_MODULE_ID,
> +                          (const hw_module_t **)&gralloc_);
> +  if (ret) {
> +    ALOGE("Failed to open gralloc module %d", ret);
> +    return ret;
> +  }
> +
> +  if (strcasecmp(gralloc_->common.author, "ARM Ltd."))
> +    ALOGW("Using non-ARM gralloc module: %s/%s\n", gralloc_->common.name,
> +          gralloc_->common.author);
> +
> +  return 0;
> +}
> +
> +#ifdef HIKEY
> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
> +  switch (hal_format) {
> +    case HAL_PIXEL_FORMAT_RGB_888:
> +      return DRM_FORMAT_BGR888;
> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> +      return DRM_FORMAT_ARGB8888;
> +    case HAL_PIXEL_FORMAT_RGBX_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGBA_8888:
> +      return DRM_FORMAT_ABGR8888;
> +    case HAL_PIXEL_FORMAT_RGB_565:
> +      return DRM_FORMAT_BGR565;
> +    case HAL_PIXEL_FORMAT_YV12:
> +      return DRM_FORMAT_YVU420;
> +    default:
> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
> +      return -EINVAL;
> +  }

This is the same as the generic case, and below is a little confusing to me.

> +}
> +#else /* HIKEY960 case*/
> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
> +  switch (hal_format) {
> +    case HAL_PIXEL_FORMAT_RGB_888:
> +      return DRM_FORMAT_BGR888;
> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGBX_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGBA_8888:
> +      return DRM_FORMAT_XBGR8888;
> +    case HAL_PIXEL_FORMAT_RGB_565:
> +      return DRM_FORMAT_BGR565;
> +    case HAL_PIXEL_FORMAT_YV12:
> +      return DRM_FORMAT_YVU420;
> +    default:
> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
> +      return -EINVAL;
> +  }

So it seems like we're smashing a whole bunch of formats into XBGR8888 here, and
I'm not sure how it'll work. If you look above, the primary difference between
the HAL and drm formats is that the subpixels are reversed (ie: RGBA becomes
ABGR). In this snippet, you've got BGRA->XBGR, does this actually work?

Furthermore, when I look at the kirin driver (I think that's what you use on
hikey?), I see full support for all the above formats. So 2 questions:

1- Does this work as expected
2- Is there an opportunity to share code between platforms instead of
copy-pasting this function over and over again? Perhaps each platform provides
the formats they support and we have the base class do a mapping based on what
is present?

> +}
> +#endif /* HIKEY */
> +
> +EGLImageKHR HisiImporter::ImportImage(EGLDisplay egl_display, buffer_handle_t handle) {
> +  private_handle_t const *hnd = reinterpret_cast < private_handle_t const *>(handle);
> +  if (!hnd)
> +    return NULL;
> +  EGLint attr[] = {
> +    EGL_WIDTH, hnd->width,
> +    EGL_HEIGHT, hnd->height,
> +    EGL_LINUX_DRM_FOURCC_EXT, (EGLint)ConvertHalFormatToDrm(hnd->req_format),
> +    EGL_DMA_BUF_PLANE0_FD_EXT, hnd->share_fd,
> +    EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
> +    EGL_DMA_BUF_PLANE0_PITCH_EXT, hnd->byte_stride,
> +    EGL_NONE,
> +  };
> +  return eglCreateImageKHR(egl_display, EGL_NO_CONTEXT, EGL_LINUX_DMA_BUF_EXT, NULL, attr);
> +}
> +
> +int HisiImporter::ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) {
> +  private_handle_t const *hnd = reinterpret_cast < private_handle_t const *>(handle);
> +  if (!hnd)
> +    return -EINVAL;
> +
> +  uint32_t gem_handle;
> +  int ret = drmPrimeFDToHandle(drm_->fd(), hnd->share_fd, &gem_handle);
> +  if (ret) {
> +    ALOGE("failed to import prime fd %d ret=%d", hnd->share_fd, ret);
> +    return ret;
> +  }
> +
> +  memset(bo, 0, sizeof(hwc_drm_bo_t));
> +  bo->width = hnd->width;
> +  bo->height = hnd->height;
> +  bo->format = ConvertHalFormatToDrm(hnd->req_format);
> +  bo->usage = hnd->usage;
> +#ifdef HIKEY
> +  bo->pitches[0] = hnd->width * 4;

Does this work for formats that are not 32-bit?

> +#else
> +  bo->pitches[0] = hnd->byte_stride;
> +#endif
> +  bo->gem_handles[0] = gem_handle;
> +  bo->offsets[0] = 0;
> +
> +  ret = drmModeAddFB2(drm_->fd(), bo->width, bo->height, bo->format,
> +                      bo->gem_handles, bo->pitches, bo->offsets, &bo->fb_id, 0);
> +  if (ret) {
> +    ALOGE("could not create drm fb %d", ret);
> +    return ret;
> +  }
> +
> +  return ret;
> +}
> +
> +int HisiImporter::ReleaseBuffer(hwc_drm_bo_t *bo) {
> +  if (bo->fb_id)
> +    if (drmModeRmFB(drm_->fd(), bo->fb_id))
> +      ALOGE("Failed to rm fb");
> +
> +  struct drm_gem_close gem_close;
> +  memset(&gem_close, 0, sizeof(gem_close));
> +  int num_gem_handles = sizeof(bo->gem_handles) / sizeof(bo->gem_handles[0]);
> +  for (int i = 0; i < num_gem_handles; i++) {
> +    if (!bo->gem_handles[i])
> +      continue;
> +
> +    gem_close.handle = bo->gem_handles[i];
> +    int ret = drmIoctl(drm_->fd(), DRM_IOCTL_GEM_CLOSE, &gem_close);
> +    if (ret)
> +      ALOGE("Failed to close gem handle %d %d", i, ret);
> +    else
> +      bo->gem_handles[i] = 0;
> +  }
> +  return 0;
> +}

This function is a straight copy-paste from generic, right? Let's try to do
better than that. Perhaps you should be subclassing the drm generic platform
instead?

> +
> +#ifdef USE_HISI_IMPORTER
> +std::unique_ptr<Planner> Planner::CreateInstance(DrmResources *) {
> +  std::unique_ptr<Planner> planner(new Planner);
> +  planner->AddStage<PlanStageGreedy>();
> +  return planner;
> +}
> +#endif
> +
> +}
> +
> +
> diff --git a/platformhisi.h b/platformhisi.h
> new file mode 100644
> index 0000000..f7a7d8c
> --- /dev/null
> +++ b/platformhisi.h
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (C) 2015 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef ANDROID_PLATFORM_HISI_H_
> +#define ANDROID_PLATFORM_HISI_H_
> +
> +#include "drmresources.h"
> +#include "platform.h"
> +#include "platformdrmgeneric.h"
> +
> +#include <stdatomic.h>
> +
> +#include <hardware/gralloc.h>
> +
> +namespace android {
> +
> +class HisiImporter : public Importer {
> + public:
> +  HisiImporter(DrmResources *drm);
> +  ~HisiImporter() override;
> +
> +  int Init();
> +
> +  EGLImageKHR ImportImage(EGLDisplay egl_display, buffer_handle_t handle) override;
> +  int ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) override;
> +  int ReleaseBuffer(hwc_drm_bo_t *bo) override;
> +
> + private:
> +  uint32_t ConvertHalFormatToDrm(uint32_t hal_format);
> +
> +  DrmResources *drm_;
> +
> +  const gralloc_module_t *gralloc_;
> +};
> +}
> +
> +#endif
> -- 
> 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: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[John Stultz]

On Wed, Jan 24, 2018 at 7:23 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
>> This allows for importing buffers allocated from the
>> hikey and hikey960 gralloc implelementations.
>>
>> Cc: Marissa Wall <marissaw@google.com>
>> Cc: Sean Paul <seanpaul@google.com>
>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>> Cc: Robert Foss <robert.foss@collabora.com>
>> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
>> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
>> Cc: David Hanna <david.hanna11@gmail.com>
>> Cc: Rob Herring <rob.herring@linaro.org>
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>> v2:
>> * Make platformhisi and the generic importer exclusive in the build
>
> I actually prefer the opposite. If everything is always compiled, we reduce the
> chance of breaking boards when the base class is updated. I'm sure there is a
> good reason for this, but perhaps there's another way?

The main reason for this was avoiding the build breaking when
"gralloc_priv.h" isn't supplied or present.

Similarly the current freedesktop/master code won't build in the
Android environment as the platformdrmgeneric.cpp file depends on the
gralloc_drm_handle.h, which doesn't exist.

Earlier to avoid this I had included Rob's "provide a common gralloc
handle definition" patch, but build-conditionalizing the importers
seemed like a sane solution.

I'm of course open to alternatives, but I'm not sure how to add the
hisi importer and the drmgeneric one and keep things building without
also adding copies of the various gralloc headers to the project as
well.

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

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

On Wed, Jan 24, 2018 at 7:26 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, Jan 23, 2018 at 03:16:39PM -0800, John Stultz wrote:
>> When using drm_hwcomposer with the hikey board, the resulting
>> display shows lots of tearing.
>>
>> This seems to be due to EGLcomposition not initializing
>> properly, potentially due to I'm guessing limitations of what
>> the  utgard mali driver can do. I've noted that with the
>> HiKey960 board, this patch is *not* necessary.
>>
>> Hacking around a bit, I found that since the glworker code
>> isn't running properly, we never call glFinish(), which
>> is required to fix the tearing.
>>
>> Ideas for a better way to implement this would be greatly
>> appreciated!
>
> Sounds like you'll have to dig into the gl compositor to fix this. I think
> chances are quite good there's a better way than below.

Yes, I did spent a little bit of time earlier trying to rewrite the gl
shader to try to build for the utgard level hardware, but wasn't very
successful. At a deeper level I guess I'm not sure the glcompositor is
useful in this case, since we're doing single plane client side
compositing (as short of the glFinish bit, not running it doesn't seem
to keep things from working).  But I'll look into that again.

Again forgive as I really a bit in the dark on most graphics details,
but the other more basic question I'm a bit unsure of is, does this
patch even make any functional sense?  If we're not using the
glcompositor and are using the atomic commit in ApplyFrame(), why the
need for glFinish to avoid tearing?  Is it that the we commit the
frame atomically to the display, but if we don't block w/ glFinish()
the gpu is still drawing into it?  It seems we'd want a buffer
specific fence to wait on rather then waiting for all gl calls to
complete (if I'm understanding how glFinish() works).

Thanks again for your review feedback!

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

Re: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[Rob Herring]

On Wed, Jan 24, 2018 at 1:05 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 24, 2018 at 7:23 AM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
>>> This allows for importing buffers allocated from the
>>> hikey and hikey960 gralloc implelementations.
>>>
>>> Cc: Marissa Wall <marissaw@google.com>
>>> Cc: Sean Paul <seanpaul@google.com>
>>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>>> Cc: Robert Foss <robert.foss@collabora.com>
>>> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
>>> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
>>> Cc: David Hanna <david.hanna11@gmail.com>
>>> Cc: Rob Herring <rob.herring@linaro.org>
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>> v2:
>>> * Make platformhisi and the generic importer exclusive in the build
>>
>> I actually prefer the opposite. If everything is always compiled, we reduce the
>> chance of breaking boards when the base class is updated. I'm sure there is a
>> good reason for this, but perhaps there's another way?
>
> The main reason for this was avoiding the build breaking when
> "gralloc_priv.h" isn't supplied or present.
>
> Similarly the current freedesktop/master code won't build in the
> Android environment as the platformdrmgeneric.cpp file depends on the
> gralloc_drm_handle.h, which doesn't exist.

That will be fixed soon. We're moving the handle definition to libdrm.

The idea was to provide an accessor API to return all the data needed
and then custom gralloc handle implementations can provide different
accessor functions. This may be enough to get rid of different
importers, but you still need some magic to pull in different
implementations whether header inlines or library functions.

The intent was to not be runtime selected because there's only ever
one handle implementation and we want to encourage converging on one
handle implementation.

> Earlier to avoid this I had included Rob's "provide a common gralloc
> handle definition" patch, but build-conditionalizing the importers
> seemed like a sane solution.
>
> I'm of course open to alternatives, but I'm not sure how to add the
> hisi importer and the drmgeneric one and keep things building without
> also adding copies of the various gralloc headers to the project as
> well.

Sometimes just copying the headers is the easiest, but whomever
maintains the original needs to know that to avoid breaking people.
And you don't have control of that since this comes from ARM.

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

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[Rob Herring]

On Wed, Jan 24, 2018 at 1:26 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 24, 2018 at 7:26 AM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Tue, Jan 23, 2018 at 03:16:39PM -0800, John Stultz wrote:
>>> When using drm_hwcomposer with the hikey board, the resulting
>>> display shows lots of tearing.
>>>
>>> This seems to be due to EGLcomposition not initializing
>>> properly, potentially due to I'm guessing limitations of what
>>> the  utgard mali driver can do. I've noted that with the
>>> HiKey960 board, this patch is *not* necessary.
>>>
>>> Hacking around a bit, I found that since the glworker code
>>> isn't running properly, we never call glFinish(), which
>>> is required to fix the tearing.
>>>
>>> Ideas for a better way to implement this would be greatly
>>> appreciated!
>>
>> Sounds like you'll have to dig into the gl compositor to fix this. I think
>> chances are quite good there's a better way than below.
>
> Yes, I did spent a little bit of time earlier trying to rewrite the gl
> shader to try to build for the utgard level hardware, but wasn't very
> successful. At a deeper level I guess I'm not sure the glcompositor is
> useful in this case, since we're doing single plane client side
> compositing (as short of the glFinish bit, not running it doesn't seem
> to keep things from working).  But I'll look into that again.
>
> Again forgive as I really a bit in the dark on most graphics details,
> but the other more basic question I'm a bit unsure of is, does this
> patch even make any functional sense?

I'd think not. The only thing that makes me question that is I've seen
glFinish calls in gralloc (framebuffer) cases. But those cases were
prior to explicit fence support.

>  If we're not using the
> glcompositor and are using the atomic commit in ApplyFrame(), why the
> need for glFinish to avoid tearing?  Is it that the we commit the
> frame atomically to the display, but if we don't block w/ glFinish()
> the gpu is still drawing into it?  It seems we'd want a buffer
> specific fence to wait on rather then waiting for all gl calls to
> complete (if I'm understanding how glFinish() works).

That seems like the right approach. Are we failing to pass fences
associated with input layers to DRM?

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

Re: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[John Stultz]

On Wed, Jan 24, 2018 at 7:46 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
>> +#ifdef HIKEY
>> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
>> +  switch (hal_format) {
>> +    case HAL_PIXEL_FORMAT_RGB_888:
>> +      return DRM_FORMAT_BGR888;
>> +    case HAL_PIXEL_FORMAT_BGRA_8888:
>> +      return DRM_FORMAT_ARGB8888;
>> +    case HAL_PIXEL_FORMAT_RGBX_8888:
>> +      return DRM_FORMAT_XBGR8888;
>> +    case HAL_PIXEL_FORMAT_RGBA_8888:
>> +      return DRM_FORMAT_ABGR8888;
>> +    case HAL_PIXEL_FORMAT_RGB_565:
>> +      return DRM_FORMAT_BGR565;
>> +    case HAL_PIXEL_FORMAT_YV12:
>> +      return DRM_FORMAT_YVU420;
>> +    default:
>> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
>> +      return -EINVAL;
>> +  }
>
> This is the same as the generic case, and below is a little confusing to me.

Yes. HiKey is the same as the generic case.

>> +}
>> +#else /* HIKEY960 case*/
>> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
>> +  switch (hal_format) {
>> +    case HAL_PIXEL_FORMAT_RGB_888:
>> +      return DRM_FORMAT_BGR888;
>> +    case HAL_PIXEL_FORMAT_BGRA_8888:
>> +      return DRM_FORMAT_XBGR8888;
>> +    case HAL_PIXEL_FORMAT_RGBX_8888:
>> +      return DRM_FORMAT_XBGR8888;
>> +    case HAL_PIXEL_FORMAT_RGBA_8888:
>> +      return DRM_FORMAT_XBGR8888;
>> +    case HAL_PIXEL_FORMAT_RGB_565:
>> +      return DRM_FORMAT_BGR565;
>> +    case HAL_PIXEL_FORMAT_YV12:
>> +      return DRM_FORMAT_YVU420;
>> +    default:
>> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
>> +      return -EINVAL;
>> +  }
>
> So it seems like we're smashing a whole bunch of formats into XBGR8888 here, and
> I'm not sure how it'll work. If you look above, the primary difference between
> the HAL and drm formats is that the subpixels are reversed (ie: RGBA becomes
> ABGR). In this snippet, you've got BGRA->XBGR, does this actually work?

Honestly the above is some sort of dyslexia nightmare for me, so I may
have gotten it slightly wrong. :)

So my understanding is that for the HiKey960/bifrost the As need to be
Xs (and I've verified that we don't get anything if we try to leave
the X's), and we have to switch the R/Bs to get the colors right.  It
may be the R/B switching is due to other quirks in gralloc and the drm
driver, I'm not sure.

> Furthermore, when I look at the kirin driver (I think that's what you use on
> hikey?), I see full support for all the above formats. So 2 questions:

So, on HiKey960 we have the out-of-tree kirin960 drm driver, which is
one of those vendor code drops that is similar but different enough
that its a pain to whittle down if extending the kirin driver can be
done to support it, I've not been involved in the upstreaming effort
for that driver, so I'm not sure what the plan is yet.

> 1- Does this work as expected

So yes, the code here does work.  But we're only exercising the
BGRA_8888 path, so the rest could very well be wrong.


> 2- Is there an opportunity to share code between platforms instead of
> copy-pasting this function over and over again? Perhaps each platform provides
> the formats they support and we have the base class do a mapping based on what
> is present?

My C++ is ~20 years stale, but I'll take a look to see what I can do there.

>> +#ifdef HIKEY
>> +  bo->pitches[0] = hnd->width * 4;
>
> Does this work for formats that are not 32-bit?

Probably not. I'll try to sort that out in a better way too.


>> +int HisiImporter::ReleaseBuffer(hwc_drm_bo_t *bo) {
>> +  if (bo->fb_id)
>> +    if (drmModeRmFB(drm_->fd(), bo->fb_id))
>> +      ALOGE("Failed to rm fb");
>> +
>> +  struct drm_gem_close gem_close;
>> +  memset(&gem_close, 0, sizeof(gem_close));
>> +  int num_gem_handles = sizeof(bo->gem_handles) / sizeof(bo->gem_handles[0]);
>> +  for (int i = 0; i < num_gem_handles; i++) {
>> +    if (!bo->gem_handles[i])
>> +      continue;
>> +
>> +    gem_close.handle = bo->gem_handles[i];
>> +    int ret = drmIoctl(drm_->fd(), DRM_IOCTL_GEM_CLOSE, &gem_close);
>> +    if (ret)
>> +      ALOGE("Failed to close gem handle %d %d", i, ret);
>> +    else
>> +      bo->gem_handles[i] = 0;
>> +  }
>> +  return 0;
>> +}
>
> This function is a straight copy-paste from generic, right? Let's try to do
> better than that. Perhaps you should be subclassing the drm generic platform
> instead?

Apologies, my sense of taste here is terrible.  Do you recommend I try
to subclass the drmgenericplatform or create a common base that both
use (along with the format bits)?

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

Re: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[John Stultz]

On Wed, Jan 24, 2018 at 11:32 AM, Rob Herring <rob.herring@linaro.org> wrote:
> On Wed, Jan 24, 2018 at 1:05 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Jan 24, 2018 at 7:23 AM, Sean Paul <seanpaul@chromium.org> wrote:
>>> On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
>>>> This allows for importing buffers allocated from the
>>>> hikey and hikey960 gralloc implelementations.
>>>>
>>>> Cc: Marissa Wall <marissaw@google.com>
>>>> Cc: Sean Paul <seanpaul@google.com>
>>>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>>>> Cc: Robert Foss <robert.foss@collabora.com>
>>>> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
>>>> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
>>>> Cc: David Hanna <david.hanna11@gmail.com>
>>>> Cc: Rob Herring <rob.herring@linaro.org>
>>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>>> ---
>>>> v2:
>>>> * Make platformhisi and the generic importer exclusive in the build
>>>
>>> I actually prefer the opposite. If everything is always compiled, we reduce the
>>> chance of breaking boards when the base class is updated. I'm sure there is a
>>> good reason for this, but perhaps there's another way?
>>
>> The main reason for this was avoiding the build breaking when
>> "gralloc_priv.h" isn't supplied or present.
>>
>> Similarly the current freedesktop/master code won't build in the
>> Android environment as the platformdrmgeneric.cpp file depends on the
>> gralloc_drm_handle.h, which doesn't exist.
>
> That will be fixed soon. We're moving the handle definition to libdrm.

How soon is that eta? Do you have a current patch for libdrm I should
work against?

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

Re: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[Robert Foss]

Hey John,

On 01/26/2018 08:29 PM, John Stultz wrote:
> On Wed, Jan 24, 2018 at 11:32 AM, Rob Herring <rob.herring@linaro.org> wrote:
>> On Wed, Jan 24, 2018 at 1:05 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> On Wed, Jan 24, 2018 at 7:23 AM, Sean Paul <seanpaul@chromium.org> wrote:
>>>> On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
>>>>> This allows for importing buffers allocated from the
>>>>> hikey and hikey960 gralloc implelementations.
>>>>>
>>>>> Cc: Marissa Wall <marissaw@google.com>
>>>>> Cc: Sean Paul <seanpaul@google.com>
>>>>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>>>>> Cc: Robert Foss <robert.foss@collabora.com>
>>>>> Cc: Matt Szczesiak <matt.szczesiak@arm.com>
>>>>> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
>>>>> Cc: David Hanna <david.hanna11@gmail.com>
>>>>> Cc: Rob Herring <rob.herring@linaro.org>
>>>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>>>> ---
>>>>> v2:
>>>>> * Make platformhisi and the generic importer exclusive in the build
>>>>
>>>> I actually prefer the opposite. If everything is always compiled, we reduce the
>>>> chance of breaking boards when the base class is updated. I'm sure there is a
>>>> good reason for this, but perhaps there's another way?
>>>
>>> The main reason for this was avoiding the build breaking when
>>> "gralloc_priv.h" isn't supplied or present.
>>>
>>> Similarly the current freedesktop/master code won't build in the
>>> Android environment as the platformdrmgeneric.cpp file depends on the
>>> gralloc_drm_handle.h, which doesn't exist.
>>
>> That will be fixed soon. We're moving the handle definition to libdrm.
> 
> How soon is that eta? Do you have a current patch for libdrm I should
> work against?

Currently it is a bit more up in the air than I was hoping for.
I'll make another stab at it today :)


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

Re: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[Sean Paul]

On Thu, Jan 25, 2018 at 09:23:45PM -0800, John Stultz wrote:
> On Wed, Jan 24, 2018 at 7:46 AM, Sean Paul <seanpaul@chromium.org> wrote:
> > On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
> >> +#ifdef HIKEY
> >> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
> >> +  switch (hal_format) {
> >> +    case HAL_PIXEL_FORMAT_RGB_888:
> >> +      return DRM_FORMAT_BGR888;
> >> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> >> +      return DRM_FORMAT_ARGB8888;
> >> +    case HAL_PIXEL_FORMAT_RGBX_8888:
> >> +      return DRM_FORMAT_XBGR8888;
> >> +    case HAL_PIXEL_FORMAT_RGBA_8888:
> >> +      return DRM_FORMAT_ABGR8888;
> >> +    case HAL_PIXEL_FORMAT_RGB_565:
> >> +      return DRM_FORMAT_BGR565;
> >> +    case HAL_PIXEL_FORMAT_YV12:
> >> +      return DRM_FORMAT_YVU420;
> >> +    default:
> >> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
> >> +      return -EINVAL;
> >> +  }
> >
> > This is the same as the generic case, and below is a little confusing to me.
> 
> Yes. HiKey is the same as the generic case.
> 
> >> +}
> >> +#else /* HIKEY960 case*/
> >> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
> >> +  switch (hal_format) {
> >> +    case HAL_PIXEL_FORMAT_RGB_888:
> >> +      return DRM_FORMAT_BGR888;
> >> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> >> +      return DRM_FORMAT_XBGR8888;
> >> +    case HAL_PIXEL_FORMAT_RGBX_8888:
> >> +      return DRM_FORMAT_XBGR8888;
> >> +    case HAL_PIXEL_FORMAT_RGBA_8888:
> >> +      return DRM_FORMAT_XBGR8888;
> >> +    case HAL_PIXEL_FORMAT_RGB_565:
> >> +      return DRM_FORMAT_BGR565;
> >> +    case HAL_PIXEL_FORMAT_YV12:
> >> +      return DRM_FORMAT_YVU420;
> >> +    default:
> >> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
> >> +      return -EINVAL;
> >> +  }
> >
> > So it seems like we're smashing a whole bunch of formats into XBGR8888 here, and
> > I'm not sure how it'll work. If you look above, the primary difference between
> > the HAL and drm formats is that the subpixels are reversed (ie: RGBA becomes
> > ABGR). In this snippet, you've got BGRA->XBGR, does this actually work?
> 
> Honestly the above is some sort of dyslexia nightmare for me, so I may
> have gotten it slightly wrong. :)
> 

I can certainly sympathize with this. It took me longer than I'd like to admit
to write the above. This seems like a good reason to change things up!


> So my understanding is that for the HiKey960/bifrost the As need to be
> Xs (and I've verified that we don't get anything if we try to leave
> the X's), and we have to switch the R/Bs to get the colors right.  It
> may be the R/B switching is due to other quirks in gralloc and the drm
> driver, I'm not sure.

It'd be nice to figure out. I suspect there's a bug somewhere that's re-
reversing things. 


> 
> > Furthermore, when I look at the kirin driver (I think that's what you use on
> > hikey?), I see full support for all the above formats. So 2 questions:
> 
> So, on HiKey960 we have the out-of-tree kirin960 drm driver, which is
> one of those vendor code drops that is similar but different enough
> that its a pain to whittle down if extending the kirin driver can be
> done to support it, I've not been involved in the upstreaming effort
> for that driver, so I'm not sure what the plan is yet.
> 
> > 1- Does this work as expected
> 
> So yes, the code here does work.  But we're only exercising the
> BGRA_8888 path, so the rest could very well be wrong.
> 
> 
> > 2- Is there an opportunity to share code between platforms instead of
> > copy-pasting this function over and over again? Perhaps each platform provides
> > the formats they support and we have the base class do a mapping based on what
> > is present?
> 
> My C++ is ~20 years stale, but I'll take a look to see what I can do there.
> 
> >> +#ifdef HIKEY
> >> +  bo->pitches[0] = hnd->width * 4;
> >
> > Does this work for formats that are not 32-bit?
> 
> Probably not. I'll try to sort that out in a better way too.
> 
> 
> >> +int HisiImporter::ReleaseBuffer(hwc_drm_bo_t *bo) {
> >> +  if (bo->fb_id)
> >> +    if (drmModeRmFB(drm_->fd(), bo->fb_id))
> >> +      ALOGE("Failed to rm fb");
> >> +
> >> +  struct drm_gem_close gem_close;
> >> +  memset(&gem_close, 0, sizeof(gem_close));
> >> +  int num_gem_handles = sizeof(bo->gem_handles) / sizeof(bo->gem_handles[0]);
> >> +  for (int i = 0; i < num_gem_handles; i++) {
> >> +    if (!bo->gem_handles[i])
> >> +      continue;
> >> +
> >> +    gem_close.handle = bo->gem_handles[i];
> >> +    int ret = drmIoctl(drm_->fd(), DRM_IOCTL_GEM_CLOSE, &gem_close);
> >> +    if (ret)
> >> +      ALOGE("Failed to close gem handle %d %d", i, ret);
> >> +    else
> >> +      bo->gem_handles[i] = 0;
> >> +  }
> >> +  return 0;
> >> +}
> >
> > This function is a straight copy-paste from generic, right? Let's try to do
> > better than that. Perhaps you should be subclassing the drm generic platform
> > instead?
> 
> Apologies, my sense of taste here is terrible.  Do you recommend I try
> to subclass the drmgenericplatform or create a common base that both
> use (along with the format bits)?

I think it makes sense to subclass DrmGenericPlatform, since there are minimal
differences. Ideally we wouldn't need any subclass, but tbh I haven't dug into
the differences enough to have a strong opinion on that.

For the format conversion function, given that the only difference is s/A/X/,
we can probably keep everything in drmgeneric (assuming you fix the format issue
mentioned above).

I'm thinking you add a member to DrmGenericPlatform called _has_alpha. If the
platform has alpha, this is set to true when the object is initialized. If
_has_alpha is true in CovertFormat you use the 'A' variants. If not, use 'X'.

I haven't put the Import functions side-by-side, but perhaps you can share some
code between them similar to above?

Sean

> 
> thanks
> -john

-- 
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: [RFC][PATCH 2/4 v2] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[John Stultz]

On Mon, Jan 29, 2018 at 12:02 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Thu, Jan 25, 2018 at 09:23:45PM -0800, John Stultz wrote:
>> On Wed, Jan 24, 2018 at 7:46 AM, Sean Paul <seanpaul@chromium.org> wrote:
>> > On Tue, Jan 23, 2018 at 03:16:37PM -0800, John Stultz wrote:
>> >> +}
>> >> +#else /* HIKEY960 case*/
>> >> +uint32_t HisiImporter::ConvertHalFormatToDrm(uint32_t hal_format) {
>> >> +  switch (hal_format) {
>> >> +    case HAL_PIXEL_FORMAT_RGB_888:
>> >> +      return DRM_FORMAT_BGR888;
>> >> +    case HAL_PIXEL_FORMAT_BGRA_8888:
>> >> +      return DRM_FORMAT_XBGR8888;
>> >> +    case HAL_PIXEL_FORMAT_RGBX_8888:
>> >> +      return DRM_FORMAT_XBGR8888;
>> >> +    case HAL_PIXEL_FORMAT_RGBA_8888:
>> >> +      return DRM_FORMAT_XBGR8888;
>> >> +    case HAL_PIXEL_FORMAT_RGB_565:
>> >> +      return DRM_FORMAT_BGR565;
>> >> +    case HAL_PIXEL_FORMAT_YV12:
>> >> +      return DRM_FORMAT_YVU420;
>> >> +    default:
>> >> +      ALOGE("Cannot convert hal format to drm format %u", hal_format);
>> >> +      return -EINVAL;
>> >> +  }
>> >
>> > So it seems like we're smashing a whole bunch of formats into XBGR8888 here, and
>> > I'm not sure how it'll work. If you look above, the primary difference between
>> > the HAL and drm formats is that the subpixels are reversed (ie: RGBA becomes
>> > ABGR). In this snippet, you've got BGRA->XBGR, does this actually work?
>>
>> Honestly the above is some sort of dyslexia nightmare for me, so I may
>> have gotten it slightly wrong. :)
>>
>
> I can certainly sympathize with this. It took me longer than I'd like to admit
> to write the above. This seems like a good reason to change things up!
>
>
>> So my understanding is that for the HiKey960/bifrost the As need to be
>> Xs (and I've verified that we don't get anything if we try to leave
>> the X's), and we have to switch the R/Bs to get the colors right.  It
>> may be the R/B switching is due to other quirks in gralloc and the drm
>> driver, I'm not sure.
>
> It'd be nice to figure out. I suspect there's a bug somewhere that's re-
> reversing things.

Yea, I've been digging in and it looks like we can get rid of the
special case conversion by adding DRM_FORMAT_ARGB8888 entries to the
kirin960 drm driver.

So that part seems to be easy to simplify out. I'll continue trying to
sort out the rest of your feedback.

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

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[Alexandru-Cosmin Gheorghe]

Hi John,

I took your patches for a spin on Hikey960. Some findings:
Even with this patch I'm getting some tearing on Hikey960, not a lot as you
reported on Hikey, still there are some small black squares, less than 10 of
aproximetly 20x20.
So I investigated a little bit through drm_hwcomposer and found some
issues, maybe they could help you somehow, see bellow:
 Wed, Jan 24, 2018 at 01:58:55PM -0600, Rob Herring wrote:
> On Wed, Jan 24, 2018 at 1:26 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Wed, Jan 24, 2018 at 7:26 AM, Sean Paul <seanpaul@chromium.org> wrote:
> >> On Tue, Jan 23, 2018 at 03:16:39PM -0800, John Stultz wrote:
> >>> When using drm_hwcomposer with the hikey board, the resulting
> >>> display shows lots of tearing.
> >>>
> >>> This seems to be due to EGLcomposition not initializing
> >>> properly, potentially due to I'm guessing limitations of what
> >>> the  utgard mali driver can do. I've noted that with the
> >>> HiKey960 board, this patch is *not* necessary.
> >>>


> >>> Hacking around a bit, I found that since the glworker code
> >>> isn't running properly, we never call glFinish(), which
> >>> is required to fix the tearing.
> >>>
> >>> Ideas for a better way to implement this would be greatly
> >>> appreciated!
> >>
> >> Sounds like you'll have to dig into the gl compositor to fix this. I think
> >> chances are quite good there's a better way than below.
> >
> > Yes, I did spent a little bit of time earlier trying to rewrite the gl
> > shader to try to build for the utgard level hardware, but wasn't very
> > successful. At a deeper level I guess I'm not sure the glcompositor is
> > useful in this case, since we're doing single plane client side
> > compositing (as short of the glFinish bit, not running it doesn't seem
> > to keep things from working).  But I'll look into that again.
> >
> > Again forgive as I really a bit in the dark on most graphics details,
> > but the other more basic question I'm a bit unsure of is, does this
> > patch even make any functional sense?
>
> I'd think not. The only thing that makes me question that is I've seen
> glFinish calls in gralloc (framebuffer) cases. But those cases were
> prior to explicit fence support.
>
> >  If we're not using the
> > glcompositor and are using the atomic commit in ApplyFrame(), why the
> > need for glFinish to avoid tearing?  Is it that the we commit the
> > frame atomically to the display, but if we don't block w/ glFinish()
> > the gpu is still drawing into it?  It seems we'd want a buffer
> > specific fence to wait on rather then waiting for all gl calls to
> > complete (if I'm understanding how glFinish() works).
>
> That seems like the right approach. Are we failing to pass fences
> associated with input layers to DRM?
>
> Rob
> _______________________________________________
It seems that we don't pass any explicit fences to the kernel for
IN_FENCE_FD property. This particular line seems wrong.

@@ -593,14 +594,17 @@ int
DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
       else
         rotation |= DRM_MODE_ROTATE_0;

-      if (fence_fd < 0) {
+      if (fence_fd >= 0) {

Also, if you track the value of the OUT_FENCE_PTR property, it seems
that it gets lost. As far I was able to track it down it seem that
it should have been used by AddFenceToRetireFence, and then be passed to
SurfaceFlinger through retire_fence parameter of PresentDisplay. But,
that doesn't happen because  AddFenceToRetireFence get's called each
time on a new DrmDisplayComposition object, before calling
ApplyComposition.


Both of this findings should have explained the tearing, however I
hacked something today and it didn't fix it for Hikey960, maybe
you have more luck.

Thank you,
Alex Gheorghe


> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

On Wed, Jan 31, 2018 at 10:51 AM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> Hi John,
>
> I took your patches for a spin on Hikey960. Some findings:
> Even with this patch I'm getting some tearing on Hikey960, not a lot as you
> reported on Hikey, still there are some small black squares, less than 10 of
> aproximetly 20x20.

Sweet! I'm glad you were able to get it up and running!  I think the
black squares are a separate thing, more on that below.

> So I investigated a little bit through drm_hwcomposer and found some
> issues, maybe they could help you somehow, see bellow:

Very much appreciate the extra review and eyes on the code!

>> > Yes, I did spent a little bit of time earlier trying to rewrite the gl
>> > shader to try to build for the utgard level hardware, but wasn't very
>> > successful. At a deeper level I guess I'm not sure the glcompositor is
>> > useful in this case, since we're doing single plane client side
>> > compositing (as short of the glFinish bit, not running it doesn't seem
>> > to keep things from working).  But I'll look into that again.
>> >
>> > Again forgive as I really a bit in the dark on most graphics details,
>> > but the other more basic question I'm a bit unsure of is, does this
>> > patch even make any functional sense?
>>
>> I'd think not. The only thing that makes me question that is I've seen
>> glFinish calls in gralloc (framebuffer) cases. But those cases were
>> prior to explicit fence support.
>>
>> >  If we're not using the
>> > glcompositor and are using the atomic commit in ApplyFrame(), why the
>> > need for glFinish to avoid tearing?  Is it that the we commit the
>> > frame atomically to the display, but if we don't block w/ glFinish()
>> > the gpu is still drawing into it?  It seems we'd want a buffer
>> > specific fence to wait on rather then waiting for all gl calls to
>> > complete (if I'm understanding how glFinish() works).
>>
>> That seems like the right approach. Are we failing to pass fences
>> associated with input layers to DRM?
>> _______________________________________________
> It seems that we don't pass any explicit fences to the kernel for
> IN_FENCE_FD property. This particular line seems wrong.
>
> @@ -593,14 +594,17 @@ int
> DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>        else
>          rotation |= DRM_MODE_ROTATE_0;
>
> -      if (fence_fd < 0) {
> +      if (fence_fd >= 0) {

I'll give that a whirl.

> Also, if you track the value of the OUT_FENCE_PTR property, it seems
> that it gets lost. As far I was able to track it down it seem that
> it should have been used by AddFenceToRetireFence, and then be passed to
> SurfaceFlinger through retire_fence parameter of PresentDisplay. But,
> that doesn't happen because  AddFenceToRetireFence get's called each
> time on a new DrmDisplayComposition object, before calling
> ApplyComposition.

I'll look into this some more as well. I've not yet gotten into the
details of the fence handling in drm_hwcomposer yet, so I'll have to
check that out.

> Both of this findings should have explained the tearing, however I
> hacked something today and it didn't fix it for Hikey960, maybe
> you have more luck.

The issue with the black/grey blocks is a separate problem with the
4.9 based kernels that I'm still chasing down, we see it (though less
often) w/o the hwcomposer, and I think it has to do with something off
in the CMA allocations. We don't see it with the 4.4 or the 4.14 based
tree i'm also prepping.

Thanks again for the feedback! I'm really happy to have your input
here, so if you do generate any further changes, let me know and I'll
try to integrate them into the patch set!

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

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

On Wed, Jan 31, 2018 at 11:03 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 31, 2018 at 10:51 AM, Alexandru-Cosmin Gheorghe
> <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
>> It seems that we don't pass any explicit fences to the kernel for
>> IN_FENCE_FD property. This particular line seems wrong.
>>
>> @@ -593,14 +594,17 @@ int
>> DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>>        else
>>          rotation |= DRM_MODE_ROTATE_0;
>>
>> -      if (fence_fd < 0) {
>> +      if (fence_fd >= 0) {
>
> I'll give that a whirl.

So I did give that a whirl after you suggested it, but it ended up
causing nothing to be displayed, and I unfortunately didn't have time
right then to dig much further.

Rob however re-found this issue today, and I've been digging a bit
more. At least with the HiKey board, it seem the trouble is when the
IN_FENCE_FD is added to the pset, the atomic commit calls start to
fail. I dug in and it seems we're catching in the kernel on the  if
(file->f_op != &sync_file_fops) check in sync_file_fdget().

I'm now trying to trace back to where the in fence was provided from
to see what might be going wrong there.

Curious if this is anything you ran across in your attempts?

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

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[Alexandru-Cosmin Gheorghe]

On Mon, Feb 12, 2018 at 08:43:10PM -0800, John Stultz wrote:
> On Wed, Jan 31, 2018 at 11:03 AM, John Stultz <john.stultz@linaro.org> wrote:
> > On Wed, Jan 31, 2018 at 10:51 AM, Alexandru-Cosmin Gheorghe
> > <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> >> It seems that we don't pass any explicit fences to the kernel for
> >> IN_FENCE_FD property. This particular line seems wrong.
> >>
> >> @@ -593,14 +594,17 @@ int
> >> DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
> >>        else
> >>          rotation |= DRM_MODE_ROTATE_0;
> >>
> >> -      if (fence_fd < 0) {
> >> +      if (fence_fd >= 0) {
> >
> > I'll give that a whirl.
>
> So I did give that a whirl after you suggested it, but it ended up
> causing nothing to be displayed, and I unfortunately didn't have time
> right then to dig much further.
>
> Rob however re-found this issue today, and I've been digging a bit
> more. At least with the HiKey board, it seem the trouble is when the
> IN_FENCE_FD is added to the pset, the atomic commit calls start to
> fail. I dug in and it seems we're catching in the kernel on the  if
> (file->f_op != &sync_file_fops) check in sync_file_fdget().
>
> I'm now trying to trace back to where the in fence was provided from
> to see what might be going wrong there.
I would be surprised if this fence is not created by the GPU driver.
But, the whole Android stack is new to me, so I might be wrong.
>
> Curious if this is anything you ran across in your attempts?
Sorry, on Hikey960, I don't have this problem. Are you using the
same 4.9 kernel from your branch(hikey-hwcomposer-deps)?
>
> thanks
> -john

Regards,
Alex Gheorghe
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

On Tue, Feb 13, 2018 at 2:45 AM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> On Mon, Feb 12, 2018 at 08:43:10PM -0800, John Stultz wrote:
>> On Wed, Jan 31, 2018 at 11:03 AM, John Stultz <john.stultz@linaro.org> wrote:
>> > On Wed, Jan 31, 2018 at 10:51 AM, Alexandru-Cosmin Gheorghe
>> > <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
>> >> It seems that we don't pass any explicit fences to the kernel for
>> >> IN_FENCE_FD property. This particular line seems wrong.
>> >>
>> >> @@ -593,14 +594,17 @@ int
>> >> DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>> >>        else
>> >>          rotation |= DRM_MODE_ROTATE_0;
>> >>
>> >> -      if (fence_fd < 0) {
>> >> +      if (fence_fd >= 0) {
>> >
>> > I'll give that a whirl.
>>
>> So I did give that a whirl after you suggested it, but it ended up
>> causing nothing to be displayed, and I unfortunately didn't have time
>> right then to dig much further.
>>
>> Rob however re-found this issue today, and I've been digging a bit
>> more. At least with the HiKey board, it seem the trouble is when the
>> IN_FENCE_FD is added to the pset, the atomic commit calls start to
>> fail. I dug in and it seems we're catching in the kernel on the  if
>> (file->f_op != &sync_file_fops) check in sync_file_fdget().
>>
>> I'm now trying to trace back to where the in fence was provided from
>> to see what might be going wrong there.
> I would be surprised if this fence is not created by the GPU driver.
> But, the whole Android stack is new to me, so I might be wrong.

Yes, I went digging on this last night and it ends up the utgard
driver we use (r7p0) isn't giving us a dmabuf fence, instead we're
getting a mali_sync_fence.

There's a newer kernel driver release (r8p1), which seems to correct
this, but it also is version tied to the binary blob, which I don't
have a matching update for. I spent a bit of time trying to hack out
just the dmabuf fence bits, but it rarely boots all the way and
frequently locks the board up, so I'm probably mucking the library
driver interface in some way.

Its a well known sad song.

>> Curious if this is anything you ran across in your attempts?
> Sorry, on Hikey960, I don't have this problem. Are you using the
> same 4.9 kernel from your branch(hikey-hwcomposer-deps)?

Thanks for confirming! I'll be double checking on the hikey960 soon. I
suspect the bifrost driver is giving back the right structure if it
was working for you.

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

Re: [RFC][PATCH 4/4 v2] drm_hwcomposer: Try to fallback if GLCompisition fails

[John Stultz]

On Tue, Feb 13, 2018 at 2:03 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, Feb 13, 2018 at 2:45 AM, Alexandru-Cosmin Gheorghe
> <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
>> On Mon, Feb 12, 2018 at 08:43:10PM -0800, John Stultz wrote:
>>> On Wed, Jan 31, 2018 at 11:03 AM, John Stultz <john.stultz@linaro.org> wrote:
>>> > On Wed, Jan 31, 2018 at 10:51 AM, Alexandru-Cosmin Gheorghe
>>> > <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
>>> >> It seems that we don't pass any explicit fences to the kernel for
>>> >> IN_FENCE_FD property. This particular line seems wrong.
>>> >>
>>> >> @@ -593,14 +594,17 @@ int
>>> >> DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
>>> >>        else
>>> >>          rotation |= DRM_MODE_ROTATE_0;
>>> >>
>>> >> -      if (fence_fd < 0) {
>>> >> +      if (fence_fd >= 0) {
>>> >
>>> > I'll give that a whirl.
>>>
>>> So I did give that a whirl after you suggested it, but it ended up
>>> causing nothing to be displayed, and I unfortunately didn't have time
>>> right then to dig much further.
>>>
>>> Rob however re-found this issue today, and I've been digging a bit
>>> more. At least with the HiKey board, it seem the trouble is when the
>>> IN_FENCE_FD is added to the pset, the atomic commit calls start to
>>> fail. I dug in and it seems we're catching in the kernel on the  if
>>> (file->f_op != &sync_file_fops) check in sync_file_fdget().
>>>
>>> I'm now trying to trace back to where the in fence was provided from
>>> to see what might be going wrong there.
>> I would be surprised if this fence is not created by the GPU driver.
>> But, the whole Android stack is new to me, so I might be wrong.
>
> Yes, I went digging on this last night and it ends up the utgard
> driver we use (r7p0) isn't giving us a dmabuf fence, instead we're
> getting a mali_sync_fence.
>
> There's a newer kernel driver release (r8p1), which seems to correct
> this, but it also is version tied to the binary blob, which I don't
> have a matching update for. I spent a bit of time trying to hack out
> just the dmabuf fence bits, but it rarely boots all the way and
> frequently locks the board up, so I'm probably mucking the library
> driver interface in some way.
>
> Its a well known sad song.

So, I took another stab at this tonight and actually got the dmabuf
fence handling transplanted into the older r7p0 driver we're using. So
with that and the recent fence_fd bug that has been fixed in master,
things are working *much* better. No gl_finsh() hacks needed to avoid
tearing and performance is improved nicely.

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