| Message ID | 20171031145920.20343-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 31, 2017 at 09:59:20AM -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. > > For now, this is aligned with gbm_gralloc's struct. Once we change > gbm_gralloc and mesa to point to this copy, we can make modifications to > the struct. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > Android.mk | 1 - > gralloc_drm_handle.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 87 insertions(+), 1 deletion(-) > create mode 100644 gralloc_drm_handle.h > > diff --git a/Android.mk b/Android.mk > index 8b11e375adde..ee5b8bfb44d0 100644 > --- a/Android.mk > +++ b/Android.mk > @@ -47,7 +47,6 @@ LOCAL_SHARED_LIBRARIES := \ > LOCAL_STATIC_LIBRARIES := libdrmhwc_utils > > LOCAL_C_INCLUDES := \ > - external/drm_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..e2f35dda539b > --- /dev/null > +++ b/gralloc_drm_handle.h > @@ -0,0 +1,87 @@ > +/* > + * Copyright (C) 2010-2011 Chia-I Wu <olvaffe@gmail.com> > + * Copyright (C) 2010-2011 LunarG Inc. > + * Copyright (C) 2016-2017 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 name; /* the name of the bo */ > + int stride; /* the stride in bytes */ > + int data_owner; /* owner of data (for validation) */ > + > + uint64_t modifier __attribute__((aligned(8))); /* buffer modifiers */ > + union { > + void *data; /* private pointer for gralloc */ > + uint64_t reserved; > + } __attribute__((aligned(8))); > +}; > +#define GRALLOC_HANDLE_MAGIC 0x5f47424d Maybe add a #define GRALLOC_HANDLE_VERSION 1 #define GRALLOC_HANDLE_MAGIC (0x5f47424d + GRALLOC_HANDLE_VERSION) and a comment that the version needs to be incremented for every change, to catch mismatching mesa/hwc versions? Given the layout there's no way to add more fds in a backwards-compatible way, so we need full versioning I think, and treat any change as a breaking one. -Daniel > +#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_ */ > -- > 2.14.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 2017-10-31 at 17:44 +0100, Daniel Vetter wrote: > On Tue, Oct 31, 2017 at 09:59:20AM -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. > > > > For now, this is aligned with gbm_gralloc's struct. Once we change > > gbm_gralloc and mesa to point to this copy, we can make > > modifications to > > the struct. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > Android.mk | 1 - > > gralloc_drm_handle.h | 87 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 87 insertions(+), 1 deletion(-) > > create mode 100644 gralloc_drm_handle.h > > > > diff --git a/Android.mk b/Android.mk > > index 8b11e375adde..ee5b8bfb44d0 100644 > > --- a/Android.mk > > +++ b/Android.mk > > @@ -47,7 +47,6 @@ LOCAL_SHARED_LIBRARIES := \ > > LOCAL_STATIC_LIBRARIES := libdrmhwc_utils > > > > LOCAL_C_INCLUDES := \ > > - external/drm_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..e2f35dda539b > > --- /dev/null > > +++ b/gralloc_drm_handle.h > > @@ -0,0 +1,87 @@ > > +/* > > + * Copyright (C) 2010-2011 Chia-I Wu <olvaffe@gmail.com> > > + * Copyright (C) 2010-2011 LunarG Inc. > > + * Copyright (C) 2016-2017 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 name; /* the name of the bo */ > > + int stride; /* the stride in bytes */ > > + int data_owner; /* owner of data (for validation) */ > > + > > + uint64_t modifier __attribute__((aligned(8))); /* buffer > > modifiers */ > > + union { > > + void *data; /* private pointer for gralloc */ > > + uint64_t reserved; > > + } __attribute__((aligned(8))); > > +}; > > +#define GRALLOC_HANDLE_MAGIC 0x5f47424d > > Maybe add a > > #define GRALLOC_HANDLE_VERSION 1 > #define GRALLOC_HANDLE_MAGIC (0x5f47424d + GRALLOC_HANDLE_VERSION) > > and a comment that the version needs to be incremented for every > change, > to catch mismatching mesa/hwc versions? Given the layout there's no > way to > add more fds in a backwards-compatible way, so we need full > versioning I > think, and treat any change as a breaking one. > -Daniel This seems like a good idea to me. With this change feel free to add my r-b. > > > +#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_ */ > > -- > > 2.14.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > >
On Tue, Oct 31, 2017 at 11:44 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Oct 31, 2017 at 09:59:20AM -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. [...] >> +#define GRALLOC_HANDLE_MAGIC 0x5f47424d > > Maybe add a > > #define GRALLOC_HANDLE_VERSION 1 > #define GRALLOC_HANDLE_MAGIC (0x5f47424d + GRALLOC_HANDLE_VERSION) > > and a comment that the version needs to be incremented for every change, > to catch mismatching mesa/hwc versions? Given the layout there's no way to > add more fds in a backwards-compatible way, so we need full versioning I > think, and treat any change as a breaking one. If we add or remove anything that will already be caught because sizes are stored and checked. The magic corresponds to 'GBM_'. Maybe we want to change that to something else. However, I'd like to not change it just yet. My intention was to keep it compatible, update gbm_gralloc and mesa dependencies, then make any changes to this struct. Rob
On Tue, Oct 31, 2017 at 04:10:52PM -0500, Rob Herring wrote: > On Tue, Oct 31, 2017 at 11:44 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Oct 31, 2017 at 09:59:20AM -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. > > [...] > > >> +#define GRALLOC_HANDLE_MAGIC 0x5f47424d > > > > Maybe add a > > > > #define GRALLOC_HANDLE_VERSION 1 > > #define GRALLOC_HANDLE_MAGIC (0x5f47424d + GRALLOC_HANDLE_VERSION) > > > > and a comment that the version needs to be incremented for every change, > > to catch mismatching mesa/hwc versions? Given the layout there's no way to > > add more fds in a backwards-compatible way, so we need full versioning I > > think, and treat any change as a breaking one. > > If we add or remove anything that will already be caught because sizes > are stored and checked. > > The magic corresponds to 'GBM_'. Maybe we want to change that to > something else. However, I'd like to not change it just yet. My > intention was to keep it compatible, update gbm_gralloc and mesa > dependencies, then make any changes to this struct. Hm, start out with version 0 then, to match? Anyway, was just a suggestion, feel free to ignore :-) -Daniel
diff --git a/Android.mk b/Android.mk index 8b11e375adde..ee5b8bfb44d0 100644 --- a/Android.mk +++ b/Android.mk @@ -47,7 +47,6 @@ LOCAL_SHARED_LIBRARIES := \ LOCAL_STATIC_LIBRARIES := libdrmhwc_utils LOCAL_C_INCLUDES := \ - external/drm_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..e2f35dda539b --- /dev/null +++ b/gralloc_drm_handle.h @@ -0,0 +1,87 @@ +/* + * Copyright (C) 2010-2011 Chia-I Wu <olvaffe@gmail.com> + * Copyright (C) 2010-2011 LunarG Inc. + * Copyright (C) 2016-2017 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 name; /* the name of the bo */ + int stride; /* the stride in bytes */ + int data_owner; /* owner of data (for validation) */ + + uint64_t modifier __attribute__((aligned(8))); /* buffer modifiers */ + 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_ */
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. For now, this is aligned with gbm_gralloc's struct. Once we change gbm_gralloc and mesa to point to this copy, we can make modifications to the struct. Signed-off-by: Rob Herring <robh@kernel.org> --- Android.mk | 1 - gralloc_drm_handle.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 gralloc_drm_handle.h