[RFC][PATCH v3] drm_hwcomposer: Add platformhisi buffer importer for hikey and hikey960

[John Stultz]

On Tue, Feb 13, 2018 at 8:34 AM, Alexandru-Cosmin Gheorghe
<Alexandru-Cosmin.Gheorghe at arm.com> 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 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.

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