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

[Rob Herring]

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

[...]

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

For cases where a user has different components, then they have to
copy the struct.

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

Rob