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

Fri Dec 1 13:30:44 UTC 2017

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?

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

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?

Rob.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171201/816f5439/attachment.sig>