[Mesa-dev] [RFC libdrm 0/5] Move alloc_handle_t from gralloc impls.

[Emil Velikov]

On 21 December 2017 at 18:14, Rob Herring <robh at kernel.org> wrote:
> On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh
> <gurchetansingh at chromium.org> wrote:
>> Hi Robert,
>>
>> Thanks for looking into this!  We need to decide if we want:
>>
>> (1) A common struct that implementations can subclass, i.e:
>>
>> struct blah_gralloc_handle {
>>     alloc_handle_t alloc_handle;
>>     int x, y, z;
>>     ....
>> }
>>
>> (2) An accessor library that vendors can implement, i.e:
>>
>> struct drmAndroidHandleInfo {
>>    uint32_t (*get_fourcc)(buffer_handle_t handle);
>>    uint32_t (*get_stride)(buffer_handle_t handle, uint32_t plane);
>>    uint32_t (*get_offsets)(buffer_handle_t handle, uint32_t plane);
>>    uint64_t (*get_modifier)(buffer_handle_t handle);
>> };
>>
>> From my perspective as someone who has to maintain the minigbm gralloc
>> implementation, (2) is preferable since:
>
> Yeah, I'd prefer not to encourage 1 as the default.
>
>> a) We really don't have a need for fields like data_owner, void *data, etc.
>
> We should be able to get rid of this. It's just for tracking imports.
>
>> Also, minigbm puts per plane fds, strides, offsets into the handle.
>> Separating the information for the first plane (for the alloc_handle_t) and
>> then rest of the planes would be annoying.
>
> The plan is to add those to alloc_handle_t.
>
>> b) we can avoid the struct within a struct that happens when we subclass,
>> since alignment/padding issues often pop up during
>> serialization/de-serialization.  Using __attribute__((aligned(xx))) is less
>> portable than maintaining a POD struct.
>
> Yes. Even just between 32 and 64 bit it's problematic.
>
Mostly a drive-by comment:

Iff alignment/padding is the major concern one could use something
like the wayland-egl ABI checks [1] and enforcing extra constrains.
Although in reality even without those, a similar test would be
strongly recommended. It will help greatly with versioning and ABI
stability.

Couple of important suggestions, wrt documentation:
 - is the version field bidirectional (aka Vulkan style) or not
 - how do things work, as various users support different version
 - the macro defined in the header is the current _defined_ version of
the interface and _not_ the one _implemented_ by A/B

Thanks
Emil

[1] https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c