On Tue, Feb 13, 2018 at 8:34 AM, Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@xxxxxxx> wrote: > 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@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> >> Change-Id: I81abdd4d1dc7d9f2ef31078c91679b532d3262fd >> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx> >> --- >> 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. Fair point. I'll clean that up. >> +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. I'll look into this. I'm not sure how minimal folks want to be, but I'll take a pass and see if it seems cleaner. >> +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. Will take a shot at this too. >> +#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. Likely, I'll try to clean it up. Thanks so much again for the review and feedback! I really appreciate it! -john _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
