[Mesa-dev] [PATCH v1 0/7] Implement commont gralloc_handle_t in libdrm

[Robert Foss]

Hey Tomasz,

On 01/24/2018 11:04 AM, Tomasz Figa wrote:
> Hi Robert,
> 
> On Wed, Jan 17, 2018 at 2:36 AM, Robert Foss <robert.foss at collabora.com> wrote:
>> This series moves {gbm,drm,cros}_gralloc_handle_t struct to libdrm,
>> since at least 4 implementations exist, and share a lot of contents.
>> The idea is to keep the common stuff defined in one place, and libdrm
>> is the common codebase to all of these platforms.
>>
>>
>> Additionally, having this struct defined in libdrm will make it
>> easier for mesa and gralloc implementations to communicate.
>>
>> Robert Foss (7):
>>    android: Move gralloc handle struct to libdrm
>>    android: Add version variable to gralloc_handle_t
>>    android: Mark gralloc_handle_t magic variable as const
>>    android: Remove member name from gralloc_handle_t
>>    android: Change gralloc_handle_t format from Android format to fourcc
>>    android: Change gralloc_handle_t members to be fixed width
>>    android: Add accessor functions for gralloc_handle_t variables
> 
> Again, thanks for working on this.
> 
> I looked through the series and it seems to be much different from
> what I imagined when writing my previous reply. I must have
> misunderstood your proposal back then.

Ah, glad we caught it before v2 then :)

> 
> Generally, current series doesn't solve Chromium OS main concern of
> locking down the handle struct. Even though accessors are added, they
> are implemented in libdrm and refer to the exact handle layout as per
> the handle struct defined by libdrm.

So solving the problems of multiple projects is the goal, so reconsidering is 
probably they way forward.

> 
> What I had in my mind, would be creating a secondary struct,
> consisting only of callbacks, which would be filled in by particular
> gralloc implementation running in the system with its accessors. This
> would completely eliminate any dependencies on the handle struct
> itself from consumers of gralloc buffers.

So just to sketch out the solution, it would look something like this?

struct gralloc_handle_t {
     uint32_t (*get_fd)(buffer_handle_t handle, uint32_t plane);
     uint64_t (*get_modifier)(buffer_handle_t handle, uint32_t plane);
     uint32_t (*get_offsets)(buffer_handle_t handle, uint32_t plane);
     uint32_t (*get_stride)(buffer_handle_t handle, uint32_t plane);
     ...
} gralloc_funcs_t;

struct gralloc_handle_t {
     native_handle_t base;

     /* api variables */
     const int magic; /* differentiate between allocator impls */
     const int version; /* api version */

     gralloc_funcs_t funcs;

     ...
} gralloc_handle_t;

For reasons of backwards compatability gralloc_handle_t should probably
contain whatever gbm_gralloc_handle_t contains now too.
Since we're going to version this struct, we can always drop extraneous 
variables later.
Since we'll be able to drop variables, we could add more variables to support 
the cros minigbm variables of even the intel minigbm ones.
This would be a bit high churn, but probably ease adoption.

Additionally the gralloc buffer registering mechanism doesn't exist in any of 
the gralloc implementations, so being able to start out with something that 
works on all platforms would be nice.


Rob.