[Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.

Mon Sep 3 11:07:30 UTC 2018

On 3 September 2018 at 08:14, Eric Engestrom <eric.engestrom at intel.com> wrote:
> On Monday, 2018-09-03 14:59:49 +0900, Tomasz Figa wrote:
>> On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> >
>> > On 30 August 2018 at 11:41, Eric Engestrom <eric.engestrom at intel.com> wrote:
>> > > On Thursday, 2018-08-30 17:55:14 +0900, Tomasz Figa wrote:
>> > >> Hi Erik, Emil, Eric,
>> > >>
>> > >> On Tue, Jul 10, 2018 at 12:41 AM Erik Faye-Lund <kusmabite at gmail.com> wrote:
>> > >> >
>> > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> > >> > >
>> > >> > > On 5 July 2018 at 17:17, Eric Engestrom <eric.engestrom at intel.com> wrote:
>> > >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote:
>> > >> > > >> On 5 July 2018 at 10:53, Eric Engestrom <eric.engestrom at intel.com> wrote:
>> > >> > > >> > On Monday, 2018-07-02 14:12:44 +0530, samiuddi wrote:
>> > >> > > >> >> This fixes crash due to NULL window when swap interval is set
>> > >> > > >> >> for pbuffer surface.
>> > >> > > >> >>
>> > >> > > >> >> Test: CtsDisplayTestCases pass
>> > >> > > >> >>
>> > >> > > >> >> Signed-off-by: samiuddi <sami.uddin.mohammad at intel.com>
>> > >> > > >> >> ---
>> > >> > > >> >>
>> > >> > > >> >> Kindly ignore this patch
>> > >> > > >> >> https://lists.freedesktop.org/archives/mesa-dev/2018-July/199098.html
>> > >> > > >> >>
>> > >> > > >> >>  src/egl/drivers/dri2/platform_android.c | 2 +-
>> > >> > > >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >> > > >> >>
>> > >> > > >> >> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
>> > >> > > >> >> index ca8708a..b5b960a 100644
>> > >> > > >> >> --- a/src/egl/drivers/dri2/platform_android.c
>> > >> > > >> >> +++ b/src/egl/drivers/dri2/platform_android.c
>> > >> > > >> >> @@ -485,7 +485,7 @@ droid_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy,
>> > >> > > >> >>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
>> > >> > > >> >>     struct ANativeWindow *window = dri2_surf->window;
>> > >> > > >> >>
>> > >> > > >> >> -   if (window->setSwapInterval(window, interval))
>> > >> > > >> >> +   if (window && window->setSwapInterval(window, interval))
>> > >> > > >> >>        return EGL_FALSE;
>> > >> > > >> >
>> > >> > > >> > Shouldn't we return false if we couldn't set the swap interval?
>> > >> > > >> >
>> > >> > > >> > I think this should be:
>> > >> > > >> >    if (!window || window->setSwapInterval(window, interval))
>> > >> > > >> >       return EGL_FALSE;
>> > >> > > >> >
>> > >> > > >> Changing the patch as above will lead to eglSwapInterval consistently
>> > >> > > >> failing for pbuffer surfaces.
>> > >> > > >
>> > >> > > > I'm not that familiar with pbuffers, but does SwapInterval really make
>> > >> > > > sense for them? I thought you couldn't swap a pbuffer.
>> > >> > > >
>> > >> > > > If so, failing an invalid op seems like the right thing to do.
>> > >> > > > Otherwise, I guess sure, no-op returning success is fine.
>> > >> > > >
>> > >> > > Looking at eglSwapInterval manpage [1] (with my annotation):
>> > >> > > "eglSwapInterval — specifies the minimum number of video frame periods
>> > >> > > per buffer swap for the _window_ associated with the current context."
>> > >> > > While the spec does not mention window/pbuffer/pixmap at all - it
>> > >> > > extensively refers to eglSwapBuffers.
>> > >> > >
>> > >> > > Wrt eglSwapBuffers manpage [2] and spec are consistent:
>> > >> > >
>> > >> > > "If surface is a pixel buffer or a pixmap, eglSwapBuffers has no
>> > >> > > effect, and no error is generated."
>> > >> > >
>> > >> > > Perhaps it's wrong to set eglSwapInterval for !window surfaces, but
>> > >> > > its sibling (eglSwapBuffers) does not error out.
>> > >> > > Hence I doubt many users make a distinction between window and pbuffer
>> > >> > > surfaces for eglSwap*.
>> > >> > >
>> > >> > > Worth bringing to the WG meeting - it' planned for 1st August.
>> > >> > >
>> > >> >
>> > >> > As I pointed out when I proposed this variant here:
>> > >> > https://patchwork.freedesktop.org/patch/219313/
>> > >> >
>> > >> > "Also, I don't think EGL_FALSE is the right return-value, as it doesn't
>> > >> > seem the EGL 1.5 spec defines any such error. Also, for instance
>> > >> > dri2_swap_interval returns EGL_TRUE when there's no driver-function,
>> > >> > which further backs the "silent failure" in this case IMO."
>> > >> >
>> > >> > So I think EGL_TRUE is the correct return-value for now. If the spec
>> > >> > gets changed, we can of course update our implementation.
>> > >>
>> > >> What happens to this patch in the end? It looks like we're observing a
>> > >> similar problem in Chrome OS.
>> > >>
>> > >> Emil, was there any follow-up on the WG meeting?
>> > >
>> > > Meeting was cancelled, but I raised the issue here:
>> > > https://gitlab.khronos.org/egl/API/merge_requests/17
>> > >
>> > > Right now we have ARM saying they return false + error and NVIDIA saying
>> > > they return true + no error and that ARM has a bug.
>> > > I think another party adding their opinion might nudge it forward :)
>> > >
>> > Fwiw I put forward a suggestion to add a workaround in the
>> > Android/CrOS libEGL wrapper for the ARM drivers.
>> > Although one could also consider the Nvidia/Mesa drivers as
>> > non-compliant and add a workaround for those instead.
>> >
>> > I have to admit it's not pretty, but it seems consistent with all the
>> > other workarounds in the wrapper.
>> >
>> > As Eric said - input from another party should, would be great.
>>
>> Thanks for the update.
>>
>> Still, Mesa is crashing right now, which is probably the least
>> conformant behavior. Do you think we could go with first version on
>> Erik's patch to just return EGL_TRUE?
>
> Sure; I forgot this was a crash and not just a test failing, sorry:
> Acked-by: Eric Engestrom <eric.engestrom at intel.com>
>
> We can always change this later if we need to return false.
>
> Tomasz, can you push this or do you want me to?
>
Looking at how badly things are copies across platforms a series which
harmonises everything.
Will send it out after lunch.

-Emil