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@xxxxxxxxxx> > Cc: Sean Paul <seanpaul@xxxxxxxxxx> > Cc: Dmitry Shmidt <dimitrysh@xxxxxxxxxx> > Cc: Robert Foss <robert.foss@xxxxxxxxxxxxx> > Cc: Matt Szczesiak <matt.szczesiak@xxxxxxx> > Cc: Liviu Dudau <Liviu.Dudau@xxxxxxx> > Cc: David Hanna <david.hanna11@xxxxxxxxx> > Cc: Rob Herring <rob.herring@xxxxxxxxxx> > Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
