> > Alright, so if I understand this correctly importing process would always > call gralloc registerBuffer(), which means that the FPs would always be > correct and usable by the importing process?
Yes, the Android framework calls registerBuffer() when importing the buffer to a different process. The FPs would introduce a dependency on libdrm on whichever piece of code would want to use the handle access functions, though [so would subclassing and sharing a handle]. > So if I'm understanding this correctly, something like this should work: > https://gitlab.collabora.com/robertfoss/libdrm/commits/gralloc_handle_v1 Not quite. It looks like you're defining another handle (since you subclass native_handle_t) to store the function pointers. If I understand the logic correctly, this would require the gralloc implementation to point to the relevant functions in (*registerBuffer()) and when allocating. The idea was more to take the handle as an input to the accessor library: // header file struct drmAndroidHandleInfo { uint32_t (*get_fourcc)(buffer_handle_t handle); ... } extern const struct drmAndroidHandleInfo my_info; // blah_gralloc_implementation.c #ifdef blah_GRALLOC #include "blah_gralloc_handle.h" static uint32_t blah_gralloc_get_fourcc(buffer_handle_t handle) { struct blah_gralloc_handle *blah_handle = (struct blah_gralloc_handle *) handle; return blah_handle->fourcc_format; } struct drmAndroidHandleInfo my_info = { .get_fourcc = blah_gralloc_get_fourcc, ... } #endif Then Mesa can just call my_info.get_fourcc(handle). On Fri, Jan 12, 2018 at 5:10 AM, Robert Foss <robert.f...@collabora.com> wrote: > > > On 1/12/18 9:29 AM, Tomasz Figa wrote: > >> Hi Rob, >> >> On Fri, Jan 12, 2018 at 5:26 AM, 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/ >>>>> cros_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, >>> >> >> Thanks! >> >> 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. >>> >> >> Why would be this struct passed between processes? >> >> The only data being exchanged between processes using gralloc is the >> handle and it isn't actually exchanged directly, but the exporting >> process will flatten it and send through Binder, while the importing >> one will have it unflattened and then the gralloc implementation >> called on it (the register buffer operation), which could update any >> per-process data in the handle. (But still, why would we need to >> include the function pointers there?) >> > > Alright, so if I understand this correctly importing process would always > call gralloc registerBuffer(), which means that the FPs would always be > correct and usable by the importing process. > > I'm not entirely familiar with which the different processes are apart > from SF, for which processes will registerBuffer() have to be called? > > So if I'm understanding this correctly, something like this should work: > https://gitlab.collabora.com/robertfoss/libdrm/commits/gralloc_handle_v1 > > (please excuse the struct being renamed back and forth, alloc seemed like > a slightly misleading name since it isn't generic, graphics specific) > > > Rob. > > > >> >>> 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? >>> >>> >> I guess it might not be a big deal whether FPs or structs are used, as >> long as they are not directly embedded in the handle, which we don't >> want constraints on. >> >> Best regards, >> Tomasz >> >> >>> 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
