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

[Sean Paul]

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 at 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

-- 
Sean Paul, Software Engineer, Google / Chromium OS