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

[Emil Velikov]

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