[RFC][PATCH v3] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960
Alexandru-Cosmin Gheorghe
Alexandru-Cosmin.Gheorghe at arm.com
Tue Feb 13 16:34:52 UTC 2018
Hi John,
Some comments bellow,
On Thu, Feb 08, 2018 at 04:40:05PM -0800, John Stultz wrote:
> This allows for importing buffers allocated from the
> hikey and hikey960 gralloc implelementations.
>
> Feedback or comments would be greatly appreciated!
>
> Cc: Marissa Wall <marissaw at google.com>
> Cc: Sean Paul <seanpaul at google.com>
> Cc: Dmitry Shmidt <dimitrysh at google.com>
> Cc: Robert Foss <robert.foss at collabora.com>
> Cc: Matt Szczesiak <matt.szczesiak at arm.com>
> Cc: Liviu Dudau <Liviu.Dudau at arm.com>
> Cc: David Hanna <david.hanna11 at gmail.com>
> Cc: Rob Herring <rob.herring at linaro.org>
> Change-Id: I81abdd4d1dc7d9f2ef31078c91679b532d3262fd
> Signed-off-by: John Stultz <john.stultz at linaro.org>
> ---
> The full patchset I'm testing with can be found here:
> https://github.com/johnstultz-work/drm_hwcomposer/commits/drm_hwc
>
> v2:
> * Make platformhisi and the generic importer exclusive in the build
> * Fixup vendor check
> v3:
> * Unify format conversions
> * Subclass the platformdrmgeneric importer implementation to reduce
> code duplication
> * Rework to avoid board specific logic (tweak gralloc to be consistent
> between the two)
> ---
> Android.mk | 13 +++++
> platformdrmgeneric.h | 2 +-
> platformhisi.cpp | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++
> platformhisi.h | 48 +++++++++++++++++++
> 4 files changed, 194 insertions(+), 1 deletion(-)
> create mode 100644 platformhisi.cpp
> create mode 100644 platformhisi.h
>
> diff --git a/Android.mk b/Android.mk
> index ee5b8bf..f37e4c3 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -74,7 +74,20 @@ LOCAL_CPPFLAGS += \
> -DHWC2_USE_CPP11 \
> -DHWC2_INCLUDE_STRINGIFICATION
>
> +
> +ifeq ($(TARGET_PRODUCT),hikey960)
> +LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER
> +LOCAL_SRC_FILES += platformhisi.cpp
> +LOCAL_C_INCLUDES += device/linaro/hikey/gralloc960/
> +else
> +ifeq ($(TARGET_PRODUCT),hikey)
> +LOCAL_CPPFLAGS += -DUSE_HISI_IMPORTER
> +LOCAL_SRC_FILES += platformhisi.cpp
> +LOCAL_C_INCLUDES += device/linaro/hikey/gralloc/
> +else
> LOCAL_CPPFLAGS += -DUSE_DRM_GENERIC_IMPORTER
> +endif
> +endif
>
> LOCAL_MODULE := hwcomposer.drm
> LOCAL_MODULE_TAGS := optional
> diff --git a/platformdrmgeneric.h b/platformdrmgeneric.h
> index 8376580..fbe059b 100644
> --- a/platformdrmgeneric.h
> +++ b/platformdrmgeneric.h
> @@ -35,8 +35,8 @@ class DrmGenericImporter : public Importer {
> 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);
> + private:
>
> DrmResources *drm_;
>
> diff --git a/platformhisi.cpp b/platformhisi.cpp
> new file mode 100644
> index 0000000..5f17c20
> --- /dev/null
> +++ b/platformhisi.cpp
> @@ -0,0 +1,132 @@
> +/*
> + * 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
Isn't this pointless this file seems to be added only of if
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) : DrmGenericImporter(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;
> +}
> +
> +EGLImageKHR HisiImporter::ImportImage(EGLDisplay egl_display, buffer_handle_t handle) {
If the scope is reusing, I think you could go further and create an
overload/helper ImportImage(EGLDisplay, width, height, fd, bystride),
that's called for both HisiImport and DrmGenericImporter.
> + 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)DrmGenericImporter::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 = DrmGenericImporter::ConvertHalFormatToDrm(hnd->req_format);
> + bo->usage = hnd->usage;
> + bo->pitches[0] = hnd->byte_stride;
This would work only for 1 plane formats, probably gets rejected by
drmModeAddFb2, but to save debugging time maybe it worth removing
DRM_FORMAT_YVU420 from ConvertHALFormatToDrm and checking it's return
code.
> + 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;
> +}
> +
> +#ifdef USE_HISI_IMPORTER
> +std::unique_ptr<Planner> Planner::CreateInstance(DrmResources *) {
> + std::unique_ptr<Planner> planner(new Planner);
> + planner->AddStage<PlanStageGreedy>();
> + return planner;
> +}
> +#endif
> +
> +}
I suppose this is a reminiscence, since you where planning to remove
platformdrmgeneric.
> +
> +
> diff --git a/platformhisi.h b/platformhisi.h
> new file mode 100644
> index 0000000..46f4595
> --- /dev/null
> +++ b/platformhisi.h
> @@ -0,0 +1,48 @@
> +/*
> + * 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 DrmGenericImporter {
> + 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;
> +
> + private:
> +
> + DrmResources *drm_;
> +
> + const gralloc_module_t *gralloc_;
> +};
> +}
> +
> +#endif
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list