> > So I had a look into implementing this, and using function pointers is > problematic due to this struct being passed between processes which would > prevent mesa calling a function assigned in gbm_gralloc for example.
Which function? In theory, anything with a dependency on libdrm and a gralloc handle should be able use the FPs. It could be used to provide runtime support for multiple gralloc > implementations, but that seems to about the only advantage to adding FPs. Yes, that's the main benefit. The other two options are: (a) standardize the handle. I'm still not sure how we're going to reconcile the implementation specific differences that I pointed out in my last message. (b) subclass the handles. If I'm reading everyone's comments correctly, no one is in favor of this b/c of alignment/padding issues. (c) Standard struct in libdrm + a perform call in gralloc. However, the issue with that is (*perform) is removed in gralloc1 and HIDL gralloc (/hardware/interfaces/graphics/mapper/2.0/IMapper.hal and hardware/interfaces/graphics/allocator/2.0/IAllocator.hal in AOSP). On Thu, Jan 11, 2018 at 12:26 PM, Robert Foss <robert.f...@collabora.com> wrote: > Heya, > > > On 12/22/17 1:09 PM, 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/cr >>> os_gralloc_handle.h >>> [consumer_usage, producer_usage, yuv_color_range, is_updated etc.] >>> >>> https://chromium.googlesource.com/chromiumos/platform/minigb >>> m/+/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 <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. >>>> >>>> > So I had a look into implementing this, and using function pointers is > problematic due to this struct being passed between processes which would > prevent mesa calling a function assigned in gbm_gralloc for example. > > It could be used to provide runtime support for multiple gralloc > implementations, but that seems to about the only advantage to adding FPs. > > Am I missing a good usecase for FPs? If not I think the added > complexity/cruft is more problematic than good. > > Any thoughts? > > > Rob. > > > 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.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 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-Novembe >>>>>> r/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 >>>>>> >>>>> >>>>> >>>>> >>> >>>
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
