[Mesa-dev] [RFC libdrm 0/5] Move alloc_handle_t from gralloc impls.

[Robert Foss]

Hey,

On 01/13/2018 12:49 AM, Gurchetan Singh wrote:
>     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.

Alright, this seems straight forward enough. As for the accessor 
implementations, does anyone mind if I start out with support for multiple 
planes even if the buffer handle currently doesn't contain multi plane support
in various fields (fds, strides, offsets, etc.).


Rob.

> 
> On Fri, Jan 12, 2018 at 11:44 AM, Rob Herring <robh at kernel.org 
> <mailto:robh at kernel.org>> wrote:
> 
>     On Fri, Jan 12, 2018 at 2:29 AM, Tomasz Figa <tfiga at chromium.org
>     <mailto:tfiga at chromium.org>> wrote:
>      > Hi Rob,
>      >
>      > On Fri, Jan 12, 2018 at 5:26 AM, Robert Foss <robert.foss at collabora.com
>     <mailto:robert.foss at 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
>      >>> <gurchetansingh at chromium.org <mailto:gurchetansingh at 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
>     <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
>     <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 <robh at kernel.org
>     <mailto:robh at kernel.org>> wrote:
>      >>>>>
>      >>>>>
>      >>>>> On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh
>      >>>>> <gurchetansingh at chromium.org <mailto:gurchetansingh at 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
> 
>