On Thu, Jan 25, 2018 at 10:21 AM, Robert Foss <robert.f...@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.f...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
