On 1/12/18 9:29 AM, Tomasz Figa 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?)
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.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
