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

[Gurchetan Singh]

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.
> >
> >
> > 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171221/3dfc6771/attachment.html>