[Mesa-dev] [PATCH 07/10] egl/android: Make drm_gralloc headers optional
Emil Velikov
emil.l.velikov at gmail.com
Mon Jul 18 17:35:36 UTC 2016
On 18 July 2016 at 16:38, Tomasz Figa <tfiga at chromium.org> wrote:
> 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.
>
Looks like I wasn't clear enough here, realyl sorry about that. No
objection on nuking _any_ of the gem/flink paths, but hoping to have
the behaviour consistent with the one described in
get_native_buffer_fd.
>>
>>>>
>>>> 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,
Can you please provide an example how the current open-source
gralloc/EGL stack might hit this ? Only a mix of closed and
open-source components comes to mind :-\
> - gralloc and Mesa might be stepping over each other - they would
> share the GEM handle namespace, which is not refcounted,
When working with gbm_gralloc that is a bugfix, imho. Other
implementations such as drm_gralloc (and/or minigbm ?) might be more
susceptible to such issues.
> - it is generally undesirable to let an external library mess with
> graphics context, because it is bug-prone (see above).
>
Hmm true, exposing this to users is not a good idea. We could keep it
as a interface private between {drm,gbm,X}gralloc and mesa.
> 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.
I'm afraid I cannot parse that, can you please elaborate ?
> 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.
>
Yes, the HAL should provide generic/opaque handles to users. At the
same time having a private interface between (vendor) specific driver
components (gralloc and EGL here) is not that crazy of an idea is it ?
I'd imagine that at least a few vendors have them in their stack.
One of the reasons behind gbm_gralloc was so that one can reuse the
existing DRI screen, thus granting access to the buffer metadata/etc.
making sharing easier/possible. Using it one can safely drop the final
EGL image restriction [1].
The alternative is to have an explicit interface between gralloc and
EGL. Doing that may be the better option in the long term, but it
could take some time to materialise.
> On top of that, there are already EGL DRI2 backends which open the
> device on their own, i.e. surfaceless and drm.
>
If you check the archives you'll see that there were concerns about
the direct probing in surfaceless. Furthermore I believe that it was
only ever tested with the [1] workaround and never with a pure
upstream mesa. The drm platform reuses the gbm provided DRI primitives
- exactly like gbm_gralloc :-)
TL:DR Abstracting things may be the better route in the long term, but
reusing the DRI primitives (which starts with reusing the fd) and thus
getting buffer sharing to work is the more robust option that works
with open-source drivers.
All of the above is my take on things, based on previous discussions
with Rob H/Rob C/Eric A. Although I could have misunderstood things ?
Regards,
Emil
[1] https://chromium.googlesource.com/chromiumos/third_party/mesa/+/54f583f346f7a5e628262622813363726837d668
More information about the mesa-dev
mailing list