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