Hey Tomasz,

I'm tempted to split this work into two parts.
1) Move gbm&drm gralloc struct
2) Accessor functions

I would like to get 1) out the door to support John Stultzs current HiKey 960 efforts. As for 2), it would seem that we have some more discussing to do. But I'll keep pushing that forward.

Separately, I'd like to know if the below sketch of a func ptr solution is what you had in mind. From what I've gathered this is exactly what you've requested, but I would like to confirm it too.

I'll send out a v2, that covers 1) later today.


Rob.

On 01/29/2018 01:03 PM, Robert Foss wrote:


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

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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to