On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov <emil.l.veli...@gmail.com> wrote: > > On 30 August 2018 at 11:41, Eric Engestrom <eric.engest...@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 <kusmab...@gmail.com> > >> wrote: > >> > > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov <emil.l.veli...@gmail.com> > >> > wrote: > >> > > > >> > > On 5 July 2018 at 17:17, Eric Engestrom <eric.engest...@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.engest...@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.moham...@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? Best regards, Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
