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

[Tomasz Figa]

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.

>
>
>> @@ -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).

>
> 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?

Best regards,
Tomasz