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

[Robert Foss]

On 01/25/2018 08:52 PM, Rob Herring wrote:
> 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...

Ah, yes. gralloc_funcs_t was the intention.

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

I don't think I understand why? Exchanging handles containing function pointers 
may not be the the cleanest of solutions, but wouldn't it be possible?
Or is the need for having registerBuffer setting up the pointers going to be a 
problem?

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

If we're looking at non-handle based solutions then we wouldn't be able to, no.
As noted above I don't think I understand why non-handle based solutions would 
be required.

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

Thats' fair, the reason for adding the least common denominator of elements from 
the different implementation will maintain backwards compatability for 
cros&intel minigbm, not just gbm&drm gralloc.

In my mind this would be a 'nice to have' and if means simplifying this process, 
I'm happy to drop it.

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