[RFC,hwc] drm_hwcomposer: provide a common gralloc handle definition

EGL, gralloc, and HWC must all have a common definition of fd's and int's
in native_handle_t to share the fd and width, height, format, etc.
of a dmabuf.

Move the definition into HWC so we aren't dependent on a specific gralloc
implementation and so we don't have to create an importer just for
different native_handle_t layouts. This will allow supporting multiple
gralloc implementations that conform to this layout.

Perhaps we should allow for multiple levels of subclassing by only defining
the fields we care about here and not validating magic and the size.

Signed-off-by: Rob Herring <robh@kernel.org>
---
I'm not sure if there's a better way, but I can't find a way to remove 
the gralloc dependency. At least for EGL, only the fd is needed as it 
has the ANativeWindow and can get all the info from that.


 Android.mk           |  1 -
 gralloc_drm_handle.h | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 gralloc_drm_handle.h

2017-10-25 7:40 GMT+08:00 Rob Herring <robh@kernel.org>:
> EGL, gralloc, and HWC must all have a common definition of fd's and int's
> in native_handle_t to share the fd and width, height, format, etc.
> of a dmabuf.
>
> Move the definition into HWC so we aren't dependent on a specific gralloc
> implementation and so we don't have to create an importer just for
> different native_handle_t layouts. This will allow supporting multiple
> gralloc implementations that conform to this layout.
>
> Perhaps we should allow for multiple levels of subclassing by only defining
> the fields we care about here and not validating magic and the size.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> I'm not sure if there's a better way, but I can't find a way to remove
> the gralloc dependency. At least for EGL, only the fd is needed as it
> has the ANativeWindow and can get all the info from that.
>
>
>  Android.mk           |  1 -
>  gralloc_drm_handle.h | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 gralloc_drm_handle.h
>
> diff --git a/Android.mk b/Android.mk
> index 611fcb75190b..ee5b8bfb44d0 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -47,7 +47,6 @@ LOCAL_SHARED_LIBRARIES := \
>  LOCAL_STATIC_LIBRARIES := libdrmhwc_utils
>
>  LOCAL_C_INCLUDES := \
> -       external/gbm_gralloc \
>         system/core/libsync
>
>  LOCAL_SRC_FILES := \
> diff --git a/gralloc_drm_handle.h b/gralloc_drm_handle.h
> new file mode 100644
> index 000000000000..aa89e46db619
> --- /dev/null
> +++ b/gralloc_drm_handle.h
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (C) 2010-2011 Chia-I Wu <olvaffe@gmail.com>
> + * Copyright (C) 2010-2011 LunarG Inc.
> + * Copyright (C) 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> + * in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _GRALLOC_DRM_HANDLE_H_
> +#define _GRALLOC_DRM_HANDLE_H_
> +
> +#include <cutils/native_handle.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct gralloc_drm_handle_t {
> +       native_handle_t base;
> +
> +       /* file descriptors */
> +       int prime_fd;
> +
> +       /* integers */
> +       int magic;
> +
> +       int width;
> +       int height;
> +       int format;
> +       int usage;
> +       int stride; /* the stride in bytes */
> +       uint64_t modifier; /* buffer modifiers */

As a fix [1] I sent to you before,
it's better to force align the 64-bit int to 8.

[1]: https://github.com/robherring/gbm_gralloc/commit/d8b95e3ed2f0b38949c9339b6f5c9ffb32ca1089

