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

[Tapani Pälli]

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?

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.

// Tapani