<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">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.</blockquote><div> </div><div>Which function? In theory, anything with a dependency on libdrm and a gralloc handle should be able use the FPs.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">It could be used to provide runtime support for multiple gralloc implementations, but that seems to about the only advantage to adding FPs.</blockquote><div> </div><div>Yes, that's the main benefit. The other two options are:</div><div> </div><div>(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.</div><div>(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.</div><div>(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). </div><div class="gmail_extra"> <div class="gmail_quote">On Thu, Jan 11, 2018 at 12:26 PM, Robert Foss <<a href="mailto:robert.foss@collabora.com" target="_blank">robert.foss@collabora.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Heya,<div><div class="gmail-m_2247219931219668624gmail-h5"> On 12/22/17 1:09 PM, Tomasz Figa wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On Fri, Dec 22, 2017 at 10:09 AM, Gurchetan Singh <<a href="mailto:gurchetansingh@chromium.org" target="_blank">gurchetansingh@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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: <a href="https://github.com/intel/minigbm/blob/master/cros_gralloc/cros_gralloc_handle.h" rel="noreferrer" target="_blank">https://github.com/intel/minigbm/blob/master/cros_gralloc/cros_gralloc_handle.h</a> [consumer_usage, producer_usage, yuv_color_range, is_updated etc.] <a href="https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/cros_gralloc/cros_gralloc_handle.h" rel="noreferrer" target="_blank">https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/cros_gralloc/cros_gralloc_handle.h</a> [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 <<a href="mailto:robh@kernel.org" target="_blank">robh@kernel.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh <<a href="mailto:gurchetansingh@chromium.org" target="_blank">gurchetansingh@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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: </blockquote> Yeah, I'd prefer not to encourage 1 as the default. </blockquote></blockquote></blockquote> </div></div> 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.<div class="gmail-m_2247219931219668624gmail-HOEnZb"><div class="gmail-m_2247219931219668624gmail-h5"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> a) We really don't have a need for fields like data_owner, void *data, etc. </blockquote> We should be able to get rid of this. It's just for tracking imports. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote> The plan is to add those to alloc_handle_t. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote> Yes. Even just between 32 and 64 bit it's problematic. <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote></blockquote></blockquote> 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 <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On Wed, Dec 13, 2017 at 9:30 AM, Robert Foss <<a href="mailto:robert.foss@collabora.com" target="_blank">robert.foss@collabora.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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] <a href="https://lists.freedesktop.org/archives/mesa-dev/2017-November/178199.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/mesa-dev/2017-November/178199.html</a> 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 <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote> </blockquote></blockquote> </blockquote></blockquote> </div></div></blockquote></div> </div></div>