On 18 July 2016 at 13:02, Tomasz Figa <tf...@chromium.org> wrote:
> On Mon, Jul 18, 2016 at 7:28 PM, Emil Velikov <emil.l.veli...@gmail.com>
> wrote:
>> Hi Tomasz,
>>
>> On 15 July 2016 at 08:53, Tomasz Figa <tf...@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 <tf...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
