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

[Chad Versace]

On Tue 16 May 2017, Tapani Pälli wrote:
> 
> 
> 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?

I'm unsure. I haven't yet tried that experiment. I'll give it a try when
I'm back at my desk.

Which EGL function is emitting EGL_BAD_NATIVE_WINDOW in logcat?
eglMakeCurrent or eglSwapBuffers, or something else?

What was the behavior before this patch? And before
commit 23c86c74 (egl: Emit error when EGLSurface is lost)?

> > 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.
> >       */
> > 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev