<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">We can define accessor functions too (not ptrs), then the struct is opaque and you can do your own accessor implementation if aligning is not possible or desired. </blockquote><div> </div><div>Accessor functions in libdrm sound good to me. </div></div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Jan 12, 2018 at 11:44 AM, Rob Herring <<a href="mailto:robh@kernel.org" target="_blank">robh@kernel.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Jan 12, 2018 at 2:29 AM, Tomasz Figa <<a href="mailto:tfiga@chromium.org">tfiga@chromium.org</a>> wrote: > Hi Rob, > > On Fri, Jan 12, 2018 at 5:26 AM, Robert Foss <<a href="mailto:robert.foss@collabora.com">robert.foss@collabora.com</a>> wrote: >> Heya, >> >> >> On 12/22/17 1:09 PM, Tomasz Figa wrote: >>> >>> On Fri, Dec 22, 2017 at 10:09 AM, Gurchetan Singh >>> <<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> 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: >>>> >>>> >>>> <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">robh@kernel.org</a>> wrote: >>>>> >>>>> >>>>> On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh >>>>> <<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> 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?) > >> >> 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. </div></div>Why no constraints? Is converging on a common handle really that hard? It is mostly all the same data. Some of the differences are just because a particular implementation hasn't addressed some feature (e.g. planar formats). There's other things like the pid and data ptr in drm_gralloc and gbm_gralloc that really should be removed (it's just for tracking imports). If we have some fields that are unused by some implementations, that seems fine to me. We're really only talking about what is the interface to mesa and drm_hwc for handles (and mostly drm_hwc because mesa just needs the dma-buf fd(s)). Today it is simply accessing the raw handle. Moving that to libdrm doesn't change that and is no worse. It's at least better in that drm_gralloc and gbm_gralloc can share it. Making it work with cros_gralloc doesn't seem hard. We can define accessor functions too (not ptrs), then the struct is opaque and you can do your own accessor implementation if aligning is not possible or desired. Or you can do your own importer within drm_hwc too. Rob </blockquote></div> </div>