On Fri, 2017-12-22 at 21:09 +0900, Tomasz Figa wrote: > On Fri, Dec 22, 2017 at 10:09 AM, Gurchetan Singh > <gurchetansi...@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 <r...@kernel.org> > > wrote: > > > > > > On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh > > > <gurchetansi...@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@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@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> > > >
> >
> >
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
