[Mesa-stable] [Mesa-dev] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent

[Tapani Pälli]

On 05/16/2017 02:04 AM, Chad Versace wrote:
> Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS devices
> due to incorrect error handling in eglMakeCurrent. See below on how to
> confirm the regression is fixed.
> 
> This partially reverts
> 
>      commit 23c86c74cc450a23848b85cfe914376caede1cdf
>      Author:  Chad Versace <chadversary at chromium.org>
>      Subject: egl: Emit error when EGLSurface is lost
> 
> The bad commit added the error handling below. #2 and #3 were right,
> but #1 was wrong.
> 
>      1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the calling
>         thread has unflushed commands and either previous surface is no
>         longer valid.
> 
>      2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new surface
>         is no longer valid.
> 
>      3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped surface
>         is no longer valid.
> 
> Whe I wrote the bad commit, I misunderstood the EGL spec language
> for #1. The correct behavior is, if I understand correctly now:
> 
>      - Assume a bound EGLSurface is no longer valid.
>      - Assume the bound EGLContext has unflushed commands.
>      - The app calls eglMakeCurrent. The spec requires eglMakeCurrent to
>        implicitly flush. After flushing, eglMakeCurrent emits
>        EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
>        current bindings.
>      - If the app calls eglMakeCurrent again, and the app inserts no
>        commands into the GL command stream between the two eglMakeCurrent
>        calls, then this second eglMakeCurrent succeeds without emitting an
>        error.
> 
> How to confirm this fixes the regression:
> 
>      Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
>      source.android.com, unpack, and `adb install CtsVerifier.apk`.
>      Run test "Projection Cube". Click the Pass button (a
>      green checkmark). Then run test "Projection Widget". Confirm that
>      widgets are visible and that logcat does not complain about
>      eglMakeCurrent failure.
> 
>      Then confirm there are no regressions in the cts-traded module that
>      commit 263243b1 fixed:
> 
>          cts-tf > run cts --skip-preconditions --skip-device-info \
>                   -m CtsCameraTestCases \
>                   -t android.hardware.camera2.cts.RobustnessTest
> 
>      Tested with Chrome OS board "reef".

both tests passed on Android-IA with this patch ... but if I minimize 
"Projection Widget" test it starts to bang EGL_BAD_NATIVE_WINDOW 
heavily. Is this expected behavior?

> Cc: "17.1" <mesa-stable at lists.freedesktop.org>
> Cc: Tomasz Figa <tfiga at chromium.org>
> Cc: Nicolas Boichat <drinkcat at chromium.org>
> Cc: Emil Velikov <emil.velikov at collabora.com>
> Fixes: 23c86c74 (egl: Emit error when EGLSurface is lost)
> ---
>   src/egl/main/eglapi.c | 19 -------------------
>   1 file changed, 19 deletions(-)
> 
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index aa0eb94666..9cea2f41ff 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -828,25 +828,6 @@ eglMakeCurrent(EGLDisplay dpy, EGLSurface draw, EGLSurface read,
>            RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE);
>      }
>   
> -   _EGLThreadInfo *t =_eglGetCurrentThread();
> -   _EGLContext *old_ctx = t->CurrentContext;
> -   _EGLSurface *old_draw_surf = old_ctx ? old_ctx->DrawSurface : NULL;
> -   _EGLSurface *old_read_surf = old_ctx ? old_ctx->ReadSurface : NULL;
> -
> -   /* From the EGL 1.5 spec, Section 3.7.3 Binding Context and Drawables:
> -    *
> -    *    If the previous context of the calling thread has unflushed commands,
> -    *    and the previous surface is no longer valid, an
> -    *    EGL_BAD_CURRENT_SURFACE error is generated.
> -    *
> -    * It's difficult to check if the context has unflushed commands, but it's
> -    * easy to check if the surface is no longer valid.
> -    */
> -   if (old_draw_surf && old_draw_surf->Lost)
> -      RETURN_EGL_ERROR(disp, EGL_BAD_CURRENT_SURFACE, EGL_FALSE);
> -   if (old_read_surf && old_read_surf->Lost)
> -      RETURN_EGL_ERROR(disp, EGL_BAD_CURRENT_SURFACE, EGL_FALSE);
> -
>      /*    If a native window underlying either draw or read is no longer valid,
>       *    an EGL_BAD_NATIVE_WINDOW error is generated.
>       */
>