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@xxxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
