[Mesa-dev] [PATCH] egl/android: Partially handle HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED

[Rob Herring]

On Thu, Nov 30, 2017 at 12:11 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 11/30/2017 06:13 AM, Tomasz Figa wrote:
>>
>> On Thu, Nov 30, 2017 at 3:43 AM, Robert Foss <robert.foss at collabora.com>
>> wrote:
>>>
>>> Hey,
>>>
>>> On Tue, 2017-11-28 at 11:49 +0000, Emil Velikov wrote:
>>>>
>>>> On 28 November 2017 at 10:45, Tapani Pälli <tapani.palli at intel.com>
>>>> wrote:
>>>>>
>>>>> Hi;
>>>>>
>>>>>
>>>>> On 11/27/2017 04:14 PM, Robert Foss wrote:
>>>>>>
>>>>>>
>>>>>> From: Tomasz Figa <tfiga at chromium.org>
>>>>>>
>>>>>> There is no API available to properly query the
>>>>>> IMPLEMENTATION_DEFINED
>>>>>> format. As a workaround we rely here on gralloc allocating either
>>>>>> an arbitrary YCbCr 4:2:0 or RGBX_8888, with the latter being
>>>>>> recognized
>>>>>> by lock_ycbcr failing.
>>>>>>
>>>>>> Reviewed-on: https://chromium-review.googlesource.com/566793
>>>>>>
>>>>>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>>>>>> Reviewed-by: Chad Versace <chadversary at chromium.org>
>>>>>> Signed-off-by: Robert Foss <robert.foss at collabora.com>
>>>>>> ---
>>>>>>    src/egl/drivers/dri2/platform_android.c | 39
>>>>>> +++++++++++++++++++++++++++++++--
>>>>>>    1 file changed, 37 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/egl/drivers/dri2/platform_android.c
>>>>>> b/src/egl/drivers/dri2/platform_android.c
>>>>>> index 63223e9a69..ae914d79c1 100644
>>>>>> --- a/src/egl/drivers/dri2/platform_android.c
>>>>>> +++ b/src/egl/drivers/dri2/platform_android.c
>>>>>> @@ -59,6 +59,10 @@ static const struct droid_yuv_format
>>>>>> droid_yuv_formats[] = {
>>>>>>       { HAL_PIXEL_FORMAT_YCbCr_420_888,   0, 1,
>>>>>> __DRI_IMAGE_FOURCC_YUV420
>>>>>> },
>>>>>>       { HAL_PIXEL_FORMAT_YCbCr_420_888,   1, 1,
>>>>>> __DRI_IMAGE_FOURCC_YVU420
>>>>>> },
>>>>>>       { HAL_PIXEL_FORMAT_YV12,            1, 1,
>>>>>> __DRI_IMAGE_FOURCC_YVU420
>>>>>> },
>>>>>> +   /* HACK: See droid_create_image_from_prime_fd() and
>>>>>> b/32077885. */
>>>>>> +   { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED,   0, 2,
>>>>>> __DRI_IMAGE_FOURCC_NV12 },
>>>>>> +   { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED,   0, 1,
>>>>>> __DRI_IMAGE_FOURCC_YUV420 },
>>>>>> +   { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED,   1, 1,
>>>>>> __DRI_IMAGE_FOURCC_YVU420 },
>>>>>
>>>>>
>>>>>
>>>>> One alternative way would be to ask gralloc about these formats. On
>>>>> gralloc0
>>>>> this would need a perform() hook and gralloc1 has getFormat(). This
>>>>> is how
>>>>> it is done currently on Android-IA, see following commits:
>>>>>
>>>>> https://github.com/intel/external-mesa/commit/deb323eafa321c725805a
>>>>> 702ed19cb4983346b60
>>>>>
>>>>> https://github.com/intel/external-mesa/commit/7cc01beaf540e29862853
>>>>> 561ef93c6c4e86c4c1a
>>>>>
>>>>> Do you think this approach would work with Chromium as well?
>>>>>
>>>>
>>>> i think the Android-IA approach looks good, although it depends on
>>>> local gralloc0 changes. With gralloc1 on the horizon, I don't know
>>>> how
>>>> much sense it makes to extend the predecessor.
>>>> AFAICT the patch should not cause any issues and it's nicely
>>>> documented.
>>>
>>>
>>> I had a look at the chromiumos/minigbm implementation, and it does not
>>> contain a gralloc1 implementation as far as I can see. I assume that it
>>> is available somewhere, but maybe not on a public branch.
>>>
>>> Would it be possible to make the minigbm gralloc1 impl. public? That
>>> way I could submit a patch mirroring what intel/minigbm does.
>>>
>>> If you fine folks as at Google prefer to roll it yourselves, just give
>>> me a poke.
>>
>>
>> There is no gralloc1 implementation for ChromiumOS minigbm and AFAIK
>> we don't have any plans of adding one. AFAICT there is nothing we
>> would gain with it over gralloc0.
>>
>>>
>>> Those are the two options I'm seeing.
>>>
>>> As for gralloc0 support, would it be needed?
>>>
>>>>
>>>> Perhaps someone from the Google/CrOS team can assist in making the
>>>> bug
>>>> public, although even then it might be better to focus on a 'perfect'
>>>> gralloc1?
>>>>
>>>> IMHO the patch looks perfectly reasonable and we could merge it even,
>>>> if we were to switch to gralloc1 in the not too distant future ;-)
>>>
>>>
>>> Maybe doing both is reasonable.
>>
>>
>> I believe there isn't much adoption of gralloc1 in the wild.
>> Android-IA is the first I saw (might have missed something, though).
>> Tapani, what was the reason for switching to gralloc1?
>
>
> Main reason was that we thought this is something Android will be moving in
> to (and deprecating gralloc0). But now if it's gone, it does not make sense
> to support it.
>
>> Could we just support gralloc0 for now in Mesa, make sure the next
>> generation IAllocator/IMapper stuff suites our needs and switch to it
>> later when it happens?

For CTS/VTS compliance, AIUI, you may have to switch based on the
release a device is shipping on.

> Yes, this sounds good to me.
>
>> (As a side note, I had an idea to create a new interface, standardized
>> by Mesa, let's say libdri_android, completely free of any
>> gralloc-internals. It would have to be exposed additionally by any
>> Android that intends to run Mesa. Given the need to deal with 3
>> different gralloc versions already, it could be something easier to
>> manage.)
>
>
> Makes sense, it is a bit messy and we have bit too much patches on our tree
> because of these differences.

Seems overly complicated to me. The information needed is within the
ints in the native_handle in most/all implementations. I don't think
there's another way to globally store dmabuf metadata unless you have
a custom interface in your DRM driver. So standardizing to a common
library implies a common handle struct here. I think the options are:

- common struct definition (native_handle_t + dmabuf fd(s) + width,
height, stride, format, usage, etc.)
- common struct plus inline accessor functions
- common opaque struct plus accessor library

Also, I don't think whatever is standardized should live in Mesa.
There's a need to support drm_hwcomposer (which has the same
dependencies as mesa) with non-Mesa GL implementations (yes, vendor
binaries).

Rob