> +       int data_owner; /* owner of data (for validation) */
> +       union {
> +               void *data; /* private pointer for gralloc */
> +               uint64_t reserved;
> +       } __attribute__((aligned(8)));
> +};
> +#define GRALLOC_HANDLE_MAGIC 0x5f47424d
> +#define GRALLOC_HANDLE_NUM_FDS 1
> +#define GRALLOC_HANDLE_NUM_INTS (                                              \
> +       ((sizeof(struct gralloc_drm_handle_t) - sizeof(native_handle_t))/sizeof(int))   \
> +        - GRALLOC_HANDLE_NUM_FDS)
> +
> +static inline struct gralloc_drm_handle_t *gralloc_drm_handle(buffer_handle_t _handle)
> +{
> +       struct gralloc_drm_handle_t *handle =
> +               (struct gralloc_drm_handle_t *) _handle;
> +
> +       if (handle && (handle->base.version != sizeof(handle->base) ||
> +                      handle->base.numInts != GRALLOC_HANDLE_NUM_INTS ||
> +                      handle->base.numFds != GRALLOC_HANDLE_NUM_FDS ||
> +                      handle->magic != GRALLOC_HANDLE_MAGIC))
> +               return NULL;
> +
> +       return handle;
> +}
> +
> +static inline int gralloc_drm_get_prime_fd(buffer_handle_t _handle)
> +{
> +       struct gralloc_drm_handle_t *handle = gralloc_drm_handle(_handle);
> +       return (handle) ? handle->prime_fd : -1;
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +#endif /* _GRALLOC_DRM_HANDLE_H_ */
> --

Hey Rob,

On Tue, 2017-10-24 at 18:40 -0500, Rob Herring wrote:
> EGL, gralloc, and HWC must all have a common definition of fd's and
> int's
> in native_handle_t to share the fd and width, height, format, etc.
> of a dmabuf.
> 
> Move the definition into HWC so we aren't dependent on a specific
> gralloc
> implementation and so we don't have to create an importer just for
> different native_handle_t layouts. This will allow supporting
> multiple
> gralloc implementations that conform to this layout.

I think this makes sense to me, and is an improvement over the current
situation.
As far as I can see there is no perfect way of doing this. But this is
definitely better.

> 
> Perhaps we should allow for multiple levels of subclassing by only
> defining
> the fields we care about here and not validating magic and the size.

So the idea with the subclassing would be for each gralloc
implementations to be able to add their special sauce?

> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> I'm not sure if there's a better way, but I can't find a way to
> remove 
> the gralloc dependency. At least for EGL, only the fd is needed as
> it 
> has the ANativeWindow and can get all the info from that.
> 
> 
>  Android.mk           |  1 -
>  gralloc_drm_handle.h | 85
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 gralloc_drm_handle.h
> 
> diff --git a/Android.mk b/Android.mk
> index 611fcb75190b..ee5b8bfb44d0 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -47,7 +47,6 @@ LOCAL_SHARED_LIBRARIES := \
>  LOCAL_STATIC_LIBRARIES := libdrmhwc_utils
>  
>  LOCAL_C_INCLUDES := \
> -	external/gbm_gralloc \
>  	system/core/libsync
>  
>  LOCAL_SRC_FILES := \
> diff --git a/gralloc_drm_handle.h b/gralloc_drm_handle.h
> new file mode 100644
> index 000000000000..aa89e46db619
> --- /dev/null
> +++ b/gralloc_drm_handle.h
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (C) 2010-2011 Chia-I Wu <olvaffe@gmail.com>
> + * Copyright (C) 2010-2011 LunarG Inc.
> + * Copyright (C) 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> included
> + * in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _GRALLOC_DRM_HANDLE_H_
> +#define _GRALLOC_DRM_HANDLE_H_
> +
> +#include <cutils/native_handle.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct gralloc_drm_handle_t {
> +	native_handle_t base;
> +
> +	/* file descriptors */
> +	int prime_fd;
> +
> +	/* integers */
> +	int magic;
> +
> +	int width;
> +	int height;
> +	int format;
> +	int usage;
> +	int stride; /* the stride in bytes */
> +	uint64_t modifier; /* buffer modifiers */
> +
> +	int data_owner; /* owner of data (for validation) */
> +	union {
> +		void *data; /* private pointer for gralloc */
> +		uint64_t reserved;
> +	} __attribute__((aligned(8)));
> +};
> +#define GRALLOC_HANDLE_MAGIC 0x5f47424d
> +#define GRALLOC_HANDLE_NUM_FDS 1
> +#define GRALLOC_HANDLE_NUM_INTS (					
> 	\
> +	((sizeof(struct gralloc_drm_handle_t) -
> sizeof(native_handle_t))/sizeof(int))	\
> +	 - GRALLOC_HANDLE_NUM_FDS)
> +
> +static inline struct gralloc_drm_handle_t
> *gralloc_drm_handle(buffer_handle_t _handle)
> +{
> +	struct gralloc_drm_handle_t *handle =
> +		(struct gralloc_drm_handle_t *) _handle;
> +
> +	if (handle && (handle->base.version != sizeof(handle->base)
> ||
> +	               handle->base.numInts !=
> GRALLOC_HANDLE_NUM_INTS ||
> +	               handle->base.numFds != GRALLOC_HANDLE_NUM_FDS
> ||
> +	               handle->magic != GRALLOC_HANDLE_MAGIC))
> +		return NULL;
> +
> +	return handle;
> +}
> +
> +static inline int gralloc_drm_get_prime_fd(buffer_handle_t _handle)
> +{
> +	struct gralloc_drm_handle_t *handle =
> gralloc_drm_handle(_handle);
> +	return (handle) ? handle->prime_fd : -1;
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +#endif /* _GRALLOC_DRM_HANDLE_H_ */

On Thu, Oct 26, 2017 at 6:59 AM, Robert Foss <robert.foss@collabora.com> wrote:
> Hey Rob,
>
> On Tue, 2017-10-24 at 18:40 -0500, Rob Herring wrote:
>> EGL, gralloc, and HWC must all have a common definition of fd's and
>> int's
>> in native_handle_t to share the fd and width, height, format, etc.
>> of a dmabuf.
>>
>> Move the definition into HWC so we aren't dependent on a specific
>> gralloc
>> implementation and so we don't have to create an importer just for
>> different native_handle_t layouts. This will allow supporting
>> multiple
>> gralloc implementations that conform to this layout.
>
> I think this makes sense to me, and is an improvement over the current
> situation.
> As far as I can see there is no perfect way of doing this. But this is
> definitely better.

There's also still a mesa to gralloc dependency to get the device FD.
We don't need that if using render nodes, but we still support
android-x86 using the card node and GEM names. Also, I'm not sure what
to do in the s/w rendering case that still uses the card node. Plus
with Treble, things are moving to separate processes, so retrieving an
FD is broken.

>> Perhaps we should allow for multiple levels of subclassing by only
>> defining
>> the fields we care about here and not validating magic and the size.
>
> So the idea with the subclassing would be for each gralloc
> implementations to be able to add their special sauce?

Right. So the owner and data pointer fields would be gone from here.
Not sure if it is worth the churn and breakage that may cause until
everyone is in sync.

Rob