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

[Robert Foss]

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> 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 <robh at kernel.org> wrote:
>>>
>>> On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh
>>> <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, 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.

>>>> 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.foss at 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-November/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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>>>
>>
>>