[Mesa-dev] [RFC libdrm 0/5] Move alloc_handle_t from gralloc impls.
Gurchetan Singh
gurchetansingh at chromium.org
Fri Jan 12 17:04:15 UTC 2018
>
> Alright, so if I understand this correctly importing process would always
> call gralloc registerBuffer(), which means that the FPs would always be
> correct and usable by the importing process?
Yes, the Android framework calls registerBuffer() when importing the buffer
to a different process. The FPs would introduce a dependency on libdrm on
whichever piece of code would want to use the handle access functions,
though [so would subclassing and sharing a handle].
> So if I'm understanding this correctly, something like this should work:
> https://gitlab.collabora.com/robertfoss/libdrm/commits/gralloc_handle_v1
Not quite. It looks like you're defining another handle (since you
subclass native_handle_t) to store the function pointers. If I understand
the logic correctly, this would require the gralloc implementation to point
to the relevant functions in (*registerBuffer()) and when allocating.
The idea was more to take the handle as an input to the accessor library:
// header file
struct drmAndroidHandleInfo {
uint32_t (*get_fourcc)(buffer_handle_t handle);
...
}
extern const struct drmAndroidHandleInfo my_info;
// blah_gralloc_implementation.c
#ifdef blah_GRALLOC
#include "blah_gralloc_handle.h"
static uint32_t blah_gralloc_get_fourcc(buffer_handle_t handle)
{
struct blah_gralloc_handle *blah_handle = (struct blah_gralloc_handle
*) handle;
return blah_handle->fourcc_format;
}
struct drmAndroidHandleInfo my_info = {
.get_fourcc = blah_gralloc_get_fourcc,
...
}
#endif
Then Mesa can just call my_info.get_fourcc(handle).
On Fri, Jan 12, 2018 at 5:10 AM, Robert Foss <robert.foss at collabora.com>
wrote:
>
>
> On 1/12/18 9:29 AM, Tomasz Figa wrote:
>
>> Hi Rob,
>>
>> On Fri, Jan 12, 2018 at 5:26 AM, Robert Foss <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> 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/minigb
>>>>> m/+/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,
>>>
>>
>> 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?)
>>
>
> Alright, so if I understand this correctly importing process would always
> call gralloc registerBuffer(), which means that the FPs would always be
> correct and usable by the importing process.
>
> I'm not entirely familiar with which the different processes are apart
> from SF, for which processes will registerBuffer() have to be called?
>
> So if I'm understanding this correctly, something like this should work:
> https://gitlab.collabora.com/robertfoss/libdrm/commits/gralloc_handle_v1
>
> (please excuse the struct being renamed back and forth, alloc seemed like
> a slightly misleading name since it isn't generic, graphics specific)
>
>
> Rob.
>
>
>
>>
>>> 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.
>>
>> Best regards,
>> Tomasz
>>
>>
>>> 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-Novembe
>>>>>>>> r/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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180112/81731779/attachment-0001.html>
More information about the mesa-dev
mailing list