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

Tue Dec 5 17:01:36 UTC 2017

On Tue, 2017-12-05 at 18:22 +0900, Tomasz Figa wrote:
> On Sat, Dec 2, 2017 at 4:43 AM, Rob Herring <robh at kernel.org> wrote:
> > On Fri, Dec 1, 2017 at 8:44 AM, Tomasz Figa <tfiga at chromium.org>
> > wrote:
> > > 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 collab
> > > > ora.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.pall
> > > > > > i 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.fo
> > > > > > > > ss at collabo
> > > > > > > > ra.com>
> > > > > > > > wrote:
> > 
> > [...]
> > 
> > > > > > > > (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.
> > 
> > What you say implies that you don't need any metadata in the
> > handle,
> > but you do have pretty much all the same data. Whether you
> > 
> > > 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.
> > 
> > How does a library solve this?
> > 
> > Everything in Android gets built together and the handle pretty
> > much
> > has to stay the same across components in any implementation I've
> > seen. Maybe someday that will change and we'll need versioning and
> > backwards compatibility, but for now that's unnecessary complexity.
> > We'd have to get to a single, well controlled and defined handle
> > first
> > anyway before we start versioning.
> > 
> > Anyone is still free to change things downstream however they want.
> > We're only talking about what does mainline/upstream do.
> > 
> > > > > > 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.)
> > 
> > I think you are confusing libdrm_intel which was removed with
> > libdrm
> > (the ioctl wrappers) which is still a dependency. I don't think
> > there
> > is any plan to remove libdrm completely.
> 
> Okay, my bad. libdrm is used for opening and manipulating DRI device
> nodes after all.
> 
> > 
> > For cases where a user has different components, then they have to
> > copy the struct.
> 
> Yeah, I guess that's not much different from what we're doing in
> Chromium OS with proprietary vendor components already...
> 
> > 
> > > 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?
> > 
> > I'm still going to say libdrm. If that's really a problem, then we
> > can
> > split it out later.
> 
> At least for Chromium OS, libdrm would work fine indeed.

Excellent, so with the question of where (libdrm) covered, that leaves
us with what.

Currently this is what what the XXX_handle structs look like:
gbm_gralloc: https://github.com/robherring/gbm_gralloc/blob/master/gral
loc_drm_handle.h#L36

minigbm: https://chromium.googlesource.com/chromiumos/platform/minigbm/
+/master/cros_gralloc/cros_gralloc_handle.h#20

intel-minigbm: https://github.com/intel/minigbm/blob/master/cros_grallo
c/cros_gralloc_handle.h#L20

A lot seems to be shared between the 3, gbm_gralloc seems to have
modifier support, but lack multi-planar YUV support.

DRV_MAX_PLANES sounds to be a bit of a misnomer, as (I think) it refers
to YUV planes and not planes supported by the driver/hw.

I would assume that all shared variables would be available in the
libdrm-struct, as well the ones planar YUV support.

As for the non-obvious ones, maybe the should be listed so that we can
dig into if they are really needed, implementation specific or unused.

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/20171205/225506f4/attachment.sig>