[Mesa-dev] [Mesa-stable] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent
Tomasz Figa
tfiga at chromium.org
Wed May 17 21:16:24 UTC 2017
Hi,
On Tue, May 16, 2017 at 11:35 PM, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 05/16/2017 08:10 PM, Chad Versace wrote:
>>
>> 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?
>
>
> doh sorry, I actually meant 'Projection Cube' test, not the widget test. But
> yep, when tapping switcher button eglMakeCurrent starts to emit the error
> (likely the app just continues to submit frames even when it's
> 'minimized'?). When switching back to app it cannot draw anymore as it keeps
> getting the error.
I don't remember exactly and I'm not an Android app expert either, but
I remember hearing something that the app needs to stop drawing when
the activity is paused (stopped?). Isn't that what's happening when an
app gets minimized?
Best regards,
Tomasz
>
>> What was the behavior before this patch? And before
>> commit 23c86c74 (egl: Emit error when EGLSurface is lost)?
>
>
> Without this patch (and before commit 23c86c74) I can minimize app to
> switcher and bring it back and it continues to draw.
>
> Note that everything else seems to work just fine though (tested lots of GL
> apps) so is this just something specific what cts-verifier is doing or not
> handling properly?
>
>
>
>>>> 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
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
More information about the mesa-dev
mailing list