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.
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.f...@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@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>>
>>>>>
>>>>>
>>>
>>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
