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

[Tomasz Figa]

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?

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?

(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.)

Best regards,
Tomasz