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

[Rob Herring]

On Thu, Jan 25, 2018 at 10:21 AM, Robert Foss <robert.foss at collabora.com> wrote:
> 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 {

This is not a handle...

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

This doesn't go in the handle, but rather you would retrieve this
struct I guess with a "perform" call to gralloc AIUI.

Of course, if you have 1 perform call, then why not just a perform op
for each accessor. Does perform even exist in a gralloc 2
implementation?

>     ...
> } gralloc_handle_t;
>
> For reasons of backwards compatability gralloc_handle_t should probably
> contain whatever gbm_gralloc_handle_t contains now too.

Being backwards compatible with upstream (to the extent there is one)
was a goal. You can't really have that and what Tomasz proposes.

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

I've yet to hear technical reasons why the handle struct needs to be different.

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