Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
On Wed, Sep 27, 2017 at 11:06 PM, Emil Velikov wrote: > On 25 September 2017 at 08:25, Tomasz Figa wrote: > >> Gentle ping. :) >> > > I forgot that you don't have commit access. Can you please apply for one? Yep, I have it on my list, sorry for being slow. Thanks for useful links. (Need to dig out my PGP key.) Best regards, Tomasz > > Thanks > Emil > > [1] https://www.freedesktop.org/wiki/AccountRequests/ > [2] https://bugs.freedesktop.org/show_bug.cgi?id=99601 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
On 25 September 2017 at 08:25, Tomasz Figa wrote: > Gentle ping. :) > I forgot that you don't have commit access. Can you please apply for one? Thanks Emil [1] https://www.freedesktop.org/wiki/AccountRequests/ [2] https://bugs.freedesktop.org/show_bug.cgi?id=99601 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
On Fri, Aug 11, 2017 at 1:31 PM, Tomasz Figa wrote: > On Fri, Aug 11, 2017 at 2:29 AM, Emil Velikov > wrote: >> On 10 August 2017 at 14:59, Tomasz Figa wrote: >>> dri2_fallback_swap_interval() currently used to stub out swap interval >>> support in Android backend does nothing besides returning EGL_FALSE. >>> This causes at least one known application (Android Snapchat) to fail >>> due to an unexpected error and my loose interpretation of the EGL 1.5 >>> specification justifies it. Relevant quote below: >>> >>> The function >>> >>> EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); >>> >>> specifies the minimum number of video frame periods per buffer swap >>> for the draw surface of the current context, for the current rendering >>> API. [...] >>> >>> The parameter interval specifies the minimum number of video frames >>> that are displayed before a buffer swap will occur. The interval >>> specified by the function applies to the draw surface bound to the >>> context that is current on the calling thread. [...] interval is >>> silently clamped to minimum and maximum implementation dependent >>> values before being stored; these values are defined by EGLConfig >>> attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL >>> respectively. >>> >>> The default swap interval is 1. >>> >>> Even though it does not specify the exact behavior if the platform does >>> not support changing the swap interval, the default assumed state is the >>> swap interval of 1, which I interpret as a value that eglSwapInterval() >>> should succeed if called with, even if there is no ability to change the >>> interval (but there is no change requested). Moreover, since the >>> behavior is defined to clamp the requested value to minimum and maximum >>> and at least the default value of 1 must be present in the range, the >>> implementation might be expected to have a valid range, which in case of >>> the feature being unsupported, would correspond to {1} and any request >>> might be expected to be clamped to this value. >>> >>> Fix this by defaulting dri2_dpy's min_swap_interval, max_swap_interval >>> and default_swap_interval to 1 in dri2_setup_screen() and let platforms, >>> which support this functionality set their own values after this >>> function returns. Thanks to patches merged earlier, we can also remove >>> the dri2_fallback_swap_interval() completely, as with a singular range >>> it would not be called anyway. >>> >>> v2: Remove dri2_fallback_swap_interval() completely thanks to higher >>> layer already clamping the requested interval and not calling the >>> driver layer if the clamped value is the same as current. >>> >> Not every driver is EGL 1.5, although the 1.4 spec has exact same text. >> >> Reviewed-by: Emil Velikov >> >> We could have this in stable, but will need a clamp like v1 since >> Eric's rework did not land there. >> Gents, how do you feel on the topic? > > FWIW, Eric's patches applied cleanly onto our chromium branch, which > was based on something not very far away from what 17.2 branched at. > If for some reasons we prefer to avoid further cherry picks, we can go > with my v1 for stable, which doesn't depend on them. Gentle ping. :) Thanks, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
On Fri, Aug 11, 2017 at 2:29 AM, Emil Velikov wrote: > On 10 August 2017 at 14:59, Tomasz Figa wrote: >> dri2_fallback_swap_interval() currently used to stub out swap interval >> support in Android backend does nothing besides returning EGL_FALSE. >> This causes at least one known application (Android Snapchat) to fail >> due to an unexpected error and my loose interpretation of the EGL 1.5 >> specification justifies it. Relevant quote below: >> >> The function >> >> EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); >> >> specifies the minimum number of video frame periods per buffer swap >> for the draw surface of the current context, for the current rendering >> API. [...] >> >> The parameter interval specifies the minimum number of video frames >> that are displayed before a buffer swap will occur. The interval >> specified by the function applies to the draw surface bound to the >> context that is current on the calling thread. [...] interval is >> silently clamped to minimum and maximum implementation dependent >> values before being stored; these values are defined by EGLConfig >> attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL >> respectively. >> >> The default swap interval is 1. >> >> Even though it does not specify the exact behavior if the platform does >> not support changing the swap interval, the default assumed state is the >> swap interval of 1, which I interpret as a value that eglSwapInterval() >> should succeed if called with, even if there is no ability to change the >> interval (but there is no change requested). Moreover, since the >> behavior is defined to clamp the requested value to minimum and maximum >> and at least the default value of 1 must be present in the range, the >> implementation might be expected to have a valid range, which in case of >> the feature being unsupported, would correspond to {1} and any request >> might be expected to be clamped to this value. >> >> Fix this by defaulting dri2_dpy's min_swap_interval, max_swap_interval >> and default_swap_interval to 1 in dri2_setup_screen() and let platforms, >> which support this functionality set their own values after this >> function returns. Thanks to patches merged earlier, we can also remove >> the dri2_fallback_swap_interval() completely, as with a singular range >> it would not be called anyway. >> >> v2: Remove dri2_fallback_swap_interval() completely thanks to higher >> layer already clamping the requested interval and not calling the >> driver layer if the clamped value is the same as current. >> > Not every driver is EGL 1.5, although the 1.4 spec has exact same text. > > Reviewed-by: Emil Velikov > > We could have this in stable, but will need a clamp like v1 since > Eric's rework did not land there. > Gents, how do you feel on the topic? FWIW, Eric's patches applied cleanly onto our chromium branch, which was based on something not very far away from what 17.2 branched at. If for some reasons we prefer to avoid further cherry picks, we can go with my v1 for stable, which doesn't depend on them. Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
Reviewed-by: Tapani Pälli On 08/10/2017 04:59 PM, Tomasz Figa wrote: dri2_fallback_swap_interval() currently used to stub out swap interval support in Android backend does nothing besides returning EGL_FALSE. This causes at least one known application (Android Snapchat) to fail due to an unexpected error and my loose interpretation of the EGL 1.5 specification justifies it. Relevant quote below: The function EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); specifies the minimum number of video frame periods per buffer swap for the draw surface of the current context, for the current rendering API. [...] The parameter interval specifies the minimum number of video frames that are displayed before a buffer swap will occur. The interval specified by the function applies to the draw surface bound to the context that is current on the calling thread. [...] interval is silently clamped to minimum and maximum implementation dependent values before being stored; these values are defined by EGLConfig attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL respectively. The default swap interval is 1. Even though it does not specify the exact behavior if the platform does not support changing the swap interval, the default assumed state is the swap interval of 1, which I interpret as a value that eglSwapInterval() should succeed if called with, even if there is no ability to change the interval (but there is no change requested). Moreover, since the behavior is defined to clamp the requested value to minimum and maximum and at least the default value of 1 must be present in the range, the implementation might be expected to have a valid range, which in case of the feature being unsupported, would correspond to {1} and any request might be expected to be clamped to this value. Fix this by defaulting dri2_dpy's min_swap_interval, max_swap_interval and default_swap_interval to 1 in dri2_setup_screen() and let platforms, which support this functionality set their own values after this function returns. Thanks to patches merged earlier, we can also remove the dri2_fallback_swap_interval() completely, as with a singular range it would not be called anyway. v2: Remove dri2_fallback_swap_interval() completely thanks to higher layer already clamping the requested interval and not calling the driver layer if the clamped value is the same as current. Signed-off-by: Tomasz Figa --- src/egl/drivers/dri2/egl_dri2.c | 12 src/egl/drivers/dri2/egl_dri2_fallbacks.h | 7 --- src/egl/drivers/dri2/platform_android.c | 1 - src/egl/drivers/dri2/platform_drm.c | 1 - src/egl/drivers/dri2/platform_surfaceless.c | 1 - src/egl/drivers/dri2/platform_x11.c | 2 +- 6 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index f0d1ded408..686dd68d29 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -630,6 +630,18 @@ dri2_setup_screen(_EGLDisplay *disp) struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); unsigned int api_mask; + /* +* EGL 1.5 specification defines the default value to 1. Moreover, +* eglSwapInterval() is required to clamp requested value to the supported +* range. Since the default value is implicitly assumed to be supported, +* use it as both minimum and maximum for the platforms that do not allow +* changing the interval. Platforms, which allow it (e.g. x11, wayland) +* override these values already. +*/ + dri2_dpy->min_swap_interval = 1; + dri2_dpy->max_swap_interval = 1; + dri2_dpy->default_swap_interval = 1; + if (dri2_dpy->image_driver) { api_mask = dri2_dpy->image_driver->getAPIMask(dri2_dpy->dri_screen); } else if (dri2_dpy->dri2) { diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h b/src/egl/drivers/dri2/egl_dri2_fallbacks.h index 604db881a8..a664677572 100644 --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h @@ -55,13 +55,6 @@ dri2_fallback_create_image_khr(_EGLDriver *drv, _EGLDisplay *disp, return NULL; } -static inline EGLBoolean -dri2_fallback_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy, -_EGLSurface *surf, EGLint interval) -{ - return EGL_FALSE; -} - static inline EGLBoolean dri2_fallback_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf, diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c index 50a8248695..c1bae34f2d 100644 --- a/src/egl/drivers/dri2/platform_android.c +++ b/src/egl/drivers/dri2/platform_android.c @@ -1118,7 +1118,6 @@ static const struct dri2_egl_display_vtbl droid_display_vtbl = { .create_pbuffer_surface = droid_create_pb
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
On Thu 10 Aug 2017, Tomasz Figa wrote: > dri2_fallback_swap_interval() currently used to stub out swap interval > support in Android backend does nothing besides returning EGL_FALSE. > This causes at least one known application (Android Snapchat) to fail > due to an unexpected error and my loose interpretation of the EGL 1.5 > specification justifies it. Relevant quote below: ... > Signed-off-by: Tomasz Figa > --- > src/egl/drivers/dri2/egl_dri2.c | 12 > src/egl/drivers/dri2/egl_dri2_fallbacks.h | 7 --- > src/egl/drivers/dri2/platform_android.c | 1 - > src/egl/drivers/dri2/platform_drm.c | 1 - > src/egl/drivers/dri2/platform_surfaceless.c | 1 - > src/egl/drivers/dri2/platform_x11.c | 2 +- > 6 files changed, 13 insertions(+), 11 deletions(-) Reviewed-by: Chad Versace ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way (v2)
On 10 August 2017 at 14:59, Tomasz Figa wrote: > dri2_fallback_swap_interval() currently used to stub out swap interval > support in Android backend does nothing besides returning EGL_FALSE. > This causes at least one known application (Android Snapchat) to fail > due to an unexpected error and my loose interpretation of the EGL 1.5 > specification justifies it. Relevant quote below: > > The function > > EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); > > specifies the minimum number of video frame periods per buffer swap > for the draw surface of the current context, for the current rendering > API. [...] > > The parameter interval specifies the minimum number of video frames > that are displayed before a buffer swap will occur. The interval > specified by the function applies to the draw surface bound to the > context that is current on the calling thread. [...] interval is > silently clamped to minimum and maximum implementation dependent > values before being stored; these values are defined by EGLConfig > attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL > respectively. > > The default swap interval is 1. > > Even though it does not specify the exact behavior if the platform does > not support changing the swap interval, the default assumed state is the > swap interval of 1, which I interpret as a value that eglSwapInterval() > should succeed if called with, even if there is no ability to change the > interval (but there is no change requested). Moreover, since the > behavior is defined to clamp the requested value to minimum and maximum > and at least the default value of 1 must be present in the range, the > implementation might be expected to have a valid range, which in case of > the feature being unsupported, would correspond to {1} and any request > might be expected to be clamped to this value. > > Fix this by defaulting dri2_dpy's min_swap_interval, max_swap_interval > and default_swap_interval to 1 in dri2_setup_screen() and let platforms, > which support this functionality set their own values after this > function returns. Thanks to patches merged earlier, we can also remove > the dri2_fallback_swap_interval() completely, as with a singular range > it would not be called anyway. > > v2: Remove dri2_fallback_swap_interval() completely thanks to higher > layer already clamping the requested interval and not calling the > driver layer if the clamped value is the same as current. > Not every driver is EGL 1.5, although the 1.4 spec has exact same text. Reviewed-by: Emil Velikov We could have this in stable, but will need a clamp like v1 since Eric's rework did not land there. Gents, how do you feel on the topic? -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way
On 10 August 2017 at 13:51, Tomasz Figa wrote: > On Thu, Aug 10, 2017 at 9:40 PM, Eric Engestrom wrote: >> On 10 August 2017 06:43:45 BST, Tomasz Figa wrote: >>> dri2_fallback_swap_interval() currently used to stub out swap interval >>> support in Android backend does nothing besides returning EGL_FALSE. >>> This causes at least one known application (Android Snapchat) to fail >>> due to an unexpected error and my loose interpretation of the EGL 1.5 >>> specification justifies it. Relevant quote below: >>> >>> The function >>> >>> EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); >>> >>>specifies the minimum number of video frame periods per buffer swap >>> for the draw surface of the current context, for the current rendering >>> API. [...] >>> >>>The parameter interval specifies the minimum number of video frames >>> that are displayed before a buffer swap will occur. The interval >>> specified by the function applies to the draw surface bound to the >>> context that is current on the calling thread. [...] interval is >>> silently clamped to minimum and maximum implementation dependent >>> values before being stored; these values are defined by EGLConfig >>> attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL >>> respectively. >>> >>> The default swap interval is 1. >>> >>> Even though it does not specify the exact behavior if the platform >>> does >>> not support changing the swap interval, the default assumed state is >>> the >>> swap interval of 1, which I interpret as a value that >>> eglSwapInterval() >>> should succeed if called with, even if there is no ability to change >>> the >>> interval (but there is no change requested). Moreover, since the >>> behavior is defined to clamp the requested value to minimum and >>> maximum >>> and at least the default value of 1 must be present in the range, the >>> implementation might be expected to have a valid range, which in case >>> of >>> the feature being unsupported, would correspond to {1} and any request >>> might be expected to be clamped to this value. >>> >>> This is further confirmed by the code in _eglSwapInterval() in >>> src/egl/main/eglsurface.c, which is the default fallback >>> implementation >>> for EGL drivers not implementing its own. The problem with it is that >>> the DRI2 EGL driver provides its own implementation that calls into >>> platform backends, so we cannot just simply fall back to it. >>> >>> Signed-off-by: Tomasz Figa >>> --- >>> src/egl/drivers/dri2/egl_dri2.c | 12 >>> src/egl/drivers/dri2/egl_dri2_fallbacks.h | 9 - >>> src/egl/drivers/dri2/platform_x11.c | 1 + >>> 3 files changed, 21 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/egl/drivers/dri2/egl_dri2.c >>> b/src/egl/drivers/dri2/egl_dri2.c >>> index f0d1ded408..686dd68d29 100644 >>> --- a/src/egl/drivers/dri2/egl_dri2.c >>> +++ b/src/egl/drivers/dri2/egl_dri2.c >>> @@ -630,6 +630,18 @@ dri2_setup_screen(_EGLDisplay *disp) >>> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); >>> unsigned int api_mask; >>> >>> + /* >>> +* EGL 1.5 specification defines the default value to 1. Moreover, >>> +* eglSwapInterval() is required to clamp requested value to the >>> supported >>> +* range. Since the default value is implicitly assumed to be >>> supported, >>> +* use it as both minimum and maximum for the platforms that do >>> not allow >>> +* changing the interval. Platforms, which allow it (e.g. x11, >>> wayland) >>> +* override these values already. >>> +*/ >>> + dri2_dpy->min_swap_interval = 1; >>> + dri2_dpy->max_swap_interval = 1; >>> + dri2_dpy->default_swap_interval = 1; >>> + >>> if (dri2_dpy->image_driver) { >>> api_mask = dri2_dpy->image_driver->getAPIMask(dri2_dpy->dri_screen); >>> } else if (dri2_dpy->dri2) { >>> diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h >>> b/src/egl/drivers/dri2/egl_dri2_fallbacks.h >>> index 604db881a8..c70c686f8d 100644 >>> --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h >>> +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h >>> @@ -59,7 +59,14 @@ static inline EGLBoolean >>> dri2_fallback_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy, >>> _EGLSurface *surf, EGLint interval) >>> { >>> - return EGL_FALSE; >>> + if (interval > surf->Config->MaxSwapInterval) >>> + interval = surf->Config->MaxSwapInterval; >>> + else if (interval < surf->Config->MinSwapInterval) >>> + interval = surf->Config->MinSwapInterval; >>> + >>> + surf->SwapInterval = interval; >>> + >>> + return EGL_TRUE; >> >> Agreed with the interpretation, but if memory serves (on my phone on a plane >> right now) I already took care of clamping and setting the value one layer >> above, so the only change needed is s/EGL_FALSE/EGL_TRUE/ in this function. >> Eric, you beat me to it. I thought you're on holidays :-) >> Look for a commit of
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way
On Thu, Aug 10, 2017 at 9:40 PM, Eric Engestrom wrote: > On 10 August 2017 06:43:45 BST, Tomasz Figa wrote: >> dri2_fallback_swap_interval() currently used to stub out swap interval >> support in Android backend does nothing besides returning EGL_FALSE. >> This causes at least one known application (Android Snapchat) to fail >> due to an unexpected error and my loose interpretation of the EGL 1.5 >> specification justifies it. Relevant quote below: >> >> The function >> >> EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); >> >>specifies the minimum number of video frame periods per buffer swap >> for the draw surface of the current context, for the current rendering >> API. [...] >> >>The parameter interval specifies the minimum number of video frames >> that are displayed before a buffer swap will occur. The interval >> specified by the function applies to the draw surface bound to the >> context that is current on the calling thread. [...] interval is >> silently clamped to minimum and maximum implementation dependent >> values before being stored; these values are defined by EGLConfig >> attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL >> respectively. >> >> The default swap interval is 1. >> >> Even though it does not specify the exact behavior if the platform >> does >> not support changing the swap interval, the default assumed state is >> the >> swap interval of 1, which I interpret as a value that >> eglSwapInterval() >> should succeed if called with, even if there is no ability to change >> the >> interval (but there is no change requested). Moreover, since the >> behavior is defined to clamp the requested value to minimum and >> maximum >> and at least the default value of 1 must be present in the range, the >> implementation might be expected to have a valid range, which in case >> of >> the feature being unsupported, would correspond to {1} and any request >> might be expected to be clamped to this value. >> >> This is further confirmed by the code in _eglSwapInterval() in >> src/egl/main/eglsurface.c, which is the default fallback >> implementation >> for EGL drivers not implementing its own. The problem with it is that >> the DRI2 EGL driver provides its own implementation that calls into >> platform backends, so we cannot just simply fall back to it. >> >> Signed-off-by: Tomasz Figa >> --- >> src/egl/drivers/dri2/egl_dri2.c | 12 >> src/egl/drivers/dri2/egl_dri2_fallbacks.h | 9 - >> src/egl/drivers/dri2/platform_x11.c | 1 + >> 3 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/src/egl/drivers/dri2/egl_dri2.c >> b/src/egl/drivers/dri2/egl_dri2.c >> index f0d1ded408..686dd68d29 100644 >> --- a/src/egl/drivers/dri2/egl_dri2.c >> +++ b/src/egl/drivers/dri2/egl_dri2.c >> @@ -630,6 +630,18 @@ dri2_setup_screen(_EGLDisplay *disp) >> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); >> unsigned int api_mask; >> >> + /* >> +* EGL 1.5 specification defines the default value to 1. Moreover, >> +* eglSwapInterval() is required to clamp requested value to the >> supported >> +* range. Since the default value is implicitly assumed to be >> supported, >> +* use it as both minimum and maximum for the platforms that do >> not allow >> +* changing the interval. Platforms, which allow it (e.g. x11, >> wayland) >> +* override these values already. >> +*/ >> + dri2_dpy->min_swap_interval = 1; >> + dri2_dpy->max_swap_interval = 1; >> + dri2_dpy->default_swap_interval = 1; >> + >> if (dri2_dpy->image_driver) { >> api_mask = dri2_dpy->image_driver->getAPIMask(dri2_dpy->dri_screen); >> } else if (dri2_dpy->dri2) { >> diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h >> b/src/egl/drivers/dri2/egl_dri2_fallbacks.h >> index 604db881a8..c70c686f8d 100644 >> --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h >> +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h >> @@ -59,7 +59,14 @@ static inline EGLBoolean >> dri2_fallback_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy, >> _EGLSurface *surf, EGLint interval) >> { >> - return EGL_FALSE; >> + if (interval > surf->Config->MaxSwapInterval) >> + interval = surf->Config->MaxSwapInterval; >> + else if (interval < surf->Config->MinSwapInterval) >> + interval = surf->Config->MinSwapInterval; >> + >> + surf->SwapInterval = interval; >> + >> + return EGL_TRUE; > > Agreed with the interpretation, but if memory serves (on my phone on a plane > right now) I already took care of clamping and setting the value one layer > above, so the only change needed is s/EGL_FALSE/EGL_TRUE/ in this function. > > Look for a commit of mine (`git log --author=engestrom -- src/egl/`) about 3 > weeks ago. Good catch, thanks for pointing out. I should have checked upstream first. Looks like the rebase of our branch was just few days before that commi
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way
On 10 August 2017 06:43:45 BST, Tomasz Figa wrote: > dri2_fallback_swap_interval() currently used to stub out swap interval > support in Android backend does nothing besides returning EGL_FALSE. > This causes at least one known application (Android Snapchat) to fail > due to an unexpected error and my loose interpretation of the EGL 1.5 > specification justifies it. Relevant quote below: > > The function > > EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); > >specifies the minimum number of video frame periods per buffer swap > for the draw surface of the current context, for the current rendering > API. [...] > >The parameter interval specifies the minimum number of video frames > that are displayed before a buffer swap will occur. The interval > specified by the function applies to the draw surface bound to the > context that is current on the calling thread. [...] interval is > silently clamped to minimum and maximum implementation dependent > values before being stored; these values are defined by EGLConfig > attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL > respectively. > > The default swap interval is 1. > > Even though it does not specify the exact behavior if the platform > does > not support changing the swap interval, the default assumed state is > the > swap interval of 1, which I interpret as a value that > eglSwapInterval() > should succeed if called with, even if there is no ability to change > the > interval (but there is no change requested). Moreover, since the > behavior is defined to clamp the requested value to minimum and > maximum > and at least the default value of 1 must be present in the range, the > implementation might be expected to have a valid range, which in case > of > the feature being unsupported, would correspond to {1} and any request > might be expected to be clamped to this value. > > This is further confirmed by the code in _eglSwapInterval() in > src/egl/main/eglsurface.c, which is the default fallback > implementation > for EGL drivers not implementing its own. The problem with it is that > the DRI2 EGL driver provides its own implementation that calls into > platform backends, so we cannot just simply fall back to it. > > Signed-off-by: Tomasz Figa > --- > src/egl/drivers/dri2/egl_dri2.c | 12 > src/egl/drivers/dri2/egl_dri2_fallbacks.h | 9 - > src/egl/drivers/dri2/platform_x11.c | 1 + > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/src/egl/drivers/dri2/egl_dri2.c > b/src/egl/drivers/dri2/egl_dri2.c > index f0d1ded408..686dd68d29 100644 > --- a/src/egl/drivers/dri2/egl_dri2.c > +++ b/src/egl/drivers/dri2/egl_dri2.c > @@ -630,6 +630,18 @@ dri2_setup_screen(_EGLDisplay *disp) > struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); > unsigned int api_mask; > > + /* > +* EGL 1.5 specification defines the default value to 1. Moreover, > +* eglSwapInterval() is required to clamp requested value to the > supported > +* range. Since the default value is implicitly assumed to be > supported, > +* use it as both minimum and maximum for the platforms that do > not allow > +* changing the interval. Platforms, which allow it (e.g. x11, > wayland) > +* override these values already. > +*/ > + dri2_dpy->min_swap_interval = 1; > + dri2_dpy->max_swap_interval = 1; > + dri2_dpy->default_swap_interval = 1; > + > if (dri2_dpy->image_driver) { > api_mask = dri2_dpy->image_driver->getAPIMask(dri2_dpy->dri_screen); > } else if (dri2_dpy->dri2) { > diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h > b/src/egl/drivers/dri2/egl_dri2_fallbacks.h > index 604db881a8..c70c686f8d 100644 > --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h > +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h > @@ -59,7 +59,14 @@ static inline EGLBoolean > dri2_fallback_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy, > _EGLSurface *surf, EGLint interval) > { > - return EGL_FALSE; > + if (interval > surf->Config->MaxSwapInterval) > + interval = surf->Config->MaxSwapInterval; > + else if (interval < surf->Config->MinSwapInterval) > + interval = surf->Config->MinSwapInterval; > + > + surf->SwapInterval = interval; > + > + return EGL_TRUE; Agreed with the interpretation, but if memory serves (on my phone on a plane right now) I already took care of clamping and setting the value one layer above, so the only change needed is s/EGL_FALSE/EGL_TRUE/ in this function. Look for a commit of mine (`git log --author=engestrom -- src/egl/`) about 3 weeks ago. Cheers, Eric > } > > static inline EGLBoolean > diff --git a/src/egl/drivers/dri2/platform_x11.c > b/src/egl/drivers/dri2/platform_x11.c > index 4610ec579f..97cdd09078 100644 > --- a/src/egl/drivers/dri2/platform_x11.c > +++ b/src/egl/drivers/dri2/platform_x11.c > @@ -1283,6 +1283,7 @@ dri2_x11_setu
Re: [Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way
I agree to the interpretation, it is stated that EGL_FALSE happens 'on failure' and there are 2 error cases specified for eglSwapInterval. It seems right to assume those are the only cases where it can fail (no current context or surface bound). Reviewed-by: Tapani Pälli On 08/10/2017 08:43 AM, Tomasz Figa wrote: dri2_fallback_swap_interval() currently used to stub out swap interval support in Android backend does nothing besides returning EGL_FALSE. This causes at least one known application (Android Snapchat) to fail due to an unexpected error and my loose interpretation of the EGL 1.5 specification justifies it. Relevant quote below: The function EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval); specifies the minimum number of video frame periods per buffer swap for the draw surface of the current context, for the current rendering API. [...] The parameter interval specifies the minimum number of video frames that are displayed before a buffer swap will occur. The interval specified by the function applies to the draw surface bound to the context that is current on the calling thread. [...] interval is silently clamped to minimum and maximum implementation dependent values before being stored; these values are defined by EGLConfig attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL respectively. The default swap interval is 1. Even though it does not specify the exact behavior if the platform does not support changing the swap interval, the default assumed state is the swap interval of 1, which I interpret as a value that eglSwapInterval() should succeed if called with, even if there is no ability to change the interval (but there is no change requested). Moreover, since the behavior is defined to clamp the requested value to minimum and maximum and at least the default value of 1 must be present in the range, the implementation might be expected to have a valid range, which in case of the feature being unsupported, would correspond to {1} and any request might be expected to be clamped to this value. This is further confirmed by the code in _eglSwapInterval() in src/egl/main/eglsurface.c, which is the default fallback implementation for EGL drivers not implementing its own. The problem with it is that the DRI2 EGL driver provides its own implementation that calls into platform backends, so we cannot just simply fall back to it. Signed-off-by: Tomasz Figa --- src/egl/drivers/dri2/egl_dri2.c | 12 src/egl/drivers/dri2/egl_dri2_fallbacks.h | 9 - src/egl/drivers/dri2/platform_x11.c | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index f0d1ded408..686dd68d29 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -630,6 +630,18 @@ dri2_setup_screen(_EGLDisplay *disp) struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); unsigned int api_mask; + /* +* EGL 1.5 specification defines the default value to 1. Moreover, +* eglSwapInterval() is required to clamp requested value to the supported +* range. Since the default value is implicitly assumed to be supported, +* use it as both minimum and maximum for the platforms that do not allow +* changing the interval. Platforms, which allow it (e.g. x11, wayland) +* override these values already. +*/ + dri2_dpy->min_swap_interval = 1; + dri2_dpy->max_swap_interval = 1; + dri2_dpy->default_swap_interval = 1; + if (dri2_dpy->image_driver) { api_mask = dri2_dpy->image_driver->getAPIMask(dri2_dpy->dri_screen); } else if (dri2_dpy->dri2) { diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h b/src/egl/drivers/dri2/egl_dri2_fallbacks.h index 604db881a8..c70c686f8d 100644 --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h @@ -59,7 +59,14 @@ static inline EGLBoolean dri2_fallback_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf, EGLint interval) { - return EGL_FALSE; + if (interval > surf->Config->MaxSwapInterval) + interval = surf->Config->MaxSwapInterval; + else if (interval < surf->Config->MinSwapInterval) + interval = surf->Config->MinSwapInterval; + + surf->SwapInterval = interval; + + return EGL_TRUE; } static inline EGLBoolean diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c index 4610ec579f..97cdd09078 100644 --- a/src/egl/drivers/dri2/platform_x11.c +++ b/src/egl/drivers/dri2/platform_x11.c @@ -1283,6 +1283,7 @@ dri2_x11_setup_swap_interval(struct dri2_egl_display *dri2_dpy) */ dri2_dpy->min_swap_interval = 0; dri2_dpy->max_swap_interval = 0; + dri2_dpy->default_swap_interval = 0; if (!dri2_dpy->swap_available) return;