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

[Tomasz Figa]

On Mon, Jul 18, 2016 at 11:58 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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

I'd prefer if any code using flink names was not added back. On top of
that, our drm_gralloc doesn't really have much in common with that
from android-x86 anymore (as I said, it was heavily rewritten) and
there is not even a chance that with its current design flink names
could even work.

Also I'm wondering why we want to consider current brokenness of
drm_gralloc as something to be consistent with. It's supposed to be a
HAL library providing an uniform abstraction, but it exports private
APIs on the side instead. Moreover, as I mentioned before, flink names
are considered insecure and it would be really much better if we could
just forget about them.

> and you can use gbm_gralloc directly in the
> (hopefully) not too distant future.

I agree with this part, though. gbm_gralloc is definitely something
that we might want to migrate to in the future. Although it's a bit
lacking at the moment, so it might need a bit more time to develop the
missing bits. [I'm CCing Gurchetan, who was investigating GBM-backed
gralloc usable for our purposes.]

In any case, the missing flink API is quite easy to handle and can be
just stubbed out in a local header as you suggested. I don't think it
would hurt anyone and would definitely help us and anyone not willing
to export any private APIs from their gralloc and rely only on the
public HAL API.

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

I think the answer is a bit of yes and no at the same time.

Starting with no, it's incorrect for gralloc to share the DRI device
FD with Mesa for multiple reasons:
 - there are cases when the allocator used is different that the render node,
 - gralloc and Mesa might be stepping over each other - they would
share the GEM handle namespace, which is not refcounted,
 - it is generally undesirable to let an external library mess with
graphics context, because it is bug-prone (see above).

Then we have a small yes: We don't have to share the same FD, we can
just make the implementation of that perform call create a new FD
every time it's called.

However, after a bit of reflection, there is another no, because:
- gralloc has no knowledge what drivers are available in Mesa, so in
case there are multiple DRI nodes, it has no way to know which one to
open for Mesa to be happy. This is actually the solution that is flaky
and can open wrong render node, because Mesa at least can open a node
that it can use for rendering.
 - it's just yet another private API to rely on and it's not even
strictly necessary, because Mesa can open the device itself. My
intention is to remove the need for any private APIs from gralloc
implementation, including making the handle opaque, which is what
something pretending to be called HAL should do.

On top of that, there are already EGL DRI2 backends which open the
device on their own, i.e. surfaceless and drm.

Best regards,
Tomasz