<div dir="ltr">Hi Robert,<br><div class="gmail_quote"><div dir="ltr">Il giorno mer 15 ago 2018 alle ore 09:37 Robert Foss <<a href="mailto:robert.foss@collabora.com">robert.foss@collabora.com</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hey Mauro,<br>
<br>
Thanks for catching this.<br>
<br>
On 14/08/2018 22.27, Mauro Rossi wrote:<br>
> This patch fixes a regression in mesa 18.2 and mesa-dev branches<br>
> for HAVE_DRM_GRALLOC code path which is causing black screen on Android<br>
> and prevents boot due to SIGSEGV MAPERR crash related to unproper handling<br>
> of drm_gralloc drm FD in new droid_open_device() path.<br>
> <br>
> The problem due to c7bb82136b ("egl/android: Add DRM node probing and filtering")<br>
> <br>
> ... 3173 3307 D GRALLOC-DRM: drmOpen radeon: 71<br>
> ... 3173 3307 I GRALLOC-RADEON: detected chipset 0x6841 family 0x31 (vram size 238MiB, gart size 1021MiB)<br>
> ... 3173 3307 I GRALLOC-DRM: create radeon for driver radeon<br>
> ... 3173 3307 W EGL-MAIN: Could not get native buffer FD<br>
> --------- beginning of crash<br>
> ... 3173 3307 F libc : Fatal signal 11 (SIGSEGV), code 1, fault addr 0x18 in tid 3307 (RenderThread), pid 3173 (ndroid.systemui)<br>
> ... 0 0 D : [drm:radeon_crtc_page_flip_target [radeon]] flip-ioctl() cur_rbo = 000000003512328a, new_rbo = 000000<br>
> ... 3420 3420 I crash_dump64: performing dump of process 3173 (target tid = 3307)<br>
> ... 3420 3420 F DEBUG : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***<br>
> ... 3420 3420 F DEBUG : Build fingerprint: 'Android-x86/android_x86_64/x86_64:8.1.0/OPM6.171019.030.E1/uten07210645:userdebug/test-keys'<br>
> ... 3420 3420 F DEBUG : Revision: '0'<br>
> ... 3420 3420 F DEBUG : ABI: 'x86_64'<br>
> ... 3420 3420 F DEBUG : pid: 3173, tid: 3307, name: RenderThread >>> com.android.systemui <<<<br>
> ... 3420 3420 F DEBUG : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x18<br>
> ... 3420 3420 F DEBUG : Cause: null pointer dereference<br>
> ... 3420 3420 F DEBUG : rax 0000000000000000 rbx 00007c6ac3a3eee0 rcx 0000000000000000 rdx 0000000000000038<br>
> ... 3420 3420 F DEBUG : rsi 0000000000000000 rdi 00007c6ab5bfeaa0<br>
> ... 3420 3420 F DEBUG : r8 00007c6ab04c16e4 r9 00007c6b4ee6a220 r10 0000000000000000 r11 0000000000000200<br>
> ... 3420 3420 F DEBUG : r12 0000000000000000 r13 0000000000000000 r14 0000000000000001 r15 00007c6ab04c1600<br>
> ... 3420 3420 F DEBUG : cs 0000000000000033 ss 000000000000002b<br>
> ... 3420 3420 F DEBUG : rip 00007c6ab0cee444 rbp 00007c6ac3ae8400 rsp 00007c6ab5bfea80 eflags 0000000000010246<br>
> ... 3420 3420 F DEBUG :<br>
> ... 3420 3420 F DEBUG : backtrace:<br>
> ... 3420 3420 F DEBUG : #00 pc 000000000056b444 /system/vendor/lib64/dri/gallium_dri.so (st_update_framebuffer_state+660)<br>
> ... 3420 3420 F DEBUG : #01 pc 0000000000569d61 /system/vendor/lib64/dri/gallium_dri.so (st_validate_state+561)<br>
> ... 3420 3420 F DEBUG : #02 pc 0000000000572374 /system/vendor/lib64/dri/gallium_dri.so (st_Clear+116)<br>
> ... 3420 3420 F DEBUG : #03 pc 000000000004a9c0 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #04 pc 0000000000085cab /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #05 pc 0000000000085f31 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #06 pc 000000000006e8c1 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #07 pc 000000000006e3d9 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #08 pc 000000000006c4e3 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #09 pc 00000000000704c8 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #10 pc 0000000000077fb9 /android/system/lib64/libhwui.so<br>
> ... 3420 3420 F DEBUG : #11 pc 00000000000117fa /android/system/lib64/libutils.so<br>
> ... 3420 3420 F DEBUG : #12 pc 00000000000ba193 /android/system/lib64/libandroid_runtime.so<br>
> ... 3420 3420 F DEBUG : #13 pc 0000000000079f0b /android/system/lib64/libc.so<br>
> ... 3420 3420 F DEBUG : #14 pc 0000000000028c5d /android/system/lib64/libc.so<br>
> ... 3420 3420 F DEBUG : #15 pc 0000000000027555 /android/system/lib64/libc.so<br>
> <br>
> To avoid the crash the former existing working droid_open_device() is restored,<br>
> renamed droid_open_device_drm_gralloc() and kept within HAVE_DRM_GRALLOC braces.<br>
> <br>
> NOTE: Definition of enum{} for GRALLOC_MODULE_PERFORM_GET_DRM_FD<br>
> is not necessary and it is actually causing a redefinition building error,<br>
> because in HAVE_DRM_GRALLOC path gralloc_drm.h is already exported<br>
> by libgralloc_drm which is currently still a dependency.<br>
> <br>
> Tested with mesa-dev and mesa 18.2 branch and oreo-x86 bootanimation<br>
> and Androdi GUI booting is fixed with i965, nouveau, radeon.<br>
> <br>
> The changes are compatible with gbm_gralloc, I've tested build with hwc too.<br>
<br>
I would maybe consider shortening the commit message a little bit, or at least <br>
remove<br>
the crash-logs.<br></blockquote><div><br></div><div>Of course, I will reduce the final commit message.</div><div><br></div><div>The segfault log was provided as a mean to understand what went wrong,</div><div>in case you/other developers will pursue the goal of having one droid_open_device() procedure</div><div>applicable to all gralloc implementation</div><div><br></div><div>and I will be available to test there is no regression on android-x86 drm_gralloc.</div><div>Kind regards</div><div><br></div><div>Mauro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> Fixes: c7bb82136b ("egl/android: Add DRM node probing and filtering")<br>
> Cc: "18.2" <<a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop.org</a>><br>
> Signed-off-by: Mauro Rossi <<a href="mailto:issor.oruam@gmail.com" target="_blank">issor.oruam@gmail.com</a>><br>
> ---<br>
> src/egl/drivers/dri2/platform_android.c | 23 +++++++++++++++++++++++<br>
> 1 file changed, 23 insertions(+)<br>
> <br>
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c<br>
> index cc16fd8118..834bbd258e 100644<br>
> --- a/src/egl/drivers/dri2/platform_android.c<br>
> +++ b/src/egl/drivers/dri2/platform_android.c<br>
> @@ -1134,6 +1134,25 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)<br>
> return (config_count != 0);<br>
> }<br>
> <br>
> +#ifdef HAVE_DRM_GRALLOC<br>
> +static int<br>
> +droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)<br>
> +{<br>
> + int fd = -1, err = -EINVAL;<br>
> +<br>
> + if (dri2_dpy->gralloc->perform)<br>
> + err = dri2_dpy->gralloc->perform(dri2_dpy->gralloc,<br>
> + GRALLOC_MODULE_PERFORM_GET_DRM_FD,<br>
> + &fd);<br>
> + if (err || fd < 0) {<br>
> + _eglLog(_EGL_WARNING, "fail to get drm fd");<br>
> + fd = -1;<br>
> + }<br>
> +<br>
> + return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;<br>
> +}<br>
> +#endif /* HAVE_DRM_GRALLOC */<br>
> +<br>
> static const struct dri2_egl_display_vtbl droid_display_vtbl = {<br>
> .authenticate = NULL,<br>
> .create_window_surface = droid_create_window_surface,<br>
> @@ -1384,7 +1403,11 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *disp)<br>
> <br>
> disp->DriverData = (void *) dri2_dpy;<br>
> <br>
> + #ifdef HAVE_DRM_GRALLOC<br>
> + dri2_dpy->fd = droid_open_device_drm_gralloc(dri2_dpy);<br>
> + #else<br>
> dri2_dpy->fd = droid_open_device(disp);<br>
> + #endif<br>
> if (dri2_dpy->fd < 0) {<br>
> err = "DRI2: failed to open device";<br>
> goto cleanup;<br>
> <br>
<br>
This does seem fairly un-intrusive if the GRALLOC_MODULE_PERFORM_GET_DRM_FD <br>
define is already being provided by drm_gralloc.<br>
<br>
If we are going to support the drm_gralloc usecase and this patch is needed<br>
to do so, I'm all for it.<br>
<br>
With the above suggestion fixed:<br>
Reviewed-by: Robert Foss <<a href="mailto:robert.foss@collabora.com" target="_blank">robert.foss@collabora.com</a>><br>
<br>
<br>
Rob.<br>
</blockquote></div></div>