[Mesa-dev] [RFC libdrm 0/5] Move alloc_handle_t from gralloc impls.
Robert Foss
robert.foss at collabora.com
Thu Jan 4 15:25:30 UTC 2018
On Fri, 2017-12-22 at 21:09 +0900, Tomasz Figa wrote:
> 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_gral
> > loc_handle.h
> > [consumer_usage, producer_usage, yuv_color_range, is_updated etc.]
> >
> > https://chromium.googlesource.com/chromiumos/platform/minigbm/+/mas
> > ter/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.
I think that would be the best way to do 1), and would be a small
change to the RFC.
As to 1) vs. 2), in my mind 2) seems like the best option.
I think it will have lower friction to making changes going forward.
Friction seems to be how we ended up where we are now, with a handful
of different open source projects all filling the same role.
But I don't maintain any of the gralloc implementations, so I don't
think my opinion particularly matters. Rob Herring and Gurchetan Singh
seem to prefer different solutions and we still haven't heard from the
intel-minigbm folks.
+ Kalyan Kondapally
>
> P.S. Top-posting is bad.
>
> Best regards,
> Tomasz
>
> > > >
> > > > On Wed, Dec 13, 2017 at 9:30 AM, Robert Foss <robert.foss at colla
> > > > bora.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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180104/9f2069f6/attachment.sig>
More information about the mesa-dev
mailing list