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

[Rob Herring]

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.

>> 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?

> On a seperate note, my immidiate concern is making the Android platform
> more functional, and in my mind the ^^ patch is minimally invasive and
> won't alter other usecases, but simply make mesa more predictable.
> I would like to make the bug IDs into full URLs though, but with that
> change in place, is the patch acceptable?

No issues for me applying the patch. I did just notice something I'm
confused about with it. I'll reply separately on that.

Rob