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

[Tomasz Figa]

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

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.

Best regards,
Tomasz