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

Fri Dec 1 14:44:31 UTC 2017

On Fri, Dec 1, 2017 at 11:20 PM, Rob Herring <robh at kernel.org> wrote:
> On Fri, Dec 1, 2017 at 7:30 AM, Robert Foss <robert.foss at collabora.com> wrote:
>> On Thu, 2017-11-30 at 11:14 -0600, Rob Herring wrote:
>>> On Thu, Nov 30, 2017 at 12:11 AM, Tapani Pälli <tapani.palli at intel.co
>>> m> 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 collabo
>>> > > ra.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 inte
>>> > > > > l.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/566
>>> > > > > > > 793
>>> > > > > > >
>>> > > > > > > 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/deb323eafa321
>>> > > > > > c725805a
>>> > > > > > 702ed19cb4983346b60
>>> > > > > >
>>> > > > > > https://github.com/intel/external-mesa/commit/7cc01beaf540e
>>> > > > > > 29862853
>>> > > > > > 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
>>
>> So these common parts would be much like what currently exists in
>> minigbm/cros_gralloc_handle.h and gbm_gralloc/gralloc_drm_handle.h
>> then, but extended with the above suggestions?
>
> Yes, but which part do you think needs to be extended?
>
> As we discussed on IRC, I think perhaps we just need to change the
> handle format field in gralloc_drm_handle.h to use fourcc (aka DRM and
> GBM formats) instead of the Android format. I think all the users just
> end up converting it to their own internal format anyway.

We keep the handle opaque for consumers and even minigbm dereferences
it only when creating/registering the buffer, further using the handle
pointer only as a key to internal bookkeeping map.

Relying on the struct itself is not only error prone, as there is no
way to check if the struct on gralloc implementation side matches what
we expect, but also makes it difficult to change the handle struct at
our convenience.

>
>>> 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).
>>
>> Excluding Mesa and the composer leaves us with the allocator or
>> creating a new library.
>> I would assume that creating a new library is the worse option.
>
> Why excluding the composer? If we have to pick, I'd put it there or
> perhaps libdrm?

There is neither a single central composer nor libdrm is used on every
system... (The latter is not even used by Intel driver in Mesa
anymore.)

However I fully agree that there are other upstream components (e.g.
drm_hwcomposer), which would benefit from it and nobody wants to
include Mesa in the build just for one header. Should we have a
separate freedesktop project for it?

Best regards,
Tomasz