[Mesa-dev] [RFC libdrm 0/5] Move alloc_handle_t from gralloc impls.

Fri Dec 22 12:09:14 UTC 2017

On Fri, Dec 22, 2017 at 10:09 AM, Gurchetan Singh
<gurchetansingh at chromium.org> wrote:
> So the plan is for alloc_handle_t to not be sub-classed by the
> implementations, but have all necessary information that an implementation
> would need?
>
> If so, how do we reconcile the implementation specific information that is
> often in the handle:
>
> https://github.com/intel/minigbm/blob/master/cros_gralloc/cros_gralloc_handle.h
> [consumer_usage, producer_usage, yuv_color_range, is_updated etc.]
>
> https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/cros_gralloc/cros_gralloc_handle.h
> [use_flags, pixel_stride]
>
> In our case, removing our minigbm specific use flags from the handle would
> add complexity to our (*registerBuffer) path.
>
> On Thu, Dec 21, 2017 at 10:14 AM, Rob Herring <robh at kernel.org> wrote:
>>
>> On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh
>> <gurchetansingh at chromium.org> wrote:
>> > Hi Robert,
>> >
>> > Thanks for looking into this!  We need to decide if we want:
>> >
>> > (1) A common struct that implementations can subclass, i.e:
>> >
>> > struct blah_gralloc_handle {
>> >     alloc_handle_t alloc_handle;
>> >     int x, y, z;
>> >     ....
>> > }
>> >
>> > (2) An accessor library that vendors can implement, i.e:
>> >
>> > struct drmAndroidHandleInfo {
>> >    uint32_t (*get_fourcc)(buffer_handle_t handle);
>> >    uint32_t (*get_stride)(buffer_handle_t handle, uint32_t plane);
>> >    uint32_t (*get_offsets)(buffer_handle_t handle, uint32_t plane);
>> >    uint64_t (*get_modifier)(buffer_handle_t handle);
>> > };
>> >
>> > From my perspective as someone who has to maintain the minigbm gralloc
>> > implementation, (2) is preferable since:
>>
>> Yeah, I'd prefer not to encourage 1 as the default.
>>
>> > a) We really don't have a need for fields like data_owner, void *data,
>> > etc.
>>
>> We should be able to get rid of this. It's just for tracking imports.
>>
>> > Also, minigbm puts per plane fds, strides, offsets into the handle.
>> > Separating the information for the first plane (for the alloc_handle_t)
>> > and
>> > then rest of the planes would be annoying.
>>
>> The plan is to add those to alloc_handle_t.
>>
>> > b) we can avoid the struct within a struct that happens when we
>> > subclass,
>> > since alignment/padding issues often pop up during
>> > serialization/de-serialization.  Using __attribute__((aligned(xx))) is
>> > less
>> > portable than maintaining a POD struct.
>>
>> Yes. Even just between 32 and 64 bit it's problematic.
>>
>>
>> > c) IMO creating the handle should be left to the gralloc implementation.
>> > Having accessor functions clearly defines what we need from libdrm -- to
>> > make up for shortcomings of the gralloc API for DRM/KMS use cases.
>> >

I think there might be also d). Define a standard struct in libdrm
headers and add a custom call to gralloc that would fill in such
struct for given buffer. This would keep the libdrm handle independent
of gralloc's internal handle.

P.S. Top-posting is bad.

Best regards,
Tomasz

>> >
>> > On Wed, Dec 13, 2017 at 9:30 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 grallocs to communicate.
>> >>
>> >> Curretly missing is:
>> >>  - Planar formats
>> >>  - Get/Set functions
>> >>
>> >>
>> >> Planar formats
>> >> --------------
>> >> Support for planar formats is needed, but has not been added
>> >> yet, mostly since this was not already implemented in {gbm,drm}_gralloc
>> >> and the fact the having at least initial backwards compatability would
>> >> be nice. Anonymous unions can of course be used later on to provide
>> >> backwards compatability if so desired.
>> >>
>> >>
>> >> Get/Set functions
>> >> -----------------
>> >> During the previous discussion[1] one suggestion was to add accessor
>> >> functions. In this RFC I've only provided a alloc_handle_create()
>> >> function.
>> >>
>> >> The Get/Set functions have not been added yet, I was hoping for some
>> >> conclusive arguments for them being adeded.
>> >>
>> >> Lastly it was suggested by Rob Herring that having a fourcc<->android
>> >> pixel format conversion function would be useful.
>> >>
>> >>
>> >> [1]
>> >>
>> >> https://lists.freedesktop.org/archives/mesa-dev/2017-November/178199.html
>> >>
>> >> Robert Foss (5):
>> >>   android: Move gralloc handle struct to libdrm
>> >>   android: Add version variable to alloc_handle_t
>> >>   android: Mark alloc_handle_t magic variable as const
>> >>   android: Remove member name from alloc_handle_t
>> >>   android: Change alloc_handle_t format from Android format to fourcc
>> >>
>> >>  Android.mk                   |  8 +++-
>> >>  Makefile.sources             |  3 ++
>> >>  android/alloc_handle.h       | 87
>> >> ++++++++++++++++++++++++++++++++++++++++++++
>> >>  android/gralloc_drm_handle.h |  1 +
>> >>  4 files changed, 97 insertions(+), 2 deletions(-)
>> >>  create mode 100644 android/alloc_handle.h
>> >>  create mode 120000 android/gralloc_drm_handle.h
>> >>
>> >> --
>> >> 2.14.1
>> >>
>> >> _______________________________________________
>> >> mesa-dev mailing list
>> >> mesa-dev at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> >
>> >
>
>