[Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional

Emil Velikov emil.l.velikov at gmail.com
Mon Jul 18 14:58:26 UTC 2016


On 18 July 2016 at 13:02, Tomasz Figa <tfiga at chromium.org> wrote:
> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> Hi Tomasz,
>>
>> On 15 July 2016 at 08:53, Tomasz Figa <tfiga at chromium.org> wrote:
>>> We can support render nodes alone without any private headers, so let's
>>> make support for control nodes depend on presence of private drm_gralloc
>>> headers.
>>>
>>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>>> ---
>>>  src/egl/Android.mk                      |   1 +
>>>  src/egl/drivers/dri2/egl_dri2.h         |   2 +
>>>  src/egl/drivers/dri2/platform_android.c | 194 ++++++++++++++++++++++----------
>>>  3 files changed, 138 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/src/egl/Android.mk b/src/egl/Android.mk
>>> index bfd56a7..72ec02a 100644
>>> --- a/src/egl/Android.mk
>>> +++ b/src/egl/Android.mk
>>> @@ -41,6 +41,7 @@ LOCAL_SRC_FILES := \
>>>  LOCAL_CFLAGS := \
>>>         -D_EGL_NATIVE_PLATFORM=_EGL_PLATFORM_ANDROID \
>>>         -D_EGL_BUILT_IN_DRIVER_DRI2 \
>>> +       -DHAS_GRALLOC_DRM_HEADERS \
>>>         -DHAVE_ANDROID_PLATFORM
>>>
>>>  LOCAL_C_INCLUDES := \
>>> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
>>> index 3ffc177..6f9623b 100644
>>> --- a/src/egl/drivers/dri2/egl_dri2.h
>>> +++ b/src/egl/drivers/dri2/egl_dri2.h
>>> @@ -65,7 +65,9 @@
>>>  #endif
>>>
>>>  #include <hardware/gralloc.h>
>>> +#ifdef HAS_GRALLOC_DRM_HEADERS
>>>  #include <gralloc_drm_handle.h>
>>> +#endif
>> All of this/these can be simplified, by using a local header which
>> includes gralloc_drm_handle.h (if possible) and alternatively
>> providing dummy defines and static inline function(s).
>
> Sounds good to me. I'll give it a try.
>
My grammar is a bit off so here and example of what I meant, just in case:

cat local_header.h
#ifdef HAS_GRALLOC_DRM_HEADERS
#include <gralloc_drm_handle.h>
#include <gralloc_drm.h>
#else
#define FOO
static inline bar(...)
#endif

>>
>>
>>> @@ -509,53 +516,43 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
>>>  }
>>>
>>>  static _EGLImage *
>>> -dri2_create_image_android_native_buffer(_EGLDisplay *disp,
>>> -                                        _EGLContext *ctx,
>>> -                                        struct ANativeWindowBuffer *buf)
>>> +droid_create_image_from_prime_fd(_EGLDisplay *disp, _EGLContext *ctx,
>>> +                                 struct ANativeWindowBuffer *buf)
>> Please keep have this as a separate patch - "factorise
>> dri2_create_image_android_native_buffer"
>
> Okay.
>
>>
>>
>>> +   _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR: Only PRIME buffers are supported");
>>> +   return NULL;
>> This (s/NULL/0/) can live in as the static inline
>> gralloc_drm_get_gem_handle() in our local header.
>
> Okay.
>
>>
>>
>>> +#define DRM_RENDER_DEV_NAME  "%s/renderD%d"
>>> +
>>> +static int
>>> +droid_open_device(_EGLDisplay *dpy)
>>> +{
>>> +   struct dri2_egl_display *dri2_dpy = dpy->DriverData;
>>> +   const int limit = 64;
>>> +   const int base = 128;
>>> +   int fd;
>>> +   int i;
>>> +
>>> +   for (i = 0; i < limit; ++i) {
>>> +      char *card_path;
>>> +      if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + i) < 0)
>> Why do we need any of this ? What gralloc implementation are you guys using ?
>
> We are using our heavily rewritten fork of some old drm_gralloc
> release. It supports only render nodes and PRIME FDs and doesn't
> export the DRI device FD outside of its internals (which isn't
> actually even fully correct, at least for PRIME and render nodes, see
> my reply to Rob's comments).
>
That explain it, since https://chromium.googlesource.com/ does not
have gralloc, and
https://android.googlesource.com/platform/external/drm_gralloc/ has
both the DRM_FD define and the gem/flink function(s)?

Can I suggest porting the fd drm_gralloc/gbm_gralloc patches to your
private copy/repo. This way we'll have some consistency throughout
gralloc implementations and you can use gbm_gralloc directly in the
(hopefully) not too distant future.

>>
>> Afaict the latter must provide reasonable result for
>> hw_get_module(GRALLOC_HARDWARE_MODULE_ID...) and as it's missing the
>> perform hook existing code should work just fine. Right ?
>
> Existing code would fail with -1 as file descriptor, wouldn't it? Or
> I'm failing to see something?
>
Nope you're spot on - I had a dull moment. May I suggest revering the
patch which removed the GRALLOC_MODULE_PERFORM_GET_DRM_FD handling in
your gralloc ? Reason being is that the proposed code is very 'flaky'
and can open the wrong render node on systems which have more than
one.

Regards,
Emil


More information about the mesa-dev mailing list