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

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

-- 
2.7.4

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
>

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
>

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

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

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

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

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.

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

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