> > 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. >
Accessor functions in libdrm sound good to me. On Fri, Jan 12, 2018 at 11:44 AM, Rob Herring <r...@kernel.org> wrote: > On Fri, Jan 12, 2018 at 2:29 AM, Tomasz Figa <tf...@chromium.org> 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/ > minigbm/+/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?) > > > >> > >> 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. > > 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 >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
